Add target argument to Copy{Sub}TextureCHROMIUM entry point

Add target argument to Copy{Sub}TextureCHROMIUM entry point

This CL just modify the entry point to support target argument which is
preliminary work to allow CUBE_MAP target. It doesn't change chrome's
behavior currently, while the real support for CUBE_MAP target will be
implemented in a follow-up CL. This CL partly reverts
https://codereview.chromium.org/1551143002. In that CL, target argument
was removed.

TBR=reveman@chromium.org
BUG=612542
TEST=n/a; no behavior changes
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2639973002
Cr-Commit-Position: refs/heads/master@{#445303}
Committed: https://chromium.googlesource.com/chromium/src/+/728aa8652fcdef20b24213d96eeb7db6a2543748

[qiankun, 2017-01-18 12:21:06]

Description was changed from

==========
Add target argument to Copy{Sub}TextureCHROMIUM entry point

This CL just modify the entry point to support target argument which is
preliminary work to allow CUBE_MAP target. It doesn't change chrome's
behavior currently, while the real support for CUBE_MAP target will be
implemented in a follow-up CL. This CL partly reverts
https://codereview.chromium.org/1551143002. In that CL, target argument
was removed.

BUG=612542
TEST=n/a; no behavior changes
==========

to

==========
Add target argument to Copy{Sub}TextureCHROMIUM entry point

This CL just modify the entry point to support target argument which is
preliminary work to allow CUBE_MAP target. It doesn't change chrome's
behavior currently, while the real support for CUBE_MAP target will be
implemented in a follow-up CL. This CL partly reverts
https://codereview.chromium.org/1551143002. In that CL, target argument
was removed.

BUG=612542
TEST=n/a; no behavior changes
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[qiankun, 2017-01-18 12:21:07]

Description was changed from

==========
Add target argument to Copy{Sub}TextureCHROMIUM entry point

This CL just modify the entry point to support target argument which is
preliminary work to allow CUBE_MAP target. It doesn't change chrome's
behavior currently, while the real support for CUBE_MAP target will be
implemented in a follow-up CL. This CL partly reverts
https://codereview.chromium.org/1551143002. In that CL, target argument
was removed.

BUG=612542
TEST=n/a; no behavior changes
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add target argument to Copy{Sub}TextureCHROMIUM entry point

This CL just modify the entry point to support target argument which is
preliminary work to allow CUBE_MAP target. It doesn't change chrome's
behavior currently, while the real support for CUBE_MAP target will be
implemented in a follow-up CL. This CL partly reverts
https://codereview.chromium.org/1551143002. In that CL, target argument
was removed.

BUG=612542
TEST=n/a; no behavior changes
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[qiankun, 2017-01-18 15:23:59]

qiankun.miao@intel.com changed reviewers:
+ bbudge@chromium.org, erikchen@chromium.org, kbr@chromium.org,
liberato@chromium.org, qinmin@chromium.org, reveman@chromium.org,
yunchao.he@intel.com, zmo@chromium.org

[qiankun, 2017-01-18 15:24:00]

Preliminary work to GL_TEXTURE_CUBE_MAP for CopyTextureCHROMIUM extension. This
CL
just adds target argument to glCopyTextureCHROMIUM and glCopySubTextureCHROMIUM.
Target was removed in https://codereview.chromium.org/1551143002 because target
can be queried from dest_id. If we want to support GL_TEXTURE_CUBE_MAP, we need
to add back target argument which may be GL_TEXTURE_CUBE_MAP_POSITIVE_X/Y/Z
GL_TEXTURE_CUBE_MAP_NEGATIVE_X/Y/Z.
zmo,kbr,erikchen: please review whole patch.
reveman: owner review for cc/ and components/exo/
qinmin: owner review for content/renderer/media/android/
liberato: owner review for media/renderers/
bbudge: owner review for ppapi/

[liberato (no reviews please), 2017-01-18 17:36:58]

content/renderer/media/android, media/renderers lgtm

-fl

[Zhenyao Mo, 2017-01-18 22:44:07]

https://codereview.chromium.org/2639973002/diff/20001/gpu/command_buffer/cmd_...
File gpu/command_buffer/cmd_buffer_functions.txt (right):

https://codereview.chromium.org/2639973002/diff/20001/gpu/command_buffer/cmd_...
gpu/command_buffer/cmd_buffer_functions.txt:283: GL_APICALL void        
GL_APIENTRY glCopyTextureCHROMIUM (GLenum target, GLenum source_id, GLint
source_level, GLenum dest_id, GLint dest_level, GLintTextureInternalFormat
internalformat, GLenumPixelType dest_type, GLboolean unpack_flip_y, GLboolean
unpack_premultiply_alpha, GLboolean unpack_unmultiply_alpha);
Does it make more sense to move this arg to right before dest_id and name it
dest_target?  Otherwise it is confusing (it could be interpreted as target for
source or target for both source/dest).

[bbudge, 2017-01-18 23:29:38]

ppapi stamp lgtm

[Ken Russell (switch to Gerrit), 2017-01-18 23:46:27]

I defer to zmo@'s review. I see his point about calling this "dest_target" and
putting it elsewhere in the argument list, but don't have a strong opinion.

Please schedule time to update
src/gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt . If it's not too
much work to add the new argument there and update the documentation slightly,
please do that in this CL so the docs are self-consistent.

Thanks.

[Ken Russell (switch to Gerrit), 2017-01-18 23:48:21]

On 2017/01/18 23:46:27, Ken Russell wrote:
> I defer to zmo@'s review. I see his point about calling this "dest_target" and
> putting it elsewhere in the argument list, but don't have a strong opinion.

Actually -- thinking about this more, it's much simpler to consider the source
target always "TEXTURE_2D" rather than allowing arbitrary copying between cube
map faces, for example. For this reason I agree with zmo's comment to call this
new argument "dest_target" and document the behavior well.

> Please schedule time to update
> src/gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt . If it's not too
> much work to add the new argument there and update the documentation slightly,
> please do that in this CL so the docs are self-consistent.
> 
> Thanks.

[qiankun, 2017-01-19 11:54:16]

Patchset #3 (id:40001) has been deleted

[qiankun, 2017-01-19 15:01:11]

I updated the CL according your comments. Thanks for reviewing. 

kbr: I also updated the spec description. PTAL and owner review for
third_party/WebKit/Source/platform/graphics/.

https://codereview.chromium.org/2639973002/diff/20001/gpu/command_buffer/cmd_...
File gpu/command_buffer/cmd_buffer_functions.txt (right):

https://codereview.chromium.org/2639973002/diff/20001/gpu/command_buffer/cmd_...
gpu/command_buffer/cmd_buffer_functions.txt:283: GL_APICALL void        
GL_APIENTRY glCopyTextureCHROMIUM (GLenum target, GLenum source_id, GLint
source_level, GLenum dest_id, GLint dest_level, GLintTextureInternalFormat
internalformat, GLenumPixelType dest_type, GLboolean unpack_flip_y, GLboolean
unpack_premultiply_alpha, GLboolean unpack_unmultiply_alpha);
On 2017/01/18 22:44:07, Zhenyao Mo wrote:
> Does it make more sense to move this arg to right before dest_id and name it
> dest_target?  Otherwise it is confusing (it could be interpreted as target for
> source or target for both source/dest).

Done. I also intended to do like this before, but I finally interpreted target
same as glCopyTexImage2D. glCopyTextureCHROMIUM has more arguments than
glCopyTexImage2D. It makes sense to move target to right before dest_id.

[Zhenyao Mo, 2017-01-19 18:58:31]

lgtm

https://codereview.chromium.org/2639973002/diff/60001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt (right):

https://codereview.chromium.org/2639973002/diff/60001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt:137: GLint xoffset,
GLint yoffset,
We should also consider support uploading to a 3D texture layer

[qinmin, 2017-01-20 00:50:55]

content/renderer/media/android lgtm

[Ken Russell (switch to Gerrit), 2017-01-20 03:34:39]

Thanks Qiankun for updating the spec. It and
third_party/WebKit/Source/platform/graphics/ LGTM.

[qiankun, 2017-01-20 23:48:11]

reveman, could you do owner review for cc/ and components/exo/? Thanks.

[Zhenyao Mo, 2017-01-20 23:50:15]

On 2017/01/20 23:48:11, qiankun wrote:
> reveman, could you do owner review for cc/ and components/exo/? Thanks.

Qiankun,

The changes are trivia.  Please set TBR=email to land this CL.

[qiankun, 2017-01-20 23:56:22]

The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 23:56:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-21 01:29:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-21 01:29:54]

