Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2401513002:
DrawingBuffer cleanup: Merge structures (Closed)
|
Created:
4 years, 2 months ago by ccameron Modified:
4 years, 2 months ago CC:
chromium-reviews, krit, drott+blinkwatch_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, pdr+graphicswatchlist_chromium.org, jbroman, Justin Novosad, Rik, f(malita), blink-reviews, piman+watch_chromium.org, danakj+watch_chromium.org, ajuma+watch_chromium.org, Stephen Chennney, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDrawingBuffer cleanup: Merge structures
There exist four related structures for storing color textures:
TextureInfo, MailboxInfo, RecycledMailbox, and FrontBufferInfo. Merge
all of these into one structure, called ColorBuffer.
Make ColorBuffer be a refcounted structure.
Make the destructor of ColorBuffer destroy the GL resources. Delete all
of the various helper functions and calls that existed to destroy GL
resources.
There are many instances where we look up a MailboxInfo in
m_textureMailboxes via its gpu::Mailbox. Change all of these instances
to just point to the same structure.
BUG=648707
Committed: https://crrev.com/57351f256cfe16ba1718a9a2f3c1ef8c58dceda4
Cr-Commit-Position: refs/heads/master@{#423983}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Fix dtor ordering #
Total comments: 11
Patch Set 4 : Add DCHECK and rebase #
Total comments: 2
Patch Set 5 : Move frontbuffer reset to mailbox return #
Dependent Patchsets: Messages
Total messages: 36 (23 generated)
The CQ bit was checked by ccameron@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ccameron@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Merge 3 structures into ColorBuffer BUG= ========== to ========== DrawingBuffer cleanup: Merge structures There exist four related structures for storing color textures: TextureInfo, MailboxInfo, RecycledMailbox, and FrontBufferInfo. Merge all of these into one structure, called ColorBuffer. Make ColorBuffer be a refcounted structure. Make the destructor of ColorBuffer destroy the GL resources. Delete all of the various helper functions and calls that existed to destroy GL resources. There are many instances where we look up a MailboxInfo in m_textureMailboxes via its gpu::Mailbox. Change all of these instances to just point to the same structure. BUG= ==========
The CQ bit was unchecked by ccameron@chromium.org
The CQ bit was checked by ccameron@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== DrawingBuffer cleanup: Merge structures There exist four related structures for storing color textures: TextureInfo, MailboxInfo, RecycledMailbox, and FrontBufferInfo. Merge all of these into one structure, called ColorBuffer. Make ColorBuffer be a refcounted structure. Make the destructor of ColorBuffer destroy the GL resources. Delete all of the various helper functions and calls that existed to destroy GL resources. There are many instances where we look up a MailboxInfo in m_textureMailboxes via its gpu::Mailbox. Change all of these instances to just point to the same structure. BUG= ========== to ========== DrawingBuffer cleanup: Merge structures There exist four related structures for storing color textures: TextureInfo, MailboxInfo, RecycledMailbox, and FrontBufferInfo. Merge all of these into one structure, called ColorBuffer. Make ColorBuffer be a refcounted structure. Make the destructor of ColorBuffer destroy the GL resources. Delete all of the various helper functions and calls that existed to destroy GL resources. There are many instances where we look up a MailboxInfo in m_textureMailboxes via its gpu::Mailbox. Change all of these instances to just point to the same structure. BUG=648707 ==========
ccameron@chromium.org changed reviewers: + kbr@chromium.org
This is the most complicated of the refactoring changes. I've intentionally left many variable names as they were before (e.g, "mailboxInfo", or "info") to minimize the diffs. The next patch after this will do more cosmetic clean-up. https://codereview.chromium.org/2401513002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (left): https://codereview.chromium.org/2401513002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:428: RefPtr<MailboxInfo> mailboxInfo; This block is looking up the MailboxInfo from its mailbox handle. Now these just point to the same structure. https://codereview.chromium.org/2401513002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/2401513002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:221: m_frontColorBuffer = nullptr; m_frontColorBuffer remained populated, but its backing texture was also referenced in the recycle queue, and was deleted. Because we're using RefPtrs, we need to remove both references if we want the GL resources to actually be deleted (which some tests do). https://codereview.chromium.org/2401513002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h (left): https://codereview.chromium.org/2401513002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h:520: Vector<RefPtr<MailboxInfo>> m_textureMailboxes; m_textureMailboxes was serving as a O(n) lookup of some texture information from mailbox. We now just point to the structures directly. https://codereview.chromium.org/2401513002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h (right): https://codereview.chromium.org/2401513002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h:322: ~ColorBuffer(); This destructor now deletes the GL resources. https://codereview.chromium.org/2401513002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h:336: gpu::Mailbox mailbox; This was previously MailboxInfo::mailbox, FrontBufferInfo::mailbox, and RecycledMailbox::mailbox, and used as a key to map between them. https://codereview.chromium.org/2401513002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h:339: gpu::SyncToken produceSyncToken; This was previously FrontBufferInfo::produceSyncToken. https://codereview.chromium.org/2401513002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h:343: gpu::SyncToken receiveSyncToken; This was previously RecycledMailbox::syncToken.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ccameron@chromium.org changed reviewers: + erikchen@chromium.org
Adding erikchen
nice CL, I really like this clean up. Thanks! https://codereview.chromium.org/2401513002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (left): https://codereview.chromium.org/2401513002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:601: DCHECK(mailboxInfo->size == m_size); why get rid of this DCHECK? seems like a good sanity check. https://codereview.chromium.org/2401513002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/2401513002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:221: m_frontColorBuffer = nullptr; On 2016/10/06 09:24:53, ccameron wrote: > m_frontColorBuffer remained populated, but its backing texture was also > referenced in the recycle queue, and was deleted. Because we're using RefPtrs, > we need to remove both references if we want the GL resources to actually be > deleted (which some tests do). This induces a behavior change, specifically with the sequence: e.g. setIsHidden(true); setIsHidden(false); copyToPlatformTexture() It's possible that this new sequence is [more] correct, but it's also possible that this new sequence breaks expected functionality. Could you investigate this?
The CQ bit was checked by ccameron@chromium.org to run a CQ dry run
Thanks -- updated! https://codereview.chromium.org/2401513002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (left): https://codereview.chromium.org/2401513002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:601: DCHECK(mailboxInfo->size == m_size); On 2016/10/06 23:32:51, erikchen wrote: > why get rid of this DCHECK? seems like a good sanity check. Yes, this is needed! Was a bit too aggressive with the cutting. https://codereview.chromium.org/2401513002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/2401513002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:221: m_frontColorBuffer = nullptr; On 2016/10/06 23:32:51, erikchen wrote: > On 2016/10/06 09:24:53, ccameron wrote: > > m_frontColorBuffer remained populated, but its backing texture was also > > referenced in the recycle queue, and was deleted. Because we're using RefPtrs, > > we need to remove both references if we want the GL resources to actually be > > deleted (which some tests do). > > This induces a behavior change, specifically with the sequence: > e.g. setIsHidden(true); setIsHidden(false); copyToPlatformTexture() > It's possible that this new sequence is [more] correct, but it's also possible > that this new sequence breaks expected functionality. Could you investigate > this? Previously, this sequence would result in solid black being read back (since the underlying texture is gone). With this patch, it would fall back to using the backbuffer, which may or may not be solid black (if some content has been drawn but not sent to the compositor, it would have that content). I'd rather keep this behavior than change to not discarding the frontbuffer.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> > Previously, this sequence would result in solid black being read back (since the > underlying texture is gone). With this patch, it would fall back to using the > backbuffer, which may or may not be solid black (if some content has been drawn > but not sent to the compositor, it would have that content). > > I'd rather keep this behavior than change to not discarding the frontbuffer. I went through the WebGL1/2 specs, and couldn't find any references to "hide" or "hidden". Assuming this doesn't break any of the conformance tests, it seems like an unspecced edge case, and I guess I don't care if the behavior changes. lgtm
On 2016/10/07 00:51:29, erikchen wrote: > > > > Previously, this sequence would result in solid black being read back (since > the > > underlying texture is gone). With this patch, it would fall back to using the > > backbuffer, which may or may not be solid black (if some content has been > drawn > > but not sent to the compositor, it would have that content). > > > > I'd rather keep this behavior than change to not discarding the frontbuffer. > > I went through the WebGL1/2 specs, and couldn't find any references to "hide" or > "hidden". Assuming this doesn't break any of the conformance tests, it seems > like an unspecced edge case, and I guess I don't care if the behavior changes. This happens when a tab is moved to the background and back to the foreground. It's called from HTMLCanvasElement::pageVisibilityChanged(). There are some operations which can't be tested by the WebGL conformance suite, because doing so requires privileged operations like reading back the page's rendering results, and the conformance suite must run in an unmodified browser. Chrome might not have multi-tab WebGL pixel tests in place -- in fact, I'm sure not; we should add one. This operation is supposed to be non-destructive, but free up unused GPU memory. It looks to me like freeing the front buffer should not be done. It might be necessary to re-send it to the compositor.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for working on this. It's a great cleanup. Overall it looks great. One comment. https://codereview.chromium.org/2401513002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/2401513002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:221: m_frontColorBuffer = nullptr; Per my reply on the CL's comments, I'm not sure that freeing m_frontColorBuffer here is correct. setIsHidden is called when a tab is backgrounded or foregrounded. If a page has a WebGL-rendered canvas and only draws it once, then moving that tab to the background or foreground must still continue to render correctly. We don't have any multi-tab pixel tests for WebGL right now; we should add some. (Please feel free to file a bug about this and assign it to me.) Please test manually with for example src/content/test/data/gpu/pixel_webgl_aa_alpha.html and modify the test by hand so it passes the context creation attribute: preserveDrawingBuffer: true and preserveDrawingBuffer: false and make sure that bringing that tab to the foreground and background works correctly, both when WebGLImageChromium is enabled and disabled.
Thanks! I think I figured out how to sort out the m_frontColorBuffer issue in the latest patchset. https://codereview.chromium.org/2401513002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right): https://codereview.chromium.org/2401513002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:221: m_frontColorBuffer = nullptr; I played around with this a bit, and I think I found a scheme that makes sense here. Of note is that by resetting m_frontColorBuffer, we're not destroying it -- the compositor still has a reference out on it until its mailbox is released. So, I just made this explicit. I got rid of the reset here, and added a "if the compositor is releasing m_frontColorBuffer, then reset m_frontColorBuffer". This actually makes a lot of sense -- something is the front buffer if the compositor is currently drawing -- when the compositor returns it, it is no longer the front buffer. This also got rid of the test's diffs, which is a nice plus.
It's not clear to me under what circumstances the compositor would release the buffer the DrawingBuffer considers the front one, but this looks like it can't possibly regress the display path. LGTM
Thanks!
The CQ bit was checked by ccameron@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erikchen@chromium.org Link to the patchset: https://codereview.chromium.org/2401513002/#ps80001 (title: "Move frontbuffer reset to mailbox return")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== DrawingBuffer cleanup: Merge structures There exist four related structures for storing color textures: TextureInfo, MailboxInfo, RecycledMailbox, and FrontBufferInfo. Merge all of these into one structure, called ColorBuffer. Make ColorBuffer be a refcounted structure. Make the destructor of ColorBuffer destroy the GL resources. Delete all of the various helper functions and calls that existed to destroy GL resources. There are many instances where we look up a MailboxInfo in m_textureMailboxes via its gpu::Mailbox. Change all of these instances to just point to the same structure. BUG=648707 ========== to ========== DrawingBuffer cleanup: Merge structures There exist four related structures for storing color textures: TextureInfo, MailboxInfo, RecycledMailbox, and FrontBufferInfo. Merge all of these into one structure, called ColorBuffer. Make ColorBuffer be a refcounted structure. Make the destructor of ColorBuffer destroy the GL resources. Delete all of the various helper functions and calls that existed to destroy GL resources. There are many instances where we look up a MailboxInfo in m_textureMailboxes via its gpu::Mailbox. Change all of these instances to just point to the same structure. BUG=648707 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== DrawingBuffer cleanup: Merge structures There exist four related structures for storing color textures: TextureInfo, MailboxInfo, RecycledMailbox, and FrontBufferInfo. Merge all of these into one structure, called ColorBuffer. Make ColorBuffer be a refcounted structure. Make the destructor of ColorBuffer destroy the GL resources. Delete all of the various helper functions and calls that existed to destroy GL resources. There are many instances where we look up a MailboxInfo in m_textureMailboxes via its gpu::Mailbox. Change all of these instances to just point to the same structure. BUG=648707 ========== to ========== DrawingBuffer cleanup: Merge structures There exist four related structures for storing color textures: TextureInfo, MailboxInfo, RecycledMailbox, and FrontBufferInfo. Merge all of these into one structure, called ColorBuffer. Make ColorBuffer be a refcounted structure. Make the destructor of ColorBuffer destroy the GL resources. Delete all of the various helper functions and calls that existed to destroy GL resources. There are many instances where we look up a MailboxInfo in m_textureMailboxes via its gpu::Mailbox. Change all of these instances to just point to the same structure. BUG=648707 Committed: https://crrev.com/57351f256cfe16ba1718a9a2f3c1ef8c58dceda4 Cr-Commit-Position: refs/heads/master@{#423983} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/57351f256cfe16ba1718a9a2f3c1ef8c58dceda4 Cr-Commit-Position: refs/heads/master@{#423983} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
