Add a new parameter dest_type to the GL_CHROMIUM_copy_texture extension.

Add a new parameter dest_type to the GL_CHROMIUM_copy_texture extension.
It is used to define the destination texture if needed, which is possible
for texImage2D(..., canvas) and texImage2D(..., video) in WebGL.

BUG=

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

[Jun Jiang, 2013-04-04 12:49:29]

Add the parameter dest_type for GL_CHROMIUM_copy_texture extension which is used
when the destination texture is defined. This avoids to use the previous type of
the destination texture or that of the source texture and more close to the
semantics of texImage2D().
Related issues in WebKit is at https://bugs.webkit.org/show_bug.cgi?id=113493.

[Zhenyao Mo, 2013-04-04 19:26:54]

On 2013/04/04 12:49:29, Jun Jiang wrote:
> Add the parameter dest_type for GL_CHROMIUM_copy_texture extension which is
used
> when the destination texture is defined. This avoids to use the previous type
of
> the destination texture or that of the source texture and more close to the
> semantics of texImage2D().
> Related issues in WebKit is at https://bugs.webkit.org/show_bug.cgi?id=113493.

I think this is great improvement.  Looks good. But I'll leave the final review
to gman.

[Ken Russell (switch to Gerrit), 2013-04-04 19:58:28]

This looks good overall to me, but Gregg should do the final review.

In order to check this in you'll need to adjust the WebGraphicsContext3D
subclasses incrementally. First, add another overload of copyTextureCHROMIUM
taking the additional argument, instead of just changing the current one. Then
after landing the Chromium patch, update the WebKit (Blink) side to change the
signature of the virtual. After that lands, and Blink has rolled into Chromium,
remove the old virtual from Chromium.

https://codereview.chromium.org/13613006/diff/1/gpu/GLES2/extensions/CHROMIUM...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right):

https://codereview.chromium.org/13613006/diff/1/gpu/GLES2/extensions/CHROMIUM...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:85: 8/1/2011   
Documented the extension
Please describe this change in the revision history.

[Jun Jiang, 2013-04-07 12:17:36]

On 2013/04/04 19:58:28, kbr wrote:
> This looks good overall to me, but Gregg should do the final review.
> 
> In order to check this in you'll need to adjust the WebGraphicsContext3D
> subclasses incrementally. First, add another overload of copyTextureCHROMIUM
> taking the additional argument, instead of just changing the current one. Then
> after landing the Chromium patch, update the WebKit (Blink) side to change the
> signature of the virtual. After that lands, and Blink has rolled into
Chromium,
> remove the old virtual from Chromium.
> 
>
https://codereview.chromium.org/13613006/diff/1/gpu/GLES2/extensions/CHROMIUM...
> File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right):
> 
>
https://codereview.chromium.org/13613006/diff/1/gpu/GLES2/extensions/CHROMIUM...
> gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:85: 8/1/2011   
> Documented the extension
> Please describe this change in the revision history.

Hi, Kenneth. Thanks for your comments. I have rebased the patch and added the
description on the change in the revision history.

[Ken Russell (switch to Gerrit), 2013-04-08 18:28:52]

https://codereview.chromium.org/13613006/diff/6001/content/common/gpu/client/...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
(right):

https://codereview.chromium.org/13613006/diff/6001/content/common/gpu/client/...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:576:
WGC3Denum dest_type);
Please see my earlier comments. In order to land this patch without breaking the
repository you'll need to (1) add a new overload of copyTextureCHROMIUM in the
subclasses; (2) add the new overload into WebGraphicsContext3D in the Blink
repository, switch over all code to call it, and delete the old overload; and
(3) delete the old copyTextureCHROMIUM overload.

[Jun Jiang, 2013-04-09 08:05:00]

On 2013/04/08 18:28:52, kbr wrote:
>
https://codereview.chromium.org/13613006/diff/6001/content/common/gpu/client/...
> File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h
> (right):
> 
>
https://codereview.chromium.org/13613006/diff/6001/content/common/gpu/client/...
> content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h:576:
> WGC3Denum dest_type);
> Please see my earlier comments. In order to land this patch without breaking
the
> repository you'll need to (1) add a new overload of copyTextureCHROMIUM in the
> subclasses; (2) add the new overload into WebGraphicsContext3D in the Blink
> repository, switch over all code to call it, and delete the old overload; and
> (3) delete the old copyTextureCHROMIUM overload.

