Bug 76274

Summary: [chromium] Refactor canvas, plugin, and video drawing to be more data-driven
Product: WebKit Reporter: Tim Dresser <tdresser>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cc-bugs, enne, jamesr, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Tim Dresser
Reported 2012-01-13 06:43:24 PST
From https://bugs.webkit.org/show_bug.cgi?id=73059: CCLayerImpl-derived classes all currently handle drawing themselves internally, making direct GL calls. This is problematic for several reasons. Testing these layers is tricky because testing must be done at a mock GL interface level rather than at a higher abstract level. It makes culling closely dependent on how each layer draws. It also makes some optimizations like layer squashing impossible. To fix this, CCLayerImpl-derived classes should produce a set of quads to render with some basic information in them about how they are to be drawn. This list can then be processed and drawn in a more data-driven and abstract manner. There are three CCLayerImpl-derived classes which still handle drawing internally: CCCanvasLayerImpl, CCVideoLayerImpl, and CCPluginLayerImpl. These should use the data-driven approach.
Attachments
Patch (37.17 KB, patch)
2012-01-13 08:47 PST, Tim Dresser
no flags
Patch (37.22 KB, patch)
2012-01-13 10:55 PST, Tim Dresser
no flags
Patch (43.44 KB, patch)
2012-01-17 09:52 PST, Tim Dresser
no flags
Patch (43.67 KB, patch)
2012-01-17 11:55 PST, Tim Dresser
no flags
Tim Dresser
Comment 1 2012-01-13 08:47:02 PST
Adrienne Walker
Comment 2 2012-01-13 09:46:55 PST
Comment on attachment 122433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122433&action=review Thanks! This looks great. It's nice to have all of this refactoring first before moving any draw functionality itself. > Source/WebCore/ChangeLog:6 > + Unreviewed. You should leave this saying exactly "Reviewed by NOBODY (OOPS!)." When the commit queue or somebody else lands your patch via webkit-patch, then it will update the reviewer line automatically.
Tim Dresser
Comment 3 2012-01-13 09:55:24 PST
Comment on attachment 122433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122433&action=review >> Source/WebCore/ChangeLog:6 >> + Unreviewed. > > You should leave this saying exactly "Reviewed by NOBODY (OOPS!)." When the commit queue or somebody else lands your patch via webkit-patch, then it will update the reviewer line automatically. What's the best way to resolve this now? Should I resubmit the patch?
James Robinson
Comment 4 2012-01-13 10:08:26 PST
(In reply to comment #3) > (From update of attachment 122433 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122433&action=review > > >> Source/WebCore/ChangeLog:6 > >> + Unreviewed. > > > > You should leave this saying exactly "Reviewed by NOBODY (OOPS!)." When the commit queue or somebody else lands your patch via webkit-patch, then it will update the reviewer line automatically. > > What's the best way to resolve this now? > Should I resubmit the patch? Yes you should. webkit-patch upload should make this pretty painless
James Robinson
Comment 5 2012-01-13 10:10:19 PST
Comment on attachment 122433 [details] Patch R- for the ChangeLog since that'll break our scripts, but otherwise this looks great.
Tim Dresser
Comment 6 2012-01-13 10:55:03 PST
James Robinson
Comment 7 2012-01-13 12:44:07 PST
Comment on attachment 122458 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122458&action=review Looks reasonable. R=me but can't land as is (feel free to ask any committer for help with landing the final version) > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) this will fail an SVN presubmit check, you need to delete this line and describe the testing situation (in this case, this looks like a refactor so existing tests should apply)
Adrienne Walker
Comment 8 2012-01-13 14:13:28 PST
(In reply to comment #7) > (From update of attachment 122458 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122458&action=review > > Looks reasonable. R=me but can't land as is (feel free to ask any committer for help with landing the final version) I'll get it.
Adrienne Walker
Comment 9 2012-01-13 15:00:13 PST
(In reply to comment #8) > (In reply to comment #7) > > (From update of attachment 122458 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=122458&action=review > > > > Looks reasonable. R=me but can't land as is (feel free to ask any committer for help with landing the final version) > > I'll get it. Actually, scratch that. It looks like this fails the webkit_unit_tests target in CCLayerTreeHostImplTest.blendingOffWhenDrawingOpaqueLayers. We've got a test that looks like it's expecting the CCLayerImpl::draw() function to be called via this custom layer path. Removing the code out of CCLayerImpl::appendQuads makes this test fail, although I think that's the right thing to do. I suspect maybe you need to just change the draw function in the BlendStateCheckLayer test class to an appendQuads function and verify that a CCDrawQuad with that sharedQuadState has the right needsBlending() setting, rather than testing the context itself. tdresser: Can you see if you can fix this failure and resubmit a patch (also without the string OOPS anywhere in the ChangeLog)?
Adrienne Walker
Comment 10 2012-01-13 15:15:43 PST
(In reply to comment #9) > tdresser: Can you see if you can fix this failure and resubmit a patch (also without the string OOPS anywhere in the ChangeLog)? Er, more specifically, *with* the reviewed by nobody OOPS, but *without* any other OOPS lines.
Tim Dresser
Comment 11 2012-01-16 08:59:09 PST
(In reply to comment #10) > (In reply to comment #9) > > > tdresser: Can you see if you can fix this failure and resubmit a patch (also without the string OOPS anywhere in the ChangeLog)? > > Er, more specifically, *with* the reviewed by nobody OOPS, but *without* any other OOPS lines. This test currently relies on some culling code which I think is going to behave differently once this refactor is complete. CCLayerImpl::appendQuads is called for all layers, but CCLayerImpl::draw was not called for all layers. I see two options here: 1. Modify the test to expect CCLayerImpl::appendQuads to always be called. This essentially undoes https://bugs.webkit.org/show_bug.cgi?id=75783 2. Test if the layer's quads are actually drawn. Which approach do you think I should take?
Adrienne Walker
Comment 12 2012-01-17 09:28:00 PST
(In reply to comment #11) > This test currently relies on some culling code which I think is going to behave differently once this refactor is complete. > > CCLayerImpl::appendQuads is called for all layers, but CCLayerImpl::draw was not called for all layers. > > I see two options here: > > 1. Modify the test to expect CCLayerImpl::appendQuads to always be called. > This essentially undoes https://bugs.webkit.org/show_bug.cgi?id=75783 > 2. Test if the layer's quads are actually drawn. > > Which approach do you think I should take? This is more complicated than I expected. As a part of refactoring all these layer types, I'm hoping to remove CCLayerImpl::draw entirely. And, on top of that, this test stretches across way too many layers of code. It goes all the way from layer tree => render surface lists => quads => layer drawing code => graphics context for testing. Ack. For what it's worth, I would expect appendQuads to always be called for all of those layers. The change from bug 75783 is not worth preserving, because culling is well-tested (and better-tested) elsewhere. I'd rather do that than test the drawing of quads in this test.
Tim Dresser
Comment 13 2012-01-17 09:52:11 PST
Adrienne Walker
Comment 14 2012-01-17 10:08:00 PST
Comment on attachment 122780 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122780&action=review > Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:193 > + for (size_t i = 0; i < quadList.size(); ++i) { > + CCDrawQuad* drawQuad = quadList[i].get(); > + EXPECT_EQ(m_blend, drawQuad->needsBlending()); > + } Er, this isn't quite right. appendQuads gets passed a list to append new quads to. The quads in the list already are from other layers. This function should probably create new quads and check that needsBlending is correct on them. You could probably use CCSolidColorDrawQuad as a good stand-in mock quad type to create.
Tim Dresser
Comment 15 2012-01-17 11:55:46 PST
Adrienne Walker
Comment 16 2012-01-17 13:01:00 PST
Comment on attachment 122794 [details] Patch Thanks for all the changes. This looks great to me.
James Robinson
Comment 17 2012-01-18 11:36:39 PST
Comment on attachment 122794 [details] Patch R=me
WebKit Review Bot
Comment 18 2012-01-18 13:03:35 PST
Comment on attachment 122794 [details] Patch Clearing flags on attachment: 122794 Committed r105311: <http://trac.webkit.org/changeset/105311>
WebKit Review Bot
Comment 19 2012-01-18 13:03:40 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.