Bug 72921 – [chromium] Support Core Animation plugins in compositor

• Component: Plug-ins
• Status: RESOLVED FIXED

[Kenneth Russell]

In http://crbug.com/38967 it is desired to render Mac plugins using the Core Animation drawing model via the compositor rather than the current direct-to-screen rendering path. In order to support this, a few changes are needed:

  - The renderer needs to be able to transmit the IOSurface ID and its parameters to the WebPluginContainer.
  - The compositor needs support for ARB_texture_rectangle textures, since that is the only type compatible with IOSurfaces.
  - The compositor needs access to an entry point binding the IOSurface to a texture.

[Kenneth Russell]

Created attachment 116163 [details]
Patch

[WebKit Review Bot]

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

[Darin Fisher (:fishd, Google)]

Comment on attachment 116163 [details]
Patch

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

Comments for WebKit API changes only:

> Source/WebKit/chromium/public/WebPluginContainer.h:68
> +    virtual void acceleratedPluginAllocatedIOSurface(int width,

how about naming it something shorter, like: didAllocateIOSurface.  do you want to use WebSize for the width/height params?  (doesn't matter)

given that this method is called instead of setBackingTextureId, perhaps we should give this function a name that looks similar.
setBackingIOSurfaceId.

should we consider renaming commitBackingTexture since it also gets called when there isn't a backing texture but an IOSurface instead?

[Stephen White]

Comment on attachment 116163 [details]
Patch

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

Will wait on Nat for approval of the compositor changes.

> Source/WebCore/platform/graphics/chromium/ShaderChromium.cpp:314
> +String FragmentShaderRGBATexRectFlipAlpha::getShaderString() const

Nit:  To make it clear that it's Y being flipped, not alpha, perhaps this should be called TexRectFlipYAlpha or TexRectYFlipAlpha or (my preference)  TexRectAlphaFlipY.

[Nat Duca]

Comment on attachment 116163 [details]
Patch

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

Cool. Comments about threaded compositing mode?

/me is really sad at the messy state of affairs for multiprocess surface sharing. We have a different solution for every platform and its a nightmare. I really wish we could have someone try to take what Daneil started to try to create, which was a generic mechanism, and get it landed.

> Source/WebCore/platform/graphics/chromium/PluginLayerChromium.cpp:99
> +    pluginLayer->setIOSurfaceProperties(m_ioSurfaceWidth, m_ioSurfaceHeight, m_ioSurfaceId);

That this is not 2-buffered means we will see corruption in threaded compositing mode. Is that okay?

[Kenneth Russell]

Comment on attachment 116163 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/ShaderChromium.cpp:314
>> +String FragmentShaderRGBATexRectFlipAlpha::getShaderString() const
> 
> Nit:  To make it clear that it's Y being flipped, not alpha, perhaps this should be called TexRectFlipYAlpha or TexRectYFlipAlpha or (my preference)  TexRectAlphaFlipY.

Here I'm just following the existing naming convention for the other shaders -- e.g., FragmentShaderRGBATexFlipAlpha. I'm happy to rename them all, but in a subsequent patch.

[Kenneth Russell]

(In reply to comment #5)
> (From update of attachment 116163 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116163&action=review
> 
> Cool. Comments about threaded compositing mode?
> 
> /me is really sad at the messy state of affairs for multiprocess surface sharing. We have a different solution for every platform and its a nightmare. I really wish we could have someone try to take what Daneil started to try to create, which was a generic mechanism, and get it landed.
> 
> > Source/WebCore/platform/graphics/chromium/PluginLayerChromium.cpp:99
> > +    pluginLayer->setIOSurfaceProperties(m_ioSurfaceWidth, m_ioSurfaceHeight, m_ioSurfaceId);
> 
> That this is not 2-buffered means we will see corruption in threaded compositing mode. Is that okay?

IOSurfaces seem to be implicitly double-buffered between processes. We haven't ever seen tearing or incomplete rendering results when using them for displaying the compositor's output. I am pretty sure the usage here is OK, and that if there were a problem it would already be manifesting in the single-threaded compositor.

There is a problem with occasional display of garbage during live resizing of plugins which I haven't yet been able to completely solve, despite several attempts. It's a rare issue so I'm leaving it for subsequent work.

[Stephen White]

Comment on attachment 116163 [details]
Patch

I don't see any major objections from other reviewers, so r=me

[Kenneth Russell]

Created attachment 116277 [details]
Patch

[Kenneth Russell]

Renamed acceleratedPluginAllocatedIOSurface to setBackingIOSurfaceId based on fishd's review.

[Kenneth Russell]

Submitting to cq assuming that senorblanco's r+ on previous patch carries over.

[Stephen White]

Comment on attachment 116277 [details]
Patch

I don't think it does carry over.  r=me

[Kenneth Russell]

The commit queue is stuck for reasons orthogonal to this patch so I'm going to land it by hand.

[Kenneth Russell]

Committed r101035: <http://trac.webkit.org/changeset/101035>