Bug 73485 – [chromium] Plumb damage from WebExternalTextureLayer and WebPluginContainer to CCDamageTracker

• Component: New Bugs
• Status: RESOLVED FIXED

[Jonathan Backer]

[chromium] NOT FOR REVIEW Plumb damage in WebExternalTextureLayer to CCDamageTracker

[Jonathan Backer]

Created attachment 117263 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Jonathan Backer]

Chromium side here: http://codereview.chromium.org/8764001/

[Antoine Labour]

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.

[Shawn Singh]

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

[Antoine Labour]

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

[Antoine Labour]

Ok, following up from a discussion with S

[Antoine Labour]

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.

[Jonathan Backer]

Created attachment 117694 [details]
Patch

[Eric Seidel (no email)]

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.

[Shawn Singh]

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 =)

[Antoine Labour]

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).

[Jonathan Backer]

Created attachment 118114 [details]
Patch

[Antoine Labour]

Comment on attachment 118114 [details]
Patch

LGTM

[James Robinson]

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?

[Jonathan Backer]

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).

[Jonathan Backer]

Created attachment 118205 [details]
Patch

[Darin Fisher (:fishd, Google)]

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?

[Darin Fisher (:fishd, Google)]

Comment on attachment 118205 [details]
Patch

(Looks like James already reviewed this, and it looks like his concerns were addressed.)

[Antoine Labour]

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

[Jonathan Backer]

Created attachment 118227 [details]
Patch

[Jonathan Backer]

Comment on attachment 118227 [details]
Patch

Fixed nits. Used proper bounds in WebPluginContainerImpl.cpp

[WebKit Review Bot]

Comment on attachment 118227 [details]
Patch

Clearing flags on attachment: 118227

Committed r102250: <http://trac.webkit.org/changeset/102250>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.