On Mon, 13 Nov 2023 14:53:22 GMT, Martin Desruisseaux <[email protected]> wrote:
>> The `BufferedImage` Javadoc does not mention any constraint about the data
>> type. In practice, `BufferedImage` with floating point values can be
>> rendered by Java2D as well as integers, provided that a compatible
>> `ColorModel` was supplied at construction time. However calls to
>> `setData(Raster)` unexpectedly cast floating point values to integers. For
>> example sample value 0.8 become 0. This is demonstrated by the `SetData`
>> test case in this pull request.
>>
>> An easy fix, which is proposed in this pull request, is to replace the whole
>> `BufferedImage.setData(Raster)` method body by a simple call to
>> `WritableRaster.setRect(Raster)`, which handles all `DataBuffer` types
>> correctly. The method contracts documented in their respective Javadoc are
>> compatible. Furthermore an examination of their source code shows that they
>> are equivalent, except for the differences noted in the _Behavioural
>> changes_ section below.
>>
>> # Source code comparison
>> For demonstrating that delegating to `WritableRaster.setRect(Raster)` would
>> be equivalent to the current code, one can copy the method body and apply
>> the following changes:
>>
>> * Remove `dx` parameter, replaced by 0.
>> * Remove `dy` parameter, replaced by 0.
>> * Rename `srcRaster` parameter as `r` (like in `BufferedImage`).
>> * Rename `startY` variable as `i` (like in `BufferedImage`).
>> * Rename `srcOffX` variable as `startX` (like in `BufferedImage`).
>> * Rename `srcOffY` variable as `startY` (like in `BufferedImage`).
>> * Replace `this.minX` by 0 and simplify.
>> * Replace `this.minY` by 0 and simplify.
>> * Remove the `switch` statement, keeping only the `TYPE_INT` case.
>>
>> Then compare with `BufferedImage.setData(Raster)`. The modified block of
>> code from `WritableRaster` is:
>>
>>
>> int width = r.getWidth();
>> int height = r.getHeight();
>> int startX = r.getMinX();
>> int startY = r.getMinY();
>>
>> We can see that above code is identical to `BufferedImage`. The next
>> modified block of code from `WritableRaster`:
>>
>>
>> int dstOffX = startX;
>> int dstOffY = startY;
>>
>> // Clip to this raster
>> if (dstOffX < 0) {
>> width += dstOffX;
>> startX -= dstOffX; // = 0 because dstOffX == startX
>> dstOffX = 0;
>> }
>> if (dstOffY < 0) {
>> height += dstOffY;
>> startY -= dstOffY; // = 0 because dstOffY == startY
>> dstOffY = 0;
>> }
>> if (dstOffX+width > this.width) {
>> width = this.width - dstOffX;
>> }
>> if (dstOffY+height > this.height) {
>> height = this.height - dstOffY;
>> }
>> if (width <= 0 ||...
>
> Added a commit doing the replacement of handcrafter intersection in
> `WritableRaster.setRect(…)` by a call to `Rectangle.intersection(…)`. I
> couldn't run the jtreg tests however. I tried for a few hours, but I presume
> that I didn't configured correctly.
>
> Note that `setRect(…)` is overridden in `sun.awt.image.ByteInterleavedRaster`
> and `sun.awt.image.BytePackedRaster` for optimization purposes. Those methods
> use the same handcrafter intersection code than the one replaced by this
> commit. However I didn't updated the code in those subclasses. They should
> not be subject to underflow / overflow because users should not extend these
> classes and thus access to the protected fields. We could update those
> methods anyway for consistency, but I do not know what is the OpenJDK policy
> in that case.
>
> Regarding `BufferedImage.getData()`, I remember the issue. That method does
> not have the lost of precision issue documented in this pull request. But it
> would nevertheless benefit from the same change (delegate to
> `WritableRaster.setRect(…)`) for performance reasons, because of
> optimizations in above-cited `ByteInterleavedRaster` and `BytePackedRaster`
> subclasses.
@desruisseaux please enable the github actions in your fork, it seems the
"Pre-submit test status" was skipped.
https://github.com/openjdk/jdk/pull/13797/checks?check_run_id=18627782594
-------------
PR Comment: https://git.openjdk.org/jdk/pull/13797#issuecomment-1897610914
