WebGL & 16-bit video stream: upload to FLOAT texture. (issue 2476693002 by aleksandar.stojiljkovic@intel.com)

[aleksandar....@intel.com]

Reviewers: phoglund_chrome, mcasas, chrishtr, xhwang, Ken Russell
CL: https://codereview.chromium.org/2476693002/

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

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

phog...@chromium.org: Please review changes in
content/browser/webrtc/webrtc_depth_capture_browsertest.cc
and content/test/data

mca...@chromium.org: Please review changes in
content/renderer/media/webmediaplayer_ms.*

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

Thanks.

Description:
Lossless access to 16-bit video stream using WebGL GL_FLOAT texture.

If using canvas.getImageData or GL textures of UNSIGNED_BYTE type, 16-bit depth
stream data is available only through 8-bit* API, so there is a precision loss.
Here, we add no-precision-loss** JavaScript access through WebGL float texture.

RGBA32F usage here enables lossless access to 16-bit depth information via
WebGL1.
In related work, the same code path is used to upload 16-bit data to other
WebGL2
supported formats; e.g. with GL_R16UI there is no conversion needed in
SkCanvasVideoRenderer::TexImageImpl.

* 8-bit access refers to JS ImageData and WebGL UNSIGNED_BYTE where 16-bit depth
data is now available as luminance (all 3 color channels contains upper 8 bits
of 16bit value).

** Float is used for no-precision-loss access to 16-bit data normalized to
[0-1.0] range using formula value_float = value_16bit/65535.0.

This patch also adds UNSIGNED_BYTE WebGL test which was earlier tested through
testVideoToImageBitmap since UNSIGNED_BYTE and canvas rendering still share the
same path through SkCanvasVideoRenderer::Paint.

BUG=369849, 624436
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

Affected files (+666, -145 lines):
M content/browser/webrtc/webrtc_depth_capture_browsertest.cc
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 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.h
M third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
M third_party/WebKit/public/platform/WebMediaPlayer.h

[phog...@chromium.org]

Looks nice, but I'm concerned you're covering what looks like an enormous amount
of functionality in just one test. I have some suggestions on how to split them
up into a bunch more tests on the C++ level (looks like you'd want like 8 or
more of them instead of your single one today).


https://codereview.chromium.org/2476693002/diff/100001/content/test/data/media/getusermedia-depth-capture.html
File content/test/data/media/getusermedia-depth-capture.html (right):