Dry run: This issue passed the CQ dry run.

[qiankun, 2017-01-22 01:56:36]

The CQ bit was checked by qiankun.miao@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-22 01:56:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[qiankun, 2017-01-22 03:22:45]

Description was changed from

==========
Add target argument to Copy{Sub}TextureCHROMIUM entry point

This CL just modify the entry point to support target argument which is
preliminary work to allow CUBE_MAP target. It doesn't change chrome's
behavior currently, while the real support for CUBE_MAP target will be
implemented in a follow-up CL. This CL partly reverts
https://codereview.chromium.org/1551143002. In that CL, target argument
was removed.

BUG=612542
TEST=n/a; no behavior changes
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Add target argument to Copy{Sub}TextureCHROMIUM entry point

This CL just modify the entry point to support target argument which is
preliminary work to allow CUBE_MAP target. It doesn't change chrome's
behavior currently, while the real support for CUBE_MAP target will be
implemented in a follow-up CL. This CL partly reverts
https://codereview.chromium.org/1551143002. In that CL, target argument
was removed.

TBR=reveman@chromium.org
BUG=612542
TEST=n/a; no behavior changes
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[commit-bot: I haz the power, 2017-01-22 04:51:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-22 04:51:30]

Dry run: This issue passed the CQ dry run.

