• Type: Bug
• Component: javafx
• Sub-Component: graphics
• Affected Version: fx2.1

• Priority: P3
• Status: Resolved
• Resolution: Fixed

• Submitted: 2012-03-13
• Updated: 2015-06-12
• Resolved: 2014-06-14

JDK 8
8u20Fixed

Changeset: 571a17178eaa Author: vadim Date: 2014-06-15 00:02 +0400 URL: http://hg.openjdk.java.net/openjfx/8u-dev/rt/rev/571a17178eaa 14-06-2014 That looks great. I like how you used the more formal "floor()" instead of a cast to int in the test to avoid any errors in our assumption about the cast being sufficient. Approved. 14-06-2014 Ok, I hope this is the final version - greatly simplified test, using nearest neighbor algorithm to sample original image and comparing with the scaled image. http://cr.openjdk.java.net/~vadim/RT-20295/webrev.02/ 13-06-2014 Yes, that was what I was getting at - embed what we think the "proper" algorithm is into the test code. It sounds like you are already on the solution... 12-06-2014 Yes, I agree that depending on the JDK implementation is not very reliable, but the only other option would be generating a bunch of images. Actually I think that I can modify the test so it will not use JDK scaled image at all but will sample original pixels instead. That way we can even drop 3x3 sampling at all since we know the exact algorithm for scaling. 12-06-2014 That sounds like a reasonable solution. As a general practice, I wonder about comparing our results to "the scaling results of another implementation", though. It makes our tests dependent on the stability of the specification and implementation of the other platform. Just as a loose thought - the INTERPOLATION hint is actually just a hint even though we strongly suspect that it will never be ignored by the JDK code... 12-06-2014 The test renders BufferedImage with nearest neighbor hint and compares the result of the Image loading with it. If the pixels do not match, it calculates the source pixel position in the original image and tries 3x3 area so the possible one-off pixels are still detected as correct. Otherwise the test is just a framework for converting BufferedImage to ByteInputStream, and so on I've tested that the test fails without the fix (separately with destLine calculations and colPositions) and that it passes with the fix. Oh, I forgot to rsync new webrev, I refactored the test some more, it's probably a little easier to read now. 12-06-2014 The new code in the Loader looks good. I didn't review all of the test code in depth. Was there something there that you thought might be tricky? 11-06-2014 Jim, please review the next iteration: http://cr.openjdk.java.net/~vadim/RT-20295/webrev.01/ I fixed colPositions and destLine calculations, but didn't change ImageTools, I think it should probably better be fixed separately? Also I refactored BMPImageLoaderTest and pulled some methods to the common ImageTestHelper class. 11-06-2014 With respect to the number of double calculations for small images, we only have 1 per destW and less than 1 per (srcH + dstH) so it isn't too bad. I typically use fixed long math for cases like this if everything is being done per pixel (such as rotates, or when scales are done without pre-calculating colPositions as is done here). Also consider that this is only done on image loading, not a render-time operation... 05-06-2014 colPositions is also calculated inaccurately, but it has protections for "out of range" data. The following should work fine: for (int i = 0; i < destWidth; i++) { colPositions[i] = (int) ((i + 0.5) * scaleX); } The "pos < 0" case should never have been happening. I can't imagine any combinations of srcWidth and dstWidth that would make the last entry, which is essentially ((destWidth - 0.5) * srcW / dstW) ever end up being >= srcW. I suppose it could happen for extreme values of srcW or dstW, but they would have to exceed the resolution of a double which has a mantissa large enough to hold numbers much larger than an integer without loss of precision and the values are supplied as integers. Note that we only ever use colPositions as "colPositions[i]*numBands" so it couldn't hurt to multiply by numBands up front... The calculation in putSourceScanLine can be: while ((int) ((++destLine + 0.5) * scaleY) == sourceLine) {...} That should probably be enough to fix the bug since once destLine == destHeight then we have ((destH + 0.5) * srcH / destH) which should always be > srcH and so it can't be "== sourceLine" even after casting to an int if we know that "sourceLine < sourceHeight". 05-06-2014 In this code in ImageTools.java: if (preserveAspectRatio) { // compute the final dimensions if (finalWidth == 0) { finalWidth = (int) ((float) sourceWidth * finalHeight / sourceHeight); } else if (finalHeight == 0) { finalHeight = (int) ((float) sourceHeight * finalWidth / sourceWidth); } else { float scale = Math.min((float) finalWidth / sourceWidth, (float) finalHeight / sourceHeight); finalWidth = (int) (sourceWidth * scale); finalHeight = (int) (sourceHeight * scale); } I would change those to round in all cases rather than cast to int. We don't want XX.99 to end up with an image of size XX and I'm pretty sure that for some non-power-of-2 image size ratios we will end up falling just short of an integer size and end up making a result image that seems "1 short" of what the user wanted. 05-06-2014 I thought we were talking about determining the size of the destination via (int)(X/Y*Y), but now that I look at it, the constructors give you the new size, not a scale factor so there isn't any "calculation there" (unless it is a floating point "requested size" in which case rounding would probably be appropriate). I guess you are talking about picking which pixel to use from the source. Then the proper way to do nearest neighbor is: floor((DstPixelIndex + 0.5) * newDimension) / oldDimension) "DstPixelIndex + 0.5" is the center of the destination pixel. We then map that back into the source image with "* newD / oldD" which is obviously a scaling transform. Then you need to determine which of the source "pixel centers" it is nearest. If it ends up XXX.5 then it is exactly on a source pixel center and you choose that pixel. If it ends up with XXX.0 then it is on the edge between two pixels and we typically use a "left/top edge inclusive" approach to breaking that tie. floor() handles that calculation. If we know that all of the numbers are non-negative, then a cast to (int) is appropriate. For cases where we might have a negative sample (sampling all of the pixels in the bounding box of a rotated image can do this for instance) then the cast doesn't work because negative numbers round-up. In the case of resizing an image where all dimensions are non-negative and all DstPixelIndex values are non-negative then a cast is fine. 05-06-2014 Jim, I did a little research on how this algorithm works in various programs, and found out that from 4 different tools I use (MS Paint, Paint.NET, FSViewer, ImageMagick) all 4 produce different results. Current FX implementation works as the Paint.NET works, JDK implementation works as MS Paint, and FSViewer with ImageMagick produce another 2 different results. Actual results are basically indistinguishable from each other. I resized 100x8 image to the 100x10 image so there are some corner cases there. Due to the nature of algorithm there always be cases where we will have equidistant pixels and will need to decide which one to pick. Current JDK implementation (share/native/sun/java2d/loops/ScaledBlit.c) is quite sophisticated and I'm not sure I want to port it to the FX. Do you have a preference which one to choose? I would actually leave it as it is now. 04-06-2014 If we are using casts then we exacerbate these inaccuracies - that is why we should be using rounding instead of floor() or casts to ints. The example you give shows a cast which will be off by a whole pixel if the rounding error accumulates 1 bit less than a perfect 1.0. That should be fixed, though it can't hurt to protect the loop as well, but I don't think it is fixing the underlying cause as much as adding protection for bad math and then letting the math remain bad... 04-06-2014 Moreover, simple rounding instead of the cast will produce incorrect results in many cases. I think more correct condition here would be Math.round((++destLine + 0.5) * scaleY - 0.5) so we will actually compare the distance between centers of pixels. In this case we are closer to the JDK implementation and in 50% of the cases the result is identical. But I'm thinking maybe it's too much of double precision calculations for the simplest resizing (which should be as fast as possible). 04-06-2014 Jim, Please review the fix: http://cr.openjdk.java.net/~vadim/RT-20295/webrev.00/ The root cause of this bug is floating point arithmetic where (int)((62.0 / 78.0) * 78) != 62 While resizing the image using nearest neighbor algorithm, with specific height values like in this bug we will try to fill one more scan line at the end of the image. So the proposed fix is to explicitly check that we don't write more than needed. This happens no only in JPG images, but in GIF and PNG as well. 03-06-2014 Release note for 2.2: A JPEG image will sometimes fail to load if the image is scaled with a non-zero width or height in the Image constructor. Workaround: construct the Image with the actual size of the JPEG image (by using the default width and height of 0), and instead scale the ImageView by using the fitWidth and fitHeight properties. For example, to load a JPEG image and scale it to 300x300 you could use: Image im = new Image(jpegURL); ImageView iv = new ImageView(im); iv.setFitWidth(300); iv.setFitHeight(300); 17-07-2012 SQE: OK 04-07-2012 I can now reproduce the bug with the following image: Image image = new Image("http://images.printeria.de/appdata/lib_layouttns/CP_10022001_101_v300.jpg", 215, 130, true, false); Note that if you don't scale the image in the Image constructor, that is, if you don't specify width and height, or specify them as (0, 0), it will load fine. This is a similar bug to RT-19662 although in the JPEG loader rather than the GIF loader. 23-03-2012 Exception is thrown: java.lang.ArrayIndexOutOfBoundsException at com.sun.javafx.iio.common.RoughScaler.putSourceScanline(RoughScaler.java:114) at com.sun.javafx.iio.jpeg.JPEGImageLoader.load(JPEGImageLoader.java:258) at com.sun.javafx.iio.ImageStorage.loadAll(ImageStorage.java:294) at com.sun.javafx.iio.ImageStorage.loadAll(ImageStorage.java:271) at com.sun.javafx.tk.quantum.PrismImageLoader2.loadAll(PrismImageLoader2.java:89) at com.sun.javafx.tk.quantum.PrismImageLoader2.<init>(PrismImageLoader2.java:34) at com.sun.javafx.tk.quantum.QuantumToolkit.loadImage(QuantumToolkit.java:599) at javafx.scene.image.Image.loadImage(Image.java:866) at javafx.scene.image.Image.initialize(Image.java:664) at javafx.scene.image.Image.<init>(Image.java:542) at com.infowerk.fx.service.LoadImageService$LoadImageTask.call(LoadImageService.java:39) at com.infowerk.fx.service.LoadImageService$LoadImageTask.call(LoadImageService.java:1) at javafx.concurrent.Task$TaskCallable.call(Task.java:1229) at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:334) at java.util.concurrent.FutureTask.run(FutureTask.java:166) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1110) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:603) at java.lang.Thread.run(Thread.java:722) 23-03-2012 The image attached to this bug report loads and displays fine for me. Can you verify that the URL you are passing to the image constructor your program is correct? If so, then please attach the failing image to this bug report. Another thing you can do to get more diagnostic information is to run the program with "-Dprism.verbose=true". 23-03-2012 Please check the other posted problem while loading jpeg image (low res image, without background loading). This should work. 23-03-2012 It is by design that an error or exception that occurs during the loading of an image will not throw or print the exception. The "isError()" method is used to report an error. You could file a new feature request to be able to get the exception or error in cases where an exception occurs. perhaps a "Throwable getCause()" method that returns a non-null value in case the error was due to an exception or error. 23-03-2012 Increasing heap, helped me loading multiple images at same time. Exception/Error (OutOfMemory) should be thrown in my opinion. Unfortunately there is another problem with loading simple jpeg. Image image = new Image("http://images.printeria.de/appdata/lib_layouttns/CP_10022001_101_v300.jpg", 215, 130, true, false); System.out.println(image.isError()); -->true Image image = new Image("http://images.printeria.de/appdata/lib_layouttns/CP_10022001_101_v300.jpg", 215, 0, true, false); System.out.println(image.isError()); -->true Image image = new Image("http://images.printeria.de/appdata/lib_layouttns/CP_10022001_101_v300.jpg", 215, 130, false, false); System.out.println(image.isError()); -->false Image image = new Image("http://images.printeria.de/appdata/lib_layouttns/CP_10022001_101_v300.jpg", 215, 0, false, false); System.out.println(image.isError()); -->false Image image = new Image(new File("d:\\CP_10022001_101_v300.jpg").toURI().toString(), 215, 130, true, false); System.out.println(image.isError()); -->true 23-03-2012 I filed RT-20511 to track the request for reducing the memory requirements of the JPEG loader. 21-03-2012 Given the analysis by Lubo, I don't think there is a bug in the platform. You can either increase the memory you are allocating to Java Web Start, or use a Task (or your own background thread) to load the images one at a time, without specifying background image loading. Separately, you may wish file a JIRA to request that we optimize the image loading, but this is not likely to be a high priority request. 20-03-2012 From my investigation it seems that the whole original image is loaded into memory first (5696x4272x3 = approx. 70MB) and then down-scaled. Having several such images being loaded at once exhausts the java heap quickly and results in OOM. Graphics team should check whether memory requirements could be reduced in this case (downscale during loading?). 20-03-2012 Try to load multiple Images at background. This works fine within eclipse, but not when i run the application as webstart application. With this sample code the image is loaded only two times. In my app i am using of cource multiple different images. for (int i = 0; i < 10; i++) { final Image testImage = new Image("file:///c:/work/temp/pics/test/DSCF3347.JPG", 150, 0, true, false, true); if (testImage.isBackgroundLoading()) { testImage.progressProperty().addListener(new ChangeListener<Number>() { @Override public void changed(ObservableValue<? extends Number> observable, Number oldValue, Number newValue) { if (newValue.floatValue() == 1f) { System.out.println("error while loading image"); } } }); } } 20-03-2012 This is not a valid URL. Try: new Image("file:///d:/tmb_CA_0021.jpg", 215, 0, true, false); 13-03-2012