https://codereview.chromium.org/2476693002/diff/100001/content/test/data/media/getusermedia-depth-capture.html#newcode64
content/test/data/media/getusermedia-depth-capture.html:64: {
Nit: pull up braces to previous line (apply throughout)

https://codereview.chromium.org/2476693002/diff/100001/content/test/data/media/getusermedia-depth-capture.html#newcode113
content/test/data/media/getusermedia-depth-capture.html:113: function
testVideoToWebGLTexture(videoElementName, success, error)
Will you get clear test errors when any of the sub-tests in this
function fails? Will test errors obscure other test errors? You're
testing a lot of things here for one browser test, so you run the risk
of really confusing test errors. Also, if one of the sub-tests goes
flaky your entire test will probably be disabled even if the other
sub-tests are fine.

You can remedy this by splitting up the test; for instance by splitting
up the below into one 2D and one Cubemap test, or even 2d_unsigned_byte,
2d_float, cubemap_unsigned_byte, cubemap_float. Each such test gets
called by one browser test on the c++ level. You can solve the resulting
duplication problem using a higher-order function, e.g.

function runOneGlTest(glTestCaseFunction) {
getFake16bitStream().then(function(stream) {
detectVideoInLocalView1(stream, function() {
glTestCaseFunction('local-view-1', function(skip_info) {
if (skip_info) {
console.log("SKIP " + glTestCaseFunction + ": " +
skip_info);
}
stream.getVideoTracks()[0].stop();
waitForVideoToStop('local-view-1');
}, failedCallback);
});
}, failedCallback);

It's your call, but if you keep this as-is, at least test manually
introducing some failures. That way you can check that you get
reasonable test failure messages and that the tests don't interfere with
each other.

https://codereview.chromium.org/2476693002/diff/100001/content/test/data/media/getusermedia-depth-capture.html#newcode177
content/test/data/media/getusermedia-depth-capture.html:177: //For
cubemap, binding_target is gl.TEXTURE_CUBE_MAP and target is a face id.
Nit: Space between // and For; this and next line

https://codereview.chromium.org/2476693002/diff/100001/content/test/data/media/getusermedia-depth-capture.html#newcode181
content/test/data/media/getusermedia-depth-capture.html:181: var tex =
gl.createTexture();
Nit: indent 2 here, not 1

https://codereview.chromium.org/2476693002/diff/100001/content/test/data/media/getusermedia-depth-capture.html#newcode207
content/test/data/media/getusermedia-depth-capture.html:207: if
(use_sub_image_2d) {
I don't like this kind of complexity in tests; they should generally
have a cyclomatic complexity of one (e.g. no branches). You're basically
at the level where the test is complex enough to need a test of its own.
One suggestion is to replace

var p3 = runWebGLTextureTest(gl, gl.TEXTURE_CUBE_MAP,
gl.TEXTURE_CUBE_MAP_POSITIVE_Z, type, video, false/*sub_image*/,
true/*flip_y*/, true/*premultiply_alpha*/);

with

var texture = createTexture(gl, binding_target, ...);
uploadVideoFrameSubImage2D(texture, binding_target, ...);
var p3 = readAndVerifyPixels(texture, ...);

That way you can compose your tests by calling
uploadVideoFrameSubImage2D, which is now a simple three-line function,
or uploadVideoFrame which is a one-liner.

https://codereview.chromium.org/2476693002/

[aleksandar....@intel.com]

xhwang_OOO_11_07/dalec...@chromium.org: PTAL
media/renderers/skcanvas_video_renderer.*
media/renderers/skcanvas_video_renderer_unittest.*

Thanks.

https://codereview.chromium.org/2476693002/

[dalec...@chromium.org]

=>hubbe who's done the other reviews.

https://codereview.chromium.org/2476693002/

[hu...@chromium.org]

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

https://codereview.chromium.org/2476693002/diff/100001/media/renderers/skcanvas_video_renderer.cc#newcode527
media/renderers/skcanvas_video_renderer.cc:527: void
FlipAndConvertY16(const VideoFrame* video_frame,
Add comment.

https://codereview.chromium.org/2476693002/diff/100001/media/renderers/skcanvas_video_renderer.cc#newcode819
media/renderers/skcanvas_video_renderer.cc:819: unsigned
output_bytes_per_pixel;
Please initialize to something.

https://codereview.chromium.org/2476693002/diff/100001/media/renderers/skcanvas_video_renderer.cc#newcode843
media/renderers/skcanvas_video_renderer.cc:843: NOTREACHED() << "Offsets
are not supported.";
If they are not supported, why even have those arguments?

https://codereview.chromium.org/2476693002/diff/100001/media/renderers/skcanvas_video_renderer.h
File media/renderers/skcanvas_video_renderer.h (right):

https://codereview.chromium.org/2476693002/diff/100001/media/renderers/skcanvas_video_renderer.h#newcode104
media/renderers/skcanvas_video_renderer.h:104: // Returns false if there
is no implementation for given parameters or if the
Please make it return an enum so that we can separate "no
implemnetation" from "runtime failure".

(Although, I see the code has NOTREACHED() in those places, so maybe an
enum is not needed?)

https://codereview.chromium.org/2476693002/diff/100001/media/renderers/skcanvas_video_renderer.h#newcode106
media/renderers/skcanvas_video_renderer.h:106: static bool
TexImageImpl(const char* functionID,
I don't quite get what functionID is doing here, or why it's a char*.

https://codereview.chromium.org/2476693002/

[k...@chromium.org]

I'd like to defer review until phoglund's concerns about the tests have been
addressed. The code changes under third_party/WebKit look reasonable, but I am
trusting the tests to verify these changes, more than my code review.

For what it's worth, if this can wait to land until after the M56 branch point
next Thursday the 17th I would appreciate it -- we are making changes to the
WebGL texture upload paths right now.


https://codereview.chromium.org/2476693002/

[aleksandar....@intel.com]

Thanks. Of course, won't push before 17th anyway.

https://codereview.chromium.org/2476693002/

[aleksandar....@intel.com]

Patch set #2: Fixes. phoglung@, hubbe@, thanks.
PTAL.

https://codereview.chromium.org/2476693002/diff/100001/content/test/data/media/getusermedia-depth-capture.html
File content/test/data/media/getusermedia-depth-capture.html (right):

https://codereview.chromium.org/2476693002/diff/100001/content/test/data/media/getusermedia-depth-capture.html#newcode64
content/test/data/media/getusermedia-depth-capture.html:64: {

On 2016/11/07 09:20:00, phoglund_chrome wrote:
> Nit: pull up braces to previous line (apply throughout)

Done.

https://codereview.chromium.org/2476693002/diff/100001/content/test/data/media/getusermedia-depth-capture.html#newcode113
content/test/data/media/getusermedia-depth-capture.html:113: function
testVideoToWebGLTexture(videoElementName, success, error)

Done.
The only reason to couple unsigned byte and float was to save time on
test initialization. More tests planned here for different formats and
proposed refactoring made sense. I kept cubemap and texture2d in the
same run, but with clarified error reporting. Thanks.

https://codereview.chromium.org/2476693002/diff/100001/content/test/data/media/getusermedia-depth-capture.html#newcode177
content/test/data/media/getusermedia-depth-capture.html:177: //For
cubemap, binding_target is gl.TEXTURE_CUBE_MAP and target is a face id.

On 2016/11/07 09:20:00, phoglund_chrome wrote:
> Nit: Space between // and For; this and next line

Done.

https://codereview.chromium.org/2476693002/diff/100001/content/test/data/media/getusermedia-depth-capture.html#newcode181
content/test/data/media/getusermedia-depth-capture.html:181: var tex =
gl.createTexture();

On 2016/11/07 09:20:00, phoglund_chrome wrote:
> Nit: indent 2 here, not 1

Done.

https://codereview.chromium.org/2476693002/diff/100001/content/test/data/media/getusermedia-depth-capture.html#newcode207
content/test/data/media/getusermedia-depth-capture.html:207: if
(use_sub_image_2d) {

On 2016/11/07 09:20:00, phoglund_chrome wrote:
> I don't like this kind of complexity in tests; they should generally
have a
> cyclomatic complexity of one (e.g. no branches). You're basically at
the level
> where the test is complex enough to need a test of its own. One
suggestion is to
> replace
>
> var p3 = runWebGLTextureTest(gl, gl.TEXTURE_CUBE_MAP,
> gl.TEXTURE_CUBE_MAP_POSITIVE_Z, type, video, false/*sub_image*/,
> true/*flip_y*/, true/*premultiply_alpha*/);
>
> with
>
> var texture = createTexture(gl, binding_target, ...);
> uploadVideoFrameSubImage2D(texture, binding_target, ...);
> var p3 = readAndVerifyPixels(texture, ...);
>
> That way you can compose your tests by calling
uploadVideoFrameSubImage2D, which
> is now a simple three-line function, or uploadVideoFrame which is a
one-liner.

Done. Rewritten, no branching.

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

https://codereview.chromium.org/2476693002/diff/100001/media/renderers/skcanvas_video_renderer.cc#newcode527
media/renderers/skcanvas_video_renderer.cc:527: void
FlipAndConvertY16(const VideoFrame* video_frame,

On 2016/11/07 22:16:55, hubbe wrote:
> Add comment.

Done.

https://codereview.chromium.org/2476693002/diff/100001/media/renderers/skcanvas_video_renderer.cc#newcode819
media/renderers/skcanvas_video_renderer.cc:819: unsigned
output_bytes_per_pixel;

On 2016/11/07 22:16:55, hubbe wrote:
> Please initialize to something.

Done.

https://codereview.chromium.org/2476693002/diff/100001/media/renderers/skcanvas_video_renderer.cc#newcode843
media/renderers/skcanvas_video_renderer.cc:843: NOTREACHED() << "Offsets
are not supported.";

On 2016/11/07 22:16:55, hubbe wrote:
> If they are not supported, why even have those arguments?

Done. Split the method and now supported in texSubImage2d.

https://codereview.chromium.org/2476693002/diff/100001/media/renderers/skcanvas_video_renderer.h
File media/renderers/skcanvas_video_renderer.h (right):

https://codereview.chromium.org/2476693002/diff/100001/media/renderers/skcanvas_video_renderer.h#newcode104
media/renderers/skcanvas_video_renderer.h:104: // Returns false if there
is no implementation for given parameters or if the

On 2016/11/07 22:16:55, hubbe wrote:
> Please make it return an enum so that we can separate "no
implemnetation" from
> "runtime failure".
>
> (Although, I see the code has NOTREACHED() in those places, so maybe
an enum is
> not needed?)

Done - you're right - no need for enum. Runtime failure possible here,
just "no implementation".

https://codereview.chromium.org/2476693002/diff/100001/media/renderers/skcanvas_video_renderer.h#newcode106
media/renderers/skcanvas_video_renderer.h:106: static bool
TexImageImpl(const char* functionID,

On 2016/11/07 22:16:55, hubbe wrote:
> I don't quite get what functionID is doing here, or why it's a char*.

Done. Replaced char* / enum in WebMediaPlayer.h.

https://codereview.chromium.org/2476693002/

[phog...@chromium.org]

All right. Your tests are still big, but I understand it's hard because of the
large testing matrix here. Extracting the float vs 8bit axis into browser test
methods is at least something. Have you tried introducing some manual test
failures in one of the subtests to make sure they're reported correctly, and
that tests don't interfere with each other?


https://codereview.chromium.org/2476693002/diff/120001/content/test/data/media/getusermedia-depth-capture.html
File content/test/data/media/getusermedia-depth-capture.html (right):

https://codereview.chromium.org/2476693002/diff/120001/content/test/data/media/getusermedia-depth-capture.html#newcode24
content/test/data/media/getusermedia-depth-capture.html:24: //
testVideoToImageBitmap and the tests bellow are layout tests that we
Nit: below

https://codereview.chromium.org/2476693002/diff/120001/content/test/data/media/getusermedia-depth-capture.html#newcode127
content/test/data/media/getusermedia-depth-capture.html:127:
context.drawImage(bitmap,0,0);
Nit: bitmap, 0, 0

https://codereview.chromium.org/2476693002/

[aleksandar....@intel.com]

Patsh set #3: Nits. Thanks phoglund@.

On 2016/11/17 10:11:25, phoglund_chrome wrote:
> All right. Your tests are still big, but I understand it's hard because of the
> large testing matrix here. Extracting the float vs 8bit axis into browser test
> methods is at least something. Have you tried introducing some manual test
> failures in one of the subtests to make sure they're reported correctly, and
> that tests don't interfere with each other?
>

I did, of course. Also run it on different bots with errors. From error logs I
was able to detect which state fails. However, to help someone else to debug it,
added test_name to the error message.
Thanks.

https://codereview.chromium.org/2476693002/diff/120001/content/test/data/media/getusermedia-depth-capture.html
File content/test/data/media/getusermedia-depth-capture.html (right):

https://codereview.chromium.org/2476693002/diff/120001/content/test/data/media/getusermedia-depth-capture.html#newcode24
content/test/data/media/getusermedia-depth-capture.html:24: //
testVideoToImageBitmap and the tests bellow are layout tests that we

On 2016/11/17 10:11:25, phoglund_chrome wrote:
> Nit: below

Done.

https://codereview.chromium.org/2476693002/diff/120001/content/test/data/media/getusermedia-depth-capture.html#newcode127
content/test/data/media/getusermedia-depth-capture.html:127:
context.drawImage(bitmap,0,0);

On 2016/11/17 10:11:25, phoglund_chrome wrote:
> Nit: bitmap, 0, 0

Done.

https://codereview.chromium.org/2476693002/

[aleksandar....@intel.com]

+xhwang@

media/renderers/skcanvas_video_renderer.*
media/renderers/skcanvas_video_renderer_unittest.*

PTAL.

hubbe@ already started review on this (comments processed) but adding xhwang@ in
case hubbe@ is busy and since xhwang@ reviewed previous patch that this one is
refactoring (crrev.com/2428263004).

https://codereview.chromium.org/2476693002/

[hu...@chromium.org]

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

https://codereview.chromium.org/2476693002/diff/160001/media/renderers/skcanvas_video_renderer.cc#newcode582
media/renderers/skcanvas_video_renderer.cc:582: switch (frame->format())
{
Wouldn't it be simpler to just do:

if (frame->format() == PIXEL_FORMAT_Y16 &&
format == GL_RGBA &&
type == GL_FLOAT) {
...
} else {
return false;
}

?

https://codereview.chromium.org/2476693002/

[aleksandar....@intel.com]

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

https://codereview.chromium.org/2476693002/diff/160001/media/renderers/skcanvas_video_renderer.cc#newcode582
media/renderers/skcanvas_video_renderer.cc:582: switch (frame->format())
{

On 2016/11/17 20:08:31, hubbe wrote:
> Wouldn't it be simpler to just do:
>
> if (frame->format() == PIXEL_FORMAT_Y16 &&
> format == GL_RGBA &&
> type == GL_FLOAT) {
> ...
> } else {
> return false;
> }
>
> ?

I've split this from larger patch - the next patch adds GL_R16UI and
GL_R32F support so this prepares the code for it. If you'd prefer this
formulated simpler, I'll fix it. Thanks.

https://codereview.chromium.org/2476693002/

[hu...@chromium.org]

media/* LGTM

https://codereview.chromium.org/2476693002/

[phog...@chromium.org]

> I did, of course. Also run it on different bots with errors. From error logs I
> was able to detect which state fails. However, to help someone else to debug
it,
> added test_name to the error message.
> Thanks.

Well then. browser tests lgtm

https://codereview.chromium.org/2476693002/

[aleksandar....@intel.com]

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

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

mca...@chromium.org: Please review changes in
content/renderer/media/webmediaplayer_ms.*

Thanks.

https://codereview.chromium.org/2476693002/

[aleksandar....@intel.com]

Adding reviewers as kbr@ and chrishtr@ seem busy.

dpranke@, PTAL at third_party/WebKit/*,
bajones@, PTAL at third_party/WebKit/Source/modules/webgl/*.

Thanks.

https://codereview.chromium.org/2476693002/

[dpr...@chromium.org]

I'm not the right reviewer for this. If kbr@ is unavailable, maybe try pdr@ or
schenney@ ?

https://codereview.chromium.org/2476693002/

[p...@chromium.org]

I'm afraid I am not the best reviewer either. Small (optional, feel free to
skip) suggestion about using the Optional pattern.

I think we should wait for kbr's review.


https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode510
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:510:
bool enabled = true)
This restore class is a nice way of cleaning up the reset/restore calls
but the bool for enabling it is not obvious at the callsites. WDYT of
using base::Optional for this?

The pattern would look something like when there is a conditional:
Optional<ScopedUnpackParametersResetRestore> resetRestoreScope;
if (should reset restore)
resetRestoreScope.emplace(this);

The pattern would look something like this when there's no conditional:
ScopedUnpackParametersResetRestore resetRestoreScope(this);

https://codereview.chromium.org/2476693002/

[aleksandar....@intel.com]

pdr@, dpranke@,
Thanks guys.
kbr@ is OoO until 28.11, so I will ask for review of webgl part first and
eventually will need your review on WebMediaPlayer.h.

zmo@, bajones@, PTAL at third_party/WebKit/Source/modules/webgl

kbr@ was involved from the start on this. Probably some more context could help
the review.
This patch is a split from the main prototype [1] implementing depth and
infrared camera capture.

The patch under review here is, in a way, critical as it is the first
implementation offering losless access to 16 bit depth data from WebGL/JS,... so
allowing 3D point cloud implementation.

Thanks.

[1]
https://codereview.chromium.org/2121043002/




https://codereview.chromium.org/2476693002/

[k...@chromium.org]

Thanks Alex for putting this together.

On which bots are the new tests going to run? Do those bots have WebGL support?
The only bots I'm positive run WebGL-related tests are the GPU bots. I'll be
happy to work with you to get tests running there, but in that case, please take
a look at the tests and harnesses under src/content/test/gpu/gpu_tests/ and
let's talk offline.

The code mostly looks good. Two concerns.

https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode510
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:510:
bool enabled = true)

On 2016/11/22 19:16:01, pdr. wrote:
> This restore class is a nice way of cleaning up the reset/restore
calls

+1, this is nice.

> but the
> bool for enabling it is not obvious at the callsites. WDYT of using
> base::Optional for this?
>
> The pattern would look something like when there is a conditional:
> Optional<ScopedUnpackParametersResetRestore> resetRestoreScope;
> if (should reset restore)
> resetRestoreScope.emplace(this);
>
> The pattern would look something like this when there's no
conditional:
> ScopedUnpackParametersResetRestore resetRestoreScope(this);

I prefer the way Alex has done this. The Optional class seems more
complex and harder to understand to me.

https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode5169
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5169:
target, internalformat, type, level)) {
This change to the if-test will make the semi-accelerated code path
below (using an accelerated image buffer) be taken less often. What is
the rationale for making this change? Can you figure out a way to
preserve the current properties of the code? If you want to inject the
attempted call to HTMLVideoElement::texImageImpl, can you figure out a
slightly different code structure to achieve this?

https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode5219
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5219:
// leaves early for other formats or if frame is stored on GPU.
This now needs to take into consideration the fact that a source
sub-rectangle might be specified for WebGL 2.0. The easiest way for the
moment would be to skip attempting this block if
(!sourceImageRectIsDefault). It can be expanded later to handle
sub-rectangle uploads.

https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/public/platform/WebMediaPlayer.h
File third_party/WebKit/public/platform/WebMediaPlayer.h (right):

https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/public/platform/WebMediaPlayer.h#newcode108
third_party/WebKit/public/platform/WebMediaPlayer.h:108: enum
TexImageFunctionID {
Add a comment that this must stay in sync with the same enum in
WebGLRenderingContextBase.h -- and vice versa -- or factor it into a
tiny shared header.

https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/public/platform/WebMediaPlayer.h#newcode218
third_party/WebKit/public/platform/WebMediaPlayer.h:218: // parameters
or fails, it returns false.
Please define whether this allocates the destination texture, or not,
for both texImage and texSubImage.

https://codereview.chromium.org/2476693002/

[aleksandar....@intel.com]

On 2016/11/22 22:47:45, Ken Russell OOO-till-Nov-28 wrote:
> Thanks Alex for putting this together.
>
> On which bots are the new tests going to run? Do those bots have WebGL
support?
> The only bots I'm positive run WebGL-related tests are the GPU bots. I'll be
> happy to work with you to get tests running there, but in that case, please
take
> a look at the tests and harnesses under src/content/test/gpu/gpu_tests/ and
> let's talk offline.

Thanks kbr@.
After debugging the issues with the bots I could access, the tests now run on
all the bots with this constraint[1]:

+// TODO(aleksandar.stojiljkovic): Enable the test on all the platforms when GPU
+// process initialization crashes gets resolved.
+// See https://crbug.com/662336 and https://crbug.com/510291
+#if defined(USE_OZONE) || defined(OS_WIN) || defined(MEMORY_SANITIZER)

So, on all the platforms except Win (and plan to fix the GPU process startup
there soon).
Note that other webrtc content_browser tests already run on real gpu (not on
osmesa) for Android, MAC, ChromeOS sometimes [2].

[1]
https://codereview.chromium.org/2476693002/patch/180001/190001

[2]
https://cs.chromium.org/chromium/src/content/public/test/browser_test_base.cc?q=switches::kUseGpuInTests&sq=package:chromium&l=247&dr=C

https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode5169
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5169:
target, internalformat, type, level)) {

On 2016/11/22 22:47:44, Ken Russell OOO-till-Nov-28 wrote:
> This change to the if-test will make the semi-accelerated code path
below (using
> an accelerated image buffer) be taken less often. What is the
rationale for
> making this change? Can you figure out a way to preserve the current
properties
> of the code? If you want to inject the attempted call to
> HTMLVideoElement::texImageImpl, can you figure out a slightly
different code
> structure to achieve this?

The change here is orthogonal in respect to the rest of the code. It is
resolving performance issue of using float textures regardless what the
video format is.

> be taken less often
The only difference is that semi-accelerated code is under
Extensions3DUtil::canUseCopyTextureCHROMIUM.
And it should be, as ImageBuffer::copyToPlatformtexture l.5207 returns
early false (for e.g. float formats after canUseCopyTextureCHROMIUM) in
[1].
So, there is no need to do new AcceleratedImageBufferSurface,
video->paintCurrentFrame and texImage2DBase in 5204 as the code would
just early leave call to l.5207.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp?dr=C&sq=package:chromium&rcl=1479832447&l=211

https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode5219
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5219:
// leaves early for other formats or if frame is stored on GPU.

On 2016/11/22 22:47:44, Ken Russell OOO-till-Nov-28 wrote:
> This now needs to take into consideration the fact that a source
sub-rectangle
> might be specified for WebGL 2.0. The easiest way for the moment would
be to
> skip attempting this block if (!sourceImageRectIsDefault). It can be
expanded
> later to handle sub-rectangle uploads.

Thanks. I'll do so.

https://codereview.chromium.org/2476693002/

[aleksandar....@intel.com]

Patch Set 5 : Run WebGL-related tests are the GPU bots. #64 fixes. Thanks kbr@.

On 2016/11/22 22:47:45, Ken Russell OOO-till-Nov-28 wrote:
> Thanks Alex for putting this together.
>
> On which bots are the new tests going to run? Do those bots have WebGL
support?
> The only bots I'm positive run WebGL-related tests are the GPU bots. I'll be
> happy to work with you to get tests running there, but in that case, please
take
> a look at the tests and harnesses under src/content/test/gpu/gpu_tests/ and
> let's talk offline.

Done. Now the tests also run on Windows gpu bots. Thanks.
I kept the code in the same files under content/test/data/media (and didn't
change the code that phoglund@ reviewed except for adding onLoad() method to
getusermedia-depth-capture.html) as there is (and more expected) shared code
between canvas (run from content_browsertests) and Webgl tests (run from gpu
tests).

https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode5219
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5219:
// leaves early for other formats or if frame is stored on GPU.

On 2016/11/22 23:21:06, aleksandar.stojiljkovic wrote:
> On 2016/11/22 22:47:44, Ken Russell OOO-till-Nov-28 wrote:
> > This now needs to take into consideration the fact that a source
sub-rectangle
> > might be specified for WebGL 2.0. The easiest way for the moment
would be to
> > skip attempting this block if (!sourceImageRectIsDefault). It can be
expanded
> > later to handle sub-rectangle uploads.
>
> Thanks. I'll do so.

Done.

https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/public/platform/WebMediaPlayer.h
File third_party/WebKit/public/platform/WebMediaPlayer.h (right):

https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/public/platform/WebMediaPlayer.h#newcode108
third_party/WebKit/public/platform/WebMediaPlayer.h:108: enum
TexImageFunctionID {

On 2016/11/22 22:47:44, Ken Russell OOO-till-Nov-28 wrote:

> Add a comment that this must stay in sync with the same enum in
> WebGLRenderingContextBase.h -- and vice versa -- or factor it into a
tiny shared
> header.

Done - comment added to both places.

https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/public/platform/WebMediaPlayer.h#newcode218
third_party/WebKit/public/platform/WebMediaPlayer.h:218: // parameters
or fails, it returns false.

On 2016/11/22 22:47:44, Ken Russell OOO-till-Nov-28 wrote:

> Please define whether this allocates the destination texture, or not,
for both
> texImage and texSubImage.

Done.

https://codereview.chromium.org/2476693002/

[aleksandar....@intel.com]

Patch Set 6 : Rebase after crrev.com/2527343002 land.

On 2016/11/27 20:44:00, aleksandar.stojiljkovic wrote:
> Patch Set 5 : Run WebGL-related tests are the GPU bots. #64 fixes. Thanks
kbr@.
>
> On 2016/11/22 22:47:45, Ken Russell OOO-till-Nov-28 wrote:
> > Thanks Alex for putting this together.
> >
> > On which bots are the new tests going to run? Do those bots have WebGL
> support?
> > The only bots I'm positive run WebGL-related tests are the GPU bots. I'll be
> > happy to work with you to get tests running there, but in that case, please
> take
> > a look at the tests and harnesses under src/content/test/gpu/gpu_tests/ and
> > let's talk offline.
>
> Done. Now the tests also run on Windows gpu bots. Thanks.
> I kept the code in the same files under content/test/data/media (and didn't
> change the code that phoglund@ reviewed except for adding onLoad() method to
> getusermedia-depth-capture.html) as there is (and more expected) shared code
> between canvas (run from content_browsertests) and Webgl tests (run from gpu
> tests).

kbr@,
PTAL at newly added files:
content/test/gpu/*

Thanks.

https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
File
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp#newcode5169
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:5169:
target, internalformat, type, level)) {

On 2016/11/22 22:47:44, Ken Russell wrote:
> This change to the if-test will make the semi-accelerated code path
below (using
> an accelerated image buffer) be taken less often. What is the
rationale for
> making this change? Can you figure out a way to preserve the current
properties
> of the code? If you want to inject the attempted call to
> HTMLVideoElement::texImageImpl, can you figure out a slightly
different code
> structure to achieve this?

Done. This landed in crrev.com/2527343002.

https://codereview.chromium.org/2476693002/

[k...@chromium.org]

Alex, outstanding work on the new tests. A+ job. These greatly improve my
confidence in this work. Thank you for persevering with it. LGTM with one small
change.



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

https://codereview.chromium.org/2476693002/diff/220001/media/renderers/skcanvas_video_renderer.cc#newcode538
media/renderers/skcanvas_video_renderer.cc:538: uint8_t* out_row_head =
flip_y ? out + output_row_bytes * (height - i - 1)
Perhaps add a TODO about adding SIMD or similar optimization for this
conversion?

https://codereview.chromium.org/2476693002/diff/220001/media/renderers/skcanvas_video_renderer.cc#newcode580
media/renderers/skcanvas_video_renderer.cc:580:
scoped_refptr<DataBuffer>& temp_buffer) {
Mutable references are not allowed per the Google C++ style guide:
https://google.github.io/styleguide/cppguide.html#Reference_Arguments

Please pass this by pointer.

https://codereview.chromium.org/2476693002/

[aleksandar....@intel.com]

On 2016/12/01 08:39:34, Ken Russell wrote:
> Alex, outstanding work on the new tests. A+ job. These greatly improve my
> confidence in this work. Thank you for persevering with it. LGTM with one
small
> change.

Thanks and thanks for help.

>
>
https://codereview.chromium.org/2476693002/diff/220001/media/renderers/skcanvas_video_renderer.cc
> File media/renderers/skcanvas_video_renderer.cc (right):
>
>
https://codereview.chromium.org/2476693002/diff/220001/media/renderers/skcanvas_video_renderer.cc#newcode538
> media/renderers/skcanvas_video_renderer.cc:538: uint8_t* out_row_head = flip_y
?
> out + output_row_bytes * (height - i - 1)
> Perhaps add a TODO about adding SIMD or similar optimization for this
> conversion?

Planning to submit WebGL2 patch using R16UI (and R32F) for review after this
one. R16UI would require no conversion.
Then, the benchmarks before GPU buffer support and SIMD optimizations so we that
could compare.

>
>
https://codereview.chromium.org/2476693002/diff/220001/media/renderers/skcanvas_video_renderer.cc#newcode580
> media/renderers/skcanvas_video_renderer.cc:580: scoped_refptr<DataBuffer>&
> temp_buffer) {
> Mutable references are not allowed per the Google C++ style guide:
> https://google.github.io/styleguide/cppguide.html#Reference_Arguments
>
> Please pass this by pointer.

Yes, I'll cover this before submitting.

pdr@, owner's review needed for
third_party/WebKit/public/platform/WebMediaPlayer.h. PTAL.
mcacas@, PTAL at content/renderer/media/webmediaplayer_ms.*
Thanks.

https://codereview.chromium.org/2476693002/

[mca...@chromium.org]

emircan@ can you please have a look at webmediaplayer_ms changes?
(moving myself to bcc@)

https://codereview.chromium.org/2476693002/

[emi...@chromium.org]

https://codereview.chromium.org/2476693002/diff/220001/content/renderer/media/webmediaplayer_ms.cc
File content/renderer/media/webmediaplayer_ms.cc (right):

https://codereview.chromium.org/2476693002/diff/220001/content/renderer/media/webmediaplayer_ms.cc#newcode622
content/renderer/media/webmediaplayer_ms.cc:622:
media::VideoFrame::NumPlanes(video_frame->format()) != 1) {
Should you just check for PIXEL_FORMAT_Y16 format here? There are 9
formats that have single plane.
https://cs.chromium.org/chromium/src/media/base/video_frame.cc?rcl=0&l=503

https://codereview.chromium.org/2476693002/diff/220001/content/renderer/media/webmediaplayer_ms.cc#newcode634
content/renderer/media/webmediaplayer_ms.cc:634: }
nit: If you don't expect this function to be called with any other
|functionID|, you can add NOTREACHED() here to make that clear.

https://codereview.chromium.org/2476693002/

[aleksandar....@intel.com]

https://codereview.chromium.org/2476693002/diff/220001/content/renderer/media/webmediaplayer_ms.cc
File content/renderer/media/webmediaplayer_ms.cc (right):

https://codereview.chromium.org/2476693002/diff/220001/content/renderer/media/webmediaplayer_ms.cc#newcode622
content/renderer/media/webmediaplayer_ms.cc:622:
media::VideoFrame::NumPlanes(video_frame->format()) != 1) {

On 2016/12/01 23:23:37, emircan wrote:
> Should you just check for PIXEL_FORMAT_Y16 format here? There are 9
formats that
> have single plane.
>
https://cs.chromium.org/chromium/src/media/base/video_frame.cc?rcl=0&l=503

Done.
The idea is to use this to optimize for Y8 too, and maybe other. In
follow up patches, plan to add more code to
SkCanvasVideoRenderer::Tex(Sub)Image2D/3D methods. This code is already
prototyped at crrev.com/2121043002/. So, the format check is deferred to
SkCanvasVideoRenderer to keep the check on one place and avoid the need
to change the code here. Anyway, your point is valid so using Y16 check
now as you suggested and will change it when adding more formats.

Related to other single plane formats,... when no flip_y is supplied,
this approach saves at least one memcpy and a swizzle compared to
fallback path at the bottom of void
WebGLRenderingContextBase::texImageHelperHTMLVideoElement. So, it is
potentially applicable for RGB based formats too.

https://codereview.chromium.org/2476693002/diff/220001/content/renderer/media/webmediaplayer_ms.cc#newcode634
content/renderer/media/webmediaplayer_ms.cc:634: }

On 2016/12/01 23:23:37, emircan wrote:
> nit: If you don't expect this function to be called with any other
|functionID|,
> you can add NOTREACHED() here to make that clear.

Other parameter values are expected too, we just don't handle teximage3D
in this patch yet) - adding it when the tests gets added. Anyway, as
defined returning false when call is not handled and no need for
NOTREACHED.

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

https://codereview.chromium.org/2476693002/diff/220001/media/renderers/skcanvas_video_renderer.cc#newcode580
media/renderers/skcanvas_video_renderer.cc:580:
scoped_refptr<DataBuffer>& temp_buffer) {

On 2016/12/01 08:39:34, Ken Russell wrote:
> Mutable references are not allowed per the Google C++ style guide:
> https://google.github.io/styleguide/cppguide.html#Reference_Arguments
>
> Please pass this by pointer.

Done.

https://codereview.chromium.org/2476693002/

[aleksandar....@intel.com]

https://codereview.chromium.org/2476693002/

[emi...@chromium.org]

lgtm

https://codereview.chromium.org/2476693002/diff/220001/content/renderer/media/webmediaplayer_ms.cc
File content/renderer/media/webmediaplayer_ms.cc (right):

https://codereview.chromium.org/2476693002/diff/220001/content/renderer/media/webmediaplayer_ms.cc#newcode622
content/renderer/media/webmediaplayer_ms.cc:622:
media::VideoFrame::NumPlanes(video_frame->format()) != 1) {

Thanks for the explanation. For media streams, we usually receive
texture-HW decode- or I420 frames.

https://codereview.chromium.org/2476693002/

[mca...@chromium.org]

RS lgtm for those 2 files.

https://codereview.chromium.org/2476693002/

[aleksandar....@intel.com]

p...@chromium.org, owner's review needed for
third_party/WebKit/public/platform/WebMediaPlayer.h. PTAL.
Thanks.

https://codereview.chromium.org/2476693002/

[p...@chromium.org]

https://codereview.chromium.org/2476693002/diff/240001/third_party/WebKit/public/platform/WebMediaPlayer.h
File third_party/WebKit/public/platform/WebMediaPlayer.h (right):

https://codereview.chromium.org/2476693002/diff/240001/third_party/WebKit/public/platform/WebMediaPlayer.h#newcode228
third_party/WebKit/public/platform/WebMediaPlayer.h:228: virtual bool
texImageImpl(TexImageFunctionID functionID,
Does this need to be virtual?

https://codereview.chromium.org/2476693002/

[aleksandar....@intel.com]

https://codereview.chromium.org/2476693002/diff/240001/third_party/WebKit/public/platform/WebMediaPlayer.h
File third_party/WebKit/public/platform/WebMediaPlayer.h (right):

https://codereview.chromium.org/2476693002/diff/240001/third_party/WebKit/public/platform/WebMediaPlayer.h#newcode228
third_party/WebKit/public/platform/WebMediaPlayer.h:228: virtual bool
texImageImpl(TexImageFunctionID functionID,

On 2016/12/02 21:17:07, pdr. wrote:
> Does this need to be virtual?

Yes. It gets overridden in WebMediaPlayer_MS where we access current
video frame[1]. The method follows the approach and it is used in the
the same use case as virtual bool copyVideoTextureToPlatformTexture,
virtual bool copyVideoTextureToPlatformTexture and virtual bool
copyVideoSubTextureToPlatformTexture methods.

[1] compositor_->GetCurrentFrameWithoutUpdatingStatistics()
https://codereview.chromium.org/2476693002/diff/240001/content/renderer/media/webmediaplayer_ms.cc

https://codereview.chromium.org/2476693002/

[p...@chromium.org]

Thanks! I missed that.

LGTM for third_party/WebKit/public/platform/WebMediaPlayer.h

https://codereview.chromium.org/2476693002/

[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/2476693002/240001


https://codereview.chromium.org/2476693002/

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

Try jobs failed on following builders:
win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_tests_rel/builds/5728)

https://codereview.chromium.org/2476693002/

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

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

Try jobs failed on following builders:

win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/343546)

https://codereview.chromium.org/2476693002/

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

[k...@chromium.org]

On 2016/12/04 21:29:14, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
> win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/343546)

Note: I just filed http://crbug.com/671048 about that failure.


https://codereview.chromium.org/2476693002/

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

Try jobs failed on following builders:
win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,

http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/343569)

https://codereview.chromium.org/2476693002/

[k...@chromium.org]

On 2016/12/05 01:11:52, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
> win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/343569)

My suppression for that failure of
virtual/color_space/fast/canvas/color-space/display_linear-rgb.html just landed,
so re-trying this.


https://codereview.chromium.org/2476693002/

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

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

Committed patchset #7 (id:240001)

https://codereview.chromium.org/2476693002/

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

Patchset 7 (id:??) landed as
https://crrev.com/d851a930e44e902d0d30c1fea879831bb6edeef7
Cr-Commit-Position: refs/heads/master@{#436221}

https://codereview.chromium.org/2476693002/

[hb...@chromium.org]

On 2016/12/05 03:11:43, commit-bot: I haz the power wrote:
> Patchset 7 (id:??) landed as
> https://crrev.com/d851a930e44e902d0d30c1fea879831bb6edeef7
> Cr-Commit-Position: refs/heads/master@{#436221}

Did this cause crbug.com/671206 ?

https://codereview.chromium.org/2476693002/

[aleksandar....@intel.com]

On 2016/12/05 16:56:09, hbos_chromium wrote:
> On 2016/12/05 03:11:43, commit-bot: I haz the power wrote:
> > Patchset 7 (id:??) landed as
> > https://crrev.com/d851a930e44e902d0d30c1fea879831bb6edeef7
> > Cr-Commit-Position: refs/heads/master@{#436221}
>
> Did this cause crbug.com/671206 ?

Yes, I plan to disable where flaky.

https://codereview.chromium.org/2476693002/

[aleksandar....@intel.com]

Thanks.

https://codereview.chromium.org/2476693002/

[aleksandar....@intel.com]

On 2016/12/01 12:02:32, aleksandar.stojiljkovic wrote:
> On 2016/12/01 08:39:34, Ken Russell wrote:

> > Alex, outstanding work on the new tests. A+ job. These greatly improve my
> > confidence in this work. Thank you for persevering with it. LGTM with one
> small
> > change.
>
> Thanks and thanks for help.
>
> >
> >
>

https://codereview.chromium.org/2476693002/diff/220001/media/renderers/skcanvas_video_renderer.cc
> > File media/renderers/skcanvas_video_renderer.cc (right):
> >
> >
>

https://codereview.chromium.org/2476693002/diff/220001/media/renderers/skcanvas_video_renderer.cc#newcode538
> > media/renderers/skcanvas_video_renderer.cc:538: uint8_t* out_row_head =
flip_y
> ?
> > out + output_row_bytes * (height - i - 1)
> > Perhaps add a TODO about adding SIMD or similar optimization for this
> > conversion?
>
> Planning to submit WebGL2 patch using R16UI (and R32F) for review after this
> one. R16UI would require no conversion.
> Then, the benchmarks before GPU buffer support and SIMD optimizations so we
that
> could compare.

kbr@
R16UI approach seems not possible anymore:
https://bugs.chromium.org/p/chromium/issues/detail?id=624436#c51

Also, started with a measurement on the size of potential optimization with SIMD
but need to get the data on all of the platforms. I'll continue on 624436 page.
Thanks.

https://codereview.chromium.org/2476693002/