Bug 48032 – [chromium] Added PluginLayerChromium for hardware accelerated compositing of plugins

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Al]

PluginLayerChromium composites plugin instances that have an OpenGL texture ID in the namespace of the compositor with the rest of the page. I also modified WebPlugin, WebPluginContainer and WebPluginContainerImpl so they can propagate a plugin backing texture ID to the accelerated compositor.

[Al]

Created attachment 71375 [details]
Patch

[Vangelis Kokkevis]

Comment on attachment 71375 [details]
Patch

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

> WebCore/ChangeLog:7
> +
> +        Added PluginLayerChromium, which composites plugin instances that have an associated OpenGL backing texture.

nit: typically the (short) description goes before the bug URL.

Also, it's somewhat common to start the description with [chromium] if the patch affects only chromium code.

> WebCore/loader/SubframeLoader.cpp:370
> +#if ENABLE(PLUGIN_PROXY_FOR_VIDEO) || USE(ACCELERATED_COMPOSITING)

Is there any way to check that this change won't negatively affect other WebKit ports?

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:736
> +            || !m_videoLayerSharedValues->initialized() || !m_pluginLayerSharedValues->initialized()) {

formating: || should align with !m_layerSharedValues of the line above it

> WebCore/platform/graphics/chromium/PluginLayerChromium.cpp:117
> +    GLC(context, context->texParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST));

It's somewhat wasteful to set the sampler settings every time you draw.  This block of 4 sets could move to setTextureId() ?

> WebCore/platform/graphics/chromium/PluginLayerChromium.h:41
> +// A Layer containing a WebGL or accelerated 2d canvas

Please update the comment.

> WebKit/chromium/ChangeLog:7
> +        Plugin instances can propagate the ID of the OpenGL texture thy render to.

typo: thy

> WebKit/chromium/public/WebPluginContainer.h:59
> +    virtual unsigned getBackingTextureId() { return 0; }

WebPluginContainer is an abstract class.  Ideally we don't want to change than by providing default implementations here.

> WebKit/chromium/src/WebPluginContainerImpl.cpp:291
> +unsigned WebPluginContainerImpl::getBackingTextureId()

these two methods need to have #if USE(ACCELERATED_COMPOSITING) guards.

> WebKit/chromium/src/WebPluginContainerImpl.cpp:410
> +    unsigned backingTextureId = m_webPlugin->getBackingTextureId();

In a lost context scenario, the GL context used by the compositor will change.  The texture used by the plugin needs to be re-created and passed again to the layer. I'm realizing that none of the other layer types that manage their own textures (e.g. Video and WebGL) do that correctly either.

> WebKit/chromium/src/WebPluginContainerImpl.cpp:414
> +    static_cast<PluginLayerChromium*>(m_platformLayer.get())->setTextureId(backingTextureId);

If m_platformLayer becomes a PluginLayerChromium then you can also get rid of that cast.

> WebKit/chromium/src/WebPluginContainerImpl.h:146
> +    RefPtr<WebCore::LayerChromium> m_platformLayer;

Might be better for clarity to use the specific layer type (PluginLayerChromium) here.

Also, needs to be inside #if USE(ACCELERATED_COMPOSITING) guards

[Al]

Created attachment 71587 [details]
Patch

[WebKit Review Bot]

Attachment 71587 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:735:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Al]

Created attachment 71615 [details]
Patch

[WebKit Review Bot]

Attachment 71615 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:735:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Al]

