[Webkit-unassigned] [Bug 48032] [chromium] Added PluginLayerChromium for hardware accelerated compositing of plugins

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=48032





--- Comment #7 from Al <apatrick at chromium.org>  2010-10-22 18:42:26 PST ---
(In reply to comment #3)
> Created an attachment (id=71587)
 --> (https://bugs.webkit.org/attachment.cgi?id=71587&action=review) [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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.