JDK-8209457 : [WebView] Canvas.toDataURL with image/jpeg MIME type fails
  • Type: Bug
  • Component: javafx
  • Sub-Component: web
  • Affected Version: openjfx11
  • Priority: P2
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2018-08-14
  • Updated: 2020-01-31
  • Resolved: 2018-08-27
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 8 Other
8u202Fixed openjfx11.0.3Fixed
Related Reports
Relates :  
Relates :  
Description
Load the following snippet into HelloWebView, it must render |green|red| rectangle.

https://jsbin.com/raqizibeku/edit?html,output

<body>
<img id="canvas-to-data"/>
<script>
canvas = document.createElement('canvas');
canvas.width=100;
canvas.height=100;
var ctx = canvas.getContext('2d');
ctx.fillStyle = 'green';
ctx.fillRect(0, 0, 50, 100);
ctx.fillStyle = 'red';
ctx.fillRect(50, 0, 50, 100);
data = canvas.toDataURL("image/jpeg");
document.getElementById('canvas-to-data').src = data;
</script>
</body>

Comments
lgtm +1
31-08-2018

[~kcr] [~psadhukhan], Please review the following 8-bp patch, http://cr.openjdk.java.net/~arajkumar/8209457/8-bp/webrev I ran into few conflicts while merging the changeset from jfx-dev, so requesting you to review. Changeset of JDK-8202277 applies directly with minimal test package path adjustment.
30-08-2018

Changeset: f4f1e91dbf87 Author: arajkumar Date: 2018-08-27 11:42 +0530 URL: http://hg.openjdk.java.net/openjfx/jfx-dev/rt/rev/f4f1e91dbf87 8209457: [WebView] Canvas.toDataURL with image/jpeg MIME type fails Reviewed-by: kcr, psadhukhan
27-08-2018

