On Fri, 29 Jan 2021 12:19:51 GMT, Tom Schindl <tschi...@openjdk.org> wrote:
>> As indicated in the JBS bug, using a `PixelReader` to read a scaled image in
>> HiDPI mode, for example an `@2x` image, to read more than one pixel will
>> read data from the wrong location in the image, usually leading to an IOOBE.
>>
>> The bug is in the `getPixelAccessor` method of Prism Image. The method
>> correctly creates a new `Accessor` if one hasn't already been created, but
>> then it unconditionally wraps the current `Accessor` in a
>> `ScaledPixelAccessor` if the scale is > 1. So the second time this method is
>> called, it created another `ScaledPixelAccessor` that wraps the first
>> `ScaledPixelAccessor`, meaning that the indexes into the pixel array are
>> multiplied by 4. This continues for each new call to this method.
>>
>> The fix is to only wrap an `Accessor` in a `ScaledPixelAccessor` the first
>> time, when it is created.
>>
>> I added a test, which is run for both scaled and unscaled images, to ensure
>> that we get the right value when reading a pixel, and that reading it a
>> second time returns the same result. Without the fix, the new test will fail
>> with IOOBE when the scale factor is two. Related to this, I initially
>> commented out the wrapping in a `ScaledPixelAccessor` entirely, just to see
>> what would happen, and none of the existing tests caught it. The new test
>> will catch this.
>
> modules/javafx.graphics/src/main/java/com/sun/prism/Image.java line 654:
>
>> 652: }
>> 653: if (pixelScale != 1.0f) {
>> 654: pixelaccessor = new ScaledAccessor<>(pixelaccessor,
>> pixelScale);
>
> is that really correct? I think you should not overwrite/save the scaled
> pixelAccessor in the instance variable - if I read that correct with this
> change the following happens: if I call getPixelAccessor() 3 times I get a
> ScaledAccessor wrapping a ScaleAccessor wrapping a ScaledAccessor, wrapping
> one of ByteAccess, ByteRgbAccess, IntAccess
No, that’s what used to happen before this fix.
The fix moves the wrapping of the pixelAccessor with a ScaledPixelAxcessor
inside the null check so it’s now only done once.
-------------
PR: https://git.openjdk.java.net/jfx/pull/389
