WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
81286
[chromium] Unify the drawing logic for different layer types that output textures to the compositor
https://bugs.webkit.org/show_bug.cgi?id=81286
Summary
[chromium] Unify the drawing logic for different layer types that output text...
James Robinson
Reported
2012-03-15 16:53:16 PDT
[chromium] Unify the drawing logic for different layer types that output textures to the compositor
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
James Robinson
Comment 1
2012-03-15 16:55:57 PDT
Created
attachment 132151
[details]
Patch
James Robinson
Comment 2
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(-)
Shawn Singh
Comment 3
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?
James Robinson
Comment 4
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.
Nat Duca
Comment 5
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?
Stephen White
Comment 6
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?
Adrienne Walker
Comment 7
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.
James Robinson
Comment 8
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.
Adrienne Walker
Comment 9
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.
James Robinson
Comment 10
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.
Adrienne Walker
Comment 11
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.
James Robinson
Comment 12
2012-03-16 20:03:25 PDT
Created
attachment 132439
[details]
Patch for landing
WebKit Review Bot
Comment 13
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
>
WebKit Review Bot
Comment 14
2012-03-16 21:15:44 PDT
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.
Top of Page
Format For Printing
XML
Clone This Bug