Hi, Kenneth. Thanks for your comments. I have preserved the old version of
copyTextureCHROMIUM in subclasses of WebGraphicsContext3D and delegates the call
to our new overload of copyTextureCHROMIUM with the sixth parameter set to
GL_UNSIGNED_BYTE. 
A little strange thing is that the patch can build and pass related tests in my
linux machine but the trybot fails in about 5 seconds for 8 platforms
simultaneously after I triggered the trybot .

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

On 2013/04/09 08:05:00, Jun Jiang wrote:
> A little strange thing is that the patch can build and pass related tests in
my
> linux machine but the trybot fails in about 5 seconds for 8 platforms
> simultaneously after I triggered the trybot .

I think the problem is that you aren't a Chromium committer. I've just submitted
some try jobs; let's see if they go through.

The new code looks good. I'd like to wait to approve it until Gregg reviews it.

[Jun Jiang, 2013-04-10 11:40:22]

On 2013/04/10 01:22:38, kbr wrote:
> On 2013/04/09 08:05:00, Jun Jiang wrote:
> > A little strange thing is that the patch can build and pass related tests in
> my
> > linux machine but the trybot fails in about 5 seconds for 8 platforms
> > simultaneously after I triggered the trybot .
> 
> I think the problem is that you aren't a Chromium committer. I've just
submitted
> some try jobs; let's see if they go through.
> 
> The new code looks good. I'd like to wait to approve it until Gregg reviews
it.
Hi, Kenneth. Thanks for helping me to trigger the trybot. It had caught some
compilation issue for mac and win, and I had fixed it.

[Jun Jiang, 2013-04-11 13:00:12]

For patch set 4, there are six platforms failed. But it seems none of them is
introduced by this patch. Following is from the mail received from try-server.
1. update failure
linux_layout_rel: update r193387 failed.
mac_gpu: update r193387 failed.
mac_layout_rel: update r193387 failed.
2. file missing.
win_layout_rel: 
File "..\..\..\scripts\slave\compile.py", line 872, in main_win
    sln = open(os.path.join(options.build_dir, options.solution), 'rU')
IOError: [Errno 2] No such file or directory:
'E:\\b\\build\\slave\\win_layout\\build\\src\\out\\out.sln'
3. test failed.
linux_gpu: gl_tests failed 2: GetSetUniform and UniformsInCurrentProgram.
win_gpu: content_browsertests failed 115, including
SingleWindowDoesNotExceedLimit,                
MANUAL_conformance_attribs_gl_disabled_vertex_attrib, etc.

[Ken Russell (switch to Gerrit), 2013-04-12 01:55:29]

This LGTM with a couple of nits, and assuming the try bots' failures are
unrelated. However, please file a bug if one is not filed already and reference
it in the BUG= line. We've recently had problems keeping track of CLs and
there's no good reason to not have a tracking bug for this and other changes.

Gregg needs to review this as well.

https://codereview.chromium.org/13613006/diff/26001/content/common/gpu/client...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/13613006/diff/26001/content/common/gpu/client...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:1609: //
to bridge the parameter differences.
Please add a line like:

TODO(jun.a.jiang): remove once the old copyTextureCHROMIUM is no longer being
called.

https://codereview.chromium.org/13613006/diff/26001/webkit/gpu/webgraphicscon...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/13613006/diff/26001/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:1658: // to
bridge the parameter differences.
Same TODO here.

[apatrick_chromium, 2013-04-12 19:34:56]

dest_type should have type GLenum throughout. I think I found all the instances
in non-auto-generated files. I can't figure out why dest_type is GLint in the
auto-generated files though since it is GLenumPixelType in
cmd_buffer_functions.txt. Do you need to run build_gles2_cmd_buffer.py again
maybe?

https://codereview.chromium.org/13613006/diff/26001/gpu/GLES2/extensions/CHRO...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right):

https://codereview.chromium.org/13613006/diff/26001/gpu/GLES2/extensions/CHRO...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:41: GLint
internal_format, GLint dest_type)
dest_type should have type GLenum rather than GLint.

