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

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

Signed-off-by: Jun Jiang <jun.a.jiang@intel.com>;

BUG=179754
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187827

[Jun Jiang, 2013-03-06 14:28:27]

The original discussion is at https://codereview.chromium.org/12385073/ and
https://bugs.webkit.org/show_bug.cgi?id=111126.

This is mainly to solve the problem that media::videoFrame is not accessible in
WebKit and WebVideoFrame would be dropped in the future.

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

I view this more as the 3D graphics context equivalent of void
paintCurrentFrameInContext(GraphicsContext*, const IntRect&);


I'm also curious how this ties into software decoded videos where we still want
to make use of the GPU compositor for scaling + YUV to RGB conversion. If I
understand the code path correctly we're still doing software YUV to RGB
conversion in the WebGL case.

https://codereview.chromium.org/12412007/diff/1/webkit/media/webmediaplayer_i...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/12412007/diff/1/webkit/media/webmediaplayer_i...
webkit/media/webmediaplayer_impl.cc:659: bool
WebMediaPlayerImpl::videoDecodeAcceleratedByGpu()
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

https://codereview.chromium.org/12412007/diff/1/webkit/media/webmediaplayer_i...
webkit/media/webmediaplayer_impl.cc:672: bool
WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(WebKit::WebGraphicsContext3D*
webGraphicsContext, unsigned int texture, unsigned int internalFormat)
In your WK patch you call videoDecodeAcceleratedByGpu() and
copyVideoTextureToPlatformTexture() and require both to return true.

Do we even need to have videoDecodeAcceleratedByGpu()?

https://codereview.chromium.org/12412007/diff/1/webkit/media/webmediaplayer_i...
webkit/media/webmediaplayer_impl.cc:674: if (!webGraphicsContext)
is this possible or is this programmer error?

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

https://codereview.chromium.org/12412007/diff/1/webkit/media/webmediaplayer_i...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/12412007/diff/1/webkit/media/webmediaplayer_i...
webkit/media/webmediaplayer_impl.cc:672: bool
WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(WebKit::WebGraphicsContext3D*
webGraphicsContext, unsigned int texture, unsigned int internalFormat)
On 2013/03/06 21:37:37, scherkus wrote:
> In your WK patch you call videoDecodeAcceleratedByGpu() and
> copyVideoTextureToPlatformTexture() and require both to return true.
> 
> Do we even need to have videoDecodeAcceleratedByGpu()?

I agree; it looks redundant.

[Jun Jiang, 2013-03-07 13:48:40]

Hi, scherkus and Kenneth. Thanks for your comments. It's better to finalize the
API at chromium side first and then continue the work at WebKit.
And yes, in WebKit, we're still doing the software YUV to RGB conversion for
software decoded videos. And it is the way we dream to use GPU instead of CPU
for the format conversion and scaling. The issue is that these operations are
normally provided by 2D library and I can't find relevant interface or
implementations in GPU command buffer. If there is such interface available,
WebKit side can take advantage of GPU and improve the performance for software
decoded video as well.  
For your other suggestions, please see my comments below.

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

https://codereview.chromium.org/12412007/diff/1/webkit/media/webmediaplayer_i...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/12412007/diff/1/webkit/media/webmediaplayer_i...
webkit/media/webmediaplayer_impl.cc:659: bool
WebMediaPlayerImpl::videoDecodeAcceleratedByGpu()
On 2013/03/06 21:37:37, scherkus wrote:
> 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
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.

https://codereview.chromium.org/12412007/diff/1/webkit/media/webmediaplayer_i...
webkit/media/webmediaplayer_impl.cc:672: bool
WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(WebKit::WebGraphicsContext3D*
webGraphicsContext, unsigned int texture, unsigned int internalFormat)
On 2013/03/07 00:13:23, kbr wrote:
> On 2013/03/06 21:37:37, scherkus wrote:
> > In your WK patch you call videoDecodeAcceleratedByGpu() and
> > copyVideoTextureToPlatformTexture() and require both to return true.
> > 
> > Do we even need to have videoDecodeAcceleratedByGpu()?
> 
> I agree; it looks redundant.
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.

https://codereview.chromium.org/12412007/diff/1/webkit/media/webmediaplayer_i...
webkit/media/webmediaplayer_impl.cc:674: if (!webGraphicsContext)
On 2013/03/06 21:37:37, scherkus wrote:
> is this possible or is this programmer error?
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.

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

