Add a new API in WebMediaPlayer to do a GPU-GPU textures copy if possible

webkit/media/webmediaplayer_impl.cc

Index: webkit/media/webmediaplayer_impl.cc
diff --git a/webkit/media/webmediaplayer_impl.cc b/webkit/media/webmediaplayer_impl.cc
index 645a508f88893161f01295b355b34c41edbaf336..a3a86a0d9239525d6d4a6729c8105ef236c994a4 100644
--- a/webkit/media/webmediaplayer_impl.cc
+++ b/webkit/media/webmediaplayer_impl.cc
@@ -25,6 +25,7 @@
 #include "media/filters/audio_renderer_impl.h"
 #include "media/filters/chunk_demuxer.h"
 #include "media/filters/video_renderer_base.h"
+#include "third_party/khronos/GLES2/gl2.h"
 #include "third_party/WebKit/Source/Platform/chromium/public/WebRect.h"
 #include "third_party/WebKit/Source/Platform/chromium/public/WebSize.h"
 #include "third_party/WebKit/Source/Platform/chromium/public/WebString.h"
@@ -655,6 +656,36 @@ void WebMediaPlayerImpl::putCurrentFrame(
   delete web_video_frame;
 }
 
+bool WebMediaPlayerImpl::videoDecodeAcceleratedByGpu()

[scherkus (not reviewing), 2013/03/06 21:37:37]

chromium style has { on the same line as functions

also please limit lines to 80 characters and follow the indentation and style
guides:
http://dev.chromium.org/developers/coding-style

[Jun Jiang, 2013/03/07 13:55:04]

Thanks for sharing the coding-style for chromium. Is there any separate script
can help to check rule-break code? I will appreciate for your help.

[scherkus (not reviewing), 2013/03/07 23:46:09]

cpplint.py should be part of depot_tools and it will catch some of the more
obvious errors

+{
+  scoped_refptr<media::VideoFrame> video_frame;
+  {
+    base::AutoLock auto_lock(lock_);
+    video_frame = current_frame_;
+  }
+
+  if (video_frame)
+    return video_frame->format() == media::VideoFrame::NATIVE_TEXTURE && video_frame->texture_target() == GL_TEXTURE_2D;
+  return false;
+}
+
+bool WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(WebKit::WebGraphicsContext3D* webGraphicsContext, unsigned int texture, unsigned int internalFormat)

[scherkus (not reviewing), 2013/03/06 21:37:37]

In your WK patch you call videoDecodeAcceleratedByGpu() and
copyVideoTextureToPlatformTexture() and require both to return true.

Do we even need to have videoDecodeAcceleratedByGpu()?

[Ken Russell (switch to Gerrit), 2013/03/07 00:13:23]

I agree; it looks redundant.

[Jun Jiang, 2013/03/07 13:55:04]

Yes, looking at the code alone in WebMediaPlayerImpl, the function 
videoDecodeAcceleratedByGpu() is redundant since it has similar check in
copyVideoTextureToPlatformTexture() too. But it may be different viewing from
the upper layer.
In webGLRenderingContext.cpp, the intent is to divide the logic into two parts.
The first part videoDecodeAcceleratedByGpu() is used as a condition, and the
second part copyVideoTextureToPlatformTexture() is used to do the work. That is:
if(HTMLVideoElement->videoDecodeAcceleratedByGpu())
    HTMLVideoElement->copyVideoTextureToPlatformTexture().
And the copyVideoTextureToPlatformTexture() defined in MediaPlayer is much
heavier than that defined in WebMediaPlayerImpl. It is reasonable to have a fast
return if it is not feasible.

[scherkus (not reviewing), 2013/03/07 23:46:09]

My preference is to keep it to one function for now based on our current
requirements and expand it to two functions later if we need it.

I also find the name "videoDecodeAcceleratedByGpu" potentially misleading /
overly specific. What if we have a texture available for the current video frame
that wasn't necessarily done by gpu hardware decoding?

[Jun Jiang, 2013/03/08 01:53:28]

Seeing this, it seems we'd better drop off the function
videoDecodeAcceleratedByGpu(). I have just had another idea, 
we can add another function videoFrameMaybeTextures(). The implementation is as
follows:
bool videoFrameMaybeTextures() { return true; }
This function can be defined in
Source/Webkit/chromium/src/WebMediaPlayerClientImpl.h and there is no change to
chromium public API. Another possible approach is to define it in
Source/WebKit/chromium/public/WebMediaPlayer.h and WebMediaPlayerImpl in
chromium can choose to implement it or not.
After adding this function, it may be more reasonable to call
copyVideoTextureToPlatformTexture() without abrupt since it may be textures and
we can try to do GPU-GPU textures copy. For other webkit ports, if videoFrame is
not textures for any case, then videoFrameMaybeTextures() always return false.
If videoFrame can be textures, those webkit port need to implement
copyVideoTextureToPlatformTexture() further.

scherkus and Kenneth, what's your opinion on this? Thanks in advance.

[scherkus (not reviewing), 2013/03/08 01:58:45]

My preference is to always try copyVideoTextureToPlatformTexture() (as its the
fast path) and fall back to the existing slower software path.

We can leave it up to individual ports to make sure
copyVideoTextureToPlatformTexture() fails fast.

Thoughts?

+{
+  if (!webGraphicsContext)

[scherkus (not reviewing), 2013/03/06 21:37:37]

is this possible or is this programmer error?

[Jun Jiang, 2013/03/07 13:55:04]

It is basically not possible to happen in current codebase and added to prevent
careless caller function in the future. As you suggested, I agree to remove this
check and it would look better.

+    return false;
+  scoped_refptr<media::VideoFrame> video_frame;
+  {
+    base::AutoLock auto_lock(lock_);
+    video_frame = current_frame_;
+  }
+  if (video_frame && video_frame->format() == media::VideoFrame::NATIVE_TEXTURE && video_frame->texture_target() == GL_TEXTURE_2D){
+    uint32 sourceTexture = video_frame->texture_id();
+    webGraphicsContext->copyTextureCHROMIUM(GL_TEXTURE_2D, sourceTexture, texture, 0, internalFormat);
+    return true;
+  }
+  return false;
+}
+
 #define COMPILE_ASSERT_MATCHING_STATUS_ENUM(webkit_name, chromium_name) \
     COMPILE_ASSERT(static_cast<int>(WebMediaPlayer::webkit_name) == \
                    static_cast<int>(media::ChunkDemuxer::chromium_name), \