Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1849393002:
Enable yuv decoding of progressive jpegs (Closed)
DescriptionEnable yuv decoding of progressive jpegs
BUG=598670
Committed: https://crrev.com/0a3bb6541a63a165e4fa43ad77943c6ce16d3d40
Cr-Commit-Position: refs/heads/master@{#385734}
Patch Set 1 : Disable yuv decoding of progressive jpegs #Patch Set 2 : Enable yuv decoding of progressive jpegs #Patch Set 3 : Needs rebaseline #
Messages
Total messages: 30 (11 generated)
Description was changed from ========== Enable yuv decoding of progressive jpegs BUG= ========== to ========== Enable yuv decoding of progressive jpegs BUG=598670 ==========
msarett@google.com changed reviewers: + noel@chromium.org
Patch Sets 1 and 2 are completely different. PS 1 disables the YUV decoder for all progressive jpegs. Before this fix, some (but not all) progressive jpegs were disabled. PS 2 enables the YUV decoder for all progressive jpegs and fixes the bug that was causing them to fail to decode. I've verified that both PS 1 and PS 2 on their own would be enough to fix crbug.com/597127. I've done this by checking h5.yttromobile.com on Android. I think PS 2 makes sense. I don't see any reason to disallow progressive YUV decodes. However, if we do want to keep them disabled, I think we should land PS 1. It might make sense to hold off on this until https://codereview.chromium.org/1845223005 lands.
On 2016/04/01 20:32:56, msarett wrote: > Patch Sets 1 and 2 are completely different. Noted, thanks for providing options. > It might make sense to hold off on this until > https://codereview.chromium.org/1845223005 lands. Yes, agree, lets get those tests landed first.
On 2016/04/01 22:19:49, noel gordon wrote: > On 2016/04/01 20:32:56, msarett wrote: > > Patch Sets 1 and 2 are completely different. > > Noted, thanks for providing options. > > > It might make sense to hold off on this until > > https://codereview.chromium.org/1845223005 lands. > > Yes, agree, lets get those tests landed first. Also, https://codereview.chromium.org/1846903003
If Leon is not around, you could also maybe review https://codereview.chromium.org/1845223005 ?
msarett@google.com changed reviewers: + scroggo@google.com
lgtm
> I think PS 2 makes sense. I don't see any reason to disallow progressive YUV decodes. However, if we do want to keep them disabled, I think we should land PS 1. Yeah, so are we confident about PS2? Does it work with --force-gpu-rasterization on http://pooyak.com/p/progjpeg for example?
On 2016/04/04 23:10:05, noel gordon wrote: > > I think PS 2 makes sense. I don't see any reason to disallow progressive YUV > decodes. However, if we do want to keep them disabled, I think we should land > PS 1. > > Yeah, so are we confident about PS2? Does it work with > --force-gpu-rasterization on > > http://pooyak.com/p/progjpeg > > for example? Yes I'm confident. I don't know how to use --force-gpu-rasterization. Do you use that with out/Default/chrome on a typical build? I've been testing by building for Android and using the ContentShell app. I've confirmed that it is using the yuv decode path, and I've verified that http://pooyak.com/p/progjpeg and http://h5.yttromobile.com load properly.
On 2016/04/05 14:24:34, msarett wrote: > On 2016/04/04 23:10:05, noel gordon wrote: > > > I think PS 2 makes sense. I don't see any reason to disallow progressive > YUV > > decodes. However, if we do want to keep them disabled, I think we should land > > PS 1. > > > > Yeah, so are we confident about PS2? Does it work with > > --force-gpu-rasterization on > > > > http://pooyak.com/p/progjpeg > > > > for example? > > Yes I'm confident. > > I don't know how to use --force-gpu-rasterization. Do you use that with > out/Default/chrome on a typical build? Yes, running % out/{Release,Default}/chrome --force-gpu-rasterization forces YUV JPEG decoding for desktop chrome. You can use that check / debug YUV decoding ... > I've been testing by building for Android and using the ContentShell app. I've > confirmed that it is using the yuv decode path, and I've verified that > http://pooyak.com/p/progjpeg and http://h5.yttromobile.com load properly. Ah good. I'm not sure those pages have the <meta> tag needed to enable GPU rasterization though. The test cases on our recent bug about http://h5.yttromobile.com https://bugs.chromium.org/p/chromium/issues/detail?id=597127 provide good litmus tests in desktop chrome running with --force-gpu-rasterization. Maybe check they now are successfully YUV decoded (m_yuvDecdoingFailed == false) with PS2? Same with http://pooyak.com/p/progjpeg and we'd be good to go with PS2.
On 2016/04/05 21:05:55, noel gordon wrote: > On 2016/04/05 14:24:34, msarett wrote: > > On 2016/04/04 23:10:05, noel gordon wrote: > > > > I think PS 2 makes sense. I don't see any reason to disallow progressive > > YUV > > > decodes. However, if we do want to keep them disabled, I think we should > land > > > PS 1. > > > > > > Yeah, so are we confident about PS2? Does it work with > > > --force-gpu-rasterization on > > > > > > http://pooyak.com/p/progjpeg > > > > > > for example? > > > > Yes I'm confident. > > > > I don't know how to use --force-gpu-rasterization. Do you use that with > > out/Default/chrome on a typical build? > > Yes, running > > % out/{Release,Default}/chrome --force-gpu-rasterization > > forces YUV JPEG decoding for desktop chrome. You can use that check / debug YUV > decoding ... > > > I've been testing by building for Android and using the ContentShell app. > I've > > confirmed that it is using the yuv decode path, and I've verified that > > http://pooyak.com/p/progjpeg and http://h5.yttromobile.com load properly. > > Ah good. I'm not sure those pages have the <meta> tag needed to enable GPU > rasterization though. > > The test cases on our recent bug about http://h5.yttromobile.com > > https://bugs.chromium.org/p/chromium/issues/detail?id=597127 > > provide good litmus tests in desktop chrome running with > --force-gpu-rasterization. Maybe check they now are successfully YUV decoded > (m_yuvDecdoingFailed == false) with PS2? Same with http://pooyak.com/p/progjpeg > and we'd be good to go with PS2. Yes, I used a similar strategy to verify that the YUV code path was exercised on Android. I have now tested on Android and on desktop with --force-gpu-rasterization. Everything looks good.
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1849393002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1849393002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Looks like an image diff failure on windows. I *think* it's because some gpus draw yuv differently... I'll try to figure out how to look at the output.
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1849393002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1849393002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Tests appear to be passing, LGTM. Thanks Matt, good work.
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/1849393002/#ps40001 (title: "Needs rebaseline")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1849393002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1849393002/40001
Message was sent while issue was closed.
Description was changed from ========== Enable yuv decoding of progressive jpegs BUG=598670 ========== to ========== Enable yuv decoding of progressive jpegs BUG=598670 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Enable yuv decoding of progressive jpegs BUG=598670 ========== to ========== Enable yuv decoding of progressive jpegs BUG=598670 Committed: https://crrev.com/0a3bb6541a63a165e4fa43ad77943c6ce16d3d40 Cr-Commit-Position: refs/heads/master@{#385734} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0a3bb6541a63a165e4fa43ad77943c6ce16d3d40 Cr-Commit-Position: refs/heads/master@{#385734} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
