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

• Component: WebGL
• Status: RESOLVED FIXED

[Jun Jiang]

Hardware accelerated video decode is supported in most platforms like Windows, Mac OSX, ChromeOS, etc, to improve Video playback performance. There is a possibility to do a GPU-GPU texture copy instead of CPU readback and upload in texImage2D() for HTMLVideoElement in WebGL when hardware accelerated video decode is in use.

[Jun Jiang]

Created attachment 190838 [details]
Patch

[Build Bot]

Comment on attachment 190838 [details]
Patch

Attachment 190838 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16772714

[Build Bot]

Comment on attachment 190838 [details]
Patch

Attachment 190838 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16672799

[Jun Jiang]

Created attachment 190881 [details]
Patch

[Jun Jiang]

Fix the unused parameter error for Mac builds. 

At the very beginning, let chromium the first to support the GPU-GPU texture copy in texImage2D() for HTMLVideoElement in WebGL . And there is another small patch(webkit/media/webvideoframe_impl.h/.cc) in chromium project to work with this patch together to get GPU-GPU texture copy working.

[Eric Carlson]

Comment on attachment 190881 [details]
Patch

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.

> Source/WebCore/ChangeLog:14
> +        (WebCore):

These useless entries can be removed.

> Source/WebCore/ChangeLog:15
> +        (WebCore::HTMLVideoElement::copyVideoFrameInTexture):

I think it is helpful to have per-method comments describing what changed.

> Source/WebCore/html/HTMLVideoElement.cpp:245
> +    player()->setVisible(true);
> +    return player()->isVideoFrameOfFormatNativeTexture();

It seems strange that the visibility may change as a side effect of asking this question.

[Eric Carlson]

Comment on attachment 190881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190881&action=review

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4116
> +    if (!video || !video->videoWidth() || !video->videoHeight()) {
> +        synthesizeGLError(GraphicsContext3D::INVALID_VALUE, "texSubImage2D", "no video");
> +        return;
> +    }

Is this change required by this patch or is it just an opportunistic fix?

> Source/WebCore/platform/graphics/MediaPlayer.h:334
> +    bool isVideoFrameOfFormatNativeTexture();
> +    bool copyVideoFrameInTexture(GraphicsContext3D&, Platform3DObject texture, GC3Denum internalFormat, bool premultiplyAlpha, bool flipY);

Why does copyVideoFrameInTexture() take a format parameter but isVideoFrameOfFormatNativeTexture() does not?

[Kenneth Russell]

Comment on attachment 190881 [details]
Patch

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.

[Jun Jiang]

Comment on attachment 190881 [details]
Patch

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);"

[Dana Jansens]

Is it possible to achieve this without using WebVideoFrame in webkit (ie move logic over to the chromium side of the boundary)? I'm meaning to make WebVideoFrame a completely empty base class, used only for shuffling data from chromium to chromium (and eventually removing it from WebKit entirely). Currently the only user of the WebVideoFrame is some android code which I was meaning to get around to fixing. But this moves in the opposite direction.

[Kenneth Russell]

(In reply to comment #9)
> 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.

OK, I see. I was confusing these parameters with the WebGL-specific ones (UNPACK_FLIP_Y_WEBGL, etc.) which are handled entirely by WebGLRenderingContext.

> 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);"

Yes, I can imagine there would be an issue like that. Please document very clearly any workaround like this which is needed.

[Kenneth Russell]