https://codereview.chromium.org/12412007/diff/1/webkit/media/webmediaplayer_i...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/12412007/diff/1/webkit/media/webmediaplayer_i...
webkit/media/webmediaplayer_impl.cc:659: bool
WebMediaPlayerImpl::videoDecodeAcceleratedByGpu()
On 2013/03/07 13:55:04, Jun Jiang wrote:
> On 2013/03/06 21:37:37, scherkus wrote:
> > 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
> 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.
> 

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

https://codereview.chromium.org/12412007/diff/1/webkit/media/webmediaplayer_i...
webkit/media/webmediaplayer_impl.cc:672: bool
WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(WebKit::WebGraphicsContext3D*
webGraphicsContext, unsigned int texture, unsigned int internalFormat)
On 2013/03/07 13:55:04, Jun Jiang wrote:
> On 2013/03/07 00:13:23, kbr wrote:
> > On 2013/03/06 21:37:37, scherkus wrote:
> > > In your WK patch you call videoDecodeAcceleratedByGpu() and
> > > copyVideoTextureToPlatformTexture() and require both to return true.
> > > 
> > > Do we even need to have videoDecodeAcceleratedByGpu()?
> > 
> > I agree; it looks redundant.
> 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.
> 

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]

https://codereview.chromium.org/12412007/diff/1/webkit/media/webmediaplayer_i...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/12412007/diff/1/webkit/media/webmediaplayer_i...
webkit/media/webmediaplayer_impl.cc:672: bool
WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(WebKit::WebGraphicsContext3D*
webGraphicsContext, unsigned int texture, unsigned int internalFormat)
On 2013/03/07 23:46:09, scherkus wrote:
> On 2013/03/07 13:55:04, Jun Jiang wrote:
> > On 2013/03/07 00:13:23, kbr wrote:
> > > On 2013/03/06 21:37:37, scherkus wrote:
> > > > In your WK patch you call videoDecodeAcceleratedByGpu() and
> > > > copyVideoTextureToPlatformTexture() and require both to return true.
> > > > 
> > > > Do we even need to have videoDecodeAcceleratedByGpu()?
> > > 
> > > I agree; it looks redundant.
> > 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.
> > 
> 
> 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?
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]

https://codereview.chromium.org/12412007/diff/1/webkit/media/webmediaplayer_i...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/12412007/diff/1/webkit/media/webmediaplayer_i...
webkit/media/webmediaplayer_impl.cc:672: bool
WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(WebKit::WebGraphicsContext3D*
webGraphicsContext, unsigned int texture, unsigned int internalFormat)
On 2013/03/08 01:53:28, Jun Jiang wrote:
> On 2013/03/07 23:46:09, scherkus wrote:
> > On 2013/03/07 13:55:04, Jun Jiang wrote:
> > > On 2013/03/07 00:13:23, kbr wrote:
> > > > On 2013/03/06 21:37:37, scherkus wrote:
> > > > > In your WK patch you call videoDecodeAcceleratedByGpu() and
> > > > > copyVideoTextureToPlatformTexture() and require both to return true.
> > > > > 
> > > > > Do we even need to have videoDecodeAcceleratedByGpu()?
> > > > 
> > > > I agree; it looks redundant.
> > > 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.
> > > 
> > 
> > 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?
> 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.

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?

[Jun Jiang, 2013-03-08 02:18:55]

On 2013/03/08 01:58:45, scherkus wrote:
>
https://codereview.chromium.org/12412007/diff/1/webkit/media/webmediaplayer_i...
> File webkit/media/webmediaplayer_impl.cc (right):
> 
>
https://codereview.chromium.org/12412007/diff/1/webkit/media/webmediaplayer_i...
> webkit/media/webmediaplayer_impl.cc:672: bool
>
WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(WebKit::WebGraphicsContext3D*
> webGraphicsContext, unsigned int texture, unsigned int internalFormat)
> On 2013/03/08 01:53:28, Jun Jiang wrote:
> > On 2013/03/07 23:46:09, scherkus wrote:
> > > On 2013/03/07 13:55:04, Jun Jiang wrote:
> > > > On 2013/03/07 00:13:23, kbr wrote:
> > > > > On 2013/03/06 21:37:37, scherkus wrote:
> > > > > > In your WK patch you call videoDecodeAcceleratedByGpu() and
> > > > > > copyVideoTextureToPlatformTexture() and require both to return true.
> > > > > > 
> > > > > > Do we even need to have videoDecodeAcceleratedByGpu()?
> > > > > 
> > > > > I agree; it looks redundant.
> > > > 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.
> > > > 
> > > 
> > > 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?
> > 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.
> 
> 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?

