Bug 76274 – [chromium] Refactor canvas, plugin, and video drawing to be more data-driven

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Tim Dresser]

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.

[Tim Dresser]

Created attachment 122433 [details]
Patch

[Adrienne Walker]

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 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]

(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 on attachment 122433 [details]
Patch

R- for the ChangeLog since that'll break our scripts, but otherwise this looks great.

[Tim Dresser]

Created attachment 122458 [details]
Patch

[James Robinson]

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]

(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]

(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]

(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]

(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]

(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]

Created attachment 122780 [details]
Patch

[Adrienne Walker]

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]

Created attachment 122794 [details]
Patch

[Adrienne Walker]

Comment on attachment 122794 [details]
Patch

Thanks for all the changes.  This looks great to me.

[James Robinson]

Comment on attachment 122794 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 122794 [details]
Patch

Clearing flags on attachment: 122794

Committed r105311: <http://trac.webkit.org/changeset/105311>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.