(In reply to comment #3)
> Created an attachment (id=71587) [details]
> Patch

(In reply to comment #2)
> (From update of attachment 71375 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71375&action=review
> 
> > WebCore/ChangeLog:7
> > +
> > +        Added PluginLayerChromium, which composites plugin instances that have an associated OpenGL backing texture.
> 
> nit: typically the (short) description goes before the bug URL.
> 

Done

> Also, it's somewhat common to start the description with [chromium] if the patch affects only chromium code.
> 

Done

> > WebCore/loader/SubframeLoader.cpp:370
> > +#if ENABLE(PLUGIN_PROXY_FOR_VIDEO) || USE(ACCELERATED_COMPOSITING)
> 
> Is there any way to check that this change won't negatively affect other WebKit ports?
> 

It turns out this change was not necessary. I reverted it.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:736
> > +            || !m_videoLayerSharedValues->initialized() || !m_pluginLayerSharedValues->initialized()) {
> 
> formating: || should align with !m_layerSharedValues of the line above it
> 

Done but now webkit-check-style says it should be to the right on the line above.

> > WebCore/platform/graphics/chromium/PluginLayerChromium.cpp:117
> > +    GLC(context, context->texParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST));
> 
> It's somewhat wasteful to set the sampler settings every time you draw.  This block of 4 sets could move to setTextureId() ?
>

I will optimize this later.
 
> > WebCore/platform/graphics/chromium/PluginLayerChromium.h:41
> > +// A Layer containing a WebGL or accelerated 2d canvas
> 
> Please update the comment.
>

Done.
 
> > WebKit/chromium/ChangeLog:7
> > +        Plugin instances can propagate the ID of the OpenGL texture thy render to.
> 
> typo: thy
>

Done.
 
> > WebKit/chromium/public/WebPluginContainer.h:59
> > +    virtual unsigned getBackingTextureId() { return 0; }
> 
> WebPluginContainer is an abstract class.  Ideally we don't want to change than by providing default implementations here.
>

I will make them pure virtual when this is rolled into chromium.
 
> > WebKit/chromium/src/WebPluginContainerImpl.cpp:291
> > +unsigned WebPluginContainerImpl::getBackingTextureId()
> 
> these two methods need to have #if USE(ACCELERATED_COMPOSITING) guards.
> 

Done.

> > WebKit/chromium/src/WebPluginContainerImpl.cpp:410
> > +    unsigned backingTextureId = m_webPlugin->getBackingTextureId();
> 
> In a lost context scenario, the GL context used by the compositor will change.  The texture used by the plugin needs to be re-created and passed again to the layer. I'm realizing that none of the other layer types that manage their own textures (e.g. Video and WebGL) do that correctly either.
>

I will address this when we get the accelerated compositor recovering from context lost.

 
> > WebKit/chromium/src/WebPluginContainerImpl.cpp:414
> > +    static_cast<PluginLayerChromium*>(m_platformLayer.get())->setTextureId(backingTextureId);
> 
> If m_platformLayer becomes a PluginLayerChromium then you can also get rid of that cast.
>

Done.
 
> > WebKit/chromium/src/WebPluginContainerImpl.h:146
> > +    RefPtr<WebCore::LayerChromium> m_platformLayer;
> 
> Might be better for clarity to use the specific layer type (PluginLayerChromium) here.
> 
> Also, needs to be inside #if USE(ACCELERATED_COMPOSITING) guards

Done and done.

[Vangelis Kokkevis]

Comment on attachment 71615 [details]
Patch

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

> WebCore/ChangeLog:10
> +        * loader/SubframeLoader.cpp:

You'll need to regenerate the ChangeLog or remove SubframeLoader entry.

> WebKit/chromium/public/WebPlugin.h:70
> +    // TODO(apatrick): Make this pure virtual once this change is rolled into chromium.

WebKit doesn't have TODO(name).  Please use "FIXME" instead (no name)

> WebKit/chromium/public/WebPluginContainer.h:59
> +    // TODO(apatrick): Make this pure virtual once this change is rolled into chromium.

Same for this TODO

> WebKit/chromium/public/WebPluginContainer.h:63
> +    // TODO(apatrick): Make this pure virtual once this change is rolled into chromium.

And here

> WebKit/chromium/src/WebPluginContainerImpl.cpp:292
> +

nit: typically there are no blank after the #if or before the #end

[Vangelis Kokkevis]

> 
> > > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:736
> > > +            || !m_videoLayerSharedValues->initialized() || !m_pluginLayerSharedValues->initialized()) {
> > 
> > formating: || should align with !m_layerSharedValues of the line above it
> > 
> 
> Done but now webkit-check-style says it should be to the right on the line above.
> 
It's because the boolean || needs to be moved to the line below but line up with the contents of line above.

> > > WebCore/platform/graphics/chromium/PluginLayerChromium.cpp:117
> > > +    GLC(context, context->texParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST));
> > 
> > It's somewhat wasteful to set the sampler settings every time you draw.  This block of 4 sets could move to setTextureId() ?
> >
> 
> I will optimize this later.
> 

Please add a FIXME so that we don't forget.


> > > WebKit/chromium/src/WebPluginContainerImpl.cpp:410
> > > +    unsigned backingTextureId = m_webPlugin->getBackingTextureId();
> > 
> > In a lost context scenario, the GL context used by the compositor will change.  The texture used by the plugin needs to be re-created and passed again to the layer. I'm realizing that none of the other layer types that manage their own textures (e.g. Video and WebGL) do that correctly either.
> >
> 
> I will address this when we get the accelerated compositor recovering from context lost.
> 
Please add a FIXME

[Al]

Created attachment 71809 [details]
Patch

[Al]

(In reply to comment #8)
> (From update of attachment 71615 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71615&action=review
> 
> > WebCore/ChangeLog:10
> > +        * loader/SubframeLoader.cpp:
> 
> You'll need to regenerate the ChangeLog or remove SubframeLoader entry.
> 

I had to add this file back. I removed the [chromium] tag since it isn't chromium specific.

> > WebKit/chromium/public/WebPlugin.h:70
> > +    // TODO(apatrick): Make this pure virtual once this change is rolled into chromium.
> 
> WebKit doesn't have TODO(name).  Please use "FIXME" instead (no name)
> 

Done

> > WebKit/chromium/public/WebPluginContainer.h:59
> > +    // TODO(apatrick): Make this pure virtual once this change is rolled into chromium.
> 
> Same for this TODO
> 

Done

> > WebKit/chromium/public/WebPluginContainer.h:63
> > +    // TODO(apatrick): Make this pure virtual once this change is rolled into chromium.
> 
> And here
>

Done
 
> > WebKit/chromium/src/WebPluginContainerImpl.cpp:292
> > +
> 
> nit: typically there are no blank after the #if or before the #end

Done

[Al]

(In reply to comment #9)
> > 
> > > > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:736
> > > > +            || !m_videoLayerSharedValues->initialized() || !m_pluginLayerSharedValues->initialized()) {
> > > 
> > > formating: || should align with !m_layerSharedValues of the line above it
> > > 
> > 
> > Done but now webkit-check-style says it should be to the right on the line above.
> > 
> It's because the boolean || needs to be moved to the line below but line up with the contents of line above.
> 

Done

> > > > WebCore/platform/graphics/chromium/PluginLayerChromium.cpp:117
> > > > +    GLC(context, context->texParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST));
> > > 
> > > It's somewhat wasteful to set the sampler settings every time you draw.  This block of 4 sets could move to setTextureId() ?
> > >
> > 
> > I will optimize this later.
> > 
> 
> Please add a FIXME so that we don't forget.
> 
>

Done
 
> > > > WebKit/chromium/src/WebPluginContainerImpl.cpp:410
> > > > +    unsigned backingTextureId = m_webPlugin->getBackingTextureId();
> > > 
> > > In a lost context scenario, the GL context used by the compositor will change.  The texture used by the plugin needs to be re-created and passed again to the layer. I'm realizing that none of the other layer types that manage their own textures (e.g. Video and WebGL) do that correctly either.
> > >
> > 
> > I will address this when we get the accelerated compositor recovering from context lost.
> > 
> Please add a FIXME

Done

[Kenneth Russell]

Comment on attachment 71809 [details]
Patch

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

This looks good to me. I'm only marking it r- because of the copyright headers. Please update them and re-upload the patch.

> WebCore/platform/graphics/chromium/PluginLayerChromium.cpp:28
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

It turns out that this is the wrong form of license header for contributed code to WebKit. Even though other files in this directory are also incorrect, let's not propagate the error further. You can find the correct header under WebKit/LICENSE or a C++ formatted version with the Google copyright in e.g. WebKit/chromium/tests/PODIntervalTreeTest.cpp.

> WebCore/platform/graphics/chromium/PluginLayerChromium.h:28
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

Fix copyright header here too.

> WebKit/chromium/public/WebPlugin.h:68
> +    // If the plugin instance is backed by an OpenGL, return its ID in the

by an OpenGL -> by an OpenGL context (?)

> WebKit/chromium/public/WebPluginContainer.h:57
> +    // If the plugin instance is backed by an OpenGL, return its ID in the

Same as above.

> WebKit/chromium/src/WebPluginContainerImpl.cpp:291
> +#if USE(ACCELERATED_COMPOSITING)

If the intent is to make WebPluginContainer::getBackingTextureId() pure virtual then you need an alternate path when USE(ACCELERATED_COMPOSITING) is not enabled. Perhaps conditionalize the method bodies.

> WebKit/chromium/src/WebPluginContainerImpl.h:92
> +#if USE(ACCELERATED_COMPOSITING)

Same comment as above about USE(ACCELERATED_COMPOSITING).

[Darin Fisher (:fishd, Google)]

Comment on attachment 71809 [details]
Patch

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

> WebKit/chromium/public/WebPlugin.h:70
> +    // FIXME: Make this pure virtual once this change is rolled into chromium.

note: methods implemented by the embedder do not need to be pure virtual.
it is actually encouraged to make them have default implementations to
simplify webkit rolls.

> WebKit/chromium/public/WebPluginContainer.h:60
> +    virtual unsigned getBackingTextureId() { return 0; };

why do you need this method here?  can't callers just get the WebPlugin
from the WebPluginContainer and call getBackingTextureId from there?

nit: no trailing ";"

> WebKit/chromium/public/WebPluginContainer.h:64
> +    virtual void commitBackingTexture() {};

nit: no trailing ";"

[Al]

Created attachment 72104 [details]
Patch

[Al]

(In reply to comment #13)
> (From update of attachment 71809 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71809&action=review
> 
> This looks good to me. I'm only marking it r- because of the copyright headers. Please update them and re-upload the patch.
> 
> > WebCore/platform/graphics/chromium/PluginLayerChromium.cpp:28
> > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> 
> It turns out that this is the wrong form of license header for contributed code to WebKit. Even though other files in this directory are also incorrect, let's not propagate the error further. You can find the correct header under WebKit/LICENSE or a C++ formatted version with the Google copyright in e.g. WebKit/chromium/tests/PODIntervalTreeTest.cpp.
> 

Done

> > WebCore/platform/graphics/chromium/PluginLayerChromium.h:28
> > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> 
> Fix copyright header here too.
>

Done
 
> > WebKit/chromium/public/WebPlugin.h:68
> > +    // If the plugin instance is backed by an OpenGL, return its ID in the
> 
> by an OpenGL -> by an OpenGL context (?)
> 

Done

> > WebKit/chromium/public/WebPluginContainer.h:57
> > +    // If the plugin instance is backed by an OpenGL, return its ID in the
> 
> Same as above.
> 

Done

> > WebKit/chromium/src/WebPluginContainerImpl.cpp:291
> > +#if USE(ACCELERATED_COMPOSITING)
> 
> If the intent is to make WebPluginContainer::getBackingTextureId() pure virtual then you need an alternate path when USE(ACCELERATED_COMPOSITING) is not enabled. Perhaps conditionalize the method bodies.
> 

As fishd advised, this will not be pure virtual.

> > WebKit/chromium/src/WebPluginContainerImpl.h:92
> > +#if USE(ACCELERATED_COMPOSITING)
> 
> Same comment as above about USE(ACCELERATED_COMPOSITING).

As above.

[Al]

(In reply to comment #14)
> (From update of attachment 71809 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71809&action=review
> 
> > WebKit/chromium/public/WebPlugin.h:70
> > +    // FIXME: Make this pure virtual once this change is rolled into chromium.
> 
> note: methods implemented by the embedder do not need to be pure virtual.
> it is actually encouraged to make them have default implementations to
> simplify webkit rolls.
> 

Done

> > WebKit/chromium/public/WebPluginContainer.h:60
> > +    virtual unsigned getBackingTextureId() { return 0; };
> 
> why do you need this method here?  can't callers just get the WebPlugin
> from the WebPluginContainer and call getBackingTextureId from there?
>

Done
 
> nit: no trailing ";"
> 
> > WebKit/chromium/public/WebPluginContainer.h:64
> > +    virtual void commitBackingTexture() {};
> 
> nit: no trailing ";"

Done

[Al]

Created attachment 72270 [details]
Patch

[Kenneth Russell]

Comment on attachment 72270 [details]
Patch

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

Sorry I didn't catch this earlier, but:

> WebKit/chromium/src/WebPluginContainerImpl.cpp:426
> +    , m_platformLayer(PluginLayerChromium::create(0))

Doesn't the initialization of m_platformLayer need to be guarded with #if USE(ACCELERATED_COMPOSITING)?

[Al]

Created attachment 72758 [details]
Patch

[Kenneth Russell]

Comment on attachment 72758 [details]
Patch

Looks good.

[WebKit Commit Bot]

Comment on attachment 72758 [details]
Patch

Clearing flags on attachment: 72758

Committed r71210: <http://trac.webkit.org/changeset/71210>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.