[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 #190881|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #8 from Kenneth Russell <kbr at google.com>  2013-03-01 16:41:52 PST ---
(From update of attachment 190881)
View in context: https://bugs.webkit.org/attachment.cgi?id=190881&action=review

Thanks for your work on this. It's a good start, but looks like it needs more iteration.

> Source/WebCore/ChangeLog:10
> +        Already covered by current tests.

How much testing did you do? Are you sure that the accelerated video decode path has been exercised?

>> Source/WebCore/html/HTMLVideoElement.cpp:245
>> +    return player()->isVideoFrameOfFormatNativeTexture();
> 
> It seems strange that the visibility may change as a side effect of asking this question.

I agree. That seems not only strange but incorrect. What if the video tag is deliberately made invisible by the application so that it renders only within the WebGL scene, and not on the rest of the page?

> Source/WebCore/html/HTMLVideoElement.h:68
> +    bool copyVideoFrameInTexture(GraphicsContext3D&, Platform3DObject texture, GC3Denum internalFormat, bool premultiplyAlpha, bool flipY);

Please document the side-effects of this method. For example, does it redefine the texture, or expect it to already be the correct size? How is the internalFormat parameter used? (I realize that some of these semantics are defined by the basically undocumented GL_CHROMIUM_copy_texture extension, but please make it clear what the requirements are for other ports implementing this.) Consider doing this documentation in MediaPlayer.h and adding a comment pointing there from here.

It would be ideal if you could use enums here instead of bools so that the call sites are more descriptive, but I realize that that might be more trouble than it's worth since you have to pass these values all the way down to the platform layer.

Using a reference instead of a pointer for the GraphicsContext3D is confusing and doesn't match the rest of the code.

Finally, I'd like to suggest "isVideoFrameOfNativeTextureFormat" and "copyVideoFrameToTexture" as easier-to-read names for these methods.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3882
> +        && (format == GraphicsContext3D::RGB || format == GraphicsContext3D::RGBA) && type == GraphicsContext3D::UNSIGNED_BYTE) {

These checks are confusing. Can you write them in a more concise way? What about adding a method to WebGLTexture to indicate whether it's been defined yet?

>> Source/WebCore/platform/graphics/MediaPlayer.h:334
>> +    bool copyVideoFrameInTexture(GraphicsContext3D&, Platform3DObject texture, GC3Denum internalFormat, bool premultiplyAlpha, bool flipY);
> 
> Why does copyVideoFrameInTexture() take a format parameter but isVideoFrameOfFormatNativeTexture() does not?

When you add documentation for the semantics of HTMLVideoElement's method, please add a comment here pointing to it -- or better, put the documentation here and point HTMLVideoElement to it.

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:804
> +        context.pixelStorei(Extensions3D::UNPACK_PREMULTIPLY_ALPHA_CHROMIUM, false);

Changing the pixel store parameters of WebGL's OpenGL context during video upload is sloppy. I'm not sure this is even needed. Since GL_CHROMIUM_copy_texture obeys these pixel store parameters, and they've already been set in the passed GraphicsContext3D, I think these can just be unused parameters in this port. Please make sure that changing these parameters (at least the flipY parameter) is tested with the accelerated video decode path.

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