[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





--- Comment #9 from Jun Jiang <jun.a.jiang at intel.com>  2013-03-04 07:29:50 PST ---
(From update of attachment 190881)
View in context: https://bugs.webkit.org/attachment.cgi?id=190881&action=review

>> Source/WebCore/ChangeLog:8
>> +        In texImage2D() for HTMLVideoElement in WebGL, it is possible to do a GPU-GPU texture copy instead of CPU readback and upload when hardware accelerated video decode is in use. Each platform needs to implement the interface isVideoFrameOfFormatNativeTexture() and copyVideoFrameInTexture() to make it truely work.
> 
> Nit: please wrap this very long line.

Thanks, I will wrap the line.

>> 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?

I had tested Chromium on Linux, Win7 and Mac. As mentioned earlier, to make chromium work as expected, another patch at chromium https://codereview.chromium.org/12385073/ is needed.
1)there is no hardware accelerated video decode support for chromium on Linux. 
2)for Mac, hardware accelerated video decode is disabled in mac_video_decode_accelerator.mm. MacVideoDecodeAccelerator::Initialize() always return false if (true). After removing the "return false", the pipeline is ready but will meet GPU decode error. 
3)it works very well on win7. Using a small test example on my laptop to upload video to a cube (win7, Qua core 2.5GHz, Intel Graphics HD 3000, Video source(720p H264 video, 24 FPS) ), the latest Google chrome consumes 50% CPU, 76% GPU and gets a ~33 FPS for WebGL while latest chromium with my patch and H264 enabled in GYP_DEFINES consumes 26% CPU, %90 GPU and enjoys ~55 FPS for WebGL. GPU-GPU texture copy have much performance gain over CPU readback and upload.

>> Source/WebCore/ChangeLog:15
>> +        (WebCore::HTMLVideoElement::copyVideoFrameInTexture):
> 
> I think it is helpful to have per-method comments describing what changed.

Yes, I should do that. Basically,
1). isVideoFrameOfFormatNativeTexture() is used to query if the format of underlying VideoFrame is NativeTexture. If it is of format NativeTexture, then hardware accelerated video decode is in use. If it is of other format, such as YV12, YV16, etc, it is decoded using SW and GPU is not in use for decoding.
2). copyVideoFrameInTexture() is to do the GPU-GPU texture copy, which will return true only if GL_CHROMIUM_copy_texture extension is supported and other conditions are  met.

>>> 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?

Yes, setVisible(true) is not needed here. I was affected by the original SW path using paintCurrentFrameInContext() by mistake. I had created a new bug for it at https://bugs.webkit.org/show_bug.cgi?id=111302

>> 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.

Thanks for your suggestions. The patch will be refined accordingly.

>> 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?

will be refined.

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4116
>> +    }
> 
> Is this change required by this patch or is it just an opportunistic fix?

The code is just moved from videoFrameToImage() to check the condition in a upper level. You can notice that these check was removed in videoFrameToImage().

>>> 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.

will be refined.

>> 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.

My idea is that, The parameters UNPACK_FLIP_Y_CHROMIUM and UNPACK_PREMULTIPLY_ALPHA_CHROMIUM are cached in GraphicsContext3D but may not be in sync with that in  GPU comand buffer. So set the two parameters down every time is needed. Since the underlying GraphicsContext in GPU may be shared, it is better to reset them back to default.
I have re-tested flipY parameters. When setting UNPACK_FLIP_Y_WEBGL to true, the video is shown as normal. When setting it to false, the video is shown flipped. It seems I need to change "context.pixelStorei(Extensions3D::UNPACK_FLIP_Y_CHROMIUM, flipY);" to "context.pixelStorei(Extensions3D::UNPACK_FLIP_Y_CHROMIUM, !flipY);"

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