Bug 76274 - [chromium] Refactor canvas, plugin, and video drawing to be more data-driven
Summary: [chromium] Refactor canvas, plugin, and video drawing to be more data-driven
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-13 06:43 PST by Tim Dresser
Modified: 2012-01-18 13:03 PST (History)
4 users (show)

See Also:


Attachments
Patch (37.17 KB, patch)
2012-01-13 08:47 PST, Tim Dresser
no flags Details | Formatted Diff | Diff
Patch (37.22 KB, patch)
2012-01-13 10:55 PST, Tim Dresser
no flags Details | Formatted Diff | Diff
Patch (43.44 KB, patch)
2012-01-17 09:52 PST, Tim Dresser
no flags Details | Formatted Diff | Diff
Patch (43.67 KB, patch)
2012-01-17 11:55 PST, Tim Dresser
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Dresser 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.
Comment 1 Tim Dresser 2012-01-13 08:47:02 PST
Created attachment 122433 [details]
Patch
Comment 2 Adrienne Walker 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.
Comment 3 Tim Dresser 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?
Comment 4 James Robinson 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
Comment 5 James Robinson 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.
Comment 6 Tim Dresser 2012-01-13 10:55:03 PST
Created attachment 122458 [details]
Patch
Comment 7 James Robinson 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)
Comment 8 Adrienne Walker 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.
Comment 9 Adrienne Walker 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)?
Comment 10 Adrienne Walker 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.
Comment 11 Tim Dresser 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?
Comment 12 Adrienne Walker 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.
Comment 13 Tim Dresser 2012-01-17 09:52:11 PST
Created attachment 122780 [details]
Patch
Comment 14 Adrienne Walker 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.
Comment 15 Tim Dresser 2012-01-17 11:55:46 PST
Created attachment 122794 [details]
Patch
Comment 16 Adrienne Walker 2012-01-17 13:01:00 PST
Comment on attachment 122794 [details]
Patch

Thanks for all the changes.  This looks great to me.
Comment 17 James Robinson 2012-01-18 11:36:39 PST
Comment on attachment 122794 [details]
Patch

R=me
Comment 18 WebKit Review Bot 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>
Comment 19 WebKit Review Bot 2012-01-18 13:03:40 PST
All reviewed patches have been landed.  Closing bug.