On Fri, 5 May 2023 01:10:18 GMT, Sergey Bylokhov <[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 ||...
>
> 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?
I do not know why `WritableRaster.setRect(Raster)` computes intersection with
its own code in the method body instead of delegating to
`Rectangle.intersection(Rectangle)` in the same way as
`BufferedImage.setData(Raster)` does. For now I see no technical reason. I'm
tempted to add a commit to this pull request for replacing the intersection
calculation of `WritableRaster.setRect(Raster)`, but abstained for now because
I though that reviewers or sponsors may want to minimize the changes. However I
believe that this replacement would be desirable at least for the extra
robustness of `Rectangle` regarding underflow and overflow.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13797#discussion_r1185856477
