Bug 73485 - [chromium] Plumb damage from WebExternalTextureLayer and WebPluginContainer to CCDamageTracker
Summary: [chromium] Plumb damage from WebExternalTextureLayer and WebPluginContainer t...
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: Jonathan Backer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-30 13:42 PST by Jonathan Backer
Modified: 2011-12-07 10:46 PST (History)
7 users (show)

See Also:


Attachments
Patch (3.85 KB, patch)
2011-11-30 13:43 PST, Jonathan Backer
no flags Details | Formatted Diff | Diff
Patch (5.15 KB, patch)
2011-12-02 14:37 PST, Jonathan Backer
no flags Details | Formatted Diff | Diff
Patch (5.68 KB, patch)
2011-12-06 14:38 PST, Jonathan Backer
no flags Details | Formatted Diff | Diff
Patch (6.49 KB, patch)
2011-12-07 06:51 PST, Jonathan Backer
no flags Details | Formatted Diff | Diff
Patch (6.48 KB, patch)
2011-12-07 10:23 PST, Jonathan Backer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Backer 2011-11-30 13:42:25 PST
[chromium] NOT FOR REVIEW Plumb damage in WebExternalTextureLayer to CCDamageTracker
Comment 1 Jonathan Backer 2011-11-30 13:43:19 PST
Created attachment 117263 [details]
Patch
Comment 2 WebKit Review Bot 2011-11-30 14:02:55 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Jonathan Backer 2011-11-30 14:11:06 PST
Chromium side here: http://codereview.chromium.org/8764001/
Comment 4 Antoine Labour 2011-11-30 14:38:25 PST
Comment on attachment 117263 [details]
Patch

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

> Source/WebKit/chromium/public/WebExternalTextureLayer.h:72
> +    WEBKIT_EXPORT void setUpdateRect(const WebFloatRect&);

Should that be named invalidateRect for consistency with WebContentLayer ?
Also, for the doc, it'd be useful to explain if the region is in UV coordinates, or layer coordinates.
I think what makes the most sense is to have it in texture coordinates, as in "the texels in the texture within this rect have changed". The layer's implementation can then apply the y flip and the uv scale/offset to transform to layer coordinates.

> Source/WebKit/chromium/src/WebExternalTextureLayerImpl.cpp:71
> +    m_updateRect = updateRect;

