On Thu, 4 May 2023 10:14:10 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 || height <= 0) {
> return;
> }
>
>
> Above is equivalent to the following code from `BufferedImage`, ex...
This all seems reasonable to me, and passes my testing.
I would however like Brian Burkhalter to take a look before it is integrated
-------------
Marked as reviewed by prr (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13797#pullrequestreview-1828097281
