On Thu, 4 May 2023 10:14:10 GMT, Martin Desruisseaux <d...@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
