Issue metadata
Sign in to add a comment
|
ctx.drawImage not rendering images properly when some render calls are off the canvas
Reported by
ken...@yttromobile.com,
Mar 23 2016
|
||||||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.87 Safari/537.36 Example URL: http://h5.yttromobile.com Steps to reproduce the problem: The above URL is our production site. Please see simplified test case (attached file). In the test file, I am creating a large canvas and drawing images tiles into the canvas. Only some of the images are rendered. Each image is 504x504. There are 5 images in each direction. The canvas is 2520x2520. What is the expected behavior? All the images should be rendered. The screen should be filled with the canvas. This works properly in Safari. What went wrong? Only some images are rendered in the canvas. Does it occur on multiple sites: N/A Is it a problem with a plugin? N/A Did this work before? Yes Not sure which earlier version worked Does this work in other browsers? Yes Chrome version: 49.0.2623.87 Channel: stable OS Version: OS X 10.10.5 Flash Version: Shockwave Flash 21.0 r0
,
Mar 23 2016
,
Mar 23 2016
Reproducible on linux if GPU-accelerated canvas is enabled by starting Chrome with --ignore-gpu-blacklist What I am observing on linux is none of the tile are rendered!
,
Mar 26 2016
xidachen@ sent me some CL suggesting that YUV JPEG decoding is at fault. But, YUV JPEG decoding (Ganesh) is not enabled on desktop chrome, and this bug was reported for Stable 49 Chrome Mac desktop. Anyway, fist we should have a test case that checks these images can be decoded at all. Test case attached. kenson@ could you load that image decoding test case and let us know if you see problems. Also, if you have experimental features enabled in Chrome, please turn them off (on the chrome://flags page, click the "reset all to default" button).
,
Mar 26 2016
noel@ thanks for the img node test case! I reset the chrome://flags as instructed and your img node test shows all the images correctly but the canvas based rendering does not.
,
Mar 27 2016
The image test case in #4 works for me, with default hrome flags: all images are shown. However, enabling --force-gpu-rasterization (Ganesh) via chrome://flags causes the bug to manifest, images are missing (picture attached).
,
Mar 27 2016
I downloaded the missing images and inspected them. They are all the same type: they are all _Progressive JPEG_ images, examples attached.
,
Mar 27 2016
,
Mar 27 2016
I instrumented the code and changed the test case in #4 to draw one of the progressive JPEG images given in #7. Progressive JPEG Image draw, --force-gpu-rasteriztion (Ganesh), DecodingImageGenerator at address 412503C0, which contains an ImageFrameGenerator at address 4139C820. Image Drawing steps (annotated). 1) A SkImage preroll calls tryLockAsBitmap() and the SkImageCacherator calls getPixels() on the SkImage as a result, which decodes the image 412503C0 DecodingImageGenerator::onGetPixels 4139C820 ImageFrameGenerator::decodeAndScale m_decodeFailed false 4139C820 ImageFrameGenerator::tryToResumeDecode 4139C820 ImageFrameGenerator::decode onGetPixels success 2) That RGBA image decode succeeds. Ganesh then requests YUV decoding of the same image ... 412503C0 DecodingImageGenerator::onRefEncodedData 412503C0 DecodingImageGenerator::onQueryYUV8 m_canYUVDecode true 4139C820 ImageFrameGenerator::getYUVComponentSizes getYUVComponentSizes: allDataReceived true updateYUVComponentSizes decoder->canDecodeToYUV success onQueryYUV8 result: success 2a) and the image say it can be YUV decoded, so Genesh tries that ... 412503C0 DecodingImageGenerator::onGetYUV8Planes m_canYUVDecode true 4139C820 ImageFrameGenerator::decodeToYUV m_decodeFailed false onGetYUV8Planes decodeToYUV failed 3) but the image fails to YUV decode because it is a Progressive JPEG image and the decode is marked failed. 4) Ganesh falls back on this failure to try using the preroll RGBA decoded image from step 1). Result: the decoded RGBA image is not drawn, due to 3) marking the decode failed, and an empty box appears on screen in the image test case. Probable fix: do not mark decoding failed at step 3) to allow Ganesh to fall back to use the preroll RGBA image.
,
Mar 27 2016
The canvas test in #10 works for me, with default chrome flags: all images are shown. When I enable --force-gpu-rasterization (Ganesh) via chrome://flags then the bug manifests, some <canvas> images are missing (picture attached). And once again, the missing images are all _Progressive JPEG_ images.
,
Mar 27 2016
Again, I instrumented the code and changed the test case in #10 to draw one of the progressive JPEG images given in #7 to the <canvas>. Progressive JPEG Image draw, --force-gpu-rasteriztion (Ganesh), DecodingImageGenerator at address 412503C0, which contains an ImageFrameGenerator at address 4139C820. <canvas> Image Drawing steps (annotated, note the <canvas> image draw flow is not the same as <img> image draw flow). 1) Ganesh requests YUV decoding of the image ... 412503C0 DecodingImageGenerator::onRefEncodedData 412503C0 DecodingImageGenerator::onQueryYUV8 m_canYUVDecode true 4139C820 ImageFrameGenerator::getYUVComponentSizes getYUVComponentSizes: allDataReceived true updateYUVComponentSizes decoder->canDecodeToYUV success onQueryYUV8 result: success 1a) and the image says it can be YUV decoded, so Genesh tries that ... 412503C0 DecodingImageGenerator::onGetYUV8Planes m_canYUVDecode true 4139C820 ImageFrameGenerator::decodeToYUV m_decodeFailed false onGetYUV8Planes decodeToYUV failed 2) but the image fails to YUV decode because it is a Progressive JPEG image, and the decode is marked failed. 412503C0 DecodingImageGenerator::onGetPixels 4139C820 ImageFrameGenerator::decodeAndScale m_decodeFailed false 4139C820 ImageFrameGenerator::tryToResumeDecode 4139C820 ImageFrameGenerator::decode onGetPixels success 3) Ganesh falls back on this failure and tries to RGBA decode the image (with success). Result: the decoded RGBA image is not drawn, due to 2) marking the decode failed, and an empty box appears on screen in the <canvas> test case. Probable fix: do not mark decoding failed at step 2) to allow Ganesh to fall back to use the decoded RGBA image.
,
Mar 27 2016
Where was the failure detected that caused the empty image to be drawn? In my debugging, line 86 in DeferredImageDecoder::createFrameAtIndex() https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp&sq=package:chromium&type=cs&l=84&q=DeferredImageDecoder.cpp%20createFrameAtIndex 86: if (m_frameGenerator && m_frameGenerator->decodeFailed()) 87: return nullptr;
,
Mar 27 2016
noel@: Yes, my debug also showed that it is return nullptr at line 86. At the reason is that m_frameGenerator->decodeFailed() is true. Question: if we don't mark decoding failed, then the image will show up correctly, does that mean it won't use GPU in this case?
,
Mar 27 2016
xidachen@ thanks for the line 86 confirm. Re your question: yes the image will show correctly if we don't mark it failed. And yes that means the GPU won't be used, but that is fine in this case (since we can't YUV decode Progressive JPEG on the GPU). The fix you proposed in https://codereview.chromium.org/1829923004/#ps1 (don't mark the decode failed) works. I tested it with the test cases in #4 and #10, and --force-gpu-rasterization ... no more missing images. Another fix would be to add ImageFrameGenerator::m_yuvDecodingFailed, default false, and in ImageFrameGenerator::decodeToYUV(), set it true if YUV decoding fails therein: 250 ASSERT(decoder->failed()); 251 m_yuvDecodeFailed = true; 252 return false; ImageFrameGenerator::getYUVComponentSizes() could also test m_yuvDecodeFailed to deny future requests for a YUV decode (since we know they won't work). Let me know how you'd like to proceed.
,
Mar 27 2016
noel@: Thank you for the detailed guidance. I will add the m_yuvDecodingFailed in the same patch.
,
Mar 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0e4b0bc92e29939d10ff76a3540b4c2b3b5549b6 commit 0e4b0bc92e29939d10ff76a3540b4c2b3b5549b6 Author: xidachen <xidachen@chromium.org> Date: Sun Mar 27 22:29:23 2016 Do not set m_decodeFailed in ImageFrameGenerator::decodeToYUV This method sets m_decodeFailed bit to be true at the end on VUY decode failure, which can result a null SkImage in DeferredImageDecoder::createFrameAtIndex() when Ganesh is enabled and the image is Progressive JPEG. Ganesh falls back to displaying an RGBA decoded image on decode failure but the null SkImage was shown instead (see bug examples). When decodeToYUV() fails, do not set m_decodeFailed. Instead set m_yuvDecodingFailed to allow Ganesh fallback logic (that decodes the image to RGBA on the CPU) to render Progressive JPEG. Update getYUVComponentSizes() to early return if m_yuvDecodingFailed to deny subsequent Ganesh requests to YUV decode the image. BUG= 597127 Review URL: https://codereview.chromium.org/1829923004 Cr-Commit-Position: refs/heads/master@{#383472} [modify] https://crrev.com/0e4b0bc92e29939d10ff76a3540b4c2b3b5549b6/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp [modify] https://crrev.com/0e4b0bc92e29939d10ff76a3540b4c2b3b5549b6/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h
,
Mar 28 2016
Removing the Needs-Bisect label as the Fix is already landed in C#17. Will verify the fix once the latest canary is available.
,
Mar 28 2016
If we expect progressive jpegs to fail to decode to YUV, we should be able to disable these decode in onQueryYUV8() rather than waiting to fail in onGetYUV8Planes(). We successfully do this *some* of the time now. yuvSubsampling() catches jpegs where (info.comps_in_scan < info.num_components). https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp&q=JPEGImageDecoder&sq=package:chromium&l=222 However, some progressive jpegs (including the ones in this bug) still have 3 components in their first scan. They just have additional information on at least one of the components in subsequent scans. We can detect all progressive/mulit-scan jpegs by instead calling jpeg_has_multiple_scans(). FWIW, skia's decoders support YUV decoding of progressive images. Is this something Chrome would want? I *think* this can be fixed in Chrome if we use "comp_info" instead of "cur_comp_info" in outputRawData(). Chrome currently decodes all progressive images in buffered image mode. I'm not sure it makes much sense to decode to YUV in buffered image mode, since we would have to repeatedly draw YUV->RGBA every time the YUV data was updated. Maybe this is not so bad though? Another alternative would be to disable buffered image mode on YUV decodes of progressive images (again if that was something we wanted to support).
,
Mar 29 2016
jpeg_has_multiple_scans() and the other fixes you suggest all seem possible, and would need testing. Might be a good subject for another bug? There are no layout tests for the YUV decoding modes we do currently support, so maybe some tests are due in that area, per :) https://codereview.chromium.org/1719533002#msg35 https://codereview.chromium.org/1719533002#msg41 and add a layout test for this current bug.
,
Mar 29 2016
,
Mar 29 2016
Yes I agree that the first step would be adding tests and I agree that this fits in another bug. I'll go ahead and file one and notify when I can start work on this.
,
Mar 29 2016
New bug has been filed. https://bugs.chromium.org/p/chromium/issues/detail?id=598670
,
Mar 29 2016
verified on Mac with latest snapshot, the bug is fixed.
,
Mar 29 2016
Verified on Win with latest snapshot, the bug is fixed.
,
Mar 30 2016
kenson@ re: the progressive JPEG images in #7. Do you own those images? We need permission from their owner to use them a Blink layout test. Possible?
,
Mar 30 2016
noel@ Unfortunately I do not own the images. The images are built from app icons from Apple App Store apps. I'll see if I can create another test case using Creative Commons-licensed images.
,
Mar 30 2016
I've found some images from http://www.gratisography.com/, under the permissive CC0 license (http://www.gratisography.com/terms.html). I've processed these images in the same manner that I processed the previous test images. The attached test cases are updated to use these new images. test-drawImage-cc0.html (using canvas) still shows the issue (missing images) test-draw-image-cc0.html (regular <img>) does not show the issue (all images visible) Can anyone else reproduce the missing images with test-draw-image-cc0.html on Chrome Stable? BTW these images are hosted on yttromobile.com so if the images become part of an automated test suite, can they be copied/hosted elsewhere?
,
Mar 30 2016
kenson@: re-opening this bug for further investigation based on your comment.
,
Mar 30 2016
kenson@: Were you trying the new tests on Chrome stable? The change that we made is not pushed to the stable channel. If you are using windows, could you try chrome canary here: https://www.google.com/chrome/browser/canary.html. If you are using Mac, could you download the latest snapshot here: https://commondatastorage.googleapis.com/chromium-browser-snapshots/index.html?prefix=Mac/383958/ The newly attached files seem to both display fine when I use the latest snapshot on mac.
,
Mar 30 2016
xidachen@ Sorry for the confusion. I was re-creating the test cases and ensuring that they are still broken on Chrome Stable. Unfortunately, I was not able to cause test-draw-image-cc0.html (regular <img>) to break again. Glad to see they are fixed on Chrome Canary!
,
Mar 30 2016
,
Mar 30 2016
#31 I was re-creating the test cases and ensuring that they are still broken on Chrome Stable. Unfortunately, I was not able to cause test-draw-image-cc0.html (regular <img>) to break again. Reload the image test case a few times. Doing that, I see missing images with in Chrome Stable Win when running with --force-gpu-rasterization.
,
Mar 30 2016
And I see _no missing images_ when running Chrome Canary Win 51.0.2694.1 canary (64-bit) when running with --force-gpu-rasterization. Looks fixed to me.
,
Apr 4 2016
Tests submitted: issue 598949 issue 598549 and issue 600087 . Updating labels: GPU rasterization ships on Android only, and then only if the web page uses a special <meta> tag (refer to issue 598949 for details).
,
Apr 4 2016
xidachen@, want to do the honors and apply for merge labels now the fix has had sufficient time to bake on canary?
,
Apr 4 2016
Happy to do so, thank you for asking.
,
Apr 4 2016
Your change meets the bar and is auto-approved for M50 (branch: 2661)
,
Apr 4 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e367e01f30ded9fef9dac6a8574cc372e5b05e0f commit e367e01f30ded9fef9dac6a8574cc372e5b05e0f Author: Xida Chen <xidachen@chromium.org> Date: Mon Apr 04 14:17:53 2016 Do not set m_decodeFailed in ImageFrameGenerator::decodeToYUV This method sets m_decodeFailed bit to be true at the end on VUY decode failure, which can result a null SkImage in DeferredImageDecoder::createFrameAtIndex() when Ganesh is enabled and the image is Progressive JPEG. Ganesh falls back to displaying an RGBA decoded image on decode failure but the null SkImage was shown instead (see bug examples). When decodeToYUV() fails, do not set m_decodeFailed. Instead set m_yuvDecodingFailed to allow Ganesh fallback logic (that decodes the image to RGBA on the CPU) to render Progressive JPEG. Update getYUVComponentSizes() to early return if m_yuvDecodingFailed to deny subsequent Ganesh requests to YUV decode the image. BUG= 597127 Review URL: https://codereview.chromium.org/1829923004 Cr-Commit-Position: refs/heads/master@{#383472} (cherry picked from commit 0e4b0bc92e29939d10ff76a3540b4c2b3b5549b6) Review URL: https://codereview.chromium.org/1860523002 . Cr-Commit-Position: refs/branch-heads/2661@{#471} Cr-Branched-From: ef6f6ae5e4c96622286b563658d5cd62a6cf1197-refs/heads/master@{#378081} [modify] https://crrev.com/e367e01f30ded9fef9dac6a8574cc372e5b05e0f/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp [modify] https://crrev.com/e367e01f30ded9fef9dac6a8574cc372e5b05e0f/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by ken...@yttromobile.com
, Mar 23 2016