Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1225583003:
Flush pending writes on Skia scratch textures used in Chromium (Closed)
|
Created:
5 years, 5 months ago by oetuaho-nv Modified:
5 years, 5 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, avayvod+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, wjia+watch_chromium.org, mlamouri+watch-media_chromium.org, reed1, bsalomon_chromium, no sievers, boliu, Sami Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFlush pending writes on Skia scratch textures used in Chromium
Chromium uses textures allocated by Skia's internal resource manager for
video rendering. In case the textures are recycled, they might have
pending write operations on them. Most commonly they have a pending
framebuffer discard that gets inserted when a texture is chosen for
recycling. These pending operations need to be flushed before the texture
is accessed outside of Skia. Otherwise it's possible that the pending
operations get deferred until after Chromium uses the texture.
In other words, we need to guarantee that the order of operations is:
1. Pending writes from Skia are flushed
2. Chromium writes to the texture without involving Skia
3. Chromium requests a readback of the texture using Skia
4. Skia reads back the texture
instead of
1. Chromium writes to the texture without involving Skia
2. Chromium requests a readback of the texture using Skia
3. Pending writes from Skia are flushed
4. Skia reads back the texture
BUG=504773, 499555
TEST=WebGL conformance tests
Committed: https://crrev.com/140faf7e1dbc0f41a57242b4d3114bad6b9552fd
Cr-Commit-Position: refs/heads/master@{#338089}
Patch Set 1 #Patch Set 2 : Use flushSurfaceWrites() instead of flushWrites() #Patch Set 3 : Use flushWrites() again #Patch Set 4 : Rebase #
Messages
Total messages: 46 (12 generated)
oetuaho@nvidia.com changed reviewers: + dalecurtis@chromium.org, kbr@chromium.org, qinmin@chromium.org
Please review this bug fix!
reed@, bsalomon@: could one or both of you please review this bug fix? It seems to be the most expedient solution to getting the video-related WebGL conformance tests running again on Android. Thanks.
Olli, thanks very much for getting to the bottom of this. Your fix looks good from my standpoint, but I'm not sure if there are other ramifications of flushing writes to these scratch textures. If there's no performance impact for the common case then this LGTM.
bsalomon@google.com changed reviewers: + bsalomon@google.com
We'd like to completely remove the wrapping of textures into bitmaps (and most direct uses of GrTextureProvider in most instances in Chromium). But until that time, this lgtm.
I wouldn't expect any performance impact from this since the pending writes will be executed either way, this change just makes them to be executed at the correct time. Discarding the framebuffer before it's being written by CopyTextureCHROMIUM could even improve performance in some cases.
oetuaho@nvidia.com changed reviewers: + sandersd@chromium.org, wolenetz@chromium.org, xhwang@chromium.org
The patch still needs owner approval, adding xhwang, sandersd, wolenetz.
dalecurtis@chromium.org changed reviewers: - sandersd@chromium.org, wolenetz@chromium.org
I'll handle OWNERS stamp, but was waiting for qinmin to approve. If xhwang@ is comfortable subbing for qinmin@ he may stamp as well.
lgtm
The CQ bit was checked by qinmin@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1225583003/1
The author oetuaho@nvidia.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
On 2015/07/07 17:06:00, commit-bot: I haz the power wrote: > The author mailto:oetuaho@nvidia.com has not signed Google Contributor License > Agreement. Please visit https://cla.developers.google.com to sign and manage > CLA. Wat? NVIDIA has signed the corporate CLA and Olli has contributed multiple patches to the Chromium project. Who do I complain to about this false negative?
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) (exceeded global retry quota)
On 2015/07/07 17:20:10, Ken Russell wrote: > On 2015/07/07 17:06:00, commit-bot: I haz the power wrote: > > The author mailto:oetuaho@nvidia.com has not signed Google Contributor License > > Agreement. Please visit https://cla.developers.google.com to sign and manage > > CLA. > > Wat? NVIDIA has signed the corporate CLA and Olli has contributed multiple > patches to the Chromium project. Who do I complain to about this false negative? FYI, I've filed a Google-internal issue about this omission. It looks like there are some legitimate compile failures to be resolved anyway before this patch can land (is a Skia roll necessary?)
On 2015/07/07 17:38:33, Ken Russell wrote: > It looks like there are some legitimate compile failures to be resolved anyway > before this patch can land (is a Skia roll necessary?) The failure seems to be happening in component build because GrSurface is not qualified with SK_API. However, I think we can actually get around this by using GrContext::flushSurfaceWrites() instead. I'll put up a new version of the patch once I have verified this.
On 2015/07/08 13:33:33, oetuaho-nv wrote: > On 2015/07/07 17:38:33, Ken Russell wrote: > > It looks like there are some legitimate compile failures to be resolved anyway > > before this patch can land (is a Skia roll necessary?) > > The failure seems to be happening in component build because GrSurface is not > qualified with SK_API. However, I think we can actually get around this by using > GrContext::flushSurfaceWrites() instead. I'll put up a new version of the patch > once I have verified this. That is purely an oversight. I'm landing https://codereview.chromium.org/1209043012/ which adds SK_API to GrSurface. Can we go back to flushWrites()?
On 2015/07/08 14:46:36, bsalomon wrote: > That is purely an oversight. I'm landing > https://codereview.chromium.org/1209043012/ which adds SK_API to GrSurface. Can > we go back to flushWrites()? I changed the patch to use flushWrites() again. Trying to land again on top of the latest Skia roll.
The CQ bit was checked by oetuaho@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, qinmin@chromium.org, dalecurtis@chromium.org, bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1225583003/#ps40001 (title: "Use flushWrites() again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1225583003/40001
The author oetuaho@nvidia.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The author oetuaho@nvidia.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The author oetuaho@nvidia.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
The CQ bit was unchecked by commit-bot@chromium.org
What does "Failed to apply the patch" signify? Do I need to rebase and try to land again?
Yes, that's generally what it means.
The CQ bit was checked by oetuaho@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, qinmin@chromium.org, dalecurtis@chromium.org, bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1225583003/#ps60001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1225583003/60001
The author oetuaho@nvidia.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The author oetuaho@nvidia.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
Looks like the CLA tool is still broken, kbr@ did anything come of that internal issue. Should we just land this manually?
Patches should still be able to land even with the CLA nags until next week (I just landed one a few minutes ago). The reason for the nags is that NVIDIA hasn't yet completed transition to the new CLA system, but it should be done soon.
The author oetuaho@nvidia.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The author oetuaho@nvidia.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/140faf7e1dbc0f41a57242b4d3114bad6b9552fd Cr-Commit-Position: refs/heads/master@{#338089}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1226093008/ by kbr@chromium.org. The reason for reverting is: Broke three video-related WebGL conformance tests on Nexus 5; see https://code.google.com/p/chromium/issues/detail?id=504773#c3 . . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
