Bug ID: JDK-8177393 Result of RescaleOp for 4BYTE

JDK 10	JDK 8	JDK 9	Other
10Fixed	8u291Fixed	9 b172Fixed	openjdk8u322Fixed

If you could write a Request For Approval (RFA) mail to jdk8u-dev for JDK-8080287, just so it is clearly on record, I can approve it there. Approving this one.
29-07-2021
Fix request [8u] I would like this backported to 8u for parity with Oracle 8u291. It depends upon JDK-8080287 which is a closed bug, with an open patch: <https://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/8b318cc1e0ba> that looks worthwhile. That patch (JDK-8080287) applies cleanly to 8u after unshuffling. It introduces a test (RescaleAlphaTest.java) which fails with current jdk8u (as expected) and passes after applying the patch. After JDK-8080287, this patch for JDK-8177393 applies cleanly to jdk8u after unshuffling. It introduces a new test (ImageRescaleOpTest.java) which fails for jdk8u now and passes after applying. jtreg results for test/awt/image fails one test for me (bug8017626), same as jdk8u292-b10.
29-07-2021
URL: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/c1ddb97ee2ab User: lana Date: 2017-05-31 20:16:38 +0000
31-05-2017
Fix Request This is a recent regression with obvious wrong results. Fix is confined to RescaleOp. Regression tests have been run and a new test provided.
19-05-2017
URL: http://hg.openjdk.java.net/jdk9/client/jdk/rev/c1ddb97ee2ab User: prr Date: 2017-05-19 21:59:19 +0000
19-05-2017
Looks fine.
19-05-2017
> > Question on update(1) above - shouldn't LookupOp vet its own internal medialib issues? > I did this the way I did since canUseLookupOp() already has several conditions and a couple > of them don't seem to be mandated by the spec. of LookupOp so I assumed them to > be implementation limitations and I am just adding to that. That sounds fine. There is a lot of cleanup that this code needs beyond these bug fixes. > > - Is it safe to hard-code dependencies on 3/4 scales/bands? > > For most of our BufferedImage formats they are 3color+alpha, but what about Rasters? > I think this is OK. For rasters you can specify only 1 or the exact number of bands. > You can't get here if you don't have that as the filter(Raster, Raster) method checks. > In fact I had to add the extra condition at line 508 to account for that. I guess I'm worried that we might have color images with a different number of color components, but unless they are written for alien eyes, I'm not sure that I know how that would work. Still, it feels limiting to hardcode 3,4 into the implementation. The caller originally knew how many color and alpha bands there were and didn't have to guess, but that information doesn't make it down this far so you insert a guess where higher levels actually checked with the data structures. > >- Line 222 (et al) - why test for nBands == 4? Shouldn't it be enough that > > there are uninitialized bands and to initialize all the rest with NOP lookups? > The only case I think I need to address is R+G+B+Alpha .. or B+G+R+Alpha for the I guess I'd be happier if the callers all supplied explicit values for "this is the number of bands to propagate scales to and this is the number of bands beyond that to default to NOP if they exist" so we don't have to assume what the original data might have been. > > - Line 580 - at first I thought that perhaps this line should be > > something like "continue;" because this is a NOP, but perhaps the > You are right .. it is really a no-op. > And you are right that the clamping code has a problem. > I actually mention this in the eval in the bug report and even filed > a separate bug against it do deal with it later. It manifests in 565<->555 > So leaving it as is probably makes more sense. I'm not sure it makes any more sense to do more work, but it is about as benign as everything else in this code. > If I have satisfactorily answered your questions I think the change to SRC composite > and the additonal "&& numBands == 4" seem to be the two important changes. I'm not saying that fixing the above issues is required, but I feel like we are adding half a fix if we add new assumptions just to get this one test case (and related test cases) to pass. I'd be happier if we made the number of bands to propagate scale factors to and the number to just pass through explicit in the call chain, but I'm not going to hold up the fix beyond the SRC && numbands issues.
18-05-2017
> Question on update(1) above - shouldn't LookupOp vet its own internal medialib issues? > You have code in RescaleOp that has intimate knowledge of LookupOp's implementation. > This part should either be handled by LookupOp testing those conditions and not handing > it to the native medialib, or if your intent is that Rescale should use its own non-optimal > path in that case then perhaps this code should be moved to a package-private method > in LookupOp called something like "canOptimizeRescaleOn()" so that we don't have Rescale > knowing about an implementation caveat of Lookup? Yes, I am intending that RescaleOp handle it itself in the case that LookupOp can't handle the case. I was also trying to confine the changes to RescaleOp, although logically a package private method in LookupOp that is called only from RescaleOp amounts to the same thing. I did this the way I did since canUseLookupOp() already has several conditions and a couple of them don't seem to be mandated by the spec. of LookupOp so I assumed them to be implementation limitations and I am just adding to that. If you see something different about the existing checks, then I could make this new check as a package private method on LookupOp as you suggest. I recognise that LookupOp should probably itself be checking the same conditions before calling medialib but that would be just another of many issues that I am uncovering. > - Is it safe to hard-code dependencies on 3/4 scales/bands? For most of our BufferedImage formats they are 3color+alpha, but what about Rasters? I think this is OK. For rasters you can specify only 1 or the exact number of bands. You can't get here if you don't have that as the filter(Raster, Raster) method checks. In fact I had to add the extra condition at line 508 to account for that. >- Line 222 (et al) - why test for nBands == 4? Shouldn't it be enough that > there are uninitialized bands and to initialize all the rest with NOP lookups? The only case I think I need to address is R+G+B+Alpha .. or B+G+R+Alpha for the standard image types. The spec. requires you to supply either 1 or 3 lookup values if you want the 4th alpha channel unscaled. This code is used only when you are filtering BufferedImages. If there is some custom image such as greyscale byte +alpha I suspect you will always need to supply all values explicitly regardless of this fix. > - Line 447 - set SRC composite mode first? Yes. Very probably should. I half thought about composite mode then moved on .. >- Line 508 - if they pass in 2 scale factors for an RGBA image that should be an error, > no (valid numbers are 1, #color, #color+alpha)? The tests are different > for the BImg case, but there are still tests, aren't there? > Or were they already done in the filter(BI, BI) method? Yes filter(BI, BI) tests so I don't need to test again here. > - Line 579 - hardcoding 3 again? I think this is fine .. although it occurs that I should add && numBands == 4, since again I want to deal with precisely that case. > - Line 580 - at first I thought that perhaps this line should be > something like "continue;" because this is a NOP, but perhaps the > intent is to pass through to the dstMax comparison code below? > If the dstMax != srcMax, then shouldn't some scaling be involved to > really be a NOP? So, either dstMax==srcMax and the entire band > operation is a NOP or we have bigger problems here? You are right .. it is really a no-op. And you are right that the clamping code has a problem. I actually mention this in the eval in the bug report and even filed a separate bug against it do deal with it later. It manifests in 565<->555 So leaving it as is probably makes more sense. If I have satisfactorily answered your questions I think the change to SRC composite and the additonal "&& numBands == 4" seem to be the two important changes. But I won't do those until I hear back from you whether you think I really should do the package private method or there is something else in my replies that needs further discussion.
18-05-2017
Question on update(1) above - shouldn't LookupOp vet its own internal medialib issues? You have code in RescaleOp that has intimate knowledge of LookupOp's implementation. This part should either be handled by LookupOp testing those conditions and not handing it to the native medialib, or if your intent is that Rescale should use its own non-optimal path in that case then perhaps this code should be moved to a package-private method in LookupOp called something like "canOptimizeRescaleOn()" so that we don't have Rescale knowing about an implementation caveat of Lookup? About the fix: - Is it safe to hard-code dependencies on 3/4 scales/bands? For most of our BufferedImage formats they are 3color+alpha, but what about Rasters? If someone wants to rescale a 2 or a 7 channel raster with a single scale value, will this code work properly? Should the argument list be adjusted to more accurately represent how many "color" vs "alpha" channels there are so that the code doesn't have to make assumptions about how many bands to copy and how many to replace with NOP tables? - Line 222 (et al) - why test for nBands == 4? Shouldn't it be enough that there are uninitialized bands and to initialize all the rest with NOP lookups? - Lines 328-340 - see comment above about where this code should live - Line 447 - set SRC composite mode first? - Line 508 - if they pass in 2 scale factors for an RGBA image that should be an error, no (valid numbers are 1, #color, #color+alpha)? The tests are different for the BImg case, but there are still tests, aren't there? Or were they already done in the filter(BI, BI) method? - Line 579 - hardcoding 3 again? - Line 580 - at first I thought that perhaps this line should be something like "continue;" because this is a NOP, but perhaps the intent is to pass through to the dstMax comparison code below? If the dstMax != srcMax, then shouldn't some scaling be involved to really be a NOP? So, either dstMax==srcMax and the entire band operation is a NOP or we have bigger problems here?
18-05-2017
Fix on review: http://mail.openjdk.java.net/pipermail/2d-dev/2017-May/008354.html
18-05-2017
This is a problem with applying a RescaleOp image filter to a 4BYTE_ABGR image. The observed bug here is that only the left-most 75% of the destination image is filled. Although this started occurring only in a recent build the change (8130737) that provoked this is innocent and just exposed existing problems. RescaleOp has no direct native implementation so it tries to delegate to LookupOp if certain criteria are met. To do this it uses filter(Raster, Raster) even if the application is calling filter(BufferedImage, BufferedImage). LookupOp does have a native implementation using medialib code but this was not being used until the fix for https://bugs.openjdk.java.net/browse/JDK-8130737 corrected some maths which caused that code to bail and ended up in a fallback Java code path in LookupOp. Now that the code enters medialib for the LookupOp we hit what is a long-standing bug. When re-scaling a BufferedImage the alpha channel (if any) is re-scaled only if the application supplies an array of re-scaling factors which include the alpha channel. Otherwise the alpha channel should not be scaled. Whereas when re-scaling a Raster the API for either RescaleOp or LookupOp will re-scale ALL channels - always. So before delegating to the raster filter the filter(BI, BI) method creates child rasters that exclude the alpha band. Thus when it asks to filter the raster it expects they will not be filtered. However medialib code has no clue how to interpret this Java 2D Raster which is re-using the DataBuffer of the original image. It is ignorant of the SampleModel and that it should ignore every 4th byte (band). Hence the observed bug !! So how to fix it ? First it seems that updating medialib is quite risky as it may affect many code paths. Also it could be a lot of places in medialib that need to be updated. Whilst it is true that this must mean there are other bugs lurking due to this, (and indeed I was able to find one in AffineTransformOp, see JDK-8180502), they are not regressions like this one. A fix that is confined to RescaleOp would be better. The proposed solution at http://cr.openjdk.java.net/~prr/8177393/ is to avoid creating the child raster and instead to create a LookupOp which has an explicit lookup table for the alpha channel that does the identity lookup. The fix has a couple of other updates. (1) the "canUseLookupOp()" method now checks that the DataBuffer is something that the medialib code used by LookupOp can handle. This would have fixed the bug by itself although at the expense of always falling back to slow Java code in RescaleOp. However it is still needed in case the application itself supplies a child raster. (2) The slow path in RescaleOp turns out to have the same bug that I think was seen in some other bug report or fix. It is always re-scaling alpha as well as colour components. I fixed that too. A couple of things I have NOT addressed (a) IllegalArgumentException for different sizes is not documented Filed as https://bugs.openjdk.java.net/browse/JDK-8180501. This would be a doc. only change that needs a CCC and can be handled separately. (b) In the case of 565->555 or 555->565 filtering there is no re-mapping of the case where the band has a different number of bits, so the colour of that band is incorrect. This is not a regression and is probably not a common case and isn't related to the specific issue so can be handled later. Filed as https://bugs.openjdk.java.net/browse/JDK-8180503 The supplied regression test covers various src/dst cases including all the important ones. Some performance testing showed that each of these cases where SRC and DST are compatible are roughly the same as the last build that worked correctly. Some times a bit (or a lot) better, a few marginally worse.
17-05-2017
The proposed fix: http://cr.openjdk.java.net/~alexsch/8177393/webrev.00
10-04-2017
I roughly see what is going on but I have to think a bit more about who to blame .. I believe this is likely specific to images with an alpha channel, and perhaps just the 4BYTE case but perhaps INT too (needs testing) There was an update a while ago to java/awt/image/RescaleOp.java user: psadhukhan date: Thu Aug 06 11:36:52 2015 +0300 summary: 8080287: The image of BufferedImage.TYPE_INT_ARGB and BufferedImage .TYPE_INT_ARGB_PRE is blank What this does is avoid rescaling the alpha channel by creating a new child raster with only 3 bands instead of 4 - ie just the colour components. That is what is then filtered. Prior to the fix 8170737 we were miscalculating and failing to enter medialib. Now that's fixed and we do. The media lib code in mlib_c_ImageLookUp_U8_U8 then sees that it has just 3 bands .. else if (csize == 3) { .. and does the following calculation : mlib_s32 size = xsize * 3; .. So it looks to me as if it misses the last 1/4 of the image because it expects that the number of bands can be used to infer the pixel stride. Maybe we should never be entering the medialib routines in such a case if they aren't able to navigate the Raster when the whole DataBuffer isn't to be considered. One quick way to fix this might be for RescaleOp to not try to "optimise" creating a writeable child with fewer bands. Just use the correct number. There'd be 33% extra redundant work to be discarded but I think the results would be correct.
31-03-2017
Yes .. I worked that one out that yesterday, but I am not yet sure if it is not a problem with that fix. It may be that the fix is enabling a different code path and then hitting a bug in a medialib function.
31-03-2017
Regression from the fix for the issue JDK-8130737
31-03-2017
Automated test case to reproduce the issue: {code} import java.awt.Color; import java.awt.Graphics; import java.awt.image.BufferedImage; import java.awt.image.RescaleOp; public class ImageRescaleOpTest { public static void main(String[] args) throws Exception { int w = 100; int h = 100; BufferedImage baseImage = new BufferedImage(w, h, BufferedImage.TYPE_4BYTE_ABGR); Graphics g = baseImage.getGraphics(); g.setColor(Color.RED); g.fillRect(0, 0, w, h); g.dispose(); RescaleOp res = new RescaleOp(1f, 1f, null); BufferedImage filteredImage = res.filter(baseImage, null); int x = w - 5; int y = h / 2; if (filteredImage.getRGB(x, y) == Color.BLACK.getRGB()) { throw new RuntimeException("Wrong color!"); } } } {code}
31-03-2017
The issue is reproduced in JDK 9ea b153 and is not reproduced in JDK 9ea b152
30-03-2017
Simple test case to reproduce the issue. - Put the class SimpleImageRescaleOpTest and trees.jpg image in the same folder. - Compile and run the SimpleImageRescaleOpTest class - See the saved image-filtered.png image ---------------- import java.awt.Graphics; import java.awt.image.BufferedImage; import java.awt.image.RescaleOp; import java.io.File; import javax.imageio.ImageIO; public class SimpleImageRescaleOpTest { public static void main(String[] args) throws Exception { String imageName = "trees.jpg"; BufferedImage img = ImageIO.read(SimpleImageRescaleOpTest.class.getResource(imageName)); int w = img.getWidth(null); int h = img.getHeight(null); BufferedImage baseImage = new BufferedImage(w, h, BufferedImage.TYPE_4BYTE_ABGR); Graphics g = baseImage.getGraphics(); g.drawImage(img, 0, 0, w, h, null); g.dispose(); RescaleOp res = new RescaleOp(0.7f, 0.7f, null); BufferedImage filteredImage = res.filter(baseImage, null); ImageIO.write(baseImage, "png", new File("image-base.png")); ImageIO.write(filteredImage, "png", new File("image-filtered.png")); } } ----------------
30-03-2017
This is a recent regression - somewhere around b150 and affects Oracle JDK. Looks like a must fix for 9 to me.
27-03-2017
Having reviewed the changes in those builds my first guess is this B150 fix :- https://bugs.openjdk.java.net/browse/JDK-8162350
22-03-2017
Confirmed: for these formats the right side of the image is clipped and black. It is not OpenJDK issue: a promoted b160 behave exactly the same. Moreover, b155 does the same, too -- but not b148, so the change is somewhere in b150 or later. I tried on machines with ATI cards, OEL7 and Ubuntu 14.04: reproduced always.
22-03-2017
It has the same issue on win10 x64 with openJDK 9 b160. RULE "2D_Image/ImageRescaleOpTest" ExitCode -1
17-03-2017
RULE "2D_Image/ImageRescaleOpTest" ExitCode 255
17-03-2017

Duplicate :	JDK-8257503 - java.awt.image.RescaleOp produces incorrect results
Relates :	JDK-8130737 - [macosx] AffineTransformOp can't handle child raster with non-zero x-offset