[qiankun, 2017-01-22 14:58:17]

The CQ bit was checked by qiankun.miao@intel.com

[qiankun, 2017-01-22 14:58:18]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/2639973002/#ps60001
(title: "use dest_target instead of target")

[commit-bot: I haz the power, 2017-01-22 14:58:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-22 15:03:07]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485097097638290,
"parent_rev": "4e6e774857ae99aa9dc13287e0c36c1e69bf92b6", "commit_rev":
"728aa8652fcdef20b24213d96eeb7db6a2543748"}

[commit-bot: I haz the power, 2017-01-22 15:04:06]

Description was changed from

==========
Add target argument to Copy{Sub}TextureCHROMIUM entry point

This CL just modify the entry point to support target argument which is
preliminary work to allow CUBE_MAP target. It doesn't change chrome's
behavior currently, while the real support for CUBE_MAP target will be
implemented in a follow-up CL. This CL partly reverts
https://codereview.chromium.org/1551143002. In that CL, target argument
was removed.

TBR=reveman@chromium.org
BUG=612542
TEST=n/a; no behavior changes
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Add target argument to Copy{Sub}TextureCHROMIUM entry point

This CL just modify the entry point to support target argument which is
preliminary work to allow CUBE_MAP target. It doesn't change chrome's
behavior currently, while the real support for CUBE_MAP target will be
implemented in a follow-up CL. This CL partly reverts
https://codereview.chromium.org/1551143002. In that CL, target argument
was removed.

TBR=reveman@chromium.org
BUG=612542
TEST=n/a; no behavior changes
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2639973002
Cr-Commit-Position: refs/heads/master@{#445303}
Committed:
https://chromium.googlesource.com/chromium/src/+/728aa8652fcdef20b24213d96eeb...
==========

[commit-bot: I haz the power, 2017-01-22 15:04:08]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/728aa8652fcdef20b24213d96eeb...