Bug 81286 – [chromium] Unify the drawing logic for different layer types that output textures to the compositor

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Unify the drawing logic for different layer types that output textures to the compositor

[James Robinson]

Created attachment 132151 [details]
Patch

[James Robinson]

I hope to be able to collapse our API types for texture-backed layers a bit as well, but first we need to unify the implementation a bit.

Diff stat says this is a good change: 583 insertions(+), 752 deletions(-)

[Shawn Singh]

Yes, this is very refreshing indeed...  will we eventually (not in this patch, of course) be able to do the same for main thread side?

[James Robinson]

(In reply to comment #3)
> Yes, this is very refreshing indeed...  will we eventually (not in this patch, of course) be able to do the same for main thread side?

I hope so! That's the motivation for this patch, actually (in addition to just cleaning things up).  I'm still thinking through exactly how the main thread API will look.

[Nat Duca]

Comment on attachment 132151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132151&action=review

Looks awesome. Bikeshed comment only.

> Source/WebCore/WebCore.gypi:3517
> +            'platform/graphics/chromium/cc/CCTextureLayerImpl.h',

Externaltexture a la WebExternalTextureLayer?

[Stephen White]

Comment on attachment 132151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132151&action=review

W00t!  Nice work!

> Source/WebCore/platform/graphics/chromium/cc/CCTextureDrawQuad.cpp:45
> +    , m_textureId(textureId)
> +    , m_hasAlpha(hasAlpha)
> +    , m_premultipliedAlpha(premultipliedAlpha)
> +    , m_uvRect(uvRect)
> +    , m_flipped(flipped)
> +    , m_ioSurfaceSize(ioSurfaceSize)
> +    , m_ioSurfaceTextureId(ioSurfaceTextureId)

These member vars seem to be common between CCTextureDrawQuad and CCTextureLayerImpl.  Would it be worthwhile refactoring them out into a struct or class that they both can aggregate?

[Adrienne Walker]

Comment on attachment 132151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132151&action=review

I realize you're trying to cut down on the number of classes that need to be exposed at the WebLayer level, but CCTextureLayerImpl (like CCPluginLayerImpl before it) seems to have multiple personalities--sometimes it's an io surface and sometimes it's a normal external texture.  You have to care about this distinction in every function and even at the quad level.  Unless I'm misunderstanding something, these can never switch back and forth for a given layer, so if something is an IO surface-using plugin, it's always going to be one?

Can you see any easy way to have some more separation of interests here without increasing GTFO complexity?

> Source/WebCore/ChangeLog:14
> +        Canvas 2d, WebGL and plugin composited rendering are all covered by layout tests.

Can you verify manually that plugin layers work in both the IO surface and non-IO surface path? I think this comment is lying about our plugin test coverage.  ;)

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:932
> +    // FIXME: setting the texture parameters every time is redundant. Move this code somewhere
> +    // where it will only happen once per texture.
> +    GLC(context, context()->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_MIN_FILTER, GraphicsContext3D::LINEAR));
> +    GLC(context, context()->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_MAG_FILTER, GraphicsContext3D::LINEAR));
> +    GLC(context, context()->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_WRAP_S, GraphicsContext3D::CLAMP_TO_EDGE));
> +    GLC(context, context()->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_WRAP_T, GraphicsContext3D::CLAMP_TO_EDGE));

I'm pretty sure this is redundant to every path.

[James Robinson]

(In reply to comment #7)
> (From update of attachment 132151 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132151&action=review
> 
> I realize you're trying to cut down on the number of classes that need to be exposed at the WebLayer level, but CCTextureLayerImpl (like CCPluginLayerImpl before it) seems to have multiple personalities--sometimes it's an io surface and sometimes it's a normal external texture.  You have to care about this distinction in every function and even at the quad level.  Unless I'm misunderstanding something, these can never switch back and forth for a given layer, so if something is an IO surface-using plugin, it's always going to be one?
> 
> Can you see any easy way to have some more separation of interests here without increasing GTFO complexity?

I could split the IO surface backed logic into a separate quad (or possibly layer) type, if that's what you mean.  This patch doesn't make that situation any better or worse, though - we used to have split personality layer and quad types for plugins, now we have split personality 

> 
> > Source/WebCore/ChangeLog:14
> > +        Canvas 2d, WebGL and plugin composited rendering are all covered by layout tests.
> 
> Can you verify manually that plugin layers work in both the IO surface and non-IO surface path? I think this comment is lying about our plugin test coverage.  ;)

Sure. (we really should have layout test coverage for the CoreAnimation drawing model too, but this code landed without it under time pressure so the next person through (me) gets to pay the effort. sigh).

> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:932
> > +    // FIXME: setting the texture parameters every time is redundant. Move this code somewhere
> > +    // where it will only happen once per texture.
> > +    GLC(context, context()->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_MIN_FILTER, GraphicsContext3D::LINEAR));
> > +    GLC(context, context()->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_MAG_FILTER, GraphicsContext3D::LINEAR));
> > +    GLC(context, context()->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_WRAP_S, GraphicsContext3D::CLAMP_TO_EDGE));
> > +    GLC(context, context()->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_WRAP_T, GraphicsContext3D::CLAMP_TO_EDGE));
> 
> I'm pretty sure this is redundant to every path.

Do you mean the parameters are already being set somewhere else?  WebGLLayerChromium is the only bit of code that sets these on the composited texture as far as I can tell.  I don't think it would be a tremendous amount of work to set these parameters elsewhere for canvas 2d and plugin layers somewhere else but they do have to be set.  I think the code in this patch is equivalent to what was already happening in drawCanvasQuad()/drawPluginQuad() before this patch.

[Adrienne Walker]

Comment on attachment 132151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132151&action=review

You're right--this patch doesn't make the IO surface vs. texture ID situation any worse.  We can shave that yak later, I guess.

>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:932
>>> +    GLC(context, context()->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_WRAP_T, GraphicsContext3D::CLAMP_TO_EDGE));
>> 
>> I'm pretty sure this is redundant to every path.
> 
> Do you mean the parameters are already being set somewhere else?  WebGLLayerChromium is the only bit of code that sets these on the composited texture as far as I can tell.  I don't think it would be a tremendous amount of work to set these parameters elsewhere for canvas 2d and plugin layers somewhere else but they do have to be set.  I think the code in this patch is equivalent to what was already happening in drawCanvasQuad()/drawPluginQuad() before this patch.

Oh, you're quite right, sorry.  TrackingTextureAllocator sets this for internally generated textures, but external textures might not be set properly.

[James Robinson]

I patched this in on a mac and confirmed that drawingmodel=coreanimation flash still shows up.

Looking closer it should be really easy to support the CoreAnimation drawing model in the test WebKit plugin, and then use layout tests that are already in the tree but marked = SKIPPED for chromium to verify this behavior.  Fun for another day.

[Adrienne Walker]

Comment on attachment 132151 [details]
Patch

R=me.  I totally agree that this patch unearths some mess, but all of that mess was there before.  I think that the patch as-is is a net positive change and should just get landed.

Also, I think CCTextureLayerImpl really is the right name here.  Many layers are going to have external textures, but the plugin layer already uses an internally generated texture id, as will video layers for software decode once this change gets propagated to the main thread side.

[James Robinson]

Created attachment 132439 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 132439 [details]
Patch for landing

Clearing flags on attachment: 132439

Committed r111112: <http://trac.webkit.org/changeset/111112>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.