I basically agree with you. The only remaining concern is that the name of
copyVideoTextureToPlatformTexture() suggest it is a Video texture. Somehow, if
we view this function as a fast path, it is Ok to call it directly.

[Jun Jiang, 2013-03-08 10:26:58]

On 2013/03/08 02:18:55, Jun Jiang wrote:
> On 2013/03/08 01:58:45, scherkus wrote:
> >
>
https://codereview.chromium.org/12412007/diff/1/webkit/media/webmediaplayer_i...
> > File webkit/media/webmediaplayer_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/12412007/diff/1/webkit/media/webmediaplayer_i...
> > webkit/media/webmediaplayer_impl.cc:672: bool
> >
>
WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(WebKit::WebGraphicsContext3D*
> > webGraphicsContext, unsigned int texture, unsigned int internalFormat)
> > On 2013/03/08 01:53:28, Jun Jiang wrote:
> > > On 2013/03/07 23:46:09, scherkus wrote:
> > > > On 2013/03/07 13:55:04, Jun Jiang wrote:
> > > > > On 2013/03/07 00:13:23, kbr wrote:
> > > > > > On 2013/03/06 21:37:37, scherkus wrote:
> > > > > > > In your WK patch you call videoDecodeAcceleratedByGpu() and
> > > > > > > copyVideoTextureToPlatformTexture() and require both to return
true.
> > > > > > > 
> > > > > > > Do we even need to have videoDecodeAcceleratedByGpu()?
> > > > > > 
> > > > > > I agree; it looks redundant.
> > > > > 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.
> > > > > 
> > > > 
> > > > 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?
> > > 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.
> > 
> > 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?
> 
> I basically agree with you. The only remaining concern is that the name of
> copyVideoTextureToPlatformTexture() suggest it is a Video texture. Somehow, if
> we view this function as a fast path, it is Ok to call it directly.

upload the new patch according to our discussion.

[scherkus (not reviewing), 2013-03-08 20:17:36]

few minor style nits -- thanks for doing this!

also: can you update the CL description to reflect the new API?
  - there's no longer an API to check
  - typo: "vidoe"

https://codereview.chromium.org/12412007/diff/13001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/12412007/diff/13001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:675: WebKit::WebGraphicsContext3D*
webGraphicsContext,
web_graphics_context

https://codereview.chromium.org/12412007/diff/13001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:678: unsigned int internalFormat) {
internal_format

https://codereview.chromium.org/12412007/diff/13001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:684: if (video_frame
chromium puts && at the end of lines

https://codereview.chromium.org/12412007/diff/13001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:687: uint32 sourceTexture =
video_frame->texture_id();
source_texture

https://codereview.chromium.org/12412007/diff/13001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/12412007/diff/13001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.h:147: // Do the GPU-GPU texture copy using
CHROMIUM_copy_texture extension
technically this comment belongs on the interface definition as opposed to the
implementation here, so I'd just remove it here

https://codereview.chromium.org/12412007/diff/13001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.h:149: WebKit::WebGraphicsContext3D*
webGraphicsContext,
chromium uses unix_hacker style for variable names -- yes, I understand things
get a bit confusing when you implement a WebKit interface in Chromium :)

web_graphics_context

https://codereview.chromium.org/12412007/diff/13001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.h:152: unsigned int internalFormat);
internal_format

[commit-bot: I haz the power, 2013-03-10 09:51:22]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Jun Jiang, 2013-03-10 13:45:30]

Hi,scherkus. Thanks for your comments. I have updated the patches at both
chromium and WebKit(https://bugs.webkit.org/show_bug.cgi?id=111126) to make it
easy to see the whole picture.

[scherkus (not reviewing), 2013-03-11 20:33:07]

lgtm

kbr/danakj: any other comments?

[danakj, 2013-03-11 23:17:34]

Nothin from me :)

[Ken Russell (switch to Gerrit), 2013-03-12 02:08:13]

LGTM

https://codereview.chromium.org/12412007/diff/20001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/12412007/diff/20001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:704: web_graphics_context->flush();
This flush shouldn't be necessary.

