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

[Martin Desruisseaux]

On Thu, 4 May 2023 10:14:10 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 || height <= 0) {
>     return;
> }
> 
> 
> Above is equivalent to the following code from `BufferedImage`, ex...

A comment for keeping this merge request alive. Reminder: it is about resolving data lost in calls to `BufferedImage.setData(Raster)` caused by unnecessary casts of sample values.

Ping for keeping this pull request alive. Can we have a reviewer for it, or open a discussion about whether to expand the issue scope as mentioned in the description?

Issue reminder: `BufferedImage.setData(Raster)` lost data when using floating point values, without anything in the Javadoc suggesting that it is the expected behaviour. The proposed fix would resolve the data lost, *simplify* the code, possibly improve performances (but it is not the main goal) and be closer to what javadoc suggests.

Open question: current pull request does the smallest amount of changes, but more simplifications are possible. Should we add them to this pull request?

Thanks. Regarding the second issue (`getData()` and its derivatives), a closer look shows that I was wrong to said that it suffers the same problem than `setData(Raster)`. Only the latter needs to be fixed, so the current request should be sufficient.

Regarding the second open question about replacing current handcrafter intersection code by a call to `Rectangle.intersection(Rectangle)`, the following code:


        if (dstOffX < this.minX) {
            int skipX = this.minX - dstOffX;
            width -= skipX;
            srcOffX += skipX;
            dstOffX = this.minX;
        }
        if (dstOffY < this.minY) {
            int skipY = this.minY - dstOffY;
            height -= skipY;
            srcOffY += skipY;
            dstOffY = this.minY;
        }
        if (dstOffX+width > this.minX+this.width) {
            width = this.minX + this.width - dstOffX;
        }
        if (dstOffY+height > this.minY+this.height) {
            height = this.minY + this.height - dstOffY;
        }
        if (width <= 0 || height <= 0) {
            return;
        }


would be replaced by the following (this code contains a call to `getBounds()` overrideable method):


        Rectangle dstRect = new Rectangle(dstOffX, dstOffY, width, height);
        dstRect = dstRect.intersection(getBounds());
        if (dstRect.isEmpty()) {
            return;
        }
        srcOffX += dstRect.x - dstOffX;
        srcOffY += dstRect.y - dstOffY;
        dstOffX  = dstRect.x;
        dstOffY  = dstRect.y;
        width    = dstRect.width;
        height   = dstRect.height;


Would it be a worthly change?

I do not see variant of `Rectangle.intersection(…)` with `int` arguments [in the Javadoc](https://docs.oracle.com/en/java/javase/21/docs/api/java.desktop/java/awt/Rectangle.html). Maybe HotSpot can avoid the allocation with escape analysis. But anyway, the cost of `Rectangle` allocation is probably small comparatively to the cost of copying some thousands of pixel values.

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.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/13797#issuecomment-1607179108
PR Comment: https://git.openjdk.org/jdk/pull/13797#issuecomment-1685255511
PR Comment: https://git.openjdk.org/jdk/pull/13797#issuecomment-1798294061
PR Comment: https://git.openjdk.org/jdk/pull/13797#issuecomment-1800282141
PR Comment: https://git.openjdk.org/jdk/pull/13797#issuecomment-1808314079