Skip to content

[Win32] Ensure consistent image data returned for data-based images #1996

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

HeikoKlare
Copy link
Contributor

@HeikoKlare HeikoKlare commented Apr 4, 2025

Due to the on-demand creation of image handles, there is not necessarily a handles anymore from which image data is retrieved when requesting is via the getImageData(...) methods. This results in potentially different kinds of image data (including different anti-aliasing results) depending on whether a handle has already been created for an image at the given zoom or not.

This change adapts the implementation of Image based on static ImageData and streams to always use the image data retrieved from a native handle. To this end, it temporarily creates a handle if necessary. In order to avoid repeated loading and handle creation for the same source of image, a cache for the already retrieved image data is introduced.

⚠️ This is based on and should thus be merged after #2036

Fixes #2052

Supercedes and thus closes #2051

Copy link
Contributor

github-actions bot commented Apr 4, 2025

Test Results

   539 files  ±0     539 suites  ±0   29m 3s ⏱️ +51s
 4 337 tests +1   4 321 ✅ +1   15 💤 ±0  1 ❌ ±0 
16 601 runs  +4  16 463 ✅ +2  137 💤 +2  1 ❌ ±0 

For more details on these failures, see this check.

Results for commit ab716d9. ± Comparison against base commit 504c82b.

♻️ This comment has been updated with latest results.

@HeikoKlare HeikoKlare force-pushed the image-static-providers-cache branch from 2f885c6 to eb8637e Compare April 4, 2025 15:11
@HeikoKlare HeikoKlare marked this pull request as ready for review April 4, 2025 16:01
Copy link
Contributor

@akoch-yatta akoch-yatta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me except the one question/remark, I have. I still need to do some testing.

@akoch-yatta
Copy link
Contributor

@HeikoKlare As I already had the change drafted some time back, I wrapped that up and created #2027 to make sure imageProvider is always != null, which makes my comment not relevant anymore. I would propose to process that one first

Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this and it looks good. I would even change the commit-text and add the it contributes (or even fixes) #2052. I'm not sure if one can call it a "fix" because I always compare these 2 constructors of Image:

  1. public Image(Device device, ImageData data) {...}
  2. public Image(Device device, Image srcImage, int flag) {...}

and the 2nd one produces sharper images.

That being said, I also tested constructor nr. 1 in a for-loop, like this:

Image image = new Image(cachedFolder.getDevice(), clonedImageData);
for (int i=0; i<15; i++) {
	// Try to make it even more blurry
	image = new Image(image.getDevice(), image.getImageData());
}

return image;

And it doesn't matter how many times I iterate, the returned image has always the same quality/sharpness.

@HeikoKlare HeikoKlare force-pushed the image-static-providers-cache branch from eb8637e to 705b4b5 Compare April 24, 2025 09:09
@HeikoKlare
Copy link
Contributor Author

HeikoKlare commented Apr 24, 2025

I have rebased this on the cleanup PR:

This required minor adaptations as the adapted image provider wrapper needs to take into account the specific handling for ICON-type images to avoid errors during smooth scaling.

Copy link
Contributor

@akoch-yatta akoch-yatta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a second look after the rebase. Changes still look fine to me

Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I retested it and I saw no problems with this PR ✔️

@HeikoKlare HeikoKlare force-pushed the image-static-providers-cache branch from 705b4b5 to 41f7b9a Compare April 24, 2025 13:40
@HeikoKlare
Copy link
Contributor Author

I tested this and it looks good. I would even change the commit-text and add the it contributes (or even fixes) #2052. I'm not sure if one can call it a "fix" because I always compare these 2 constructors of Image:

  1. public Image(Device device, ImageData data) {...}
  2. public Image(Device device, Image srcImage, int flag) {...}

and the 2nd one produces sharper images.

I missed this for the previous update of the PR. I just adapted the commit message and added an according fixes keyword to the PR description to auto-close the referenced issue.

The copy constructor for image will of course always produce better results than one taking a specific ImageData, as that specific ImageData represent the image at one single zoom and all other zooms have to be (badly) generated out of it while an "ordinary" image usually only contains the meta-information to retrieve the ImageData for any zoom in the best way (like rasterizing an SVG, loading a hi-res PNG or the like).

@HeikoKlare HeikoKlare force-pushed the image-static-providers-cache branch 2 times, most recently from 9fcdd69 to 05db35c Compare April 24, 2025 16:40

Verified

This commit was signed with the committer’s verified signature.
pradyunsg Pradyun Gedam
Due to the on-demand creation of image handles, there is not necessarily
a handles anymore from which image data is retrieved when requesting is
via the getImageData(...) methods. This results in potentially different
kinds of image data (including different anti-aliasing results)
depending on whether a handle has already been created for an image at
the given zoom or not.

This change adapts the implementation of Image based on static ImageData
and streams to always use the image data retrieved from a native handle.
To this end, it temporarily creates a handle if necessary. In order to
avoid repeated loading and handle creation for the same source of image,
a cache for the already retrieved image data is introduced.

Fixes eclipse-platform#2052
@HeikoKlare HeikoKlare force-pushed the image-static-providers-cache branch from 05db35c to ab716d9 Compare April 24, 2025 16:41
@HeikoKlare
Copy link
Contributor Author

Version increment check is failing for infrastructure reasons and no further version bump is required here.

Windows test failure is known and unrelated: #1843

@HeikoKlare HeikoKlare merged commit 28fdeb6 into eclipse-platform:master Apr 24, 2025
7 of 10 checks passed
@HeikoKlare HeikoKlare deleted the image-static-providers-cache branch April 24, 2025 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants