Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1829923004:
Do not set m_decodeFailed in ImageFrameGenerator::decodeToYUV (Closed)
|
Created:
4 years, 9 months ago by xidachen Modified:
4 years, 8 months ago CC:
chromium-reviews, krit, pdr+graphicswatchlist_chromium.org, drott+blinkwatch_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, jbroman, Justin Novosad, Rik, Stephen Chennney, blink-reviews, f(malita), danakj+watch_chromium.org, kinuko+watch, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not set m_decodeFailed in ImageFrameGenerator::decodeToYUV
The method was setting m_decodeFailed to be true dynamically, on
a VUY decode failure, resulting in a null SkImage return 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 the 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
Patch Set 1 #
Total comments: 6
Patch Set 2 : add a new unit test #Patch Set 3 : changing outputRawData makes it work #Patch Set 4 : back to PS1, add a new m_yuvDecodingFailed #
Messages
Total messages: 23 (10 generated)
junov@chromium.org changed reviewers: + junov@chromium.org
This CL is missing a test https://codereview.chromium.org/1829923004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/1829923004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:243: ASSERT(decoder->canDecodeToYUV()); Does this assert hit? https://codereview.chromium.org/1829923004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:250: ASSERT(decoder->failed()); Does this assert fire when you try to repro the bug with assertions enabled (debug build)? https://codereview.chromium.org/1829923004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:251: return false; Can you explain why the decode flow reaches this point if the decode did not actually fail? Is there something special about the way the jpegs are encoded?
junov@chromium.org changed reviewers: + noel@chromium.org
+noel for guidance
I am really confused by this debugging result... https://codereview.chromium.org/1829923004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/1829923004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:243: ASSERT(decoder->canDecodeToYUV()); On 2016/03/24 14:13:57, Justin Novosad wrote: > Does this assert hit? This condition is always true, so it doesn't hit this ASSERT. https://codereview.chromium.org/1829923004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:250: ASSERT(decoder->failed()); On 2016/03/24 14:13:57, Justin Novosad wrote: > Does this assert fire when you try to repro the bug with assertions enabled > (debug build)? The example loads and calls drawImage() on 25 images, debugging shows that 15 of them hit this assert which means decoding failed. But 10 of them go into the above if() block which returns true.
In PS#2, I added a new unit test into JPEGImageDeocderTest.cpp, and the test currently fails.
https://codereview.chromium.org/1829923004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp (right): https://codereview.chromium.org/1829923004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp:251: return false; On 2016/03/24 14:13:57, Justin Novosad wrote: > Can you explain why the decode flow reaches this point if the decode did not > actually fail? Is there something special about the way the jpegs are encoded? Concur with these questions. https://codereview.chromium.org/1719533002 was a the most recent change that touched the body of the YUV decoding path in the JPEGImageDecoder. Possible regression (not sure), bugs is marked NeedsBisect so we can wait for that But meanwhile, you could try reverting the changes to JPEGImageDecoder routine static bool outputRawData(JPEGImageReader* reader, ImagePlanes* imagePlanes) that got added on https://codereview.chromium.org/1719533002 to see if that helps?
noel@: Thank you for your suggestions. I did bisect, and it eventually points to one of your CLs here: https://codereview.chromium.org/1527433002. So that's why I came up with PS#1. I totally agree that PS#1 is a bad idea because it doesn't really fix the problem. I made changes to the outputRawData() function, and the unit test that I added in PS#2 now passes under release build but still fails under debug build. I need some more debugging.
On 2016/03/26 00:37:58, xidachen wrote: > noel@: > > Thank you for your suggestions. > I made changes to the outputRawData() function, and the unit test that I added > in PS#2 now passes under release build but still fails under debug build. I need > some more debugging. Seems like you have found a separate bug related to YUV decoding. Suggest you deal with that on a separate bug and cc Matt about it. > I did bisect, and it eventually points to one of your CLs here: > https://codereview.chromium.org/1527433002. So that's why I came up with PS#1. I > totally agree that PS#1 is a bad idea because it doesn't really fix the problem. Maybe, but the original bug was reported on OSX, right? We do not support YUV JPEG decoding on any desktop Chrome in my understanding, so I don't know why the YUV JPEG decoder is even being called.
Description was changed from ========== Do not set m_decodeFailed in ImageFrameGenerator::decodeToYUV Currently this method sets the m_decodeFailed bit to be true at the end, which results a null SkImage in DeferredImageDecoder::createFrameAtIndex and that breaks drawImage() such as the example shown in the bug. This CL removes the line of setting the m_decodeFailed and the example renders fine. BUG=597127 ========== to ========== Do not set m_decodeFailed in ImageFrameGenerator::decodeToYUV Currently this method sets the m_decodeFailed bit to be true at the end, which results a null SkImage in DeferredImageDecoder::createFrameAtIndex and that breaks drawImage() such as the example shown in the bug. To be more specific, when GPU acceleration is involved, decodeToYUV will be called, and it fails when decoding a progressive JPEG image, which sets m_decodeFailed to be true. As a result, a nullptr is returned in DeferredImageDecoder::createFrameAtIndex. This CL removes the line of setting the m_decodeFailed, when the decodeToYUV fails, it will use fall back logic which decodes to RGB. With the change, the example renders fine. BUG=597127 ==========
noel@ please take a look at PS#4. I have verified that the example attached in the bug works fine with this change. Also, any suggestions about a good test case? We are not changing the behavior of the decodeToYUV, so I cannot think of a good test case for this change?
LGTM
On 2016/03/27 15:11:55, xidachen wrote: > noel@ please take a look at PS#4. I have verified that the example attached in > the bug works fine with this change. OK good, thanks for checking. > Also, any suggestions about a good test > case? We are not changing the behavior of the decodeToYUV, so I cannot think of > a good test case for this change? Agree, hard to test. We'll maybe need a layout test. Matt was going to file a bug about that and add some YUV decoding layout tests, see https://codereview.chromium.org/1719533002#msg41 ... but we will need a test or Progressive JPEG images. Suggest we land here, and work out what to do about testing in another CL.
The CQ bit was checked by noel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1829923004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1829923004/60001
Description was changed from ========== Do not set m_decodeFailed in ImageFrameGenerator::decodeToYUV Currently this method sets the m_decodeFailed bit to be true at the end, which results a null SkImage in DeferredImageDecoder::createFrameAtIndex and that breaks drawImage() such as the example shown in the bug. To be more specific, when GPU acceleration is involved, decodeToYUV will be called, and it fails when decoding a progressive JPEG image, which sets m_decodeFailed to be true. As a result, a nullptr is returned in DeferredImageDecoder::createFrameAtIndex. This CL removes the line of setting the m_decodeFailed, when the decodeToYUV fails, it will use fall back logic which decodes to RGB. With the change, the example renders fine. BUG=597127 ========== to ========== Do not set m_decodeFailed in ImageFrameGenerator::decodeToYUV This method sets m_decodeFailed bit to be true at the end on VUY decode failure, and that can result a null SkImage in DeferredImageDecoder::createFrameAtIndex when Ganesh is enabled and the image is Progressive JPEG. Ganesh fallbacks to displaying an RGBA decoded image, on failure, but a null SkImage is shown instead (see the examples on the bug). This CL removes the line of setting the m_decodeFailed, when the decodeToYUV fails. Instead, record m_yuvDecodingFailed, to allow Ganesh fall back logic (which decodes the image to RGBA) to work for Progressive JPEG images. BUG=597127 ==========
Description was changed from ========== Do not set m_decodeFailed in ImageFrameGenerator::decodeToYUV This method sets m_decodeFailed bit to be true at the end on VUY decode failure, and that can result a null SkImage in DeferredImageDecoder::createFrameAtIndex when Ganesh is enabled and the image is Progressive JPEG. Ganesh fallbacks to displaying an RGBA decoded image, on failure, but a null SkImage is shown instead (see the examples on the bug). This CL removes the line of setting the m_decodeFailed, when the decodeToYUV fails. Instead, record m_yuvDecodingFailed, to allow Ganesh fall back logic (which decodes the image to RGBA) to work for Progressive JPEG images. BUG=597127 ========== to ========== Do not set m_decodeFailed in ImageFrameGenerator::decodeToYUV This method sets m_decodeFailed bit to be true at the end on VUY decode failure, and that 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 the 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) to render Progressive JPEG. BUG=597127 ==========
Description was changed from ========== Do not set m_decodeFailed in ImageFrameGenerator::decodeToYUV This method sets m_decodeFailed bit to be true at the end on VUY decode failure, and that 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 the 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) to render Progressive JPEG. BUG=597127 ========== to ========== 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 ==========
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/0e4b0bc92e29939d10ff76a3540b4c2b3b5549b6 Cr-Commit-Position: refs/heads/master@{#383472} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0e4b0bc92e29939d10ff76a3540b4c2b3b5549b6 Cr-Commit-Position: refs/heads/master@{#383472}
Message was sent while issue was closed.
Description was changed from ========== 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 Committed: https://crrev.com/0e4b0bc92e29939d10ff76a3540b4c2b3b5549b6 Cr-Commit-Position: refs/heads/master@{#383472} ========== to ========== Do not set m_decodeFailed in ImageFrameGenerator::decodeToYUV The method was setting m_decodeFailed to be true dynamically, on a VUY decode failure, resulting in a null SkImage return 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 the 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 ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
