16-bit video upload to float: intermediate R16_EXT and copy to float.

16-bit video upload to float: intermediate R16_EXT and copy to float.

R16_EXT is supported on desktop core profile (OSX and Linux), and via
OpenGL ES 3.1 GL_EXT_texture_norm16 extension (including ANGLE on Windows).

Update khronos headers to get GL_R16_EXT definition.

It is not exposed through WebGL but only used internally.
    
Brings significant performance improvement, cutting the time spent on
WebGL TexImage2D from ~1.2ms to ~0.2ms - see https://crbug.com/624436.

BUG=624436
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

Review-Url: https://codereview.chromium.org/2767063002
Cr-Commit-Position: refs/heads/master@{#467924}
Committed: https://chromium.googlesource.com/chromium/src/+/10751e1866092a059e6725361dfea8553b449f43

[aleksandar.stojiljkovic, 2017-03-22 15:04:56]

Description was changed from

==========
16-bit video upload to float: intermediate R16_EXT and copy to float.

R16_EXT is supported on desktop and with OpenGL ES 3.1 GL_EXT_texture_norm16
extension. It is not exposed through WebGL but only used internally.

Brings significant performance improvement. See https://crbug/624436

BUG=624436
==========

to

==========
16-bit video upload to float: intermediate R16_EXT and copy to float.

R16_EXT is supported on desktop and with OpenGL ES 3.1 GL_EXT_texture_norm16
extension. It is not exposed through WebGL but only used internally.

Brings significant performance improvement. See https://crbug/624436

BUG=624436
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
==========

[aleksandar.stojiljkovic, 2017-03-22 15:05:48]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 15:06:14]

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-03-22 15:37:36]

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

[commit-bot: I haz the power, 2017-03-22 15:37:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aleksandar.stojiljkovic, 2017-03-22 16:35:26]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 16:35:42]

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-03-22 17:59:10]

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

[commit-bot: I haz the power, 2017-03-22 17:59:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aleksandar.stojiljkovic, 2017-03-27 10:33:17]

Patchset #3 (id:40001) has been deleted

[aleksandar.stojiljkovic, 2017-03-27 10:33:36]

Patchset #1 (id:1) has been deleted

[aleksandar.stojiljkovic, 2017-03-27 10:36:45]

Patchset #1 (id:20001) has been deleted

[aleksandar.stojiljkovic, 2017-03-27 13:22:58]

Patchset #2 (id:80001) has been deleted

[aleksandar.stojiljkovic, 2017-03-31 09:09:17]

Patchset #2 (id:100001) has been deleted

[aleksandar.stojiljkovic, 2017-03-31 09:09:55]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 09:10:11]

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-03-31 09:12:30]

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

[commit-bot: I haz the power, 2017-03-31 09:12:31]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[aleksandar.stojiljkovic, 2017-03-31 09:25:40]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[aleksandar.stojiljkovic, 2017-03-31 09:25:46]

Patchset #2 (id:120001) has been deleted

[commit-bot: I haz the power, 2017-03-31 09:25:54]

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-03-31 09:36:49]

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

[commit-bot: I haz the power, 2017-03-31 09:36:51]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[aleksandar.stojiljkovic, 2017-03-31 10:18:01]

Patchset #2 (id:140001) has been deleted

[aleksandar.stojiljkovic, 2017-03-31 10:20:00]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 10:20:13]

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-03-31 12:22:58]

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

[commit-bot: I haz the power, 2017-03-31 12:22:59]

Dry run: This issue passed the CQ dry run.

[aleksandar.stojiljkovic, 2017-03-31 12:30:08]

Description was changed from

==========
16-bit video upload to float: intermediate R16_EXT and copy to float.

R16_EXT is supported on desktop and with OpenGL ES 3.1 GL_EXT_texture_norm16
extension. It is not exposed through WebGL but only used internally.

Brings significant performance improvement. See https://crbug/624436

BUG=624436
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
16-bit video upload to float: intermediate R16_EXT and copy to float.

R16_EXT is supported on desktop core profile (OSX and Linux), and via
OpenGL ES 3.1 GL_EXT_texture_norm16 extension (including ANGLE on Windows).

It is not exposed through WebGL but only used internally.
    
Brings significant performance improvement, cutting time spent in
WebGL TexImage2D from ~1.2ms to ~0.2ms - see https://crbug/624436.

BUG=624436
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
==========

[aleksandar.stojiljkovic, 2017-03-31 12:30:27]

