16-bit video upload to float: intermediate R16_EXT and copy to float. (issue 2767063002 by aleksandar.stojiljkovic@intel.com)

[aleksandar....@intel.com]

Reviewers: Ken Russell, kinuko, hubbe
CL: https://codereview.chromium.org/2767063002/

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

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

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

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


Description:
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


Affected files (+211, -93 lines):
M content/renderer/media/webmediaplayer_ms.h
M content/renderer/media/webmediaplayer_ms.cc
M content/test/data/media/depth_stream_test_utilities.js
M content/test/data/media/getusermedia-depth-capture.html
M gpu/command_buffer/common/capabilities.h
M gpu/command_buffer/common/gles2_cmd_utils.cc
M gpu/command_buffer/service/feature_info.h
M gpu/command_buffer/service/feature_info.cc
M gpu/command_buffer/service/feature_info_unittest.cc
M gpu/command_buffer/service/gles2_cmd_copy_texture_chromium.cc
M gpu/command_buffer/service/gles2_cmd_decoder.cc
M gpu/command_buffer/service/gles2_cmd_decoder_passthrough.cc
M gpu/command_buffer/service/texture_manager.cc
M gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc
M gpu/ipc/common/gpu_command_buffer_traits_multi.h
M media/renderers/skcanvas_video_renderer.h
M media/renderers/skcanvas_video_renderer.cc
M media/renderers/skcanvas_video_renderer_unittest.cc
M third_party/WebKit/Source/core/html/HTMLVideoElement.h
M third_party/WebKit/Source/core/html/HTMLVideoElement.cpp
M third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
M third_party/WebKit/public/platform/WebMediaPlayer.h
M ui/gl/gl_bindings.h

[hu...@chromium.org]

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

https://codereview.chromium.org/2767063002/diff/160001/media/renderers/skcanvas_video_renderer.cc#newcode931
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..

https://codereview.chromium.org/2767063002/

[aleksandar....@intel.com]

Yes, need to update khronos header files.

k...@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.

https://codereview.chromium.org/2767063002/

[k...@chromium.org]

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/skcanvas_video_renderer.cc
> > File media/renderers/skcanvas_video_renderer.cc (right):
> >
> >
>
https://codereview.chromium.org/2767063002/diff/160001/media/renderers/skcanvas_video_renderer.cc#newcode931
> > 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:k...@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.


https://codereview.chromium.org/2767063002/

[k...@chromium.org]

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/skcanvas_video_renderer.cc
File media/renderers/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/2767063002/diff/160001/media/renderers/skcanvas_video_renderer.cc#newcode917
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?

https://codereview.chromium.org/2767063002/

[aleksandar....@intel.com]

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/skcanvas_video_renderer.cc
> > > File media/renderers/skcanvas_video_renderer.cc (right):
> > >
> > >
> >
>

https://codereview.chromium.org/2767063002/diff/160001/media/renderers/skcanvas_video_renderer.cc#newcode931
> > > 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:k...@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.




https://codereview.chromium.org/2767063002/

[aleksandar....@intel.com]

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

https://codereview.chromium.org/2767063002/diff/160001/media/renderers/skcanvas_video_renderer.cc#newcode917
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_copy_texture_chromium.cc?l=283

https://codereview.chromium.org/2767063002/diff/160001/media/renderers/skcanvas_video_renderer.cc#newcode931
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.

https://codereview.chromium.org/2767063002/

[qianku...@intel.com]

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

https://codereview.chromium.org/2767063002/diff/180001/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode16555
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.

https://codereview.chromium.org/2767063002/

[aleksandar....@intel.com]

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.

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

third_party/khronos

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

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

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

https://codereview.chromium.org/2767063002/diff/180001/gpu/command_buffer/service/gles2_cmd_decoder.cc#newcode16555
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.

https://codereview.chromium.org/2767063002/

[k...@chromium.org]

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/service/feature_info.cc
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/2767063002/diff/200001/gpu/command_buffer/service/feature_info.cc#newcode1306
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/skcanvas_video_renderer.cc
File media/renderers/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/2767063002/diff/200001/media/renderers/skcanvas_video_renderer.cc#newcode916
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/skcanvas_video_renderer.h
File media/renderers/skcanvas_video_renderer.h (right):

https://codereview.chromium.org/2767063002/diff/200001/media/renderers/skcanvas_video_renderer.h#newcode107
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/skcanvas_video_renderer.h#newcode123
media/renderers/skcanvas_video_renderer.h:123: static bool
TexSubImage2D(unsigned target,
Same comment about the naming of this function.

https://codereview.chromium.org/2767063002/

[kin...@chromium.org]

content/ lgtm

https://codereview.chromium.org/2767063002/

[aleksandar....@intel.com]

hubbe@, PTAL

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

https://codereview.chromium.org/2767063002/diff/200001/gpu/command_buffer/service/feature_info.cc#newcode1306
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/skcanvas_video_renderer.cc
File media/renderers/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/2767063002/diff/200001/media/renderers/skcanvas_video_renderer.cc#newcode916
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/skcanvas_video_renderer.h
File media/renderers/skcanvas_video_renderer.h (right):

https://codereview.chromium.org/2767063002/diff/200001/media/renderers/skcanvas_video_renderer.h#newcode107
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.

https://codereview.chromium.org/2767063002/

[hu...@chromium.org]

good overall, just a few minor nits



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

https://codereview.chromium.org/2767063002/diff/220001/gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc#newcode573
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/GLES3/gl3platform.h
File third_party/khronos/GLES3/gl3platform.h (right):

https://codereview.chromium.org/2767063002/diff/220001/third_party/khronos/GLES3/gl3platform.h#newcode5
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?

https://codereview.chromium.org/2767063002/

[aleksandar....@intel.com]

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

https://codereview.chromium.org/2767063002/diff/220001/gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc#newcode573
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/GLES3/gl3platform.h
File third_party/khronos/GLES3/gl3platform.h (right):

https://codereview.chromium.org/2767063002/diff/220001/third_party/khronos/GLES3/gl3platform.h#newcode5
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_tests_rel/builds/8653

https://codereview.chromium.org/2767063002/

[hu...@chromium.org]

lgtm



https://codereview.chromium.org/2767063002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2767063002/240001

https://codereview.chromium.org/2767063002/

[commit-bot@chromium.org via codereview.chromium.org]

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_presubmit/builds/421826)

https://codereview.chromium.org/2767063002/

[aleksandar....@intel.com]

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

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

Thanks.



https://codereview.chromium.org/2767063002/

[na...@chromium.org]

gpu/ipc/common/gpu_command_buffer_traits_multi.h LGTM

https://codereview.chromium.org/2767063002/

[commit-bot@chromium.org via codereview.chromium.org]

[commit-bot@chromium.org via codereview.chromium.org]

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

https://codereview.chromium.org/2767063002/