Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2476693002:
WebGL & 16-bit video stream: upload to FLOAT texture. (Closed)
|
Created:
4 years, 1 month ago by aleksandar.stojiljkovic Modified:
4 years ago CC:
blink-reviews, blink-reviews-api_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, ehmaldonado_chromium, eric.carlson_apple.com, feature-media-reviews_chromium.org, haraken, jam, mcasas+watch+vc_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-blink_chromium.org, posciak+watch_chromium.org, srirama.m Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLossless 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
Committed: https://crrev.com/d851a930e44e902d0d30c1fea879831bb6edeef7
Cr-Commit-Position: refs/heads/master@{#436221}
Patch Set 1 : Lossless access to 16-bit video stream using WebGL GL_FLOAT texture. #
Total comments: 20
Patch Set 2 : Fixes. phoglung@, hubbe@, thanks. #
Total comments: 4
Patch Set 3 : Nits. Thanks phoglund@. #
Total comments: 2
Patch Set 4 : rebase #
Total comments: 12
Patch Set 5 : Run WebGL-related on GPU bots. #64 fixes. Thanks kbr@. #Patch Set 6 : Rebase after crrev.com/2527343002 land. #
Total comments: 8
Patch Set 7 : Nits. #
Total comments: 2
Messages
Total messages: 124 (73 generated)
Description was changed from ========== WebGL & 16-bit video stream: upload to FLOAT texture. Currently, 16-bit depth capture is available through 8-bit* API, so there is a precision loss. Here, we add no-precision-loss** JavaScript access - WebGL support. * 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. BUG=369849, 624436 ========== to ========== WebGL & 16-bit video stream: upload to FLOAT texture. Currently, 16-bit depth capture is available through 8-bit* API, so there is a precision loss. Here, we add no-precision-loss** JavaScript access - WebGL support. * 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. 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 ==========
The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #2 (id:20001) has been deleted
Description was changed from
==========
WebGL & 16-bit video stream: upload to FLOAT texture.
Currently, 16-bit depth capture is available through 8-bit* API, so there is a
precision loss. Here, we add no-precision-loss** JavaScript access - WebGL
support.
* 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.
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
==========
to
==========
WebGL & 16-bit video stream: upload to 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.
* 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.
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
==========
The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Description was changed from
==========
WebGL & 16-bit video stream: upload to 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.
* 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.
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
==========
to
==========
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.
* 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
==========
Description was changed from ========== 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. * 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 ========== to ========== 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. * 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
Patchset #1 (id:80001) has been deleted
Description was changed from ========== 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. * 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 ========== to ========== 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 ==========
kbr@chromium.org: Please review changes in third_party/WebKit/Source/* chrishtr@chromium.org: Please review changes in third_party/WebKit/public/platform/WebMediaPlayer.h phoglund@chromium.org: Please review changes in content/browser/webrtc/webrtc_depth_capture_browsertest.cc and content/test/data mcasas@chromium.org: Please review changes in content/renderer/media/webmediaplayer_ms.* xhwang@chromium.org: Please review changes in media/renderers/skcanvas_video_renderer.* media/renderers/skcanvas_video_renderer_unittest.* Thanks.
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/medi... File content/test/data/media/getusermedia-depth-capture.html (right): https://codereview.chromium.org/2476693002/diff/100001/content/test/data/medi... 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/medi... 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/medi... 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/medi... 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/medi... 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.
aleksandar.stojiljkovic@intel.com changed reviewers: + dalecurtis@chromium.org - xhwang@chromium.org
xhwang_OOO_11_07/dalecurtis@chromium.org: PTAL media/renderers/skcanvas_video_renderer.* media/renderers/skcanvas_video_renderer_unittest.* Thanks.
dalecurtis@chromium.org changed reviewers: + hubbe@chromium.org - dalecurtis@chromium.org
=>hubbe who's done the other reviews.
https://codereview.chromium.org/2476693002/diff/100001/media/renderers/skcanv... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2476693002/diff/100001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:527: void FlipAndConvertY16(const VideoFrame* video_frame, Add comment. https://codereview.chromium.org/2476693002/diff/100001/media/renderers/skcanv... 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/skcanv... 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/skcanv... File media/renderers/skcanvas_video_renderer.h (right): https://codereview.chromium.org/2476693002/diff/100001/media/renderers/skcanv... 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/skcanv... 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*.
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.
On 2016/11/09 00:57:55, Ken Russell wrote: > 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. Thanks. Of course, won't push before 17th anyway.
Patch set #2: Fixes. phoglung@, hubbe@, thanks. PTAL. https://codereview.chromium.org/2476693002/diff/100001/content/test/data/medi... File content/test/data/media/getusermedia-depth-capture.html (right): https://codereview.chromium.org/2476693002/diff/100001/content/test/data/medi... 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/medi... content/test/data/media/getusermedia-depth-capture.html:113: function testVideoToWebGLTexture(videoElementName, success, error) On 2016/11/07 09:20:00, phoglund_chrome wrote: > 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. 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/medi... 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/medi... 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/medi... 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/skcanv... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2476693002/diff/100001/media/renderers/skcanv... 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/skcanv... 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/skcanv... 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/skcanv... File media/renderers/skcanvas_video_renderer.h (right): https://codereview.chromium.org/2476693002/diff/100001/media/renderers/skcanv... 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/skcanv... 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.
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/medi... File content/test/data/media/getusermedia-depth-capture.html (right): https://codereview.chromium.org/2476693002/diff/120001/content/test/data/medi... 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/medi... content/test/data/media/getusermedia-depth-capture.html:127: context.drawImage(bitmap,0,0); Nit: bitmap, 0, 0
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/medi... File content/test/data/media/getusermedia-depth-capture.html (right): https://codereview.chromium.org/2476693002/diff/120001/content/test/data/medi... 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/medi... 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.
Patchset #3 (id:140001) has been deleted
aleksandar.stojiljkovic@intel.com changed reviewers: + xhwang@chromium.org
+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/diff/160001/media/renderers/skcanv... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2476693002/diff/160001/media/renderers/skcanv... 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/diff/160001/media/renderers/skcanv... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2476693002/diff/160001/media/renderers/skcanv... 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.
media/* LGTM
> 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
The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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@intel.com changed reviewers: - xhwang@chromium.org
kbr@chromium.org: Please review changes in third_party/WebKit/Source/* chrishtr@chromium.org: Please review changes in third_party/WebKit/public/platform/WebMediaPlayer.h mcasas@chromium.org: Please review changes in content/renderer/media/webmediaplayer_ms.* Thanks.
aleksandar.stojiljkovic@intel.com changed reviewers: + bajones@chromium.org, dpranke@chromium.org - chrishtr@chromium.org
Adding reviewers as kbr@ and chrishtr@ seem busy. dpranke@, PTAL at third_party/WebKit/*, bajones@, PTAL at third_party/WebKit/Source/modules/webgl/*. Thanks.
dpranke@chromium.org changed reviewers: + pdr@chromium.org, schenney@chromium.org
I'm not the right reviewer for this. If kbr@ is unavailable, maybe try pdr@ or schenney@ ?
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/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/Sou... 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);
aleksandar.stojiljkovic@intel.com changed reviewers: + zmo@chromium.org - dpranke@chromium.org
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/
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/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/Sou... 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/Sou... 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/Sou... 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/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/pub... 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/pub... 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.
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... https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/Sou... 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/graph... https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/Sou... 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.
The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/Sou... 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/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/pub... 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/pub... 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/2476693002/diff/180001/third_party/WebKit/Sou... 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)
The CQ bit was checked by aleksandar.stojiljkovic@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/skcanv... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2476693002/diff/220001/media/renderers/skcanv... 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/skcanv... 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.
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/skcanv... > File media/renderers/skcanvas_video_renderer.cc (right): > > https://codereview.chromium.org/2476693002/diff/220001/media/renderers/skcanv... > 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/skcanv... > 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.
mcasas@chromium.org changed reviewers: + emircan@chromium.org
emircan@ can you please have a look at webmediaplayer_ms changes? (moving myself to bcc@)
https://codereview.chromium.org/2476693002/diff/220001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2476693002/diff/220001/content/renderer/media... 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... 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.
aleksandar.stojiljkovic@intel.com changed reviewers: - bajones@chromium.org, mcasas@chromium.org, schenney@chromium.org, zmo@chromium.org
https://codereview.chromium.org/2476693002/diff/220001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2476693002/diff/220001/content/renderer/media... 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... 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/skcanv... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2476693002/diff/220001/media/renderers/skcanv... 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.
aleksandar.stojiljkovic@intel.com changed reviewers: + mcasas@chromium.org
lgtm https://codereview.chromium.org/2476693002/diff/220001/content/renderer/media... File content/renderer/media/webmediaplayer_ms.cc (right): https://codereview.chromium.org/2476693002/diff/220001/content/renderer/media... content/renderer/media/webmediaplayer_ms.cc:622: media::VideoFrame::NumPlanes(video_frame->format()) != 1) { On 2016/12/02 20:23:41, aleksandar.stojiljkovic wrote: > 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. Thanks for the explanation. For media streams, we usually receive texture-HW decode- or I420 frames.
On 2016/12/02 20:33:35, emircan wrote: > lgtm > > https://codereview.chromium.org/2476693002/diff/220001/content/renderer/media... > File content/renderer/media/webmediaplayer_ms.cc (right): > > https://codereview.chromium.org/2476693002/diff/220001/content/renderer/media... > content/renderer/media/webmediaplayer_ms.cc:622: > media::VideoFrame::NumPlanes(video_frame->format()) != 1) { > On 2016/12/02 20:23:41, aleksandar.stojiljkovic wrote: > > 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. > > Thanks for the explanation. For media streams, we usually receive texture-HW > decode- or I420 frames. RS lgtm for those 2 files.
pdr@chromium.org, owner's review needed for third_party/WebKit/public/platform/WebMediaPlayer.h. PTAL. Thanks.
https://codereview.chromium.org/2476693002/diff/240001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2476693002/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebMediaPlayer.h:228: virtual bool texImageImpl(TexImageFunctionID functionID, Does this need to be virtual?
https://codereview.chromium.org/2476693002/diff/240001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/2476693002/diff/240001/third_party/WebKit/pub... 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...
On 2016/12/02 at 21:28:51, aleksandar.stojiljkovic wrote: > https://codereview.chromium.org/2476693002/diff/240001/third_party/WebKit/pub... > File third_party/WebKit/public/platform/WebMediaPlayer.h (right): > > https://codereview.chromium.org/2476693002/diff/240001/third_party/WebKit/pub... > 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... Thanks! I missed that. LGTM for third_party/WebKit/public/platform/WebMediaPlayer.h
The CQ bit was checked by aleksandar.stojiljkovic@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from hubbe@chromium.org, phoglund@chromium.org, kbr@chromium.org Link to the patchset: https://codereview.chromium.org/2476693002/#ps240001 (title: "Nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@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_...)
The CQ bit was checked by aleksandar.stojiljkovic@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@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_...)
The CQ bit was checked by aleksandar.stojiljkovic@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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_...) Note: I just filed http://crbug.com/671048 about that failure.
The CQ bit was unchecked by commit-bot@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_...)
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_...) My suppression for that failure of virtual/color_space/fast/canvas/color-space/display_linear-rgb.html just landed, so re-trying this.
The CQ bit was checked by kbr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1480901302822750,
"parent_rev": "08bb846d28930da04565299921eba83b80c09d34", "commit_rev":
"28c6ebe0209eb810e04fe1f3e652810b209c6c67"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/d851a930e44e902d0d30c1fea879831bb6edeef7 Cr-Commit-Position: refs/heads/master@{#436221} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/d851a930e44e902d0d30c1fea879831bb6edeef7 Cr-Commit-Position: refs/heads/master@{#436221}
Message was sent while issue was closed.
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 ?
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
On 2016/12/05 01:28:15, Ken Russell wrote: > 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_...) > > My suppression for that failure of > virtual/color_space/fast/canvas/color-space/display_linear-rgb.html just landed, > so re-trying this. Thanks.
Message was sent while issue was closed.
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/skcanv... > > File media/renderers/skcanvas_video_renderer.cc (right): > > > > > https://codereview.chromium.org/2476693002/diff/220001/media/renderers/skcanv... > > 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. |