(In reply to comment #10)
> Is it possible to achieve this without using WebVideoFrame in webkit (ie move logic over to the chromium side of the boundary)? I'm meaning to make WebVideoFrame a completely empty base class, used only for shuffling data from chromium to chromium (and eventually removing it from WebKit entirely). Currently the only user of the WebVideoFrame is some android code which I was meaning to get around to fixing. But this moves in the opposite direction.

Would the preferred way to do this be to add a new method to WebMediaPlayer which does the work?

[James Robinson]

(In reply to comment #12)
> (In reply to comment #10)
> > Is it possible to achieve this without using WebVideoFrame in webkit (ie move logic over to the chromium side of the boundary)? I'm meaning to make WebVideoFrame a completely empty base class, used only for shuffling data from chromium to chromium (and eventually removing it from WebKit entirely). Currently the only user of the WebVideoFrame is some android code which I was meaning to get around to fixing. But this moves in the opposite direction.
> 
> Would the preferred way to do this be to add a new method to WebMediaPlayer which does the work?

Yeah, I think that'd be a better direction.

[Kenneth Russell]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > Is it possible to achieve this without using WebVideoFrame in webkit (ie move logic over to the chromium side of the boundary)? I'm meaning to make WebVideoFrame a completely empty base class, used only for shuffling data from chromium to chromium (and eventually removing it from WebKit entirely). Currently the only user of the WebVideoFrame is some android code which I was meaning to get around to fixing. But this moves in the opposite direction.
> > 
> > Would the preferred way to do this be to add a new method to WebMediaPlayer which does the work?
> 
> Yeah, I think that'd be a better direction.

OK. Jun, could you recast your patch in that direction? I guess it actually isn't necessary in that case to fetch the WebVideoFrame, which would make it better from Dana's standpoint.

[Jun Jiang]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (In reply to comment #10)
> > > > Is it possible to achieve this without using WebVideoFrame in webkit (ie move logic over to the chromium side of the boundary)? I'm meaning to make WebVideoFrame a completely empty base class, used only for shuffling data from chromium to chromium (and eventually removing it from WebKit entirely). Currently the only user of the WebVideoFrame is some android code which I was meaning to get around to fixing. But this moves in the opposite direction.
> > > 
> > > Would the preferred way to do this be to add a new method to WebMediaPlayer which does the work?
> > 
> > Yeah, I think that'd be a better direction.
> 
> OK. Jun, could you recast your patch in that direction? I guess it actually isn't necessary in that case to fetch the WebVideoFrame, which would make it better from Dana's standpoint.

Yes, I agree with all of you and it is the right direction. Adding new APIs in WebMediaPlayer and letting chromium side to determine the implementations are much more flexible and suitable for later change.

[James Robinson]

For the record, I'd like to move *all* of the compositor <-> media engine interactions to the chromium side.  My ideal world would have something like:

namespace WebKit {

class WebMediaPlayer {
public:
...
   WebLayer* createCompositingLayer() = 0;
};

and the implementation in chromium would construct a cc::VideoLayer, wrap it in a WebLayer, and return that back to WebKit so WebCore can insert it into the compositing tree in the correct place.  Then, from that point on, all interactions would be between cc::VideoLayer and the media:: pipeline stuff, using media::VideoFrame as the video frame data structure.  This would mean no WebVideoLayer, no WebVideoFrame, no WebVideoFrameProvider, no adapters for any of these, etc.  I just haven't had time to sit down and think through this fully (especially the startup/shutdown synchronization).

So yes, adding this new functionality on the chromium side will definitely be more future proof.

[Andrew Scherkus]

(In reply to comment #16)
> For the record, I'd like to move *all* of the compositor <-> media engine interactions to the chromium side.  My ideal world would have something like:
> 
> namespace WebKit {
> 
> class WebMediaPlayer {
> public:
> ...
>    WebLayer* createCompositingLayer() = 0;
> };
> 
> and the implementation in chromium would construct a cc::VideoLayer, wrap it in a WebLayer, and return that back to WebKit so WebCore can insert it into the compositing tree in the correct place.  Then, from that point on, all interactions would be between cc::VideoLayer and the media:: pipeline stuff, using media::VideoFrame as the video frame data structure.  This would mean no WebVideoLayer, no WebVideoFrame, no WebVideoFrameProvider, no adapters for any of these, etc.  I just haven't had time to sit down and think through this fully (especially the startup/shutdown synchronization).
> 
> So yes, adding this new functionality on the chromium side will definitely be more future proof.

jamesr: +100000 (I'd be happy to help think things through -- I have some scatterbrained ideas as well)

[Jun Jiang]

Created attachment 191738 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Jun Jiang]

Updated the patch to fix the Visible side effect, remove use of WebVideoFrame and add more comments. The relevant patch at chromium part is at https://codereview.chromium.org/12412007/ for review. The flipY value is respected and not reversed to make the GPU path get the same result as the original SW path. It should be the Application to set right FlipY value to get the expected result.

The test cases to evaluate the WebGL performance can be found at https://github.com/Jun-Jiang/WebGL_examples.
And the build out binaries for comparison can be found at https://github.com/Jun-Jiang/Chrome_binaries.

[Kenneth Russell]

Comment on attachment 191738 [details]
Patch

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.

[Jun Jiang]

Comment on attachment 191738 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191738&action=review

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

The current limitation here is that at the bottom level(shaders) GPU-GPU textures copy, it requires the type be UNSIGNED_BYTE. 
While in GPU command buffer implementation, DoCopyTexture() is implemented like this:
1): if the destination texture is defined already, but the internalFormat, width or height is not met as required, the destination texture will be re-defined.
    The internalFormat, width and height will be re-defined to the expected value. But the type would not be changed during the re-define process.
2): if the destination texture is not defined, the destination texture will be defined.
    The internalFormat, width, height and type will be defined to the same as the source textures.
To sum up, the texture is either not defined(since the type of source texture from Video is assumed to be UNSIGNED_BYTE) or defined but with the type equals UNSIGNED_BYTE. The code is should be changed form 
...(texture->getType(target, level) == type || !texture->isValid(target, level))... && type == UNSIGNED_BYTE
to ...((texture->getType(target, level) ==  UNSIGNED_BYTE) || !texture->isValid(target, level))... 
and it will be better to understand. The condition "type == UNSIGNED_BYTE" is put last in my code and may be easily ignored.

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

Yes, there is a mistake here. Although the level of destination texture is assumed to at 0 here, it can't be hard-coded with no check.
Moreover, it may be worthy to check if level > 0 works though all of current tests done set level to 0.
Will update it to add a level parameter here even the level is restricted to 0 only.

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4194
>> +        return;
> 
> The duplication of these validation checks is unfortunate. Can they be refactored?

Do you mean validateTexFuncFormatAndType() and validateSettableTexFormat() have duplicate checks?

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

The type is assumed to be UNSIGNED_BYTE(defined texture) or 0(undefined texture) here. And in GPU command buffer in chromium port, it will be unified to UNSIGNED_BYTE. 
For level, it is assumed to be 0 here. 

However, since it is a file shared between different ports and the limitation may be relaxed one day, these two places need to be changed to add arguments.

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

Yes, will refine the comments.

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

Yes. It behaves the same for GPU-accelerated video and non-GPU-accelerated video. Will change "position" to "orientation".

[Jun Jiang]

View in context: https://bugs.webkit.org/attachment.cgi?id=191738&action=review

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

The current limitation here is that at the bottom level(shaders) GPU-GPU textures copy, it requires the type be UNSIGNED_BYTE. 
While in GPU command buffer implementation, DoCopyTexture() is implemented like this:
1): if the destination texture is defined already, but the internalFormat, width or height is not met as required, the destination texture will be re-defined.
    The internalFormat, width and height will be re-defined to the expected value. But the type would not be changed during the re-define process.
2): if the destination texture is not defined, the destination texture will be defined.
    The internalFormat, width, height and type will be defined to the same as the source textures.
To sum up, the texture is either not defined(since the type of source texture from Video is assumed to be UNSIGNED_BYTE) or defined but with the type equals UNSIGNED_BYTE. The code is should be changed form 
...(texture->getType(target, level) == type || !texture->isValid(target, level))... && type == UNSIGNED_BYTE
to ...((texture->getType(target, level) ==  UNSIGNED_BYTE) || !texture->isValid(target, level))... 
and it will be better to understand. The condition "type == UNSIGNED_BYTE" is put last in my code and may be easily ignored.

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

Yes, there is a mistake here. Although the level of destination texture is assumed to at 0 here, it can't be hard-coded with no check.
Moreover, it may be worthy to check if level > 0 works though all of current tests done set level to 0.
Will update it to add a level parameter here even the level is restricted to 0 only.

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4194
>> +        return;
> 
> The duplication of these validation checks is unfortunate. Can they be refactored?

Do you mean validateTexFuncFormatAndType() and validateSettableTexFormat() have duplicate checks?

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

The type is assumed to be UNSIGNED_BYTE(defined texture) or 0(undefined texture) here. And in GPU command buffer in chromium port, it will be unified to UNSIGNED_BYTE. 
For level, it is assumed to be 0 here. 

However, since it is a file shared between different ports and the limitation may be relaxed one day, these two places need to be changed to add arguments.

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

Yes, will refine the comments.

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

Yes. It behaves the same for GPU-accelerated video and non-GPU-accelerated video. Will change "position" to "orientation".

[Kenneth Russell]

Comment on attachment 191738 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191738&action=review

>>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4194
>>> +        return;
>> 
>> The duplication of these validation checks is unfortunate. Can they be refactored?
> 
> Do you mean validateTexFuncFormatAndType() and validateSettableTexFormat() have duplicate checks?

No, I mean that the checks were removed from videoFrameToImage and duplicated into texImage2D and texSubImage2D.

[Jun Jiang]

Created attachment 192373 [details]
Patch

[Jun Jiang]

Reduced the two required APIs to one and make the code in WebCore more general. All the details are hide in platform layer. The corresponding patch for chromium project is at https://codereview.chromium.org/12412007/.

[Kenneth Russell]

Comment on attachment 192373 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192373&action=review

Thanks for your continued work on this. It looks good overall, and the Chromium API change looks fine, but there's a new layering violation that requires one more change.

> Source/WebCore/platform/graphics/MediaPlayer.h:350
> +    bool copyVideoTextureToPlatformTexture(GraphicsContext3D*, WebGLTexture*, GC3Dint level, GC3Denum type, GC3Denum internalFormat, bool premultiplyAlpha, bool flipY);

Code under platform/graphics is not supposed to reference WebCore types outside of platform/graphics like WebGLTexture*. While there are other violations of this rule in this class and in the WebGL implementation, it isn't good to add more. Please change the WebGLTexture* argument to Platform3DObject (defined in GraphicsTypes3D.h).

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:781
> +        return false;

Since according to the layering rules the WebGLTexture object won't be available here, please move these checks back up to WebGLRenderingContext.cpp and have them apply to all ports -- they aren't unreasonable restrictions. Also, please document the current restrictions in MediaPlayer.h.

[Jun Jiang]

Created attachment 192745 [details]
Patch

[Jun Jiang]

Thanks for Kenneth's suggestions. Updated the patch to remove use of WebGLTexture inside of platform/graphics and move the restriction check to upper level.

[Kenneth Russell]

Comment on attachment 192745 [details]
Patch

Very nice. Thank you for your hard work on this patch. r=me

[WebKit Review Bot]

Comment on attachment 192745 [details]
Patch

Clearing flags on attachment: 192745

Committed r145669: <http://trac.webkit.org/changeset/145669>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.