[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 #22 from Jun Jiang <jun.a.jiang at intel.com>  2013-03-07 07:53:47 PST ---
(From update of attachment 191738)
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".

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