Bug 81286 - [chromium] Unify the drawing logic for different layer types that output textures to the compositor
Summary: [chromium] Unify the drawing logic for different layer types that output text...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-15 16:53 PDT by James Robinson
Modified: 2012-03-16 21:15 PDT (History)
10 users (show)

See Also:


Attachments
Patch (85.37 KB, patch)
2012-03-15 16:55 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch for landing (85.40 KB, patch)
2012-03-16 20:03 PDT, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-03-15 16:53:16 PDT
[chromium] Unify the drawing logic for different layer types that output textures to the compositor
Comment 1 James Robinson 2012-03-15 16:55:57 PDT
Created attachment 132151 [details]
Patch
Comment 2 James Robinson 2012-03-15 16:57:27 PDT
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(-)
Comment 3 Shawn Singh 2012-03-15 17:04:22 PDT
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?
Comment 4 James Robinson 2012-03-15 17:20:52 PDT
(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.
Comment 5 Nat Duca 2012-03-15 18:23:25 PDT
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?
Comment 6 Stephen White 2012-03-15 20:54:37 PDT
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?
Comment 7 Adrienne Walker 2012-03-16 09:48:33 PDT
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.
Comment 8 James Robinson 2012-03-16 12:36:12 PDT
(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.
Comment 9 Adrienne Walker 2012-03-16 12:58:20 PDT
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.
Comment 10 James Robinson 2012-03-16 18:36:29 PDT
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.
Comment 11 Adrienne Walker 2012-03-16 19:01:44 PDT
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.
Comment 12 James Robinson 2012-03-16 20:03:25 PDT
Created attachment 132439 [details]
Patch for landing
Comment 13 WebKit Review Bot 2012-03-16 21:15:39 PDT
Comment on attachment 132439 [details]
Patch for landing

Clearing flags on attachment: 132439

Committed r111112: <http://trac.webkit.org/changeset/111112>
Comment 14 WebKit Review Bot 2012-03-16 21:15:44 PDT
All reviewed patches have been landed.  Closing bug.