https://codereview.chromium.org/13613006/diff/26001/gpu/GLES2/gl2extchromium.h
File gpu/GLES2/gl2extchromium.h (right):

https://codereview.chromium.org/13613006/diff/26001/gpu/GLES2/gl2extchromium....
gpu/GLES2/gl2extchromium.h:329: GLint internalformat, GLint dest_type);
GLenum dest_type

[Jun Jiang, 2013-04-15 10:58:08]

On 2013/04/12 01:55:29, kbr wrote:
> This LGTM with a couple of nits, and assuming the try bots' failures are
> unrelated. However, please file a bug if one is not filed already and
reference
> it in the BUG= line. We've recently had problems keeping track of CLs and
> there's no good reason to not have a tracking bug for this and other changes.
> 
> Gregg needs to review this as well.
> 
>
https://codereview.chromium.org/13613006/diff/26001/content/common/gpu/client...
> File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
> (right):
> 
>
https://codereview.chromium.org/13613006/diff/26001/content/common/gpu/client...
> content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:1609: //
> to bridge the parameter differences.
> Please add a line like:
> 
> TODO(jun.a.jiang): remove once the old copyTextureCHROMIUM is no longer being
> called.
> 
>
https://codereview.chromium.org/13613006/diff/26001/webkit/gpu/webgraphicscon...
> File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc
(right):
> 
>
https://codereview.chromium.org/13613006/diff/26001/webkit/gpu/webgraphicscon...
> webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:1658: // to
> bridge the parameter differences.
> Same TODO here.
Hi, Kenneth. Thanks for your comments. Adding "TODO" comments has been done.

[Jun Jiang, 2013-04-15 11:12:40]

On 2013/04/12 19:34:56, apatrick_chromium wrote:
> dest_type should have type GLenum throughout. I think I found all the
instances
> in non-auto-generated files. I can't figure out why dest_type is GLint in the
> auto-generated files though since it is GLenumPixelType in
> cmd_buffer_functions.txt. Do you need to run build_gles2_cmd_buffer.py again
> maybe?
> 
>
https://codereview.chromium.org/13613006/diff/26001/gpu/GLES2/extensions/CHRO...
> File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right):
> 
>
https://codereview.chromium.org/13613006/diff/26001/gpu/GLES2/extensions/CHRO...
> gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:41: GLint
> internal_format, GLint dest_type)
> dest_type should have type GLenum rather than GLint.
> 
> https://codereview.chromium.org/13613006/diff/26001/gpu/GLES2/gl2extchromium.h
> File gpu/GLES2/gl2extchromium.h (right):
> 
>
https://codereview.chromium.org/13613006/diff/26001/gpu/GLES2/gl2extchromium....
> gpu/GLES2/gl2extchromium.h:329: GLint internalformat, GLint dest_type);
> GLenum dest_type

Hi, apatrick. Thanks for your help on reviewing the patch. The issue that some
places use GLint instead of GLenum for the type parameter is because I made a
mistake and didn't use the script to auto generate files. Instead, I use "grep
CopyTextureCHROMIUM" and change the matching files manually. I sensed that these
files are auto generated by scripts from its file name but I didn't go further
and have a try. Thanks for your reminds and I have updated the patch
accordingly.

[apatrick_chromium, 2013-04-15 17:13:38]

lgtm

[Ken Russell (switch to Gerrit), 2013-04-15 22:49:56]

Thanks. I verified by hand that the autogeneration script creates the results in
this patch. Will submit to the commit queue once it reopens.

[greggman, 2013-04-15 22:54:53]

lgtm

[commit-bot: I haz the power, 2013-04-19 00:17:52]

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

[commit-bot: I haz the power, 2013-04-19 00:18:09]

Failed to apply patch for
gpu/command_buffer/common/gles2_cmd_utils_implementation_autogen.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
gpu/command_buffer/common/gles2_cmd_utils_implementation_autogen.h
  Hunk #1 FAILED at 91.
  Hunk #2 FAILED at 259.
  Hunk #3 FAILED at 561.
  Hunk #4 FAILED at 594.
  4 out of 4 hunks FAILED -- saving rejects to file
gpu/command_buffer/common/gles2_cmd_utils_implementation_autogen.h.rej