[Jun Jiang, 2013-03-12 13:38:38]

Hi, all. Thanks for your help on reviewing the patch and providing suggestions.
For the flush() operation, it may not be necessary but it can have some
performance benefit. Could we keep it?

https://codereview.chromium.org/12412007/diff/20001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/12412007/diff/20001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:704: web_graphics_context->flush();
On 2013/03/12 02:08:13, kbr wrote:
> This flush shouldn't be necessary.
Could we keep the flush operation since the flush operation could bring some
performance gain? I have tried the cases to enable threaded-composting or
disable threaded-composting on win7, the performance is always better with the
flush operation added than without that.

[Ken Russell (switch to Gerrit), 2013-03-12 22:17:46]

https://codereview.chromium.org/12412007/diff/20001/webkit/media/webmediaplay...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/12412007/diff/20001/webkit/media/webmediaplay...
webkit/media/webmediaplayer_impl.cc:704: web_graphics_context->flush();
On 2013/03/12 13:38:38, Jun Jiang wrote:
> On 2013/03/12 02:08:13, kbr wrote:
> > This flush shouldn't be necessary.
> Could we keep the flush operation since the flush operation could bring some
> performance gain? I have tried the cases to enable threaded-composting or
> disable threaded-composting on win7, the performance is always better with the
> flush operation added than without that.

Leaving it is fine, but please add a comment indicating that performance is
better with the flush than without it, and that that might warrant more
investigation.

[Jun Jiang, 2013-03-13 01:25:46]

Added comments on keeping the flush() operation according to Kenneth's
suggestions.

[Ken Russell (switch to Gerrit), 2013-03-13 01:36:01]

LGTM again. Are you able to check the commit box?