Patchset #1 (id:60001) has been deleted

[aleksandar.stojiljkovic, 2017-03-31 12:46:47]

aleksandar.stojiljkovic@intel.com changed reviewers:
+ hubbe@chromium.org, kbr@chromium.org, kinuko@chromium.org

[aleksandar.stojiljkovic, 2017-03-31 12:46:48]

kbr@chromium.org: Please review changes in 
gpu/*
third_party/WebKit/Source/*

kinuko@chromium.org: Please review changes in 
content/*
third_party/WebKit/public/platform/WebMediaPlayer.h

hubbe@chromium.org: Please review changes in 
 media/renderers/skcanvas_video_renderer*

cc ccameron@ and qiankun.miao@intel.com for non owner review.
Thanks.

[aleksandar.stojiljkovic, 2017-03-31 12:53:40]

Description was changed from

==========
16-bit video upload to float: intermediate R16_EXT and copy to float.

R16_EXT is supported on desktop core profile (OSX and Linux), and via
OpenGL ES 3.1 GL_EXT_texture_norm16 extension (including ANGLE on Windows).

It is not exposed through WebGL but only used internally.
    
Brings significant performance improvement, cutting time spent in
WebGL TexImage2D from ~1.2ms to ~0.2ms - see https://crbug/624436.

BUG=624436
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
16-bit video upload to float: intermediate R16_EXT and copy to float.

R16_EXT is supported on desktop core profile (OSX and Linux), and via
OpenGL ES 3.1 GL_EXT_texture_norm16 extension (including ANGLE on Windows).

It is not exposed through WebGL but only used internally.
    
Brings significant performance improvement, cutting the time spent on
WebGL TexImage2D from ~1.2ms to ~0.2ms - see https://crbug.com/624436.

BUG=624436
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
==========

[hubbe, 2017-03-31 17:05:27]

https://codereview.chromium.org/2767063002/diff/160001/media/renderers/skcanv...
File media/renderers/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/2767063002/diff/160001/media/renderers/skcanv...
media/renderers/skcanvas_video_renderer.cc:931: target, texture, gl, frame,
0x822A /*GL_R16_EXT*/, GL_RED,
I think we need to add GL_R16_EXT to the right header files..

[aleksandar.stojiljkovic, 2017-03-31 19:20:11]

On 2017/03/31 17:05:27, hubbe wrote:
>
https://codereview.chromium.org/2767063002/diff/160001/media/renderers/skcanv...
> File media/renderers/skcanvas_video_renderer.cc (right):
> 
>
https://codereview.chromium.org/2767063002/diff/160001/media/renderers/skcanv...
> media/renderers/skcanvas_video_renderer.cc:931: target, texture, gl, frame,
> 0x822A /*GL_R16_EXT*/, GL_RED,
> I think we need to add GL_R16_EXT to the right header files..

Yes, need to update khronos header files.

kbr@chromium.org,

