RFR: 8323108: BufferedImage.setData(Raster) should not cast float and double values to integers

[Sergey Bylokhov]

On Mon, 13 Nov 2023 14:53:22 GMT, Martin Desruisseaux <duke at openjdk.org> 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