[webkit-reviews] review denied: [Bug 111126] Enable GPU-GPU texture copy in texImage2D() for HTMLVideoElement if hardware accelerated video decode is in use : [Attachment 191738] 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 191738: Patch
https://bugs.webkit.org/attachment.cgi?id=191738&action=review

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