Enable video painting on Canvas for Chrome on Android.

Enable video painting on Canvas for Chrome on Android. This change
should land with https://codereview.chromium.org/13620008/ to make
it work.

BUG=147265
TEST= test in 13620008


Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=147974

[hkuang, 2013-04-05 00:32:07]

PTAL.

[qinmin, 2013-04-05 17:30:27]

+scherkus

https://chromiumcodereview.appspot.com/13685002/diff/1/Source/WebKit/chromium...
File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp (right):

https://chromiumcodereview.appspot.com/13685002/diff/1/Source/WebKit/chromium...
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:634: bool
WebMediaPlayerClientImpl::copyVideoTextureToCanvas(WebCore::GraphicsContext*
context, WebCore::GraphicsContext3D* context3D)
why this function takes a GraphicsContext param instead of taking canvas while
the name of the function is xxxxToCanvas?

https://chromiumcodereview.appspot.com/13685002/diff/1/Source/WebKit/chromium...
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:650: unsigned int
videoWidth = naturalSize().width(), videoHeight = naturalSize().height();
split it into multiple lines.

https://chromiumcodereview.appspot.com/13685002/diff/1/Source/WebKit/chromium...
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:666: if
(!m_webMediaPlayer->copyVideoTextureToPlatformTexture(webGraphicsContext3D,
textureId, 0, GraphicsContext3D::RGBA, true, false)) {
no { for if statement with only 1 lines

[scherkus (not reviewing), 2013-04-05 20:47:09]

https://chromiumcodereview.appspot.com/13685002/diff/1/Source/WebCore/html/HT...
File Source/WebCore/html/HTMLVideoElement.cpp (right):

https://chromiumcodereview.appspot.com/13685002/diff/1/Source/WebCore/html/HT...
Source/WebCore/html/HTMLVideoElement.cpp:250: if (context3D &&
player->copyVideoTextureToCanvas(context, context3D.get()))
this seems strange

there are other callers of paintCurrentFrameInContext() besides canvas

what happens in the cases where this is called but we're not painting into a
canvas?

https://chromiumcodereview.appspot.com/13685002/diff/1/Source/WebKit/chromium...
File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp (right):

https://chromiumcodereview.appspot.com/13685002/diff/1/Source/WebKit/chromium...
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:674: // Draw the texture
based bitmap onto the Canvas. If the canvas is hardware based, this will do a
GPU-GPU texture copy. If the canvas is software based,
do we have GPU-based canvas implementations yet?

[hkuang, 2013-04-05 21:21:35]

Thanks for min and scherkus's comments. I am changing the code structure and
naming to address. Will submit a new patch shortly.

[Ken Russell (switch to Gerrit), 2013-04-05 22:40:38]

https://codereview.chromium.org/13685002/diff/1/Source/WebCore/html/HTMLVideo...
File Source/WebCore/html/HTMLVideoElement.cpp (right):

https://codereview.chromium.org/13685002/diff/1/Source/WebCore/html/HTMLVideo...
Source/WebCore/html/HTMLVideoElement.cpp:250: if (context3D &&
player->copyVideoTextureToCanvas(context, context3D.get()))
On 2013/04/05 20:47:09, scherkus wrote:
> this seems strange
> 
> there are other callers of paintCurrentFrameInContext() besides canvas
> 
> what happens in the cases where this is called but we're not painting into a
> canvas?

I'll be more blunt and say that there is no way this can be doing the right
thing, because it is ignoring the destRect argument.

https://codereview.chromium.org/13685002/diff/1/Source/WebKit/chromium/src/We...
File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp (right):

https://codereview.chromium.org/13685002/diff/1/Source/WebKit/chromium/src/We...
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:660:
SkAutoTUnref<GrTexture> texture(ct->createUncachedTexture(desc, NULL, 0));
If this is the code structure you want, then shouldn't there be an attempt to
re-use these textures from frame to frame? Otherwise allocation and deallocation
will happen every time which will be inefficient.

https://codereview.chromium.org/13685002/diff/1/Source/WebKit/chromium/src/We...
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:676:
canvas->drawBitmap(bitmap, 0, 0);
This can't be correct. The destRect argument from
HTMLVideoElement::paintCurrentFrameInContext is being discarded at this point.

[Ken Russell (switch to Gerrit), 2013-04-05 22:43:38]

https://codereview.chromium.org/13685002/diff/8001/Source/WebCore/html/HTMLVi...
File Source/WebCore/html/HTMLVideoElement.cpp (right):

https://codereview.chromium.org/13685002/diff/8001/Source/WebCore/html/HTMLVi...
Source/WebCore/html/HTMLVideoElement.cpp:37: #include "GraphicsContext3D.h"
None of the changes in this file are needed any more.

https://codereview.chromium.org/13685002/diff/8001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp (right):

https://codereview.chromium.org/13685002/diff/8001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:634: if (context3D &&
paintVideoTextureInContext(context, context3D.get()))
This is still wrong as it's discarding the incoming destination rectangle.

https://codereview.chromium.org/13685002/diff/8001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:784: bool
WebMediaPlayerClientImpl::paintVideoTextureInContext(WebCore::GraphicsContext*
context, WebCore::GraphicsContext3D* context3D)
Either this needs to be made to work correctly on all platforms (it definitely
won't; see above) or it should be #ifdef'd so that on !OS_ANDROID it immediately
returns false.

[Ken Russell (switch to Gerrit), 2013-04-09 00:07:28]

The new patch LGTM with one comment. Thanks for pushing this through.

https://chromiumcodereview.appspot.com/13685002/diff/16001/Source/WebKit/chro...
File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h (right):

https://chromiumcodereview.appspot.com/13685002/diff/16001/Source/WebKit/chro...
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h:179: void
paintOnAndroid(WebCore::GraphicsContext* context, WebCore::GraphicsContext3D*
context3D, const WebCore::IntRect& rect, uint8_t alpha);
Please add a big FIXME here indicating that the regular paint() path should
"just work" on Android, but that changes are needed to Skia to handle
GL_TEXTURE_EXTERNAL_OES type textures internally.

[hkuang, 2013-04-09 17:46:10]

Hi, Andrew

PTAL. I have further addressed Ken's comments and your comments.

https://chromiumcodereview.appspot.com/13685002/diff/1/Source/WebCore/html/HT...
File Source/WebCore/html/HTMLVideoElement.cpp (right):

https://chromiumcodereview.appspot.com/13685002/diff/1/Source/WebCore/html/HT...
Source/WebCore/html/HTMLVideoElement.cpp:250: if (context3D &&
player->copyVideoTextureToCanvas(context, context3D.get()))
On 2013/04/05 20:47:09, scherkus wrote:
> this seems strange
> 
> there are other callers of paintCurrentFrameInContext() besides canvas
> 
> what happens in the cases where this is called but we're not painting into a
> canvas?

Done.

https://chromiumcodereview.appspot.com/13685002/diff/1/Source/WebCore/html/HT...
Source/WebCore/html/HTMLVideoElement.cpp:250: if (context3D &&
player->copyVideoTextureToCanvas(context, context3D.get()))
On 2013/04/05 20:47:09, scherkus wrote:
> this seems strange
> 
> there are other callers of paintCurrentFrameInContext() besides canvas

Fix the naming issue when addressing Ken's comments. Actually the backend of the
GraphicsContext is the SkCanvas.

> 
> what happens in the cases where this is called but we're not painting into a
> canvas?

https://chromiumcodereview.appspot.com/13685002/diff/1/Source/WebKit/chromium...
File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp (right):

https://chromiumcodereview.appspot.com/13685002/diff/1/Source/WebKit/chromium...
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:674: // Draw the texture
based bitmap onto the Canvas. If the canvas is hardware based, this will do a
GPU-GPU texture copy. If the canvas is software based,
On 2013/04/05 20:47:09, scherkus wrote:
> do we have GPU-based canvas implementations yet?

The SkCanvas will be software based when the size is smaller than a threshold as
using software get better performance than using opengGL. When above that size
threshold, the Canvas will be hardware based. Details are in the code
HTMLCanvasElement::createImageBuffer()

[scherkus (not reviewing), 2013-04-09 18:31:26]

lgtm w/ nit

https://chromiumcodereview.appspot.com/13685002/diff/20001/Source/WebKit/chro...
File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h (right):

https://chromiumcodereview.appspot.com/13685002/diff/20001/Source/WebKit/chro...
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h:181: //
https://code.google.com/p/skia/issues/detail?id=1189&q=texture&colspec=ID%20T...
nit: can you remove everything starting from "&q=" onwards from the URL?

[hkuang, 2013-04-09 21:38:09]

https://codereview.chromium.org/13685002/diff/20001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h (right):

https://codereview.chromium.org/13685002/diff/20001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h:181: //
https://code.google.com/p/skia/issues/detail?id=1189&q=texture&colspec=ID%20T...
On 2013/04/09 18:31:26, scherkus wrote:
> nit: can you remove everything starting from "&q=" onwards from the URL?

Done. Going to land this change.

[commit-bot: I haz the power, 2013-04-09 21:38:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hkuang@chromium.org/13685002/25001

[commit-bot: I haz the power, 2013-04-09 21:38:51]

Change committed as 147974