Patch:       gpu/command_buffer/common/gles2_cmd_utils_implementation_autogen.h
Index: gpu/command_buffer/common/gles2_cmd_utils_implementation_autogen.h
diff --git a/gpu/command_buffer/common/gles2_cmd_utils_implementation_autogen.h
b/gpu/command_buffer/common/gles2_cmd_utils_implementation_autogen.h
index
7a2147b69ad5d9fcdc9eb5a7558de6c9bb1d537f..e78db8632341919e24fd7fb9de985916cf3ff0a4
100644
--- a/gpu/command_buffer/common/gles2_cmd_utils_implementation_autogen.h
+++ b/gpu/command_buffer/common/gles2_cmd_utils_implementation_autogen.h
@@ -91,6 +91,7 @@ static GLES2Util::EnumToString enum_to_string_table[] = {
   { 0x0C10, "GL_SCISSOR_BOX", },
   { 0x0C11, "GL_SCISSOR_TEST", },
   { 0x80000000, "GL_MULTISAMPLE_BUFFER_BIT7_QCOM", },
+  { 0x300E, "GL_CONTEXT_LOST", },
   { 0x02000000, "GL_MULTISAMPLE_BUFFER_BIT1_QCOM", },
   { 0x8C2F, "GL_ANY_SAMPLES_PASSED_EXT", },
   { 0x8BD2, "GL_TEXTURE_WIDTH_QCOM", },
@@ -259,7 +260,7 @@ static GLES2Util::EnumToString enum_to_string_table[] = {
   { 0x00000400, "GL_STENCIL_BUFFER_BIT", },
   { 0x800A, "GL_FUNC_SUBTRACT", },
   { 0x8E2C, "GL_DEPTH_COMPONENT16_NONLINEAR_NV", },
-  { 0x8508, "GL_DECR_WRAP", },
+  { 0x889F, "GL_VERTEX_ATTRIB_ARRAY_BUFFER_BINDING", },
   { 0x8219, "GL_FRAMEBUFFER_UNDEFINED_OES", },
   { 0x8006, "GL_FUNC_ADD", },
   { 0x8007, "GL_MIN_EXT", },
@@ -561,7 +562,6 @@ static GLES2Util::EnumToString enum_to_string_table[] = {
   { 0x8CAB, "GL_RENDERBUFFER_SAMPLES_ANGLE", },
   { 0x8057, "GL_RGB5_A1", },
   { 0x8056, "GL_RGBA4", },
-  { 0x300E, "GL_CONTEXT_LOST", },
   { 0x150A, "GL_INVERT", },
   { 0x01000000, "GL_MULTISAMPLE_BUFFER_BIT0_QCOM", },
   { 0x78ED, "GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM", },
@@ -594,7 +594,7 @@ static GLES2Util::EnumToString enum_to_string_table[] = {
   { 0x8253, "GL_GUILTY_CONTEXT_RESET_EXT", },
   { 0x8CE5, "GL_COLOR_ATTACHMENT5_NV", },
   { 0x8872, "GL_MAX_TEXTURE_IMAGE_UNITS", },
-  { 0x889F, "GL_VERTEX_ATTRIB_ARRAY_BUFFER_BINDING", },
+  { 0x8508, "GL_DECR_WRAP", },
   { 0x8507, "GL_INCR_WRAP", },
   { 0x8895, "GL_ELEMENT_ARRAY_BUFFER_BINDING", },
   { 0x8894, "GL_ARRAY_BUFFER_BINDING", },

[Ken Russell (switch to Gerrit), 2013-04-19 00:59:16]

Jun: sorry, can you please rebase this patch?

[Jun Jiang, 2013-04-19 01:14:43]

On 2013/04/19 00:59:16, kbr wrote:
> Jun: sorry, can you please rebase this patch?

There is a patch landed that change glProduceTextureCHROMIUM and
glConsumeTextureCHROMIUM API and so the auto generated files. I have rebased my
patch and test it, now re-upload it for review.

[apatrick_chromium, 2013-04-19 17:52:50]

lgtm

[commit-bot: I haz the power, 2013-04-19 17:53:21]

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

[commit-bot: I haz the power, 2013-04-19 20:46:55]

Change committed as 195269