[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 #2 from Vangelis Kokkevis <vangelis at chromium.org>  2010-10-21 14:44:40 PST ---
(From update of attachment 71375)
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

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