third_party/khronos/GLES2/gl2ext.h with local changes got rebased to the Khronos
OpenGL® ES Registry version
(https://www.khronos.org/registry/OpenGL/api/GLES2/gl2ext.h) 2 years and 5
months ago: https://codereview.chromium.org/637953007.
If we update third_party/khronos/GLES2/gl2ext.h, we would need to update
third_party/khronos/GLES3 as it uses the same gl2ext.h.
Or, should I update all the header files under third_party/khronos and check if
all of the local changes listed in src/third_party/khronos/README.chromium are
still required? 

Thanks.

[Ken Russell (switch to Gerrit), 2017-03-31 23:02:05]

On 2017/03/31 19:20:11, aleksandar.stojiljkovic wrote:
> On 2017/03/31 17:05:27, hubbe wrote:
> >
>
https://codereview.chromium.org/2767063002/diff/160001/media/renderers/skcanv...
> > File media/renderers/skcanvas_video_renderer.cc (right):
> > 
> >
>
https://codereview.chromium.org/2767063002/diff/160001/media/renderers/skcanv...
> > media/renderers/skcanvas_video_renderer.cc:931: target, texture, gl, frame,
> > 0x822A /*GL_R16_EXT*/, GL_RED,
> > I think we need to add GL_R16_EXT to the right header files..
> 
> Yes, need to update khronos header files.
> 
> mailto:kbr@chromium.org,
> 
> third_party/khronos/GLES2/gl2ext.h with local changes got rebased to the
Khronos
> OpenGL® ES Registry version
> (https://www.khronos.org/registry/OpenGL/api/GLES2/gl2ext.h) 2 years and 5
> months ago: https://codereview.chromium.org/637953007.
> If we update third_party/khronos/GLES2/gl2ext.h, we would need to update
> third_party/khronos/GLES3 as it uses the same gl2ext.h.
> Or, should I update all the header files under third_party/khronos and check
if
> all of the local changes listed in src/third_party/khronos/README.chromium are
> still required? 
> 
> Thanks.

Yes, please try updating the header files to the latest versions and see whether
the old diffs still need to be applied.

Thanks.

[Ken Russell (switch to Gerrit), 2017-03-31 23:39:30]

I only had time to skim the code right now; it generally seems OK but I'd like
to defer a full review until the header files are fixed and the hardcoded
constants are removed. One question.

https://codereview.chromium.org/2767063002/diff/160001/media/renderers/skcanv...
File media/renderers/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/2767063002/diff/160001/media/renderers/skcanv...
media/renderers/skcanvas_video_renderer.cc:917: // not widely supported. It is
used on Windows, Linux and OSX.
EXT_texture_norm16 is supported on recent Qualcomm chipsets and mobile NVIDIA
GPUs. We found it was on the Nexus 5X and Google Pixel phones. See also
http://opengles.gpuinfo.org/gles_listreports.php?extension=GL_EXT_texture_norm16
(though, agreed, only 7.5% coverage). Worth changing CopyTextureCHROMIUM to add
a highp alternative for these devices?

[aleksandar.stojiljkovic, 2017-04-02 09:03:51]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-02 09:03:54]

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

[aleksandar.stojiljkovic, 2017-04-02 09:04:38]

Description was changed from

==========
16-bit video upload to float: intermediate R16_EXT and copy to float.

R16_EXT is supported on desktop core profile (OSX and Linux), and via
OpenGL ES 3.1 GL_EXT_texture_norm16 extension (including ANGLE on Windows).

It is not exposed through WebGL but only used internally.
    
Brings significant performance improvement, cutting the time spent on
WebGL TexImage2D from ~1.2ms to ~0.2ms - see https://crbug.com/624436.

BUG=624436
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
16-bit video upload to float: intermediate R16_EXT and copy to float.

R16_EXT is supported on desktop core profile (OSX and Linux), and via
OpenGL ES 3.1 GL_EXT_texture_norm16 extension (including ANGLE on Windows).

Update khronos headers to get GL_R16_EXT definition.

It is not exposed through WebGL but only used internally.
    
Brings significant performance improvement, cutting the time spent on
WebGL TexImage2D from ~1.2ms to ~0.2ms - see https://crbug.com/624436.

BUG=624436
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
==========

[commit-bot: I haz the power, 2017-04-02 10:38:38]

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

[commit-bot: I haz the power, 2017-04-02 10:38:39]

Dry run: This issue passed the CQ dry run.

[aleksandar.stojiljkovic, 2017-04-02 11:28:12]

On 2017/03/31 23:02:05, Ken Russell wrote:
> On 2017/03/31 19:20:11, aleksandar.stojiljkovic wrote:
> > On 2017/03/31 17:05:27, hubbe wrote:
> > >
> >
>
https://codereview.chromium.org/2767063002/diff/160001/media/renderers/skcanv...
> > > File media/renderers/skcanvas_video_renderer.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2767063002/diff/160001/media/renderers/skcanv...
> > > media/renderers/skcanvas_video_renderer.cc:931: target, texture, gl,
frame,
> > > 0x822A /*GL_R16_EXT*/, GL_RED,
> > > I think we need to add GL_R16_EXT to the right header files..
> > 
> > Yes, need to update khronos header files.
> > 
> > mailto:kbr@chromium.org,
> > 
> > third_party/khronos/GLES2/gl2ext.h with local changes got rebased to the
> Khronos
> > OpenGL® ES Registry version
> > (https://www.khronos.org/registry/OpenGL/api/GLES2/gl2ext.h) 2 years and 5
> > months ago: https://codereview.chromium.org/637953007.
> > If we update third_party/khronos/GLES2/gl2ext.h, we would need to update
> > third_party/khronos/GLES3 as it uses the same gl2ext.h.
> > Or, should I update all the header files under third_party/khronos and check
> if
> > all of the local changes listed in src/third_party/khronos/README.chromium
are
> > still required? 
> > 
> > Thanks.
> 
> Yes, please try updating the header files to the latest versions and see
whether
> the old diffs still need to be applied.
> 
> Thanks.

Done.
Some of the Chromium-specific changes required are now obsolete - and they are
removed from README.chromium now. 
Added "Chromium-specific" comments in the code to several places without the
comment.
Didn't apply "git cl format" on khronos header files downloaded from
https://www.khronos.org/registry/OpenGL/index_es.php#headers and EGL registry.
Hardcoded constants removed.

[aleksandar.stojiljkovic, 2017-04-02 11:28:29]

https://codereview.chromium.org/2767063002/diff/160001/media/renderers/skcanv...
File media/renderers/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/2767063002/diff/160001/media/renderers/skcanv...
media/renderers/skcanvas_video_renderer.cc:917: // not widely supported. It is
used on Windows, Linux and OSX.
On 2017/03/31 23:39:30, Ken Russell wrote:
> EXT_texture_norm16 is supported on recent Qualcomm chipsets and mobile NVIDIA
> GPUs. We found it was on the Nexus 5X and Google Pixel phones. See also
>
http://opengles.gpuinfo.org/gles_listreports.php?extension=GL_EXT_texture_norm16
> (though, agreed, only 7.5% coverage). Worth changing CopyTextureCHROMIUM to
add
> a highp alternative for these devices?

Tested it with highp and it works fine on Adreno 418.

However, I'd like to defer that work until there is a real need - Tango depth
camera support is under work [1] but the Tango depth camera API provides float
values (projected pointcloud).

Don't know yet if there will be a need for highp alternative as part of work on
HDR-10 support [2], but there is no real use case yet to justify the change in
gles2_cmd_copy_texture_chromium.cc[3].

[1] Tango support: crbug.com/674440
[2] HDR-10 related patch crrev.com/2122573003
[3]
https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gles2_cmd_cop...

https://codereview.chromium.org/2767063002/diff/160001/media/renderers/skcanv...
media/renderers/skcanvas_video_renderer.cc:931: target, texture, gl, frame,
0x822A /*GL_R16_EXT*/, GL_RED,
On 2017/03/31 17:05:27, hubbe wrote:
> I think we need to add GL_R16_EXT to the right header files..

Done.

[qiankun, 2017-04-10 09:43:26]

qiankun.miao@intel.com changed reviewers:
+ qiankun.miao@intel.com

[qiankun, 2017-04-10 09:43:27]

https://codereview.chromium.org/2767063002/diff/180001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2767063002/diff/180001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16555: //
TODO(aleksandar.stojiljkovic): Use sized internal formats: crbug.com/662644
Did you mean supporting sized internal formats for CopyTextureCHROMIUM? Could
you describe this TODO in more detail? Also file a standalone bug other than
putting a meta bug here.

[aleksandar.stojiljkovic, 2017-04-11 21:33:27]

Rebased. In previous patch, also hardcoded constant issue was resolved.
Decided to bundle the headers in the same patch as a patch with only headers
didn't trigger full testing.

I'll repeat the the first comment here as asking kbr@ to review also
third_party/khronos headers.
Thanks. 

kbr@chromium.org: Please review changes in 
gpu/*
third_party/WebKit/Source/*
third_party/khronos

kinuko@chromium.org: Please review changes in 
content/*
third_party/WebKit/public/platform/WebMediaPlayer.h

hubbe@chromium.org: Please review changes in 
 media/renderers/skcanvas_video_renderer*

https://codereview.chromium.org/2767063002/diff/180001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/2767063002/diff/180001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:16555: //
TODO(aleksandar.stojiljkovic): Use sized internal formats: crbug.com/662644
On 2017/04/10 09:43:27, qiankun wrote:
> Did you mean supporting sized internal formats for CopyTextureCHROMIUM? Could
> you describe this TODO in more detail? Also file a standalone bug other than
> putting a meta bug here.

Thanks, the bug 662644 was wrong - changed it to 628064.

[Ken Russell (switch to Gerrit), 2017-04-20 01:47:07]

LGTM assuming you add a WebGL conformance test verifying that it's not possible
to allocate textures from JavaScript manually specifying the EXT_texture_norm16
enums.

https://codereview.chromium.org/2767063002/diff/200001/gpu/command_buffer/ser...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/2767063002/diff/200001/gpu/command_buffer/ser...
gpu/command_buffer/service/feature_info.cc:1306:
validators_.texture_unsized_internal_format.AddValue(GL_RED_EXT);
Have you confirmed that these aren't accessible when calling through the WebGL
API? I'm pretty sure that WebGL2RenderingContextBase should catch attempts to
use them.

Could you add a WebGL conformance test under conformance2/textures/misc/ that
tries to allocate textures with these internal formats, and confirms that the
attempts fail?

We also need to make sure that we can't now suddenly allocate unsized GL_RED
textures from WebGL, if GL_R8 is the only legally supported internal format from
the ES3 standpoint.

https://codereview.chromium.org/2767063002/diff/200001/media/renderers/skcanv...
File media/renderers/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/2767063002/diff/200001/media/renderers/skcanv...
media/renderers/skcanvas_video_renderer.cc:916: // bellow is not used on
Android, where the extension EXT_texture_norm16 is
bellow -> below

https://codereview.chromium.org/2767063002/diff/200001/media/renderers/skcanv...
File media/renderers/skcanvas_video_renderer.h (right):

https://codereview.chromium.org/2767063002/diff/200001/media/renderers/skcanv...
media/renderers/skcanvas_video_renderer.h:107: unsigned texture,
Please document |texture| -- i.e., if it's the one currently bound to |target|,
what is the state of the binding to |target| upon return, etc.

Also, if this is really only used for 16-bit textures, then "TexImage2D" is too
generic and confusing a name, and I'd ask that you rename it to something like
"TexImage2DY16" and change the call sites.

https://codereview.chromium.org/2767063002/diff/200001/media/renderers/skcanv...
media/renderers/skcanvas_video_renderer.h:123: static bool
TexSubImage2D(unsigned target,
Same comment about the naming of this function.

[kinuko, 2017-04-26 02:23:48]

content/ lgtm

[aleksandar.stojiljkovic, 2017-04-26 13:29:40]

hubbe@, PTAL

https://codereview.chromium.org/2767063002/diff/200001/gpu/command_buffer/ser...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/2767063002/diff/200001/gpu/command_buffer/ser...
gpu/command_buffer/service/feature_info.cc:1306:
validators_.texture_unsized_internal_format.AddValue(GL_RED_EXT);
On 2017/04/20 01:47:06, Ken Russell wrote:
> Have you confirmed that these aren't accessible when calling through the WebGL
> API? I'm pretty sure that WebGL2RenderingContextBase should catch attempts to
> use them.
> 
> Could you add a WebGL conformance test under conformance2/textures/misc/ that
> tries to allocate textures with these internal formats, and confirms that the
> attempts fail?
> 
> We also need to make sure that we can't now suddenly allocate unsized GL_RED
> textures from WebGL, if GL_R8 is the only legally supported internal format
from
> the ES3 standpoint.

Thanks.
Done.
https://github.com/KhronosGroup/WebGL/pull/2379

https://codereview.chromium.org/2767063002/diff/200001/media/renderers/skcanv...
File media/renderers/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/2767063002/diff/200001/media/renderers/skcanv...
media/renderers/skcanvas_video_renderer.cc:916: // bellow is not used on
Android, where the extension EXT_texture_norm16 is
On 2017/04/20 01:47:06, Ken Russell wrote:
> bellow -> below

Done.

https://codereview.chromium.org/2767063002/diff/200001/media/renderers/skcanv...
File media/renderers/skcanvas_video_renderer.h (right):

https://codereview.chromium.org/2767063002/diff/200001/media/renderers/skcanv...
media/renderers/skcanvas_video_renderer.h:107: unsigned texture,
On 2017/04/20 01:47:07, Ken Russell wrote:
> Please document |texture| -- i.e., if it's the one currently bound to
|target|,
> what is the state of the binding to |target| upon return, etc.

Done.

> 
> Also, if this is really only used for 16-bit textures, then "TexImage2D" is
too
> generic and confusing a name, and I'd ask that you rename it to something like
> "TexImage2DY16" and change the call sites.

Plan to use it at least for Y8 and will try to optimize HDR RGB10_A2 upload
using it. Changed the description to clarify it.

[hubbe, 2017-04-26 18:09:32]

good overall, just a few minor nits

https://codereview.chromium.org/2767063002/diff/220001/gpu/command_buffer/tes...
File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right):

https://codereview.chromium.org/2767063002/diff/220001/gpu/command_buffer/tes...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:573: return false;
Why can't we just always use the feature flag?
Maybe add a EXPECT_EQ() to make sure that the feature flag is always true on
desktop?

https://codereview.chromium.org/2767063002/diff/220001/third_party/khronos/GL...
File third_party/khronos/GLES3/gl3platform.h (right):

https://codereview.chromium.org/2767063002/diff/220001/third_party/khronos/GL...
third_party/khronos/GLES3/gl3platform.h:5: ** Copyright (c) 2017 The Khronos
Group Inc.
Shouldn't all these header updates be in a separate CL?

[aleksandar.stojiljkovic, 2017-04-26 19:04:50]

https://codereview.chromium.org/2767063002/diff/220001/gpu/command_buffer/tes...
File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right):

https://codereview.chromium.org/2767063002/diff/220001/gpu/command_buffer/tes...
gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:573: return false;
On 2017/04/26 18:09:31, hubbe wrote:
> Why can't we just always use the feature flag?
> Maybe add a EXPECT_EQ() to make sure that the feature flag is always true on
> desktop?

Done.

https://codereview.chromium.org/2767063002/diff/220001/third_party/khronos/GL...
File third_party/khronos/GLES3/gl3platform.h (right):

https://codereview.chromium.org/2767063002/diff/220001/third_party/khronos/GL...
third_party/khronos/GLES3/gl3platform.h:5: ** Copyright (c) 2017 The Khronos
Group Inc.
On 2017/04/26 18:09:31, hubbe wrote:
> Shouldn't all these header updates be in a separate CL?

Initially tried it that way but decided to bundle it to the patch using the new
headers as mentioned; a patch with only headers
didn't trigger build and full testing.
e.g. "No compile necessary"
bots under https://codereview.chromium.org/2795553002/#ps20001
https://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu...

[hubbe, 2017-04-26 19:19:18]

lgtm

[aleksandar.stojiljkovic, 2017-04-27 05:55:55]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com

[aleksandar.stojiljkovic, 2017-04-27 05:55:56]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/2767063002/#ps240001
(title: "Nit.")

[commit-bot: I haz the power, 2017-04-27 05:56:08]

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-04-27 06:05:57]

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

[commit-bot: I haz the power, 2017-04-27 06:05:58]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[aleksandar.stojiljkovic, 2017-04-27 06:59:03]

aleksandar.stojiljkovic@intel.com changed reviewers:
+ nasko@chromium.org

[aleksandar.stojiljkovic, 2017-04-27 06:59:04]

file://ipc/SECURITY_OWNERS review needed for
gpu/ipc/common/gpu_command_buffer_traits_multi.h

nasko@chromium.org PTAL gpu/ipc/common/gpu_command_buffer_traits_multi.h

Thanks.

[nasko, 2017-04-27 23:16:31]

gpu/ipc/common/gpu_command_buffer_traits_multi.h LGTM

[aleksandar.stojiljkovic, 2017-04-28 04:59:38]

The CQ bit was checked by aleksandar.stojiljkovic@intel.com

[commit-bot: I haz the power, 2017-04-28 05:00:06]

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-04-28 07:55:15]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1493355578803830,
"parent_rev": "e80386955278208db321641577837ed7d1771172", "commit_rev":
"10751e1866092a059e6725361dfea8553b449f43"}

[commit-bot: I haz the power, 2017-04-28 07:55:25]

Description was changed from

==========
16-bit video upload to float: intermediate R16_EXT and copy to float.

R16_EXT is supported on desktop core profile (OSX and Linux), and via
OpenGL ES 3.1 GL_EXT_texture_norm16 extension (including ANGLE on Windows).

Update khronos headers to get GL_R16_EXT definition.

It is not exposed through WebGL but only used internally.
    
Brings significant performance improvement, cutting the time spent on
WebGL TexImage2D from ~1.2ms to ~0.2ms - see https://crbug.com/624436.

BUG=624436
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
16-bit video upload to float: intermediate R16_EXT and copy to float.

R16_EXT is supported on desktop core profile (OSX and Linux), and via
OpenGL ES 3.1 GL_EXT_texture_norm16 extension (including ANGLE on Windows).

Update khronos headers to get GL_R16_EXT definition.

It is not exposed through WebGL but only used internally.
    
Brings significant performance improvement, cutting the time spent on
WebGL TexImage2D from ~1.2ms to ~0.2ms - see https://crbug.com/624436.

BUG=624436
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

Review-Url: https://codereview.chromium.org/2767063002
Cr-Commit-Position: refs/heads/master@{#467924}
Committed:
https://chromium.googlesource.com/chromium/src/+/10751e1866092a059e6725361dfe...
==========

[commit-bot: I haz the power, 2017-04-28 07:55:27]

Committed patchset #5 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/10751e1866092a059e6725361dfe...