[webkit-reviews] review denied: [Bug 111126] Enable GPU-GPU texture copy in texImage2D() for HTMLVideoElement if hardware accelerated video decode is in use : [Attachment 190881] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Jun Jiang <jun.a.jiang at intel.com>'s
request for review:
Bug 111126: Enable GPU-GPU texture copy in texImage2D() for HTMLVideoElement if
hardware accelerated video decode is in use
https://bugs.webkit.org/show_bug.cgi?id=111126

Attachment 190881: Patch
https://bugs.webkit.org/attachment.cgi?id=190881&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
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.