Enable 2D Canvas painting for Chrome on Android.

Enable video painting on Canvas for Chrome on Android. This change should land after another patch landing in https://codereview.chromium.org/13685002.


BUG=147265
TEST=visited the following site:
http://html5doctor.com/demos/video-canvas-magic/demo1.html

Visit http://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_canvas_drawimage_video.
Change different parameters of the canvas and video. Compare the result between desktop chrome and android chrome to see if video painting results are the same.

Canvas 500X400 ctx.drawImage(v,60,60,320,176)                PASS
Canvas 500X400 ctx.drawImage(v,0,0,320,176)                  PASS
Canvas 200X100 ctx.drawImage(v,0,0,320,176)                  PASS
Canvas 200X100 ctx.drawImage(v,50,50,320,176)                PASS
Canvas 500X400 ctx.drawImage(v,0,0,30,30,5,5,260,125)        PASS
Canvas 500X400 ctx.drawImage(v,0,0)                          PASS
Canvas 600x700 ctx.drawImage(v,90,90,80,80,100,100,260,125)  PASS

TODO:
More complex and mixed 2D Canvas operation to see if the painting result is right(Need to learn some more JS for that).

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

[hkuang, 2013-04-04 19:39:45]

PTAL

[fgalligan1, 2013-04-04 20:38:10]

Is there a unittest that can be run already or added to test this change?

https://codereview.chromium.org/13620008/diff/1/gpu/command_buffer/service/gl...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h (right):

https://codereview.chromium.org/13620008/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:37: GLenum
source_target, GLenum dest_target,
Indentation is off. Either line up with "(" or 4 space indent on next line.

https://codereview.chromium.org/13620008/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:41: const GLfloat
transform_matrix[16]);
16 is meaningless here. Basically you are passing a const GLfloat *. Should
probably change to const GLfloat* transform_matrix.

https://codereview.chromium.org/13620008/diff/1/webkit/media/android/webmedia...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/13620008/diff/1/webkit/media/android/webmedia...
webkit/media/android/webmediaplayer_android.cc:259: // The video is stored in a
unmultiplied format, so premultiply
nit: newline before //

https://codereview.chromium.org/13620008/diff/1/webkit/media/android/webmedia...
webkit/media/android/webmediaplayer_android.cc:262: premultiply_alpha);
nit: align to (

https://codereview.chromium.org/13620008/diff/1/webkit/media/android/webmedia...
webkit/media/android/webmediaplayer_android.cc:269: source_texture, texture,
level, internal_format);
nit: align to (

https://codereview.chromium.org/13620008/diff/1/webkit/media/android/webmedia...
webkit/media/android/webmediaplayer_android.cc:272: false);
nit: align to (

[hkuang, 2013-04-04 22:56:45]

PTAL.

https://codereview.chromium.org/13620008/diff/1/gpu/command_buffer/service/gl...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h (right):

https://codereview.chromium.org/13620008/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:37: GLenum
source_target, GLenum dest_target,
On 2013/04/04 20:38:10, fgalligan1 wrote:
> Indentation is off. Either line up with "(" or 4 space indent on next line.

Done.

https://codereview.chromium.org/13620008/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:41: const GLfloat
transform_matrix[16]);
On 2013/04/04 20:38:10, fgalligan1 wrote:
> 16 is meaningless here. Basically you are passing a const GLfloat *. Should
> probably change to const GLfloat* transform_matrix.

Keep it as it is now as see the StreamTextureProxyImpl::OnMatrixChanged use this
style to get the matrix. So just keep it.

https://codereview.chromium.org/13620008/diff/1/webkit/media/android/webmedia...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/13620008/diff/1/webkit/media/android/webmedia...
webkit/media/android/webmediaplayer_android.cc:259: // The video is stored in a
unmultiplied format, so premultiply
On 2013/04/04 20:38:10, fgalligan1 wrote:
> nit: newline before //

Done.

