Bug 72921 - [chromium] Support Core Animation plugins in compositor
Summary: [chromium] Support Core Animation plugins in compositor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.6
: P2 Normal
Assignee: Kenneth Russell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-21 17:33 PST by Kenneth Russell
Modified: 2011-11-22 18:02 PST (History)
9 users (show)

See Also:


Attachments
Patch (32.54 KB, patch)
2011-11-21 18:20 PST, Kenneth Russell
no flags Details | Formatted Diff | Diff
Patch (32.60 KB, patch)
2011-11-22 14:42 PST, Kenneth Russell
senorblanco: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2011-11-21 17:33:48 PST
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.
Comment 1 Kenneth Russell 2011-11-21 18:20:46 PST
Created attachment 116163 [details]
Patch
Comment 2 WebKit Review Bot 2011-11-21 18:25:18 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Darin Fisher (:fishd, Google) 2011-11-22 10:10:30 PST
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?
Comment 4 Stephen White 2011-11-22 12:51:37 PST
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.
Comment 5 Nat Duca 2011-11-22 13:00:30 PST
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?
Comment 6 Kenneth Russell 2011-11-22 13:40:41 PST
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.
Comment 7 Kenneth Russell 2011-11-22 13:45:58 PST
(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.
Comment 8 Stephen White 2011-11-22 14:30:23 PST
Comment on attachment 116163 [details]
Patch

I don't see any major objections from other reviewers, so r=me
Comment 9 Kenneth Russell 2011-11-22 14:42:00 PST
Created attachment 116277 [details]
Patch
Comment 10 Kenneth Russell 2011-11-22 14:42:38 PST
Renamed acceleratedPluginAllocatedIOSurface to setBackingIOSurfaceId based on fishd's review.
Comment 11 Kenneth Russell 2011-11-22 14:57:48 PST
Submitting to cq assuming that senorblanco's r+ on previous patch carries over.
Comment 12 Stephen White 2011-11-22 16:06:49 PST
Comment on attachment 116277 [details]
Patch

I don't think it does carry over.  r=me
Comment 13 Kenneth Russell 2011-11-22 17:59:37 PST
The commit queue is stuck for reasons orthogonal to this patch so I'm going to land it by hand.
Comment 14 Kenneth Russell 2011-11-22 18:01:16 PST
Committed r101035: <http://trac.webkit.org/changeset/101035>