Thanks [~psadhukhan]. >> Is getImage() guranteed to return non-null object, if not, there is a possibility of NPE in PrismImage line final int iw = (int) img.getWidth(); It should be valid unless system is running out of memory. >> 216,229 assertTrue("Color should be opque red: >> should be opaque Will fix it. >>Is there any need of 1% tolerance for all these color comparison in testToDataURLWithPNGMimeType() and testToDataURLWithJPEGMimeType, can't it be 0? Since it involves lossy compression formats, it is better use tolerance while comparing pixel values.
27-08-2018

Is getImage() guranteed to return non-null object, if not, there is a possibility of NPE in PrismImage line final int iw = (int) img.getWidth(); otherwise it's fine. One minor nit in test typo 216,229 assertTrue("Color should be opque red: should be opaque Also, Is there any need of 1% tolerance for all these color comparison in testToDataURLWithPNGMimeType() and testToDataURLWithJPEGMimeType, can't it be 0? assertTrue("Color should be opque red:" + pixelAt25x25, isColorsSimilar(Color.RED, pixelAt25x25, 1)); no need of new webrev for this if you wish to change. +1
27-08-2018

This should be backported to 8u after JDK-8202277 is backported.
24-08-2018

Looks good. +1
24-08-2018

[~kcr] Thanks for your review comments. [~kcr]: >>1. I see you also changed the check for 'image/jpeg' mimeType from a ���contains��� test to an ���equals��� test. Are there cases where there might be extra characters in the mime type? If not, this seems a good change. [~arajkumar]: mime type string is normalised by WebCore, I used "contains" accidentally, so fixed it in webrev.03. Addressed the review comments in http://cr.openjdk.java.net/~arajkumar/8209457/webrev.04
24-08-2018

1. I see you also changed the check for 'image/jpeg' mimeType from a ���contains��� test to an ���equals��� test. Are there cases where there might be extra characters in the mime type? If not, this seems a good change. 2. Some of the comments for the newly added assertion checks in testColorSimilarityAlgorithm don���t match the code. Also, ���equal to��� and ���at least��� should be two words. 204 assertFalse("(0, 0, 0, 0) must only 95% equalto (5, 5, 5, 5)", isColorsSimilar(new Color(0, true), new Color(5, 5, 5, 5), 0.05f)); The check uses 0.05% which corresponds to 99.95% not 95%. Also, might be better worded as ���different by at least ...��� as is used elsewhere. 206 assertTrue("True Red and Green must be different by atleast 75%", isColorsSimilar(Color.RED, Color.GREEN, 75)); that should be ���at least 25% equal to��� since this is an assertTrue. 207 assertFalse("True Red and Green must be different by atleast 75%", isColorsSimilar(Color.RED, Color.GREEN, 70)); That should be 70%.
24-08-2018

Thanks [~kcr] for reviewing the fix. Addressed review comments in http://cr.openjdk.java.net/~arajkumar/8209457/webrev.03 Please take a look.
24-08-2018

PrismImage.java 1. In the toData method, since toBufferedImage can return null, it might be best to replace the instanceof check with a null check rather than removing it as you have done. 2. In the fromFXImage and toBufferedImage method, maybe 'forceRGB' would be a more descriptive name than 'knownAsRGB', since it control an action taken by those methods? CanvasTest.java 3. In the unit test, you might want to add one or two more negative tests for testColorSimilarityAlgorithm that are just outside the tolerance (e.g., verify that isColorsSimilar returns false if two colors are 90% similar and the tolerance is 0.05).
23-08-2018

I ran ' HelloWebView https://jsbin.com/raqizibeku/edit?html,output ' and I observe the following: 1. Oracle JDK 10 + bundled javafx modules : the rectangles display but have the wrong color (gray and blue) 2. OpenJDK 10 + openjfx 11 : nothing is drawn 3. Oracle JDK 11-ea+17 + openjfx 11 : same as #1 (gray and blue) 4. Open JDK 11-ea+17 + openjfx 11 : nothing is drawn 5. JDK 11-ea+18 or later (either Oracle JDK or OpenJDK) + openjfx 11 : nothing is drawn With your patch, it looks correct running with any of the above (except I didn't build it such it could run with Oracle JDK 10). Given that it wasn't correct even for JDK 10, plus how late we are, I don't want to consider this for openjfx 11. We can get it into openjfx 12. I'll finish the review shortly.
23-08-2018

Thanks [~psadhukhan] for review the fix. >>In PasteBoardImpl, Shouldn't img.isNull() check be done before you access img.getPlatformImage() as if width,height is 0, there's no point getting platformImage, if I get it right. Though ImageAccessor.fromPlatformImage takes care of null, it makes sense to check for null even before that. Will do it like below. final Image fxImage = img != null && !img.isNull() ? Toolkit.getImageAccessor().fromPlatformImage(img.getPlatformImage()) : null; >>Will adding toBufferedImage() in PrismImage not cause any compatibility issue? Maybe add "default" keyword to it!!! As discussed, it is not a public API meant for external usage, it is a private implementation meant only for WebKit usage. >> What was the need of deleting testImageToDataURL() test from the testfile? Test was ignored and the new test added as part of this fix covers the scenario.
23-08-2018

In PasteBoardImpl, Shouldn't img.isNull() check be done before you access img.getPlatformImage() as if width,height is 0, there's no point getting platformImage, if I get it right. Will adding toBufferedImage() in PrismImage not cause any compatibility issue? Maybe add "default" keyword to it!!! What was the need of deleting testImageToDataURL() test from the testfile?
23-08-2018

Addressed [~psadhukhan]'s review comments in http://cr.openjdk.java.net/~arajkumar/8209457/webrev.02 In addition to that, added a small Color comparison algorithm by taking reference from WebKit's ImageDiff[1]. It simplifies the color comparison in unit tests. [1] https://trac.webkit.org/browser/webkit/trunk/Tools/ImageDiff/PlatformImage.cpp
23-08-2018

It is good that after JDK-8204187 `ImageIO.write' is failing instead of doing a buggy encoding.
22-08-2018

I see that JDK-11+17 is working & JDK-11+18 is failing. Between those builds, I could see JDK-8204187 is the only changeset[1] which affects ImageIO. Also this problem is not just affects javafx.web, I can also reproduce the problem when using SwingFXUtils.fromFXImage, Image image = new Image(getClass().getResourceAsStream("some.png")); BufferedImage buffImage = SwingFXUtils.fromFXImage(image, null); ImageIO.write(buffImage, "jpeg", new java.io.File("/tmp/hello.jpg"); // returns false on jdk-11+18 build [1] http://hg.openjdk.java.net/jdk/jdk/rev/580159eeac07
22-08-2018

Oh, so this was a regression caused by a change in the Java2D JPEG code, and not by the refactoring done in FX for JDK-8202277, which is what I had initially assumed. Thanks for the info.
22-08-2018

The new approach looks much cleaner. I'll do a more detailed review later.
20-08-2018

Did the following changes in the new webrev.01: http://cr.openjdk.java.net/~arajkumar/8209457/webrev.01 - Moved the responsibility of converting "WCImage to BufferedImage" to PrismImage (UIClientImpl is not a right place) - Updated copyrights year
20-08-2018

I think we should consider getting this into openjfx 11 since it is a serious regression introduced in 11.
18-08-2018

[~psadhukhan] Can you also review this?
18-08-2018

Root cause: BufferedImage is created with ARGB color model while converting from PrismImage to JPEG. JPEG doesn't supports alpha channel, which leads to error while encoding to JPEG image. Proposed fix: HTMl applications passes the destination image mime type, make use of it and hardwire the color model as RGB when the mime type is 'image/jpeg'. webrev: http://cr.openjdk.java.net/~arajkumar/8209457/webrev/ (Also removed dead code present in UIClientImpl.fromFXImage)
16-08-2018

>> 1. How common is this case? It is to take the snapshot of a HTML 5 canvas. I don't think it is usage is vast, otherwise we would have got bugs because of it's incorrect jpeg conversion(see 8u-incorrect-toData.png). >> 2. Is there a workaround? Workaround is to use 'image/png' instead of 'jpeg'.
14-08-2018

Two questions: 1. How common is this case? 2. Is there a workaround?
14-08-2018