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

[Martin Desruisseaux]

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`, except when there is integer underflow (see _Behavioural changes_ below):


// Clip to the current Raster
Rectangle rclip = new Rectangle(startX, startY, width, height);
Rectangle bclip = new Rectangle(0, 0, raster.width, raster.height);
Rectangle intersect = rclip.intersection(bclip);
if (intersect.isEmpty()) {
    return;
}
width = intersect.width;
height = intersect.height;
startX = intersect.x;
startY = intersect.y;


Next modified block of code from `WritableRaster`:


int[] iData = null;
for (int i=0; i < height; i++) {
    // Grab one scanline at a time
    iData = r.getPixels(startX, startY+i, width, 1, iData);
    setPixels(dstOffX, dstOffY+i, width, 1, iData);
}


Above is equivalent to `BufferedImage` code (keeping in mind that `startX == dstOffX` and same for Y, because of the `BufferedImage` constraint on raster starting at 0,0):


int[] tdata = null;
// remind use get/setDataElements for speed if Rasters are
// compatible
for (int i = startY; i < startY+height; i++)  {
    tdata = r.getPixels(startX,i,width,1,tdata);
    raster.setPixels(startX,i,width,1, tdata);
}



# Behavioural changes
This pull request would change the `BufferedImage.setData(Raster)` behaviour in the following ways:

* When using the standard `WritableRaster` implementations provided by OpenJDK, performance improvements may be observed because the JDK overrides `WritableRaster.setRect(Raster)` in various internal subclasses such as `ByteInterleavedRaster`. The overridden methods try to replace the loop by calls to `System.arraycopy(…)`.
* If the user constructed a `BufferedImage` with his own `WritableRaster` implementation which overrides `setRect(Raster)`, his overridden method would now be invoked. It would be compatible with `BufferedImage` Javadoc however, which said _"This class relies on the data fetching and setting methods of `Raster`, and on the color characterization methods of `ColorModel`."_
* The current `BufferedImage` implementation is more robust to integer underflows, because it uses `Rectangle.intersection(Rectangle)` which is underflow-safe. This pull request would lost this safety, unless we modify `WritableRaster.setRect(Raster)` for using `Rectangle.intersection(…)` as well.

# Open questions

* Do we also modify the `WritableRaster.setRect(Raster)` implementation for preserving the integer underflow safety of current `BufferedImage` implementation? The change would be to replace current intersection calculation by call to `Rectangle.intersection(…)`.
* The issue described in this pull request also applies to `BufferedImage.getData()` method and its derivative. Should we fix them as well?

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

Commit messages:
 - Use Rectangle.intersection(Rectangle) in WritableRaster.setRect(int, int, Raster) for safety against integer underflow/overflow.
 - `BufferedImage.setData(Raster)` should not cast float and double values to integers.

Changes: https://git.openjdk.org/jdk/pull/13797/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13797&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8323108
  Stats: 171 lines in 3 files changed: 120 ins; 47 del; 4 mod
  Patch: https://git.openjdk.org/jdk/pull/13797.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/13797/head:pull/13797

PR: https://git.openjdk.org/jdk/pull/13797