Skip to content

Conversation

@HeikoKlare
Copy link
Contributor

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 Image implementation for ImageDataProvider and ImageFileNameProvider 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. For both the consistent retrieval of image data and the avoidance of repeated loading of image data according test cases are added.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2025

Test Results

   539 files  ±0     539 suites  ±0   26m 55s ⏱️ - 2m 2s
 4 332 tests +2   4 322 ✅ +2    9 💤 ±0  1 ❌ ±0 
16 587 runs  +8  16 480 ✅ +3  106 💤 +5  1 ❌ ±0 

For more details on these failures, see this check.

Results for commit aff8f04. ± Comparison against base commit 0f21a6e.

♻️ This comment has been updated with latest results.

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.

Looks good to me. I tested around a bit and didn't find a regression caused by this.

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 Image implementation for ImageDataProvider and
ImageFileNameProvider 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. For both the consistent retrieval of image data and the
avoidance of repeated loading of image data according test cases are
added.
@HeikoKlare HeikoKlare force-pushed the image-providers-cache branch from e5e40f0 to aff8f04 Compare April 4, 2025 13:06
@HeikoKlare
Copy link
Contributor Author

Test failure is known and documented: #1843

@HeikoKlare HeikoKlare merged commit 6e865bc into eclipse-platform:master Apr 4, 2025
13 of 15 checks passed
@HeikoKlare HeikoKlare deleted the image-providers-cache branch April 4, 2025 13:26
@sratz
Copy link
Member

sratz commented Aug 18, 2025

I noticed that this introduces behavior that might violate the contract of Image.getImageData():

... Modifications made to the returned {@code ImageData} will not affect this {@code Image}.

But there is now the following behavior:

		ImageData initialImageData = new ImageData(1, 1, 24, new PaletteData(new RGB(0, 0, 0), new RGB(255, 255, 255)));

		Image image = new Image(new Display(), initialImageData);

		ImageData imageData1 = image.getImageData(100);

		imageData1.setPixel(0, 0, 1);

		ImageData imageData2 = image.getImageData(100);

		System.out.println("initial: " + initialImageData.hashCode() + ", value: " + initialImageData.getPixel(0, 0));
		System.out.println("1      : " + imageData1.hashCode() + ", value: " + imageData1.getPixel(0, 0));
		System.out.println("2      : " + imageData2.hashCode() + ", value: " + imageData2.getPixel(0, 0));

output:

initial: 1509514333, value: 0
1      : 991505714, value: 1
2      : 991505714, value: 1

So while modifying the ImageData retrieved from the first call to getImageData() did not affect the displayed Image itself, it still has an effect on a subsequent call to getImageData().

Is this desired / expected?

@HeikoKlare
Copy link
Contributor Author

Is this desired / expected?

That's definitely not expected but simply seems like a bug introduced with this PR. Thank you for the catch, @sratz!
I have created a fix together with a test based on your snippet:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants