[Webkit-unassigned] [Bug 111126] Enable GPU-GPU texture copy in texImage2D() for HTMLVideoElement if hardware accelerated video decode is in use

[bugzilla-daemon at webkit.org]

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


Kenneth Russell <kbr at google.com> changed:

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




--- Comment #21 from Kenneth Russell <kbr at google.com>  2013-03-06 16:17:56 PST ---
(From update of attachment 191738)
View in context: https://bugs.webkit.org/attachment.cgi?id=191738&action=review

Thanks for your continued work on this. The structure looks better but there are still some issues to be fixed. Also, let's make sure the Chromium API is in its desired form before landing this.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3922
> +    if (GraphicsContext3D::TEXTURE_2D == target && texture && (texture->getType(target, level) == type || !texture->isValid(target, level))

Since this is texImage2D, it reallocates and redefines the texture at the given level. Why then is there a requirement that the texture is either not yet valid at that level, or that its existing type at that level is equal to the specified one here?

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3924
> +        if (video->videoDecodeAcceleratedByGpu() && video->copyVideoTextureToPlatformTexture(m_context.get(), texture->object(), internalformat, m_unpackPremultiplyAlpha, m_unpackFlipY)) {

If "level" isn't an argument to copyVideoTextureToPlatformTexture then this can't be doing the right thing in all cases. Either level needs to be restricted to 0 or something else needs to be changed here.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4194
> +        return;

The duplication of these validation checks is unfortunate. Can they be refactored?

> Source/WebCore/platform/graphics/MediaPlayer.h:342
> +    // The destination texture may need to be resized to the dimensions of the source texture or wrapped to the required internalFormat.

This description still isn't clear. Does this method resize/reallocate the destination texture (if necessary) or not? If it does, then why is there no "type" argument? (In the WebGLRenderingContext, this is stored along with the internalformat to validate subsequent calls to TexSubImage2D)? Why is there no "level" argument to this method? Is it implicitly 0?

> Source/WebCore/platform/graphics/MediaPlayer.h:345
> +    // http://src.chromium.org/viewvc/chrome/trunk/src/gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt.

Requiring this Chromium-specific extension is not a reasonable requirement. Ports can implement this method any way they want to or can. You could point to this extension as having the desired semantics, but not require the extension to be available.

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:789
> +    // flipY==true means to reverse the Video position while flipY==false means to keep the intrinsic position.

Does the flipY parameter behave the same for GPU-accelerated video and non-GPU-accelerated video? Also, "orientation" would be better than "position" here.

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