Instead of directly setting m_updateRect here, I would add a invalidateRect or something to PluginLayerChromium, that can do the transformation into layer coordinates and set m_updateRect. I'd rather not touch m_updateRect directly in the WebKit API classes, and keep it inside of the layer itself.
Comment 5 Shawn Singh 2011-11-30 14:43:26 PST
(In reply to comment #4)
> (From update of attachment 117263 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117263&action=review
> 
> > Source/WebKit/chromium/public/WebExternalTextureLayer.h:72
> > +    WEBKIT_EXPORT void setUpdateRect(const WebFloatRect&);
> 
> Should that be named invalidateRect for consistency with WebContentLayer ?
> Also, for the doc, it'd be useful to explain if the region is in UV coordinates, or layer coordinates.
> I think what makes the most sense is to have it in texture coordinates, as in "the texels in the texture within this rect have changed". The layer's implementation can then apply the y flip and the uv scale/offset to transform to layer coordinates.
> 
> > Source/WebKit/chromium/src/WebExternalTextureLayerImpl.cpp:71
> > +    m_updateRect = updateRect;
> 
> Instead of directly setting m_updateRect here, I would add a invalidateRect or something to PluginLayerChromium, that can do the transformation into layer coordinates and set m_updateRect. I'd rather not touch m_updateRect directly in the WebKit API classes, and keep it inside of the layer itself.

Two questions:

(1) Are there any other "users" of the PluginLayerChromium?  I am aware of this WebExternalTexture, and Flash.  are there any others?

(2) Do you have a sense of how we should propagate "damage" from Flash?  Before seeing this patch, I was going to blindly damage the entire PluginLayer.  but now it seems like we should be doing something more correct.  Is it as simple as doing a similar invalidation for flash?
Comment 6 Antoine Labour 2011-11-30 15:22:46 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 117263 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=117263&action=review
> > 
> > > Source/WebKit/chromium/public/WebExternalTextureLayer.h:72
> > > +    WEBKIT_EXPORT void setUpdateRect(const WebFloatRect&);
> > 
> > Should that be named invalidateRect for consistency with WebContentLayer ?
> > Also, for the doc, it'd be useful to explain if the region is in UV coordinates, or layer coordinates.
> > I think what makes the most sense is to have it in texture coordinates, as in "the texels in the texture within this rect have changed". The layer's implementation can then apply the y flip and the uv scale/offset to transform to layer coordinates.
> > 
> > > Source/WebKit/chromium/src/WebExternalTextureLayerImpl.cpp:71
> > > +    m_updateRect = updateRect;
> > 
> > Instead of directly setting m_updateRect here, I would add a invalidateRect or something to PluginLayerChromium, that can do the transformation into layer coordinates and set m_updateRect. I'd rather not touch m_updateRect directly in the WebKit API classes, and keep it inside of the layer itself.
> 
> Two questions:
> 
> (1) Are there any other "users" of the PluginLayerChromium?  I am aware of this WebExternalTexture, and Flash.  are there any others?

More than just Flash, all pepper plugins that use GPU (Flash & Netflix, but also NaCl plugins). They'd all go through the same path though.

> 
> (2) Do you have a sense of how we should propagate "damage" from Flash?  Before seeing this patch, I was going to blindly damage the entire PluginLayer.  but now it seems like we should be doing something more correct.  Is it as simple as doing a similar invalidation for flash?

The Pepper API doesn't currently expose any way to swap a partial frame, so the damage is always currently the entire layer. Moving forward we could plumb that through (add the API to pepper, add the necessary hook in WebPluginContainer), and then change Flash etc. to use it and do partial frames.

This patch would be a prereq for all of this though.
Comment 7 Antoine Labour 2011-11-30 15:45:32 PST
Ok, following up from a discussion with S
Comment 8 Antoine Labour 2011-11-30 15:54:20 PST
Ok, following up from a discussion with Shawn.
WebExternalTextureLayer (WETL) should not touch m_updateRect directly, because that would conflict with the use of PluginLayerChromium by Pepper through WebPluginContainerImpl (WPCI).

What needs to happen is that both clients (WETL and WPCI) need to give a damage rect to PluginLayerChromium, so that PluginLayerChromium can generate the correct m_updateRect in updateCompositorResources like the other layers, and reset it in pushPropertiesTo.

One possible mechanism to do so is to use the m_dirtyRect, through setNeedsDisplay. The good news is that it's what WPCI does today. Then PluginLayerChromium::updateCompositorResources would generate the m_updateRect from the m_dirtyRect.

2 things:
- m_dirtyRect is going away with David's patch. We may want to keep it in PluginLayerChromium (and union it on each call to setNeedsDisplay)
- setNeedsDisplay is in layer space, and we may want to express the dirty rect in texture space instead, so we may want to use a different/new call on PluginLayerChromium and fix WPCI, or clarify the semantics of setNeedsDisplay.
Comment 9 Jonathan Backer 2011-12-02 14:37:51 PST
Created attachment 117694 [details]
Patch
Comment 10 Eric Seidel (no email) 2011-12-02 14:46:12 PST
Comment on attachment 117694 [details]
Patch

Use --no-review in webkit-patch if you don't wnat your things marked for review.  You can always submit patches to the EWSes manually using the bubble.
Comment 11 Shawn Singh 2011-12-06 11:00:22 PST
Comment on attachment 117694 [details]
Patch

This looks fine to me, with two tiny little nits.  Were you thinking of submitting this patch as-is, and we would fix the WebPluginContainerImpl separately?  Or did you want us to do it in the same patch?  It should just be one or two additional lines of code, right? 

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

> Source/WebCore/platform/graphics/chromium/PluginLayerChromium.cpp:66
> +    m_updateRect = m_dirtyLayerRect;

Could we add a comment here, explaining that plugin layers are updated outside of the compositor code, but they notify what regions are updated via the dirty rect?

> Source/WebCore/platform/graphics/chromium/PluginLayerChromium.h:70
> +    FloatRect m_dirtyLayerRect;

Would it be OK if called it either "m_dirtyRect" to follow tradition, or "m_layerSpaceDirtyRect" for clarity?   m_dirtyLayerRect feels a bit like dirty is describing the entire layer instead of the rect =)
Comment 12 Antoine Labour 2011-12-06 11:04:04 PST
Comment on attachment 117694 [details]
Patch

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

> Source/WebKit/chromium/public/WebExternalTextureLayer.h:72
> +    WEBKIT_EXPORT void setNeedsDisplayRect(const WebFloatRect&);

So, you're not setting the region, you're unioning it. Could we name this something like markRectForDisplay or something? (and fix the comment).
Comment 13 Jonathan Backer 2011-12-06 14:38:43 PST
Created attachment 118114 [details]
Patch
Comment 14 Antoine Labour 2011-12-06 16:42:34 PST
Comment on attachment 118114 [details]
Patch

LGTM
Comment 15 James Robinson 2011-12-06 16:48:58 PST
Comment on attachment 118114 [details]
Patch

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

> Source/WebKit/chromium/public/platform/WebExternalTextureLayer.h:73
> +    // Marks a region of the layer as needing a display. These are regions are
> +    // collected in a union until the display occurs.
> +    WEBKIT_EXPORT void markRectForDisplay(const WebFloatRect&);

WebContentLayer spells this function "invalidateRect()", can we try to be consistent?
Comment 16 Jonathan Backer 2011-12-07 06:46:50 PST
As per jamesr@'s suggestion, I renamed markRectForDisplay to invalidateRect.

I added a small change to WebPluginContainerImpl.cpp to propagate damage to CCDamageTracker using the same mechanism as used by WebExternalTextureLayer.

I have tested with both http://www.webkit.org/blog-files/3d-transforms/poster-circle.html (for WebExternalTexture on use_aura=1 builds of Chromium) and simple_vertex_shader_ppapi (for WebPluginContainer).
Comment 17 Jonathan Backer 2011-12-07 06:51:52 PST
Created attachment 118205 [details]
Patch
Comment 18 Darin Fisher (:fishd, Google) 2011-12-07 10:03:30 PST
Comment on attachment 118205 [details]
Patch

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

This seems OK to me, but jamesr should give the final thumbs up.

> Source/WebKit/chromium/public/platform/WebExternalTextureLayer.h:71
> +    // Marks a region of the layer as needing a display. These are regions are

nit: "These are regions are"

So normal WebLayer's do not support partial invalidations?  Only external texture layers can do that?
Comment 19 Darin Fisher (:fishd, Google) 2011-12-07 10:04:28 PST
Comment on attachment 118205 [details]
Patch

(Looks like James already reviewed this, and it looks like his concerns were addressed.)
Comment 20 Antoine Labour 2011-12-07 10:15:08 PST
(In reply to comment #18)
> So normal WebLayer's do not support partial invalidations?  Only external texture layers can do that?

WebContentLayer has partial invalidations too. Though they mean something a bit different: invalidating a WebContentLayer is telling it pixels need to be painted, and the layer needs to be drawn; invalidating a WebExternalTextureLayer means the layer needs to be drawn (pixels have already changed).
WebLayer doesn't have pixels, so invalidation doesn't mean anything on it.
Comment 21 Jonathan Backer 2011-12-07 10:23:22 PST
Created attachment 118227 [details]
Patch
Comment 22 Jonathan Backer 2011-12-07 10:24:56 PST
Comment on attachment 118227 [details]
Patch

Fixed nits. Used proper bounds in WebPluginContainerImpl.cpp
Comment 23 WebKit Review Bot 2011-12-07 10:46:06 PST
Comment on attachment 118227 [details]
Patch

Clearing flags on attachment: 118227

Committed r102250: <http://trac.webkit.org/changeset/102250>
Comment 24 WebKit Review Bot 2011-12-07 10:46:13 PST
All reviewed patches have been landed.  Closing bug.