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 change looks fine to me as is, but I think it would be useful to eliminate
the next issue with the same patch(it depends on the change size):
@prrace do you have a preference should we fix that in one or separate patches?
>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(…).
ok, let's use a solution based on the `Rectangle.intersection`
keep open
src/java.desktop/share/classes/java/awt/image/BufferedImage.java line 1523:
> 1521: Rectangle bclip = new Rectangle(0, 0, raster.width,
> raster.height);
> 1522: Rectangle intersect = rclip.intersection(bclip);
> 1523: if (intersect.isEmpty()) {
Does the "raster.setRect" has equivalent optimizations? it seems that method
implements handcrafter "intersection", any idea why it was implemented that way?
-------------
PR Comment: https://git.openjdk.org/jdk/pull/13797#issuecomment-1796884736
PR Comment: https://git.openjdk.org/jdk/pull/13797#issuecomment-1805005732
PR Comment: https://git.openjdk.org/jdk/pull/13797#issuecomment-1876243855
PR Review Comment: https://git.openjdk.org/jdk/pull/13797#discussion_r1185633352