https://codereview.chromium.org/13620008/diff/1/webkit/media/android/webmedia...
webkit/media/android/webmediaplayer_android.cc:262: premultiply_alpha);
On 2013/04/04 20:38:10, fgalligan1 wrote:
> nit: align to (

Done.

https://codereview.chromium.org/13620008/diff/1/webkit/media/android/webmedia...
webkit/media/android/webmediaplayer_android.cc:269: source_texture, texture,
level, internal_format);
On 2013/04/04 20:38:10, fgalligan1 wrote:
> nit: align to (

Done.

https://codereview.chromium.org/13620008/diff/1/webkit/media/android/webmedia...
webkit/media/android/webmediaplayer_android.cc:272: false);
On 2013/04/04 20:38:10, fgalligan1 wrote:
> nit: align to (

Done.

[fgalligan1, 2013-04-04 23:11:49]

On 2013/04/04 22:56:45, hkuang wrote:
> PTAL.
> 
>
https://codereview.chromium.org/13620008/diff/1/gpu/command_buffer/service/gl...
> File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h (right):
> 
>
https://codereview.chromium.org/13620008/diff/1/gpu/command_buffer/service/gl...
> gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:37: GLenum
> source_target, GLenum dest_target,
> On 2013/04/04 20:38:10, fgalligan1 wrote:
> > Indentation is off. Either line up with "(" or 4 space indent on next line.
> 
> Done.
> 
>
https://codereview.chromium.org/13620008/diff/1/gpu/command_buffer/service/gl...
> gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:41: const GLfloat
> transform_matrix[16]);
> On 2013/04/04 20:38:10, fgalligan1 wrote:
> > 16 is meaningless here. Basically you are passing a const GLfloat *. Should
> > probably change to const GLfloat* transform_matrix.
> 
> Keep it as it is now as see the StreamTextureProxyImpl::OnMatrixChanged use
this
> style to get the matrix. So just keep it.
> 
>
https://codereview.chromium.org/13620008/diff/1/webkit/media/android/webmedia...
> File webkit/media/android/webmediaplayer_android.cc (right):
> 
>
https://codereview.chromium.org/13620008/diff/1/webkit/media/android/webmedia...
> webkit/media/android/webmediaplayer_android.cc:259: // The video is stored in
a
> unmultiplied format, so premultiply
> On 2013/04/04 20:38:10, fgalligan1 wrote:
> > nit: newline before //
> 
> Done.
> 
>
https://codereview.chromium.org/13620008/diff/1/webkit/media/android/webmedia...
> webkit/media/android/webmediaplayer_android.cc:262: premultiply_alpha);
> On 2013/04/04 20:38:10, fgalligan1 wrote:
> > nit: align to (
> 
> Done.
> 
>
https://codereview.chromium.org/13620008/diff/1/webkit/media/android/webmedia...
> webkit/media/android/webmediaplayer_android.cc:269: source_texture, texture,
> level, internal_format);
> On 2013/04/04 20:38:10, fgalligan1 wrote:
> > nit: align to (
> 
> Done.
> 
>
https://codereview.chromium.org/13620008/diff/1/webkit/media/android/webmedia...
> webkit/media/android/webmediaplayer_android.cc:272: false);
> On 2013/04/04 20:38:10, fgalligan1 wrote:
> > nit: align to (
> 
> Done.

lgtm, but finding out if there was a test you could add that would automatically
test your CL, would be great.

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

PTAL.

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

+gman

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

This LGTM but Gregg should review it.

[qinmin, 2013-04-07 21:52:29]

https://codereview.chromium.org/13620008/diff/6001/webkit/media/android/webme...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/13620008/diff/6001/webkit/media/android/webme...
webkit/media/android/webmediaplayer_android.cc:258: uint32 source_texture =
texture_id_;
if texture_id_ is 0, return false?

[greggman, 2013-04-08 17:29:47]

https://chromiumcodereview.appspot.com/13620008/diff/6001/gpu/command_buffer/...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://chromiumcodereview.appspot.com/13620008/diff/6001/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:57: uniform mat4
matrix;
nit: we've tried to have uniforms start with u_, varyings with v_, attributes
with a_, what do you think?

https://chromiumcodereview.appspot.com/13620008/diff/6001/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:318: const GLfloat
default_matrix[16] = {1.0f, 0.0f, 0.0f, 0.0f,
any reason not to make this static? Otherwise code is generated and it gets
initialized every time this is called.

https://chromiumcodereview.appspot.com/13620008/diff/6001/gpu/command_buffer/...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:362: GLuint
matrix_handle_ = glGetUniformLocation(programs_[program], "matrix");
Looking up uniforms is slow. this should happen at initialization time.

[hkuang, 2013-04-09 00:49:54]

Hi, Gregg

Could you take another look?

https://codereview.chromium.org/13620008/diff/6001/gpu/command_buffer/service...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc (right):

https://codereview.chromium.org/13620008/diff/6001/gpu/command_buffer/service...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:57: uniform mat4
matrix;
Good style that I need to follow. I did not notice that. 
Done.

On 2013/04/08 17:29:47, greggman wrote:
> nit: we've tried to have uniforms start with u_, varyings with v_, attributes
> with a_, what do you think?

https://codereview.chromium.org/13620008/diff/6001/gpu/command_buffer/service...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:318: const GLfloat
default_matrix[16] = {1.0f, 0.0f, 0.0f, 0.0f,
No reason that I do not make it static.
Done.

On 2013/04/08 17:29:47, greggman wrote:
> any reason not to make this static? Otherwise code is generated and it gets
> initialized every time this is called.

https://codereview.chromium.org/13620008/diff/6001/gpu/command_buffer/service...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc:362: GLuint
matrix_handle_ = glGetUniformLocation(programs_[program], "matrix");
On 2013/04/08 17:29:47, greggman wrote:
> Looking up uniforms is slow. this should happen at initialization time.

Done.

[hkuang1, 2013-04-09 21:55:22]

Add scherkus. 

Hi, Andrew

Could you review the change in the webmediaplayer_android?

Thanks.

[hkuang, 2013-04-09 21:55:45]

https://codereview.chromium.org/13620008/diff/6001/webkit/media/android/webme...
File webkit/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/13620008/diff/6001/webkit/media/android/webme...
webkit/media/android/webmediaplayer_android.cc:258: uint32 source_texture =
texture_id_;
On 2013/04/07 21:52:29, qinmin wrote:
> if texture_id_ is 0, return false?

Done.

[scherkus (not reviewing), 2013-04-10 00:05:08]

What's our automated testing story for this change?

I believe there are existing video-to-canvas layout tests. Do we know if those
are now passing?

https://chromiumcodereview.appspot.com/13620008/diff/23001/gpu/command_buffer...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h (right):

https://chromiumcodereview.appspot.com/13620008/diff/23001/gpu/command_buffer...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:28: void
DoCopyTexture(const gles2::GLES2Decoder* decoder, GLenum source_target,
there only appear to be two callers:
https://code.google.com/p/chromium/codesearch#search/&q=%5C-%5C%3EDoCopyTextu...

do we really need to add a second variant of the API?

https://chromiumcodereview.appspot.com/13620008/diff/23001/gpu/command_buffer...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/13620008/diff/23001/gpu/command_buffer...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9872: const GLfloat
default_matrix[16] = {1.0f, 0.0f, 0.0f, 0.0f,
this should be static

https://chromiumcodereview.appspot.com/13620008/diff/23001/gpu/command_buffer...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9877: source_texture->target(),
fix alignment (you'll want |this| on next line + everything else on 4-space
indent)

https://chromiumcodereview.appspot.com/13620008/diff/23001/webkit/media/andro...
File webkit/media/android/webmediaplayer_android.cc (right):

https://chromiumcodereview.appspot.com/13620008/diff/23001/webkit/media/andro...
webkit/media/android/webmediaplayer_android.cc:257: if (!texture_id_) return
false;
drop return to next line

https://chromiumcodereview.appspot.com/13620008/diff/23001/webkit/media/andro...
webkit/media/android/webmediaplayer_android.cc:262: premultiply_alpha);
this should be aligned with (

https://chromiumcodereview.appspot.com/13620008/diff/23001/webkit/media/andro...
File webkit/media/android/webmediaplayer_android.h (right):

https://chromiumcodereview.appspot.com/13620008/diff/23001/webkit/media/andro...
webkit/media/android/webmediaplayer_android.h:70: bool flip_y) OVERRIDE;
this shouldn't be override since the interface is declared in the blink
repository (having override makes it much harder to land interface changes)

[hkuang1, 2013-04-10 01:32:35]

Hi, Andrew

PTAL. The only layout test I could find is video-canvas-source.html. This one is
still not passed yet due to the canvas tries to get the video at the very
beginning that the valid frame is not uploaded to the texture yet. So it only
shows a black frame. But if I change it a little bit to display after several
million second, it will work and the canvas could paint the frame. I discussed
with Min and it seems we could not address this yet due to android limitation.
Most of the test I did are put in the description. I also try the
transformation, scaling. Also this cool demo works on nexus 7 tablet now:
http://downloads.webmproject.org/hkuang/index4.html.

https://chromiumcodereview.appspot.com/13620008/diff/23001/gpu/command_buffer...
File gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h (right):

https://chromiumcodereview.appspot.com/13620008/diff/23001/gpu/command_buffer...
gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.h:28: void
DoCopyTexture(const gles2::GLES2Decoder* decoder, GLenum source_target,
On 2013/04/10 00:05:08, scherkus wrote:
> there only appear to be two callers:
>
https://code.google.com/p/chromium/codesearch#search/&q=%255C-%255C%253EDoCop...
> 
> do we really need to add a second variant of the API?

Yes. On Android, the video frames are emitted as streamtexture and there is a
transform matrix associated with it. So we should apply the transform matrix on
the texture for further usage. Also, this class's api is not for external usage.
It is only used inside the GPU process and by the DoCopyTexture extension. So
looks ok.

https://chromiumcodereview.appspot.com/13620008/diff/23001/gpu/command_buffer...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://chromiumcodereview.appspot.com/13620008/diff/23001/gpu/command_buffer...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9872: const GLfloat
default_matrix[16] = {1.0f, 0.0f, 0.0f, 0.0f,
On 2013/04/10 00:05:08, scherkus wrote:
> this should be static

Done.

https://chromiumcodereview.appspot.com/13620008/diff/23001/gpu/command_buffer...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9877: source_texture->target(),
On 2013/04/10 00:05:08, scherkus wrote:
> fix alignment (you'll want |this| on next line + everything else on 4-space
> indent)

Done.

https://chromiumcodereview.appspot.com/13620008/diff/23001/webkit/media/andro...
File webkit/media/android/webmediaplayer_android.cc (right):

https://chromiumcodereview.appspot.com/13620008/diff/23001/webkit/media/andro...
webkit/media/android/webmediaplayer_android.cc:257: if (!texture_id_) return
false;
On 2013/04/10 00:05:08, scherkus wrote:
> drop return to next line

Done.

https://chromiumcodereview.appspot.com/13620008/diff/23001/webkit/media/andro...
webkit/media/android/webmediaplayer_android.cc:262: premultiply_alpha);
On 2013/04/10 00:05:08, scherkus wrote:
> this should be aligned with (

Done.

[hkuang1, 2013-04-10 01:37:28]

Sorry for the typo. Should be "after several milliseconds" not "million
seconds".

[greggman, 2013-04-10 16:51:41]

lgtm

[xhwang, 2013-04-16 17:51:35]

lgtm on wmpa.* with comments

https://chromiumcodereview.appspot.com/13620008/diff/32001/webkit/media/andro...
File webkit/media/android/webmediaplayer_android.cc (right):

https://chromiumcodereview.appspot.com/13620008/diff/32001/webkit/media/andro...
webkit/media/android/webmediaplayer_android.cc:260: // The video is stored in a
unmultiplied format, so premultiply if
nit: s/a/an/

https://chromiumcodereview.appspot.com/13620008/diff/32001/webkit/media/andro...
webkit/media/android/webmediaplayer_android.cc:274: false);
We have web_graphics_context->flush() for better performance in
webmediaplayer_impl.cc. Does that also apply here?

[scherkus (not reviewing), 2013-04-17 19:36:23]

lgtm

can you prepare a change to either:
  a) modify the test to make it work 
  b) add a new test

we shouldn't let platform limitations be a reason for not having tests

[hkuang1, 2013-04-18 01:05:07]

https://chromiumcodereview.appspot.com/13620008/diff/32001/webkit/media/andro...
File webkit/media/android/webmediaplayer_android.cc (right):

https://chromiumcodereview.appspot.com/13620008/diff/32001/webkit/media/andro...
webkit/media/android/webmediaplayer_android.cc:260: // The video is stored in a
unmultiplied format, so premultiply if
On 2013/04/16 17:51:36, xhwang wrote:
> nit: s/a/an/

Done.

https://chromiumcodereview.appspot.com/13620008/diff/32001/webkit/media/andro...
webkit/media/android/webmediaplayer_android.cc:274: false);
On 2013/04/16 17:51:36, xhwang wrote:
> We have web_graphics_context->flush() for better performance in
> webmediaplayer_impl.cc. Does that also apply here?
This may bring some performance gain on desktop platform as Jun mention in his
change. But I do not have the tool to measure here on Android, so just skip
unnecessary flush for safety purpose.

[commit-bot: I haz the power, 2013-04-18 03:09:45]

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

[commit-bot: I haz the power, 2013-04-18 05:41:38]

Change committed as 194785