[commit-bot: I haz the power, 2013-03-13 01:39:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jun.a.jiang@intel.com/12412007/29001

[commit-bot: I haz the power, 2013-03-13 01:39:30]

Presubmit check for 12412007-29001 failed and returned exit status 1.

INFO:root:Found 2 file(s).
INFO:PRESUBMIT:Valid authors are *@chromium.org, *@google.com,
ibraaaa@gmail.com, rryk.ua@gmail.com, sanxiyn@gmail.com, alexscheele@gmail.com,
me@bramp.net, phajdan.jr@gmail.com, jesse@jmiller.biz, szymonpiechowicz@o2.pl,
vega.james@gmail.com, gothicx@gmail.com, reimarvin@gmail.com,
drpizza@quiscalusmexicanus.org, developer0420@gmail.com, yado.masa@gmail.com,
yarin.kaul@gmail.com, mendola@gmail.com, craig.schlenter@gmail.com,
ibrar.ahmad@gmail.com, takano.naoki@gmail.com, fta@sofaraway.org,
kunalt@gmail.com, m0.interactive@gmail.com, jroesslein@gmail.com,
sy3620@gmail.com, bekkra@gmail.com, raman.tenneti@gmail.com, kpn24@drexel.edu,
kimworking@gmail.com, paulrobinson85@googlemail.com, jorat1346@gmail.com,
tedoc2000@gmail.com, himikof@gmail.com, sean@cyberwang.net,
rsesek@bluestatic.org, j.dinata@gmail.com, chromium@willhirsch.co.uk,
yoav.zilberberg@gmail.com, joel@jms.id.au, jacob@mandelson.org,
yuri.gorobets@gmail.com, pwicks86@gmail.com, thiago.farina@gmail.com,
viettrungluu@gmail.com, pierre.lafayette@gmail.com, ffmpeg@gmail.com,
philippe.beauchamp@gmail.com, vedran.sajatovic@gmail.com,
randy.posynick@gmail.com, bruno@flock.com, jsorianopastor@gmail.com,
bdonlan@gmail.com, artagnon@gmail.com, dominic.jodoin@gmail.com,
googlecontrib@velox.ch, clemens@endorphin.org, sig11@reprehensible.net,
voyageur@gentoo.org, vt@foilhead.net, alexander@sulfrian.net,
philippe.beaudoin@gmail.com, mhahnenb@andrew.cmu.edu, agartrell@cmu.edu,
leith@leithalweapon.geek.nz, jchoi42@pha.jhu.edu, paul.l.kehrer@gmail.com,
chamalsl@yahoo.com, jaysoffian@gmail.com, bgmerrell@gmail.com,
appamatto@gmail.com, bgmerrell@gmail.com, ryan-chromium-dev@sleevi.com,
satoshi.matsuzaki@gmail.com, pcgod99@gmail.com, ningxin.hu@intel.com,
james.wei@intel.com, haitao.feng@intel.com, weinjared@gmail.com,
melvinxie@gmail.com, floppymaster@gmail.com, giuseppe@iuculano.it,
jwillcox@litl.com, v.a.shreyas@gmail.com, spenn@engr.uvic.ca,
jorge@tomatocannon.com, pnettleship@gmail.com, davidben@mit.edu,
venture37@geeklan.co.uk, peter@chromium.org, lauri.oherd@gmail.com,
eschew@gmail.com, sam@sammcd.com, fuzzac@gmail.com, kushi.p@gmail.com,
m.b.lankhorst@gmail.com, vipul.bhasin@gmail.com, rnorton10@gmail.com,
dill.sellars@gmail.com, seshadri.mahalingam@gmail.com, clementskau@gmail.com,
david.mike.futcher@gmail.com, ramkumar.gokarnesan@gmail.com,
mma.public@gmail.com, chromium@hybridsource.org, *@nvidia.com,
google@tk-webart.de, pph34r@gmail.com, lukezarko@gmail.com, fhd@ubercode.de,
ali.akbar@gmail.com, mathias@qiwi.be, mrs@mythic-beasts.com,
amruthraj@motorola.com, ckqr36@motorola.com, wxjg68@motorola.com,
ehsan.akhgari@gmail.com, chrelad@gmail.com, ncj674@motorola.com,
mtilburg@adobe.com, pbrophy@adobe.com, goldberg@adobe.com, woodward@adobe.com,
vinaya@adobe.com, naveenbobbili@motorola.com, vamshi@motorola.com,
robert.nagy@gmail.com, qtc746@motorola.com, blr.bmlab@gmail.com,
wrm364@motorola.com, rosen.dash@gmail.com, qghc36@motorola.com,
ravi.kasibhatla@motorola.com, nqk836@motorola.com, nrqv63@motorola.com,
tedvessenes@gmail.com, progame@chromium.org, brk376@motorola.com,
cristian.patrasciuc@gmail.com, halton.huo@intel.com, aofdwsl@gmail.com,
gaochun.dev@gmail.com, clintstaley@gmail.com, clintstaley@chromium.org,
rdevlin.cronin@gmail.com, junmin.zhu@intel.com, cem.kocagil@gmail.com,
simon.hong81@gmail.com, guanqun.lu@gmail.com, beaufort.francois@gmail.com,
eriq.augustine@gmail.com, francoisk777@gmail.com, erikghill@gmail.com,
maojie0924@gmail.com, pan.deng@intel.com, aaronlevbugs@gmail.com,
peter@pcc.me.uk, aaron.randolph@gmail.com, yumios.art@gmail.com,
matthewrobertson03@gmail.com, yujie.mao@intel.com, samuel.xu@intel.com,
jin.a.yang@intel.com, hexinchao@gmail.com, changbin.shao@intel.com,
stephen.searles@gmail.com, arun.m@samsung.com, trprice@gmail.com,
achicu@adobe.com, ekr@rtfm.com, wiss1976@gmail.com, erik.sjolund@gmail.com,
simon.arlott@gmail.com, alexkorep@gmail.com, mitchellwrosen@chromium.org,
yongsheng.zhu@intel.com, shouqun.liu@intel.com, kangyuan.shu@intel.com,
jake@helfert.us, hongbo.min@intel.com, tom.cassiotis@gmail.com,
evangelos@foutrelis.com, paivanof@gmail.com, rb@radix.io, petarj@mips.com,
carloschilazo@gmail.com, mmaerean@adobe.com, kaustubh.ra@gmail.com,
betravis@adobe.com, bear.travis@gmail.com, mvujovic@adobe.com,
jakob.j.w@googlemail.com, badea@adobe.com, joshua.lock@intel.com,
chunyang.dai@intel.com, mhx348@motorola.com, rubentopo@gmail.com,
josh.triplett@intel.com, qiankun.miao@intel.com, etienne@atnnn.com,
yang.gu@intel.com, ttr314@googlemail.com, limasdf@gmail.com,
m.s.bednorz@gmail.com, kamil.jiwa@gmail.com, keenepan@linpus.com,
unsafe@trevp.net, rosca@adobe.com, sylvinus@gmail.com, sungmann.cho@gmail.com,
fangjue23303@gmail.com, evan.peterson.ep@gmail.com, jryans@chromium.org,
matheusbrat@gmail.com, olaru@adobe.com, horia.olaru@gmail.com, *@opera.com,
johannes.rudolph@googlemail.com, samusaaron3@gmail.com, qufighter@gmail.com

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py

** Presubmit Warnings **
jun.a.jiang@intel.com is not in AUTHORS file. If you are a new contributor,
please visit
http://www.chromium.org/developers/contributing-code and read the "Legal"
section
If you are a chromite, verify the contributor signed the CLA.

Presubmit checks took 1.6s to calculate.

[commit-bot: I haz the power, 2013-03-13 01:42:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jun.a.jiang@intel.com/12412007/29001

[commit-bot: I haz the power, 2013-03-13 01:42:41]

Presubmit check for 12412007-29001 failed and returned exit status 1.

INFO:root:Found 2 file(s).
INFO:PRESUBMIT:Valid authors are *@chromium.org, *@google.com,
ibraaaa@gmail.com, rryk.ua@gmail.com, sanxiyn@gmail.com, alexscheele@gmail.com,
me@bramp.net, phajdan.jr@gmail.com, jesse@jmiller.biz, szymonpiechowicz@o2.pl,
vega.james@gmail.com, gothicx@gmail.com, reimarvin@gmail.com,
drpizza@quiscalusmexicanus.org, developer0420@gmail.com, yado.masa@gmail.com,
yarin.kaul@gmail.com, mendola@gmail.com, craig.schlenter@gmail.com,
ibrar.ahmad@gmail.com, takano.naoki@gmail.com, fta@sofaraway.org,
kunalt@gmail.com, m0.interactive@gmail.com, jroesslein@gmail.com,
sy3620@gmail.com, bekkra@gmail.com, raman.tenneti@gmail.com, kpn24@drexel.edu,
kimworking@gmail.com, paulrobinson85@googlemail.com, jorat1346@gmail.com,
tedoc2000@gmail.com, himikof@gmail.com, sean@cyberwang.net,
rsesek@bluestatic.org, j.dinata@gmail.com, chromium@willhirsch.co.uk,
yoav.zilberberg@gmail.com, joel@jms.id.au, jacob@mandelson.org,
yuri.gorobets@gmail.com, pwicks86@gmail.com, thiago.farina@gmail.com,
viettrungluu@gmail.com, pierre.lafayette@gmail.com, ffmpeg@gmail.com,
philippe.beauchamp@gmail.com, vedran.sajatovic@gmail.com,
randy.posynick@gmail.com, bruno@flock.com, jsorianopastor@gmail.com,
bdonlan@gmail.com, artagnon@gmail.com, dominic.jodoin@gmail.com,
googlecontrib@velox.ch, clemens@endorphin.org, sig11@reprehensible.net,
voyageur@gentoo.org, vt@foilhead.net, alexander@sulfrian.net,
philippe.beaudoin@gmail.com, mhahnenb@andrew.cmu.edu, agartrell@cmu.edu,
leith@leithalweapon.geek.nz, jchoi42@pha.jhu.edu, paul.l.kehrer@gmail.com,
chamalsl@yahoo.com, jaysoffian@gmail.com, bgmerrell@gmail.com,
appamatto@gmail.com, bgmerrell@gmail.com, ryan-chromium-dev@sleevi.com,
satoshi.matsuzaki@gmail.com, pcgod99@gmail.com, ningxin.hu@intel.com,
james.wei@intel.com, haitao.feng@intel.com, weinjared@gmail.com,
melvinxie@gmail.com, floppymaster@gmail.com, giuseppe@iuculano.it,
jwillcox@litl.com, v.a.shreyas@gmail.com, spenn@engr.uvic.ca,
jorge@tomatocannon.com, pnettleship@gmail.com, davidben@mit.edu,
venture37@geeklan.co.uk, peter@chromium.org, lauri.oherd@gmail.com,
eschew@gmail.com, sam@sammcd.com, fuzzac@gmail.com, kushi.p@gmail.com,
m.b.lankhorst@gmail.com, vipul.bhasin@gmail.com, rnorton10@gmail.com,
dill.sellars@gmail.com, seshadri.mahalingam@gmail.com, clementskau@gmail.com,
david.mike.futcher@gmail.com, ramkumar.gokarnesan@gmail.com,
mma.public@gmail.com, chromium@hybridsource.org, *@nvidia.com,
google@tk-webart.de, pph34r@gmail.com, lukezarko@gmail.com, fhd@ubercode.de,
ali.akbar@gmail.com, mathias@qiwi.be, mrs@mythic-beasts.com,
amruthraj@motorola.com, ckqr36@motorola.com, wxjg68@motorola.com,
ehsan.akhgari@gmail.com, chrelad@gmail.com, ncj674@motorola.com,
mtilburg@adobe.com, pbrophy@adobe.com, goldberg@adobe.com, woodward@adobe.com,
vinaya@adobe.com, naveenbobbili@motorola.com, vamshi@motorola.com,
robert.nagy@gmail.com, qtc746@motorola.com, blr.bmlab@gmail.com,
wrm364@motorola.com, rosen.dash@gmail.com, qghc36@motorola.com,
ravi.kasibhatla@motorola.com, nqk836@motorola.com, nrqv63@motorola.com,
tedvessenes@gmail.com, progame@chromium.org, brk376@motorola.com,
cristian.patrasciuc@gmail.com, halton.huo@intel.com, aofdwsl@gmail.com,
gaochun.dev@gmail.com, clintstaley@gmail.com, clintstaley@chromium.org,
rdevlin.cronin@gmail.com, junmin.zhu@intel.com, cem.kocagil@gmail.com,
simon.hong81@gmail.com, guanqun.lu@gmail.com, beaufort.francois@gmail.com,
eriq.augustine@gmail.com, francoisk777@gmail.com, erikghill@gmail.com,
maojie0924@gmail.com, pan.deng@intel.com, aaronlevbugs@gmail.com,
peter@pcc.me.uk, aaron.randolph@gmail.com, yumios.art@gmail.com,
matthewrobertson03@gmail.com, yujie.mao@intel.com, samuel.xu@intel.com,
jin.a.yang@intel.com, hexinchao@gmail.com, changbin.shao@intel.com,
stephen.searles@gmail.com, arun.m@samsung.com, trprice@gmail.com,
achicu@adobe.com, ekr@rtfm.com, wiss1976@gmail.com, erik.sjolund@gmail.com,
simon.arlott@gmail.com, alexkorep@gmail.com, mitchellwrosen@chromium.org,
yongsheng.zhu@intel.com, shouqun.liu@intel.com, kangyuan.shu@intel.com,
jake@helfert.us, hongbo.min@intel.com, tom.cassiotis@gmail.com,
evangelos@foutrelis.com, paivanof@gmail.com, rb@radix.io, petarj@mips.com,
carloschilazo@gmail.com, mmaerean@adobe.com, kaustubh.ra@gmail.com,
betravis@adobe.com, bear.travis@gmail.com, mvujovic@adobe.com,
jakob.j.w@googlemail.com, badea@adobe.com, joshua.lock@intel.com,
chunyang.dai@intel.com, mhx348@motorola.com, rubentopo@gmail.com,
josh.triplett@intel.com, qiankun.miao@intel.com, etienne@atnnn.com,
yang.gu@intel.com, ttr314@googlemail.com, limasdf@gmail.com,
m.s.bednorz@gmail.com, kamil.jiwa@gmail.com, keenepan@linpus.com,
unsafe@trevp.net, rosca@adobe.com, sylvinus@gmail.com, sungmann.cho@gmail.com,
fangjue23303@gmail.com, evan.peterson.ep@gmail.com, jryans@chromium.org,
matheusbrat@gmail.com, olaru@adobe.com, horia.olaru@gmail.com, *@opera.com,
johannes.rudolph@googlemail.com, samusaaron3@gmail.com, qufighter@gmail.com

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py

** Presubmit Warnings **
jun.a.jiang@intel.com is not in AUTHORS file. If you are a new contributor,
please visit
http://www.chromium.org/developers/contributing-code and read the "Legal"
section
If you are a chromite, verify the contributor signed the CLA.

Presubmit checks took 1.5s to calculate.

[Jun Jiang, 2013-03-13 01:46:58]

Add my name into AUTHORS file since it is the first time I submit patch to
chromium.

[commit-bot: I haz the power, 2013-03-13 01:47:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jun.a.jiang@intel.com/12412007/38001

[commit-bot: I haz the power, 2013-03-13 11:10:11]

Change committed as 187827