[Webkit-unassigned] [Bug 93010] [chromium] Add WebExternalTextureProvider; an alternate means for providing textures to a TextureLayerChromium

[bugzilla-daemon at webkit.org]

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


James Robinson <jamesr at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #168092|review?                     |review-
               Flag|                            |




--- Comment #14 from James Robinson <jamesr at chromium.org>  2012-10-11 17:01:53 PST ---
(From update of attachment 168092)
View in context: https://bugs.webkit.org/attachment.cgi?id=168092&action=review

We need some tests for this - take a look at Tools/DumpRenderTree/chromium/TestWebPlugin.cpp for one way to approach tests in layout tests.

> Source/Platform/chromium/public/WebExternalTextureLayer.h:84
> +    virtual void setTextureProvider(WebExternalTextureProvider*) { };

no ;

> Source/Platform/chromium/public/WebExternalTextureProvider.h:8
> + *     * Redistributions of source code must retain the above copyright

2-clause is preferred for new files. See Source/WebCore/LICENSE-APPLE

> Source/Platform/chromium/public/WebExternalTextureProvider.h:52
> +        // Notifies the provider's client that a call to getCurrentFrame() will return new data.

I don't see any getCurrentFrame() - what is this comment referring to?

> Source/Platform/chromium/public/WebExternalTextureProvider.h:56
> +        // Insert a synchronization point in the graphics command stream
> +        virtual unsigned insertSyncPoint() = 0; 

What does this mean and what is the return value? The inserted sync point?

> Source/Platform/chromium/public/WebExternalTextureProvider.h:59
> +    virtual void setTextureProviderClient(Client*) = 0;

This could use some documentation - what are the threading requirements of this call? How often can it be called?

> Source/Platform/chromium/public/WebExternalTextureProvider.h:64
> +    virtual bool premultipliedAlpha() const = 0;
> +    virtual bool flipped() const = 0;
> +    virtual WebFloatRect uvRect() const = 0;

Why are these all pull?  Is it expected that these could change from frame to frame and that the user of this provider has to check their value every time it presents?

> Source/WebKit/chromium/public/WebPluginContainer.h:75
> +    virtual void setBackingTextureProvider(WebExternalTextureProvider*) = 0;

Please separate this with blank lines from adjacent functions.  Also say something about how this interacts with setBackingTexture() / setBackingIOSurfaceId().  I believe they're all mutually exclusive but having that explicit here would be easier for callers.

> Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:401
> +    if (m_textureProvider == provider)

Do we really want to support setting different providers over the same plugin's lifetime?  That seems a bit tricksy to manage on the impl side and not something that we need for video layers currently

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