Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2428263004:
16 bpp video stream capture, render and createImageBitmap(video) using (CPU) shared memory buffers (Closed)
|
Created:
4 years, 2 months ago by aleksandar.stojiljkovic Modified:
4 years, 1 month ago Reviewers:
Ken Russell (switch to Gerrit), Avi (use Gerrit), tommi (sloooow) - chröme, dshwang, hubbe, enne (OOO), danakj, phoglund_chromium, mcasas, ehmaldonado_chromium, xhwang CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, blink-reviews-html_chromium.org, jam, mlamouri+watch-blink_chromium.org, eric.carlson_apple.com, blink-reviews-api_chromium.org, haraken, feature-media-reviews_chromium.org, dglazkov+blink, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, blink-reviews, cc-bugs_chromium.org, Srirama, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description16 bpp video stream capture, render and createImageBitmap(video) using (CPU) shared memory buffers
This patch implements support for depth capture on Windows/Linux and ChromeOS using shared
memory buffers. WebGL implementation with no precision loss and GPU memory buffer usage is
split to separate patches from master patch: crrev.com/2121043002/
It Supports 16 bit depth video streams from R200 and SR300.
Verified to work on Windows 10 and Ubuntu 16.04.
Rendering video element:
In cc, it is converted to RGBA format before glTexImage2D and rendered as TexturedQuad.
2D canvas + getImageData returns 8 bit color components (higher bit value) in RGBA.
BUG=624436
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/198b050b9fe647bf3799ab75df07b888c7adcf50
Cr-Commit-Position: refs/heads/master@{#429307}
Patch Set 1 #Patch Set 2 : fixes #
Total comments: 52
Patch Set 3 : Review #19 fixes. Thanks mcasas@. #Patch Set 4 : rebase #Patch Set 5 : fix windows approach and video_capture_utils. #
Total comments: 12
Patch Set 6 : review #27 fixes. Thanks danakj@. #Patch Set 7 : review #30 fix. Thanks danakj@. #
Total comments: 15
Patch Set 8 : Review #33 and #34 fixes. Removed WebGL part of code. Thanks danakj@ and mcasas@. #
Total comments: 22
Patch Set 9 : Rebase after FakeVCD (2447233002) land. #
Total comments: 1
Patch Set 10 : review #42 & #43 fixes. Thanks xhwang@ and hubbe@. #Patch Set 11 : video_frame.cc switch fix. #
Total comments: 8
Patch Set 12 : #38 fix: refactoring VideoCaptureDeviceClient::OnIncomingCapturedY16Data out. Thanks mcasas@. #
Total comments: 1
Patch Set 13 : #48 fix: Y16 SkCanvasVideoRendererTest. Thanks xhwang@. #
Total comments: 6
Patch Set 14 : Fixes. #Patch Set 15 : Set FakeVCD count-devices=2 for webrtc_getusermedia_browsertest only. #
Total comments: 3
Patch Set 16 : Split webrtc_depth_capture_browsertest. Thanks phoglund@, #
Total comments: 2
Messages
Total messages: 75 (29 generated)
Description was changed from ========== 16 bpp video stream capture, render and WebGL usage using (CPU) shared memory buffers This patch implements complete feature for Windows/Linux and ChromeOS using shared memory buffers. GPU memory buffer usage is split to separate patch: crrev.com/2121043002/ It Supports 16 bit depth and infrared video streams from R200 and SR300. Verified to work on Windows 10 and Ubuntu 16.04. Rendering video element: ======================== In cc, it is converted to RGBA format before glTexImage2D and rendered as TexturedQuad. WebGL: ====== Upload video to texture: In all the textures, R, G, B components have the same value (as luminance). - RGBA UNSIGNED_BYTE: R, G and B components presents upper byte of 16 bit video, so, with precision loss. - RGB/RGBA FLOAT: Normalized 16 bit value in R, G and B component - no precision loss. Getting full 16 bit or float data to javascript: =============================================== 1. on start, create framebuffer, attach (COLOR_ATTACHMENT0) RGBA FLOAT texture to it. 2. In loop, when video is ready, after texImage2D video to the texture, bind the framebuffer and call readpixels to FloatArray. This way the data is accessible from javascript. Complete examples are available on Issue 624436 page. 2D canvas + getImageData returns 8 bit color components (higher bit value) in RGBA. BUG=624436 ========== to ========== 16 bpp video stream capture, render and WebGL usage using (CPU) shared memory buffers This patch implements complete feature for Windows/Linux and ChromeOS using shared memory buffers. GPU memory buffer usage is split to separate patch: crrev.com/2121043002/ It Supports 16 bit depth and infrared video streams from R200 and SR300. Verified to work on Windows 10 and Ubuntu 16.04. Rendering video element: ======================== In cc, it is converted to RGBA format before glTexImage2D and rendered as TexturedQuad. WebGL: ====== Upload video to texture: In all the textures, R, G, B components have the same value (as luminance). - RGBA UNSIGNED_BYTE: R, G and B components presents upper byte of 16 bit video, so, with precision loss. - RGB/RGBA FLOAT: Normalized 16 bit value in R, G and B component - no precision loss. Getting full 16 bit or float data to javascript: =============================================== 1. on start, create framebuffer, attach (COLOR_ATTACHMENT0) RGBA FLOAT texture to it. 2. In loop, when video is ready, after texImage2D video to the texture, bind the framebuffer and call readpixels to FloatArray. This way the data is accessible from javascript. Complete examples are available on Issue 624436 page. 2D canvas + getImageData returns 8 bit color components (higher bit value) in RGBA. BUG=624436 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
Description was changed from ========== 16 bpp video stream capture, render and WebGL usage using (CPU) shared memory buffers This patch implements complete feature for Windows/Linux and ChromeOS using shared memory buffers. GPU memory buffer usage is split to separate patch: crrev.com/2121043002/ It Supports 16 bit depth and infrared video streams from R200 and SR300. Verified to work on Windows 10 and Ubuntu 16.04. Rendering video element: ======================== In cc, it is converted to RGBA format before glTexImage2D and rendered as TexturedQuad. WebGL: ====== Upload video to texture: In all the textures, R, G, B components have the same value (as luminance). - RGBA UNSIGNED_BYTE: R, G and B components presents upper byte of 16 bit video, so, with precision loss. - RGB/RGBA FLOAT: Normalized 16 bit value in R, G and B component - no precision loss. Getting full 16 bit or float data to javascript: =============================================== 1. on start, create framebuffer, attach (COLOR_ATTACHMENT0) RGBA FLOAT texture to it. 2. In loop, when video is ready, after texImage2D video to the texture, bind the framebuffer and call readpixels to FloatArray. This way the data is accessible from javascript. Complete examples are available on Issue 624436 page. 2D canvas + getImageData returns 8 bit color components (higher bit value) in RGBA. BUG=624436 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== 16 bpp video stream capture, render and WebGL usage using (CPU) shared memory buffers This patch implements complete feature for Windows/Linux and ChromeOS using shared memory buffers. GPU memory buffer usage is split to separate patch: crrev.com/2121043002/ It Supports 16 bit depth and infrared video streams from R200 and SR300. Verified to work on Windows 10 and Ubuntu 16.04. Rendering video element: ======================== In cc, it is converted to RGBA format before glTexImage2D and rendered as TexturedQuad. WebGL: ====== Upload video to texture: In all the textures, R, G, B components have the same value (as luminance). - RGBA UNSIGNED_BYTE: R, G and B components presents upper byte of 16 bit video, so, with precision loss. - RGB/RGBA FLOAT: Normalized 16 bit value in R, G and B component - no precision loss. Getting full 16 bit or float data to javascript: =============================================== 1. on start, create framebuffer, attach (COLOR_ATTACHMENT0) RGBA FLOAT texture to it. 2. In loop, when video is ready, after texImage2D video to the texture, bind the framebuffer and call readpixels to FloatArray. This way the data is accessible from javascript. Complete examples are available on Issue 624436 page. 2D canvas + getImageData returns 8 bit color components (higher bit value) in RGBA. BUG=624436 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
aleksandar.stojiljkovic@intel.com changed reviewers: + avi@chromium.org, danakj@chromium.org, hubbe@chromium.org, kbr@chromium.org, sky@chromium.org
danakj@chromium.org: Please review changes in cc, sky@chromium.org: Please review changes in content/test and WebKit/public, kbr@chromium.org: Please review changes in content/browser/renderer_host and WebKit/Source, hubbe@chromium.org: Please review changes in media, avi@chromium.org: Please review changes in content. Thanks.
danakj@, short introduction to cc changes. https://codereview.chromium.org/2428263004/diff/20001/cc/output/renderer_pixe... File cc/output/renderer_pixeltest.cc (right): https://codereview.chromium.org/2428263004/diff/20001/cc/output/renderer_pixe... cc/output/renderer_pixeltest.cc:309: TextureDrawQuad* quad = To render video frame of format PIXEL_FORMAT_Y16 we use TextureDrawQuad. 16bpp depth should be rendered as luminance here. https://codereview.chromium.org/2428263004/diff/20001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2428263004/diff/20001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:371: isYuvPlanar ? resource_provider_->YuvResourceFormat(bits_per_channel) Check above only allows Yuv and Y16, so ResourceFormat::RGBA_8888 is the same as if checking Y16, at least for now. In following patches, the plan is to add also Y8 but this line wouldn't change - Y8 would either use the same as lowbit YUV (R8) or RGBA_8888 here. GPU buffer support is part of original patch and it is planned to submit it later, once the discussion about what GPU format to use for Y16 is over.
The content code LGTM. Please also have a media expert review it.
aleksandar.stojiljkovic@intel.com changed reviewers: + mcasas@chromium.org
On 2016/10/20 21:59:09, Avi wrote: > The content code LGTM. > > Please also have a media expert review it. Thanks. mcasas@chromium.org: Please review changes in content. Thanks.
sky@chromium.org changed reviewers: - sky@chromium.org
content/test/data has an OWNERS of *, so you don't need me. I'm not a good webkit owner, so you'll need to find a better one for that. I'm removing myself as I don't think you need me for any of this. If I'm wrong add me back and let me know what files you need me for.
Took a first pass. Some generalities: - Try hard not to have special cases for the new pixel formats. - I'd recommend splitting this CL into capture-path related changes and playback/drawing/rendering, bc they have different owners. https://codereview.chromium.org/2428263004/diff/20001/content/browser/webrtc/... File content/browser/webrtc/webrtc_getusermedia_browsertest.cc (right): https://codereview.chromium.org/2428263004/diff/20001/content/browser/webrtc/... content/browser/webrtc/webrtc_getusermedia_browsertest.cc:351: // yet enabled on Android. TODO()s should have a https://crbug.com/123123 associated, which can be added ad hoc and just be a short description. https://codereview.chromium.org/2428263004/diff/20001/content/renderer/media/... File content/renderer/media/video_capture_impl.cc (right): https://codereview.chromium.org/2428263004/diff/20001/content/renderer/media/... content/renderer/media/video_capture_impl.cc:266: Restore the missing DCHECK_EQ(media::PIXEL_STORAGE_CPU, info->storage_type); ? https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... File media/capture/video/fake_video_capture_device.cc (right): https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:187: if (buffer_ownership_ != BufferOwnership::CLIENT_BUFFERS) { nit: prefer direct logic: if (buffer_ownership_ == BufferOwnership::OWN_BUFFERS) https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:271: capture_format_.frame_size); Couple of things: having the |capture_format_| determine the generated pattern is counterintuitive, can you not work with the Pacman? Also, it would break the photo-related methods. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... File media/capture/video/fake_video_capture_device_factory.cc (right): https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device_factory.cc:21: return media::PIXEL_FORMAT_I420; Hmm, if you need this specific format type to be generated, I'd say just add another sub flag and parse it appropriately: https://cs.chromium.org/chromium/src/media/capture/video/fake_video_capture_d... https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device_factory.cc:23: } nit: empty line between these } https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device_factory.cc:39: : number_of_devices_(2), Some tests also set_numer_of_devices() to 2, and expect them, are they still OK with this? https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device_factory.cc:85: gfx::Size(320, 240), gfx::Size(640, 480), nit: Indent is strange here. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... File media/capture/video/fake_video_capture_device_unittest.cc (right): https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device_unittest.cc:184: void SetUp() override {} No need for this. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device_unittest.cc:191: return client; No need for this construction: leave it like it was before, with |client_| in the initializer list. .Times(0) is redundant, GMock will always flag unexpected MOCKED calls. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/lin... File media/capture/video/linux/v4l2_capture_delegate.cc (right): https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.cc:583: #ifdef V4L2_BUF_FLAG_ERROR This is not representative enough, instead, check the Linux Kernel version, and add a comment, most likely with a TODO(). Same applies to l.22 and vicinity: it's hard to monitor the definition of a given preprocessor definition, but it's easy to verify the kernel version. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.cc:592: capture_format_, rotation_, base::TimeTicks::Now(), timestamp); Indent is incorrect. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... File media/capture/video/video_capture_device_client.cc (right): https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:32: bool isFormatSupported(media::VideoPixelFormat pixel_format) { Capitalize this: IsFormatSupported(). https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:33: // Currently, only I420 and Y16 pixel formats are supported. Remove superfluous comment (the return says the same), unless there are plans for more, case in which a TODO should be added. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:37: } Add empty line. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:122: const bool useFullSize = frame_format.pixel_format == media::PIXEL_FORMAT_Y16; Don't use camel case. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:133: : (frame_format.frame_size.height() & ~1); Parentheses are not needed around |useFullSize| (the name of which should change anyway), here and in l.128. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:153: uint8_t* v_plane_data = 0; To be precise, these three should be = nullptr; https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:179: } Hmm unlike in the content/ code, there's specific treatment of the Y16 formats here in VCDClient. Is there no way we can homogeneize them? I would rather have a specific VCDClient method OnIncomingSuperCaptureData() where you treat it specifically, WDYT? https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... File media/capture/video/video_capture_device_unittest.cc (right): https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_unittest.cc:656: #endif This test case should go into a new video_capture_utils_unittest.cc https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/win... File media/capture/video/win/video_capture_device_win.cc (right): https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/win... media/capture/video/win/video_capture_device_win.cc:180: } Here and in the equivalent MF version, I don't like that you give special treatment to the Y16 formats. We should do it more like the V4L2 delegate. I suggest: extend |kMediaSubtypeToPixelFormatCorrespondence| with an entry for |Data1|. The currently existing MEDIASUBTYPE_* won't have anything meaningful there, and the Y16 will be listed individually, and then you'll have a single for loop in l.174. Again, this will also apply to video_capture_device_mf_win.cc . https://codereview.chromium.org/2428263004/diff/20001/media/renderers/skcanva... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2428263004/diff/20001/media/renderers/skcanva... media/renderers/skcanvas_video_renderer.cc:352: video_frame->format() == media::PIXEL_FORMAT_Y16 || PIXEL_FORMAT_Y16 is not IsYuvPlanar() ?
Patch Set 3 : Review #19 fixes. Thanks mcasas@chromium.org Addressed the issues, except windows GUIDs handling. On 2016/10/21 00:10:51, mcasas wrote: > Took a first pass. Some generalities: > - Try hard not to have special cases for the new > pixel formats. > - I'd recommend splitting this CL into capture-path > related changes and playback/drawing/rendering, > bc they have different owners. Difficult. cc rendering is minor change but needed for cc pixel tests and content_browser pixel tests to verify the capture pipeline (also some existing tests would fail if there is no cc rendering for new format). Webkit WebGL part could be split out, but kbr@ is already familiar with it and also owner for some of content changes. You're the reviewer for most of the code, so let's see how it goes - if webkit part starts to affect landing, I'll split it later. Thanks. https://codereview.chromium.org/2428263004/diff/20001/content/browser/webrtc/... File content/browser/webrtc/webrtc_getusermedia_browsertest.cc (right): https://codereview.chromium.org/2428263004/diff/20001/content/browser/webrtc/... content/browser/webrtc/webrtc_getusermedia_browsertest.cc:351: // yet enabled on Android. On 2016/10/21 00:10:50, mcasas wrote: > TODO()s should have a https://crbug.com/123123 associated, > which can be added ad hoc and just be a short description. Done. Removed the special handling in fake_video_capture_device_factory for Android. https://codereview.chromium.org/2428263004/diff/20001/content/renderer/media/... File content/renderer/media/video_capture_impl.cc (right): https://codereview.chromium.org/2428263004/diff/20001/content/renderer/media/... content/renderer/media/video_capture_impl.cc:266: On 2016/10/21 00:10:50, mcasas wrote: > Restore the missing > DCHECK_EQ(media::PIXEL_STORAGE_CPU, info->storage_type); > ? Done - no need to have the change here. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... File media/capture/video/fake_video_capture_device.cc (right): https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:187: if (buffer_ownership_ != BufferOwnership::CLIENT_BUFFERS) { On 2016/10/21 00:10:50, mcasas wrote: > nit: prefer direct logic: > if (buffer_ownership_ == BufferOwnership::OWN_BUFFERS) Done. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device.cc:271: capture_format_.frame_size); On 2016/10/21 00:10:50, mcasas wrote: > Couple of things: having the |capture_format_| determine the > generated pattern is counterintuitive, can you not work with > the Pacman? Also, it would break the photo-related methods. Done. Your point is valid and since I need gradient control values, added them to corners of every Pacman rendered frame, removed separate rendering for different formats and that fixed take broken photo logic. The squares are now available for every buffer format- the formats now produce the same looking layout. Control squares are used in pixel tests, since all the pixels on the frame are computable based on values of the pixel 0,0, there is no need for reference image (and it would be difficult to have one for fake capture device). There is no video file container format for 16bpp grayscale (Y16) and fake capture device is used for pixel tests. Uploaded the screenshot of fake capture device on Android here so you could check the aesthetics of it - it sort of looks like an opaque HUD display in corners. Hope that's fine. https://crbug.com/624436#c37 https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... File media/capture/video/fake_video_capture_device_factory.cc (right): https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device_factory.cc:21: return media::PIXEL_FORMAT_I420; On 2016/10/21 00:10:50, mcasas wrote: > Hmm, if you need this specific format type to be generated, > I'd say just add another sub flag and parse it appropriately: > https://cs.chromium.org/chromium/src/media/capture/video/fake_video_capture_d... If it is important that there is only one capture device on the camera, maybe we could have number_of_devices_ in a sub flag - with the depth/IR enabled cameras it is common that the first device is color and the second is depth camera. I think this way we test more as existing tests using 2 devices are also used for testing (the point you made bellow). https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device_factory.cc:23: } On 2016/10/21 00:10:50, mcasas wrote: > nit: empty line between these } Done. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device_factory.cc:39: : number_of_devices_(2), On 2016/10/21 00:10:50, mcasas wrote: > Some tests also set_numer_of_devices() to 2, and expect them, > are they still OK with this? Yes, that's why decided to set it to 2, first RGB, second depth always - to increase the coverage. All seems to be handled properly - if I recall correctly, there was just WebRtcGetUserMediaBrowserTest.GetUserMediaWithMandatorySourceID that would fail if there was no rendering support in cc. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device_factory.cc:85: gfx::Size(320, 240), gfx::Size(640, 480), On 2016/10/21 00:10:50, mcasas wrote: > nit: Indent is strange here. git cl format media is doing it and complaining if there is column layout like before. Didn't know how to force it. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... File media/capture/video/fake_video_capture_device_unittest.cc (right): https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device_unittest.cc:184: void SetUp() override {} On 2016/10/21 00:10:50, mcasas wrote: > No need for this. Removed empty method, clarifying the need for the change bellow. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak... media/capture/video/fake_video_capture_device_unittest.cc:191: return client; On 2016/10/21 00:10:50, mcasas wrote: > No need for this construction: leave it like it was before, > with |client_| in the initializer list. .Times(0) is redundant, > GMock will always flag unexpected MOCKED calls. The need for the change comes from (line 436 bellow) - with multiple devices (descriptors) per camera, move(client_) would leave nullptr client_ and crash in second loop. for (const auto& descriptors_iterator : *descriptors) { ... device->AllocateAndStart(capture_params, std::move(client_)); https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/lin... File media/capture/video/linux/v4l2_capture_delegate.cc (right): https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.cc:583: #ifdef V4L2_BUF_FLAG_ERROR On 2016/10/21 00:10:50, mcasas wrote: > This is not representative enough, instead, check the > Linux Kernel version, and add a comment, most likely > with a TODO(). > > Same applies to l.22 and vicinity: it's hard to monitor > the definition of a given preprocessor definition, but > it's easy to verify the kernel version. Noticed the crash with Realsense cameras on multiple kernel versions (if I remember correctly on Ubuntu 16.04 and recent ChromeOS, without other changes). The patch here is orthogonal to the other changes here and I should have opened a bug and commit this separately. Should I do it now or it is fine to have it here? V4L2_BUF_FLAG_ERROR is handled this way in ffmpeg [1], so just copied the whole approach. [1] https://cs.chromium.org/chromium/src/third_party/ffmpeg/libavdevice/v4l2.c?rc... https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.cc:592: capture_format_, rotation_, base::TimeTicks::Now(), timestamp); On 2016/10/21 00:10:50, mcasas wrote: > Indent is incorrect. git cl format is doing it because of else shared by both ifdef paths: #ifdef if() <indent> else #endif <indent> https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... File media/capture/video/video_capture_device_client.cc (right): https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:32: bool isFormatSupported(media::VideoPixelFormat pixel_format) { On 2016/10/21 00:10:51, mcasas wrote: > Capitalize this: IsFormatSupported(). Done. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:33: // Currently, only I420 and Y16 pixel formats are supported. On 2016/10/21 00:10:50, mcasas wrote: > Remove superfluous comment (the return says the same), > unless there are plans for more, case in which a TODO > should be added. Done. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:37: } On 2016/10/21 00:10:51, mcasas wrote: > Add empty line. Done. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:122: const bool useFullSize = frame_format.pixel_format == media::PIXEL_FORMAT_Y16; On 2016/10/21 00:10:50, mcasas wrote: > Don't use camel case. Done. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:133: : (frame_format.frame_size.height() & ~1); On 2016/10/21 00:10:50, mcasas wrote: > Parentheses are not needed around |useFullSize| (the name > of which should change anyway), here and in l.128. Done. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:153: uint8_t* v_plane_data = 0; On 2016/10/21 00:10:50, mcasas wrote: > To be precise, these three should be = nullptr; Done. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_client.cc:179: } On 2016/10/21 00:10:50, mcasas wrote: > Hmm unlike in the content/ code, there's specific > treatment of the Y16 formats here in VCDClient. > Is there no way we can homogeneize them? I would > rather have a specific VCDClient method > OnIncomingSuperCaptureData() where you treat it > specifically, WDYT? Done. Thanks, looks much better now. Now, some formats are converted to I420 and we can skip this for Y16, and in future for Y8 (uncompressed 8bpp grayscale format of some older depth and IR cameras) and RGB based formats (don't know what is the priority on those though). Except this, no special handling for Y16 - moved the logic bellow to the same path as other formats, under MPEG handling. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... File media/capture/video/video_capture_device_unittest.cc (right): https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_unittest.cc:656: #endif On 2016/10/21 00:10:51, mcasas wrote: > This test case should go into a new video_capture_utils_unittest.cc Acknowledged, under work. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/win... File media/capture/video/win/video_capture_device_win.cc (right): https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/win... media/capture/video/win/video_capture_device_win.cc:180: } On 2016/10/21 00:10:51, mcasas wrote: > Here and in the equivalent MF version, I don't like > that you give special treatment to the Y16 formats. > We should do it more like the V4L2 delegate. I > suggest: extend |kMediaSubtypeToPixelFormatCorrespondence| > with an entry for |Data1|. The currently existing > MEDIASUBTYPE_* won't have anything meaningful > there, and the Y16 will be listed individually, and > then you'll have a single for loop in l.174. Again, this > will also apply to video_capture_device_mf_win.cc . No problem - I though it was a good idea to share the same code with sink_input_pin_win.cc, but I'll just handle it the same way as kMediaSubTypeHDYC and kMediaSubTypeI420 in [1] https://cs.chromium.org/chromium/src/media/capture/video/win/sink_filter_win.... https://codereview.chromium.org/2428263004/diff/20001/media/renderers/skcanva... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2428263004/diff/20001/media/renderers/skcanva... media/renderers/skcanvas_video_renderer.cc:352: video_frame->format() == media::PIXEL_FORMAT_Y16 || On 2016/10/21 00:10:51, mcasas wrote: > PIXEL_FORMAT_Y16 is not IsYuvPlanar() ? No. It is a 16-bit single plane, e.g. D16 / z-buffer.
Patchset 3, 4 and 5 are addressing issues from review #19. Simplified the code further as some changes got removed. mcasas@chromium.org, Could you please review it again? Thanks. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... File media/capture/video/video_capture_device_unittest.cc (right): https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/vid... media/capture/video/video_capture_device_unittest.cc:656: #endif On 2016/10/21 00:10:51, mcasas wrote: > This test case should go into a new video_capture_utils_unittest.cc Done. IsY16FormatFourCc method got removed since the approach is the same as for other formats. https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/win... File media/capture/video/win/video_capture_device_win.cc (right): https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/win... media/capture/video/win/video_capture_device_win.cc:180: } On 2016/10/21 00:10:51, mcasas wrote: > Here and in the equivalent MF version, I don't like > that you give special treatment to the Y16 formats. > We should do it more like the V4L2 delegate. I > suggest: extend |kMediaSubtypeToPixelFormatCorrespondence| > with an entry for |Data1|. The currently existing > MEDIASUBTYPE_* won't have anything meaningful > there, and the Y16 will be listed individually, and > then you'll have a single for loop in l.174. Again, this > will also apply to video_capture_device_mf_win.cc . Done. Done like for other formats.
Description was changed from ========== 16 bpp video stream capture, render and WebGL usage using (CPU) shared memory buffers This patch implements complete feature for Windows/Linux and ChromeOS using shared memory buffers. GPU memory buffer usage is split to separate patch: crrev.com/2121043002/ It Supports 16 bit depth and infrared video streams from R200 and SR300. Verified to work on Windows 10 and Ubuntu 16.04. Rendering video element: ======================== In cc, it is converted to RGBA format before glTexImage2D and rendered as TexturedQuad. WebGL: ====== Upload video to texture: In all the textures, R, G, B components have the same value (as luminance). - RGBA UNSIGNED_BYTE: R, G and B components presents upper byte of 16 bit video, so, with precision loss. - RGB/RGBA FLOAT: Normalized 16 bit value in R, G and B component - no precision loss. Getting full 16 bit or float data to javascript: =============================================== 1. on start, create framebuffer, attach (COLOR_ATTACHMENT0) RGBA FLOAT texture to it. 2. In loop, when video is ready, after texImage2D video to the texture, bind the framebuffer and call readpixels to FloatArray. This way the data is accessible from javascript. Complete examples are available on Issue 624436 page. 2D canvas + getImageData returns 8 bit color components (higher bit value) in RGBA. BUG=624436 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== 16 bpp video stream capture, render and WebGL usage using (CPU) shared memory buffers This patch implements complete feature for Windows/Linux and ChromeOS using shared memory buffers. GPU memory buffer usage is split to separate patch: crrev.com/2121043002/ It Supports 16 bit depth and infrared video streams from R200 and SR300. Verified to work on Windows 10 and Ubuntu 16.04. Rendering video element: ======================== In cc, it is converted to RGBA format before glTexImage2D and rendered as TexturedQuad. WebGL: ====== Upload video to texture: In all the textures, R, G, B components have the same value (as luminance). - RGBA UNSIGNED_BYTE: R, G and B components presents upper byte of 16 bit video, so, with precision loss. - RGB/RGBA FLOAT: Normalized 16 bit value in R, G and B component - no precision loss. Getting full 16 bit or float data to javascript: =============================================== 1. on start, create framebuffer, attach (COLOR_ATTACHMENT0) RGBA FLOAT texture to it. 2. In loop, when video is ready, after texImage2D video to the texture, bind the framebuffer and call readpixels to FloatArray. This way the data is accessible from javascript. Complete examples are available on Issue 624436 page. 2D canvas + getImageData returns 8 bit color components (higher bit value) in RGBA. BUG=624436 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
aleksandar.stojiljkovic@intel.com changed reviewers: + enne@chromium.org
+enne@chromium.org: Please review changes in cc. danakj@ seems busy. Thanks.
I cannot see media/capture/video/win/sink_filter_win.*
files: when I click on them it says "old chunk mismatch".
Also, what if we review separately the changes to
FakeVCD & relatives? I imagine they are needed for
content_{browser,unit}tests, but those could be
reviewed separately as well. That'll probably get your
CLs landed faster :-)
https://codereview.chromium.org/2428263004/diff/80001/media/capture/video/lin...
File media/capture/video/linux/v4l2_capture_delegate.cc (right):
https://codereview.chromium.org/2428263004/diff/80001/media/capture/video/lin...
media/capture/video/linux/v4l2_capture_delegate.cc:23: #ifndef V4L2_PIX_FMT_Z16
These two identifiers were introduced in
kernel 4.6 [1], so please sandwich it all
like:
#if LINUX_VERSION_CODE < KERNEL_VERSION(4, 6,0)
// 16 bit depth, Realsense F200.
#define V4L2_PIX_FMT_Z16 v4l2_fourcc('Z', '1', '6', ' ')
// 16 bit depth, Realsense SR300.
#define V4L2_PIX_FMT_INVZ v4l2_fourcc('I', 'N', 'V', 'Z')
#endif
[1] http://lxr.free-electrons.com/ident?v=4.6;i=V4L2_PIX_FMT_Z16
The WebGL changes in WebKit/Source look fine. Could you please find another reviewer for content/browser/renderer_host? I am swamped for the next two weeks.
https://codereview.chromium.org/2428263004/diff/20001/cc/output/renderer_pixe... File cc/output/renderer_pixeltest.cc (right): https://codereview.chromium.org/2428263004/diff/20001/cc/output/renderer_pixe... cc/output/renderer_pixeltest.cc:288: void CreateTestY16VideoDrawQuad_FromVideoFrame( s/VideoDrawQuad/TextureDrawQuad/ ? https://codereview.chromium.org/2428263004/diff/80001/cc/output/renderer_pixe... File cc/output/renderer_pixeltest.cc (right): https://codereview.chromium.org/2428263004/diff/80001/cc/output/renderer_pixe... cc/output/renderer_pixeltest.cc:855: void AppendBackgroundAndRunTest(const PixelComparator& comparator, Can we not do function overloading? https://codereview.chromium.org/2428263004/diff/80001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2428263004/diff/80001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:432: const bool isYuvPlanar = media::IsYuvPlanar(input_frame_format); use chromium style please for isYuvPlanar https://codereview.chromium.org/2428263004/diff/80001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:434: NOTREACHED() << media::VideoPixelFormatToString(input_frame_format); Hm where did this NOTREACHED();return; come from? Can you turn this into a DCHECK instead and drop the return Or, drop the NOTREACHED if the DCHECK would occur (I hope not?) https://codereview.chromium.org/2428263004/diff/80001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:442: : ResourceFormat::RGBA_8888; Can you leave a comment explaining this in the context of the above DCHECK because I spent some time here being confused. Specifically, if not yuv-planar then it is PIXEL_FORMAT_Y16 which will use RGBA_8888. Alternatively, restructure this to have the code explain it better. Perhaps like if (input_frame_format == ..Y16) { // Unable to display directly as yuv planes so convert it to RGBA for compositing. output_resource_format = RGBA_888 } else { // Can be composited directly from yuv planes. output_resource_format = r_p_->YuvRF(..bits..); }
On 2016/10/24 20:08:17, Ken Russell wrote: > The WebGL changes in WebKit/Source look fine. > > Could you please find another reviewer for content/browser/renderer_host? I am > swamped for the next two weeks. Thanks. mcasas@ is the reviewer for those changes, too.
Patch Set 6 : review #27 fixes. Thanks danakj@. https://codereview.chromium.org/2428263004/diff/20001/cc/output/renderer_pixe... File cc/output/renderer_pixeltest.cc (right): https://codereview.chromium.org/2428263004/diff/20001/cc/output/renderer_pixe... cc/output/renderer_pixeltest.cc:288: void CreateTestY16VideoDrawQuad_FromVideoFrame( On 2016/10/24 21:46:37, danakj wrote: > s/VideoDrawQuad/TextureDrawQuad/ ? Done. https://codereview.chromium.org/2428263004/diff/80001/cc/output/renderer_pixe... File cc/output/renderer_pixeltest.cc (right): https://codereview.chromium.org/2428263004/diff/80001/cc/output/renderer_pixe... cc/output/renderer_pixeltest.cc:855: void AppendBackgroundAndRunTest(const PixelComparator& comparator, On 2016/10/24 21:46:37, danakj wrote: > Can we not do function overloading? Is the suggestion is to overloading function template template <typename T> AppendBackgroundAndRunTest(const PixelComparator& comparator) to something like: template <typename T> AppendBackgroundAndRunTest(const PixelComparator& comparator, const base::FilePath::CharType* fileName)? There would be no need for it - template is used only for IntersectingQuadImage<T> and we don't need it if providing the path in this overload method. Guess that we could use the same method with default parameter fileName = nullptr, marking whether to use fileName or call IntersectingQuadImage<T>(). Understand that you saw it already, but for the reference, providing specialization IntersectingQuadImage<TextureQuad> cannot be used because we use TextureQuad with different expectations in two different intersecting tests - here and in TYPED_TEST(IntersectingQuadPixelTest, TexturedQuads) - l.944 bellow. https://codereview.chromium.org/2428263004/diff/80001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2428263004/diff/80001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:432: const bool isYuvPlanar = media::IsYuvPlanar(input_frame_format); On 2016/10/24 21:46:37, danakj wrote: > use chromium style please for isYuvPlanar Done. https://codereview.chromium.org/2428263004/diff/80001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:434: NOTREACHED() << media::VideoPixelFormatToString(input_frame_format); On 2016/10/24 21:46:37, danakj wrote: > Hm where did this NOTREACHED();return; come from? Can you turn this into a > DCHECK instead and drop the return > > Or, drop the NOTREACHED if the DCHECK would occur (I hope not?) NOTREACHED();return; is there from previous code; NOTREACHED contributed in [1] and "return" even earlier. [1] https://chromium.googlesource.com/chromium/src/+/7a6c2a346104a0269767c7174d09... Check is useful, when someone adds a new format like me here, existing tests triggered NOTREACHED. So, DCHECK is good. However, that is an orthogonal change (unnecessary risk) to this patch - propose to add it in separate patch: https://crrev.com/2447013002/ https://codereview.chromium.org/2428263004/diff/80001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:442: : ResourceFormat::RGBA_8888; On 2016/10/24 21:46:37, danakj wrote: > Can you leave a comment explaining this in the context of the above DCHECK > because I spent some time here being confused. Specifically, if not yuv-planar > then it is PIXEL_FORMAT_Y16 which will use RGBA_8888. Alternatively, restructure > this to have the code explain it better. Perhaps like > > if (input_frame_format == ..Y16) { > // Unable to display directly as yuv planes so convert it to RGBA for > compositing. > output_resource_format = RGBA_888 > } else { > // Can be composited directly from yuv planes. > output_resource_format = r_p_->YuvRF(..bits..); > } Done. Thanks. https://codereview.chromium.org/2428263004/diff/80001/media/capture/video/lin... File media/capture/video/linux/v4l2_capture_delegate.cc (right): https://codereview.chromium.org/2428263004/diff/80001/media/capture/video/lin... media/capture/video/linux/v4l2_capture_delegate.cc:23: #ifndef V4L2_PIX_FMT_Z16 On 2016/10/24 19:48:47, mcasas wrote: > These two identifiers were introduced in > kernel 4.6 [1], so please sandwich it all > like: > > #if LINUX_VERSION_CODE < KERNEL_VERSION(4, 6,0) > // 16 bit depth, Realsense F200. > #define V4L2_PIX_FMT_Z16 v4l2_fourcc('Z', '1', '6', ' ') > // 16 bit depth, Realsense SR300. > #define V4L2_PIX_FMT_INVZ v4l2_fourcc('I', 'N', 'V', 'Z') > #endif > > > [1] http://lxr.free-electrons.com/ident?v=4.6;i=V4L2_PIX_FMT_Z16 Done. Thanks.
https://codereview.chromium.org/2428263004/diff/80001/cc/output/renderer_pixe... File cc/output/renderer_pixeltest.cc (right): https://codereview.chromium.org/2428263004/diff/80001/cc/output/renderer_pixe... cc/output/renderer_pixeltest.cc:855: void AppendBackgroundAndRunTest(const PixelComparator& comparator, On 2016/10/25 10:12:28, aleksandar.stojiljkovic wrote: > On 2016/10/24 21:46:37, danakj wrote: > > Can we not do function overloading? > > Is the suggestion is to overloading function template > template <typename T> > AppendBackgroundAndRunTest(const PixelComparator& comparator) Just name the functions different names? > to something like: > template <typename T> > AppendBackgroundAndRunTest(const PixelComparator& comparator, const > base::FilePath::CharType* fileName)? > > There would be no need for it - template is used only for > IntersectingQuadImage<T> and we don't need it if providing the path in this > overload method. > > Guess that we could use the same method with default parameter fileName = > nullptr, marking whether to use fileName or call IntersectingQuadImage<T>(). > > Understand that you saw it already, but for the reference, providing > specialization IntersectingQuadImage<TextureQuad> cannot be used because we use > TextureQuad with different expectations in two different intersecting tests - > here and in TYPED_TEST(IntersectingQuadPixelTest, TexturedQuads) - l.944 bellow. >
On 2016/10/24 19:48:47, mcasas wrote:
> Also, what if we review separately the changes to
> FakeVCD & relatives? I imagine they are needed for
> content_{browser,unit}tests, but those could be
> reviewed separately as well. That'll probably get your
> CLs landed faster :-)
Yes, FakeVCD is the critical part for the testing and tests in the same time
serve for verifying the change there.
You are the reviewer for most of the code here so let's make it faster for
review.
I have split FakeVCD related code to this patch:
https://codereview.chromium.org/2447233002/#ps1
Thanks.
media/capture/video/win/sink_filter_win.* should also be fine with the latest
patch here.
https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak...
File media/capture/video/fake_video_capture_device_factory.cc (right):
https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak...
media/capture/video/fake_video_capture_device_factory.cc:23: }
On 2016/10/21 22:11:10, aleksandar.stojiljkovic wrote:
> On 2016/10/21 00:10:50, mcasas wrote:
> > nit: empty line between these }
>
> Done.
git cl format media is just forcing no empty line here and chromium presubmit
complains about it.
https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak...
File media/capture/video/fake_video_capture_device_unittest.cc (right):
https://codereview.chromium.org/2428263004/diff/20001/media/capture/video/fak...
media/capture/video/fake_video_capture_device_unittest.cc:191: return client;
On 2016/10/21 22:11:10, aleksandar.stojiljkovic wrote:
> On 2016/10/21 00:10:50, mcasas wrote:
> > No need for this construction: leave it like it was before,
> > with |client_| in the initializer list. .Times(0) is redundant,
> > GMock will always flag unexpected MOCKED calls.
>
> The need for the change comes from (line 436 bellow) - with multiple devices
> (descriptors) per camera, move(client_) would leave nullptr client_ and crash
in
> second loop.
>
> for (const auto& descriptors_iterator : *descriptors) {
> ...
> device->AllocateAndStart(capture_params, std::move(client_));
>
Done.
In split patch https://codereview.chromium.org/2447233002/#ps1, I've reverted
this to previous but for the reasons explained here had to also add Createclient
function.
Patch Set 7: review #30 fix. Thanks danakj@. https://codereview.chromium.org/2428263004/diff/80001/cc/output/renderer_pixe... File cc/output/renderer_pixeltest.cc (right): https://codereview.chromium.org/2428263004/diff/80001/cc/output/renderer_pixe... cc/output/renderer_pixeltest.cc:855: void AppendBackgroundAndRunTest(const PixelComparator& comparator, On 2016/10/25 20:21:23, danakj wrote: > On 2016/10/25 10:12:28, aleksandar.stojiljkovic wrote: > > On 2016/10/24 21:46:37, danakj wrote: > > > Can we not do function overloading? > > > > Is the suggestion is to overloading function template > > template <typename T> > > AppendBackgroundAndRunTest(const PixelComparator& comparator) > > Just name the functions different names? > > > to something like: > > template <typename T> > > AppendBackgroundAndRunTest(const PixelComparator& comparator, const > > base::FilePath::CharType* fileName)? > > > > There would be no need for it - template is used only for > > IntersectingQuadImage<T> and we don't need it if providing the path in this > > overload method. > > > > Guess that we could use the same method with default parameter fileName = > > nullptr, marking whether to use fileName or call IntersectingQuadImage<T>(). > > > > Understand that you saw it already, but for the reference, providing > > specialization IntersectingQuadImage<TextureQuad> cannot be used because we > use > > TextureQuad with different expectations in two different intersecting tests - > > here and in TYPED_TEST(IntersectingQuadPixelTest, TexturedQuads) - l.944 > bellow. > > > Done.
https://codereview.chromium.org/2428263004/diff/120001/cc/output/renderer_pix... File cc/output/renderer_pixeltest.cc (right): https://codereview.chromium.org/2428263004/diff/120001/cc/output/renderer_pix... cc/output/renderer_pixeltest.cc:571: void CreateTestY16VideoDrawQuad_TwoColor( This is making TextureDrawQuad too https://codereview.chromium.org/2428263004/diff/120001/cc/output/renderer_pix... cc/output/renderer_pixeltest.cc:855: void AppendBackgroundAndRunPixelTest( Uh.. now we have two functions with the same name still basically, it's not differentiating the behaviour difference. But TBH this whole IntersectingQuadImage<T> thing is a bit weird. Can you just pass a file name in every case instead and drop the template thing entirely? Sorry I shoulda just said that the first time I guess.
changes under - content/browser/renderer_host/media/* - content/rendere/media - media/capture/video (except fake* since they are reviewed in https://crrev.com/2447233002). are el-gee-tee-em, but I still want to take another look at video_capture_device_client.cc https://codereview.chromium.org/2428263004/diff/120001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/2428263004/diff/120001/content/browser/render... content/browser/renderer_host/media/video_capture_controller_unittest.cc:94: media::VideoPixelFormat expect_buffer_format_ = media::PIXEL_FORMAT_I420; nit: s/expect_buffer_format_/expected_pixel_format_/ https://codereview.chromium.org/2428263004/diff/120001/content/browser/render... content/browser/renderer_host/media/video_capture_controller_unittest.cc:298: .Times(1); micro-nit: .Times(1) is redundant, you could safely remove it throughout these few lines. https://codereview.chromium.org/2428263004/diff/120001/content/browser/render... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2428263004/diff/120001/content/browser/render... content/browser/renderer_host/media/video_capture_manager.cc:97: } Ugh, old code! Suggestion: for (const auto format : formats) { if (format.pixel_format != media::PIXEL_FORMAT_Y16) format.pixel_format = media::PIXEL_FORMAT_I420; } https://codereview.chromium.org/2428263004/diff/120001/content/renderer/media... File content/renderer/media/video_capture_impl.cc (right): https://codereview.chromium.org/2428263004/diff/120001/content/renderer/media... content/renderer/media/video_capture_impl.cc:263: DCHECK_EQ(media::PIXEL_STORAGE_CPU, info->storage_type); This DCHECK and the one in l.261-262 and the if() clause below are redundant and we shouldn't handle (D)CHECK failures [1], but they were kept here for easier logging. You can safely remove them and add instead a NOTREACHED() << "Wrong state or pixel format or storage"; inside the if() body. [1] https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2428263004/diff/120001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/2428263004/diff/120001/media/base/video_frame... media/base/video_frame.cc:776: << VideoPixelFormatToString(format); nit: Move to the previous line?
Description was changed from ========== 16 bpp video stream capture, render and WebGL usage using (CPU) shared memory buffers This patch implements complete feature for Windows/Linux and ChromeOS using shared memory buffers. GPU memory buffer usage is split to separate patch: crrev.com/2121043002/ It Supports 16 bit depth and infrared video streams from R200 and SR300. Verified to work on Windows 10 and Ubuntu 16.04. Rendering video element: ======================== In cc, it is converted to RGBA format before glTexImage2D and rendered as TexturedQuad. WebGL: ====== Upload video to texture: In all the textures, R, G, B components have the same value (as luminance). - RGBA UNSIGNED_BYTE: R, G and B components presents upper byte of 16 bit video, so, with precision loss. - RGB/RGBA FLOAT: Normalized 16 bit value in R, G and B component - no precision loss. Getting full 16 bit or float data to javascript: =============================================== 1. on start, create framebuffer, attach (COLOR_ATTACHMENT0) RGBA FLOAT texture to it. 2. In loop, when video is ready, after texImage2D video to the texture, bind the framebuffer and call readpixels to FloatArray. This way the data is accessible from javascript. Complete examples are available on Issue 624436 page. 2D canvas + getImageData returns 8 bit color components (higher bit value) in RGBA. BUG=624436 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== 16 bpp video stream capture, render and createImageBitmap(video) using (CPU) shared memory buffers This patch implements support for depth capture on Windows/Linux and ChromeOS using shared memory buffers. WebGL implementation with no precision loss and GPU memory buffer usage is split to separate patches from master patch: crrev.com/2121043002/ It Supports 16 bit depth video streams from R200 and SR300. Verified to work on Windows 10 and Ubuntu 16.04. Rendering video element: In cc, it is converted to RGBA format before glTexImage2D and rendered as TexturedQuad. 2D canvas + getImageData returns 8 bit color components (higher bit value) in RGBA. BUG=624436 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
To avoid growing this patch, removed Webkit (WebGL) related code and part of skcanvas_video_renderer.* as it was not tested with the provided browser test. I'll upload it to review soon in separate patch. mcasas@, kbr@, sorry for removing already reviewed code here - in the new patch for WebGL support I'll add this version first to avoid wasting additional time on review. https://codereview.chromium.org/2428263004/diff/120001/cc/output/renderer_pix... File cc/output/renderer_pixeltest.cc (right): https://codereview.chromium.org/2428263004/diff/120001/cc/output/renderer_pix... cc/output/renderer_pixeltest.cc:571: void CreateTestY16VideoDrawQuad_TwoColor( On 2016/10/25 22:34:19, danakj wrote: > This is making TextureDrawQuad too Done. You're right, Y16 in name is enough to differentiate. https://codereview.chromium.org/2428263004/diff/120001/cc/output/renderer_pix... cc/output/renderer_pixeltest.cc:855: void AppendBackgroundAndRunPixelTest( On 2016/10/25 22:34:19, danakj wrote: > Uh.. now we have two functions with the same name still basically, it's not > differentiating the behaviour difference. > > But TBH this whole IntersectingQuadImage<T> thing is a bit weird. Can you just > pass a file name in every case instead and drop the template thing entirely? > Sorry I shoulda just said that the first time I guess. Done. Thanks, looks better now. Couldn't decide myself which way to go with this. https://codereview.chromium.org/2428263004/diff/120001/content/browser/render... File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/2428263004/diff/120001/content/browser/render... content/browser/renderer_host/media/video_capture_controller_unittest.cc:94: media::VideoPixelFormat expect_buffer_format_ = media::PIXEL_FORMAT_I420; On 2016/10/25 22:44:43, mcasas wrote: > nit: s/expect_buffer_format_/expected_pixel_format_/ Done. https://codereview.chromium.org/2428263004/diff/120001/content/browser/render... content/browser/renderer_host/media/video_capture_controller_unittest.cc:298: .Times(1); On 2016/10/25 22:44:43, mcasas wrote: > micro-nit: .Times(1) is redundant, you could > safely remove it throughout these few lines. Checked the code further but could only guess why it is redundant. Propose to remove it in follow up patch as it is on multiple cases here,... but don't know when it is redundant. https://codereview.chromium.org/2428263004/diff/120001/content/browser/render... File content/browser/renderer_host/media/video_capture_manager.cc (right): https://codereview.chromium.org/2428263004/diff/120001/content/browser/render... content/browser/renderer_host/media/video_capture_manager.cc:97: } On 2016/10/25 22:44:43, mcasas wrote: > Ugh, old code! Suggestion: > > for (const auto format : formats) { > if (format.pixel_format != media::PIXEL_FORMAT_Y16) > format.pixel_format = media::PIXEL_FORMAT_I420; > } Thanks. Done. https://codereview.chromium.org/2428263004/diff/120001/content/renderer/media... File content/renderer/media/video_capture_impl.cc (right): https://codereview.chromium.org/2428263004/diff/120001/content/renderer/media... content/renderer/media/video_capture_impl.cc:263: DCHECK_EQ(media::PIXEL_STORAGE_CPU, info->storage_type); On 2016/10/25 22:44:43, mcasas wrote: > This DCHECK and the one in l.261-262 and the if() clause > below are redundant and we shouldn't handle (D)CHECK > failures [1], but they were kept here for easier > logging. You can safely remove them and add instead a > NOTREACHED() << "Wrong state or pixel format or storage"; > inside the if() body. > > [1] > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... One way is to leave the the DCHECK and remove the handling from if statement (l.266). That would be a change in behavior adding risk to this patch - similar issue handled yesterday for cc here [1] in separate patch: if (!cond) {NOTREACHED(); return,} replaced by DCHECK(cond). [1] https://codereview.chromium.org/2428263004/diff/80001/cc/resources/video_reso... Otherwise, code in video_frame.cc is using LOG(DFATAL) for the same issue as here inside the if body. I might have misunderstood the suggestion. Anyway, removed the DCHECK as suggested and left the handled return (as it was in non debug mode) with LOG(DFATAL) << "Wrong state or pixel format or storage..." inside the existing if branch in l.269. https://codereview.chromium.org/2428263004/diff/120001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/2428263004/diff/120001/media/base/video_frame... media/base/video_frame.cc:776: << VideoPixelFormatToString(format); On 2016/10/25 22:44:43, mcasas wrote: > nit: Move to the previous line? Done, but "git cl format media" pulled it back.
cc LGTM https://codereview.chromium.org/2428263004/diff/140001/cc/output/renderer_pix... File cc/output/renderer_pixeltest.cc (right): https://codereview.chromium.org/2428263004/diff/140001/cc/output/renderer_pix... cc/output/renderer_pixeltest.cc:1082: SCOPED_TRACE("IntersectingVideoQuads"); These SCOPED_TRACE are not adding anything, they are for differentiating multiple calls to a function inside a test, but there's only 1 per test here.
my parts lgtm with comment addressed and pre-landing the Fake* changes CL separately (bc easier to track git history). https://codereview.chromium.org/2428263004/diff/140001/media/capture/video/vi... File media/capture/video/video_capture_device_client.cc (right): https://codereview.chromium.org/2428263004/diff/140001/media/capture/video/vi... media/capture/video/video_capture_device_client.cc:119: DCHECK_GE(static_cast<size_t>(length), frame_format.ImageAllocationSize()); This line is duplicated in l.244. https://codereview.chromium.org/2428263004/diff/140001/media/capture/video/vi... media/capture/video/video_capture_device_client.cc:122: frame_format.pixel_format != media::PIXEL_FORMAT_Y16; There's too many special-cases for Y16 and anyway this format doesn't need conversion rotation, nor cropping, which are the points of this method, so I suggest: write up a private method OnIncomingCaptureY16Data() and bundle there the specifics, leaving OnIncomingCapturedData() mostly untouched. Jump to it if needed in or around l.117. WDYT?
aleksandar.stojiljkovic@intel.com changed reviewers: + xhwang@chromium.org
+xhwang@chromium.org, Please review changes in media/base/video_frame.cc media/renderers/skcanvas_video_renderer.* Thanks.
non-owner lgtm it's good idea to support Y16 using existing RGBA first. looking forward to further optimization.
I just have a few comments. hubbe: Please also help take a look at the color conversion part. https://codereview.chromium.org/2428263004/diff/140001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/2428263004/diff/140001/media/base/video_frame... media/base/video_frame.cc:800: NOTREACHED(); In prod, we are still returning a non-null frame here. Shall we just return nullptr here instead? https://codereview.chromium.org/2428263004/diff/140001/media/renderers/skcanv... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2428263004/diff/140001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:525: void ConvertY16ToARGB(const VideoFrame* video_frame, Please add a comment to summarize what we are doing here, e.g. put the upper 8 bits of the 16 bit data and convert it to ARGB etc. https://codereview.chromium.org/2428263004/diff/140001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:529: reinterpret_cast<const uint8_t*>(video_frame->visible_data(0)); why do you need the cast? https://codereview.chromium.org/2428263004/diff/140001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:529: reinterpret_cast<const uint8_t*>(video_frame->visible_data(0)); It seems these functions only convert the visible rect, is this intended? https://cs.chromium.org/chromium/src/media/renderers/skcanvas_video_renderer.... If so, maybe we should rename these functions to make it more clear. https://codereview.chromium.org/2428263004/diff/140001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:536: row < row_end; ++row) { This for-loop is confusing. Does it make sense to reorganize as: const uint8_t* row_end = source + video_frame->row_bytes(0); for (const uint8_t* row = source; row < row_end; ++row) { ... } Also, would it be more cleaner to s/source/row_head? https://codereview.chromium.org/2428263004/diff/140001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:541: uint32_t green = *++row; why this is "green"? Should it be something like "grey_value"? https://codereview.chromium.org/2428263004/diff/140001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:548: } Can this compile?
https://codereview.chromium.org/2428263004/diff/140001/media/renderers/skcanv... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2428263004/diff/140001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:542: *rgba++ = SkColorSetARGB(0xFF, green, green, green); I wonder if it would be helpful to do an inverse-gamma conversion here? ARGB textures are *usually* in sRGB space, and I assume that the Y16 texture is usually in linear space.
Patch Set 10 : review #42 & #43 fixes. Thanks xhwang@ and hubbe@. https://codereview.chromium.org/2428263004/diff/140001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/2428263004/diff/140001/media/base/video_frame... media/base/video_frame.cc:800: NOTREACHED(); On 2016/10/27 16:59:18, xhwang wrote: > In prod, we are still returning a non-null frame here. Shall we just return > nullptr here instead? Done. Replaced by LOG(DFATAL) and return nullptr, according to the handling in l.774 and l.785. https://codereview.chromium.org/2428263004/diff/140001/media/renderers/skcanv... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2428263004/diff/140001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:525: void ConvertY16ToARGB(const VideoFrame* video_frame, On 2016/10/27 16:59:18, xhwang wrote: > Please add a comment to summarize what we are doing here, e.g. put the upper 8 > bits of the 16 bit data and convert it to ARGB etc. Done. https://codereview.chromium.org/2428263004/diff/140001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:529: reinterpret_cast<const uint8_t*>(video_frame->visible_data(0)); On 2016/10/27 16:59:18, xhwang wrote: > why do you need the cast? Done. https://codereview.chromium.org/2428263004/diff/140001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:529: reinterpret_cast<const uint8_t*>(video_frame->visible_data(0)); On 2016/10/27 16:59:18, xhwang wrote: > It seems these functions only convert the visible rect, is this intended? > > https://cs.chromium.org/chromium/src/media/renderers/skcanvas_video_renderer.... > > If so, maybe we should rename these functions to make it more clear. Almost all of the functionality in the file is based on visible_rect; gpu storage is copied as it is but Paint/UpdateLastImage/code under VideoImageGenerator are all about visible_rect. It is specified in documentation as you pointed out and renaming it only for methods called from ConvertVideoFrameToRGBPixels might be problematic. https://codereview.chromium.org/2428263004/diff/140001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:536: row < row_end; ++row) { On 2016/10/27 16:59:18, xhwang wrote: > This for-loop is confusing. Does it make sense to reorganize as: > > const uint8_t* row_end = source + video_frame->row_bytes(0); > for (const uint8_t* row = source; row < row_end; ++row) { > > ... > } > > Also, would it be more cleaner to s/source/row_head? Done. That was strange looking loop - after all the fixes here, looks better now. https://codereview.chromium.org/2428263004/diff/140001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:541: uint32_t green = *++row; On 2016/10/27 16:59:18, xhwang wrote: > why this is "green"? Should it be something like "grey_value"? Done. https://codereview.chromium.org/2428263004/diff/140001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:542: *rgba++ = SkColorSetARGB(0xFF, green, green, green); On 2016/10/27 17:25:03, hubbe wrote: > I wonder if it would be helpful to do an inverse-gamma conversion here? > ARGB textures are *usually* in sRGB space, and I assume that the Y16 texture is > usually in linear space. Important to keep the values; some developers might be fine with 8 bit value precision for depth/infrared and use GetBitmapData, someone will need full precision through WebGL, but the normalized values needs to match. https://codereview.chromium.org/2428263004/diff/140001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:548: } On 2016/10/27 16:59:18, xhwang wrote: > Can this compile? Yes - it is closing // anonymous namespace. Added the comment.
Thanks! It looks good to me % a request for a test. https://codereview.chromium.org/2428263004/diff/200001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/2428263004/diff/200001/media/base/video_frame... media/base/video_frame.cc:812: return frame; nit: does it make sense to case 1 before case 3? https://codereview.chromium.org/2428263004/diff/200001/media/renderers/skcanv... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2428263004/diff/200001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:529: void ConvertY16ToARGB(const VideoFrame* video_frame, Can you add a test?
Thanks xhwang@, https://codereview.chromium.org/2428263004/diff/200001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/2428263004/diff/200001/media/base/video_frame... media/base/video_frame.cc:812: return frame; On 2016/10/27 20:28:09, xhwang wrote: > nit: does it make sense to case 1 before case 3? It is fall through - YPlane handling is shared by both. https://codereview.chromium.org/2428263004/diff/200001/media/renderers/skcanv... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2428263004/diff/200001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:529: void ConvertY16ToARGB(const VideoFrame* video_frame, On 2016/10/27 20:28:10, xhwang wrote: > Can you add a test? There are already three pixel comparison tests added in this patch: [1] cc/resources/video_resource_updater.cc is calling this to convert to RGBA before uploading texture... and two pixel tests are here: https://codereview.chromium.org/2428263004/diff/200001/cc/output/renderer_pix... [2] layout/browsertest: https://codereview.chromium.org/2428263004/diff/200001/content/test/data/medi...
On 2016/10/27 20:59:42, aleksandar.stojiljkovic wrote: > Thanks xhwang@, > > https://codereview.chromium.org/2428263004/diff/200001/media/base/video_frame.cc > File media/base/video_frame.cc (right): > > https://codereview.chromium.org/2428263004/diff/200001/media/base/video_frame... > media/base/video_frame.cc:812: return frame; > On 2016/10/27 20:28:09, xhwang wrote: > > nit: does it make sense to case 1 before case 3? > > It is fall through - YPlane handling is shared by both. > > https://codereview.chromium.org/2428263004/diff/200001/media/renderers/skcanv... > File media/renderers/skcanvas_video_renderer.cc (right): > > https://codereview.chromium.org/2428263004/diff/200001/media/renderers/skcanv... > media/renderers/skcanvas_video_renderer.cc:529: void ConvertY16ToARGB(const > VideoFrame* video_frame, > On 2016/10/27 20:28:10, xhwang wrote: > > Can you add a test? > > There are already three pixel comparison tests added in this patch: > [1] > cc/resources/video_resource_updater.cc is calling this to convert to RGBA before > uploading texture... and two pixel tests are here: > https://codereview.chromium.org/2428263004/diff/200001/cc/output/renderer_pix... > > > [2] > layout/browsertest: > https://codereview.chromium.org/2428263004/diff/200001/content/test/data/medi... I meant, all three of them are pixel testing this conversion code.
https://codereview.chromium.org/2428263004/diff/200001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/2428263004/diff/200001/media/base/video_frame... media/base/video_frame.cc:812: return frame; On 2016/10/27 20:59:41, aleksandar.stojiljkovic wrote: > On 2016/10/27 20:28:09, xhwang wrote: > > nit: does it make sense to case 1 before case 3? > > It is fall through - YPlane handling is shared by both. hmm, this is tricky. I'd rather duplicate these 3 lines than adding a "Fall through" comment... https://codereview.chromium.org/2428263004/diff/200001/media/renderers/skcanv... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2428263004/diff/200001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:529: void ConvertY16ToARGB(const VideoFrame* video_frame, On 2016/10/27 20:59:42, aleksandar.stojiljkovic wrote: > On 2016/10/27 20:28:10, xhwang wrote: > > Can you add a test? > > There are already three pixel comparison tests added in this patch: > [1] > cc/resources/video_resource_updater.cc is calling this to convert to RGBA before > uploading texture... and two pixel tests are here: > https://codereview.chromium.org/2428263004/diff/200001/cc/output/renderer_pix... > > > [2] > layout/browsertest: > https://codereview.chromium.org/2428263004/diff/200001/content/test/data/medi... Can you add a unittest in skcanvas_video_renderer_unittest.cc?
Patch Set 12 : #38 fix: refactoring VideoCaptureDeviceClient::OnIncomingCapturedY16Data out. Thanks mcasas@. https://codereview.chromium.org/2428263004/diff/140001/cc/output/renderer_pix... File cc/output/renderer_pixeltest.cc (right): https://codereview.chromium.org/2428263004/diff/140001/cc/output/renderer_pix... cc/output/renderer_pixeltest.cc:1082: SCOPED_TRACE("IntersectingVideoQuads"); On 2016/10/26 19:59:48, danakj wrote: > These SCOPED_TRACE are not adding anything, they are for differentiating > multiple calls to a function inside a test, but there's only 1 per test here. Copied the code from YUV (l.1056) and other formats - I understood it helped on splitting upper from lower half code execution traces. https://codereview.chromium.org/2428263004/diff/140001/media/capture/video/vi... File media/capture/video/video_capture_device_client.cc (right): https://codereview.chromium.org/2428263004/diff/140001/media/capture/video/vi... media/capture/video/video_capture_device_client.cc:119: DCHECK_GE(static_cast<size_t>(length), frame_format.ImageAllocationSize()); On 2016/10/27 00:44:42, mcasas wrote: > This line is duplicated in l.244. Done. https://codereview.chromium.org/2428263004/diff/140001/media/capture/video/vi... media/capture/video/video_capture_device_client.cc:122: frame_format.pixel_format != media::PIXEL_FORMAT_Y16; On 2016/10/27 00:44:42, mcasas wrote: > There's too many special-cases for Y16 and > anyway this format doesn't need conversion > rotation, nor cropping, which are the points of this > method, so I suggest: write up a private method > OnIncomingCaptureY16Data() and bundle there the > specifics, leaving OnIncomingCapturedData() > mostly untouched. Jump to it if needed in > or around l.117. WDYT? Done. https://codereview.chromium.org/2428263004/diff/160001/content/test/content_t... File content/test/content_test_launcher.cc (right): https://codereview.chromium.org/2428263004/diff/160001/content/test/content_t... content/test/content_test_launcher.cc:113: command_line->AppendSwitchASCII( On 2016/10/27 00:44:42, mcasas wrote: > my parts lgtm with comment addressed > and pre-landing the Fake* changes CL > separately (bc easier to track git history). FakeVCD code is in and this code is rebased. The changes in FakeLCD led to additional change here. There are multiple test cases benefiting (increasing the coverage) from this in get user media. That would be a selling argument why I am doing it easier way; here instead of removing the argument in individual tests and adding it again with the value.
Patchset #13 (id:240001) has been deleted
Patch Set 13 : #48 fix: Y16 SkCanvasVideoRendererTest. Thanks xhwang@. https://codereview.chromium.org/2428263004/diff/200001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/2428263004/diff/200001/media/base/video_frame... media/base/video_frame.cc:812: return frame; On 2016/10/27 21:09:06, xhwang wrote: > On 2016/10/27 20:59:41, aleksandar.stojiljkovic wrote: > > On 2016/10/27 20:28:09, xhwang wrote: > > > nit: does it make sense to case 1 before case 3? > > > > It is fall through - YPlane handling is shared by both. > > hmm, this is tricky. I'd rather duplicate these 3 lines than adding a "Fall > through" comment... Done. https://codereview.chromium.org/2428263004/diff/200001/media/renderers/skcanv... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2428263004/diff/200001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:529: void ConvertY16ToARGB(const VideoFrame* video_frame, On 2016/10/27 21:09:06, xhwang wrote: > On 2016/10/27 20:59:42, aleksandar.stojiljkovic wrote: > > On 2016/10/27 20:28:10, xhwang wrote: > > > Can you add a test? > > > > There are already three pixel comparison tests added in this patch: > > [1] > > cc/resources/video_resource_updater.cc is calling this to convert to RGBA > before > > uploading texture... and two pixel tests are here: > > > https://codereview.chromium.org/2428263004/diff/200001/cc/output/renderer_pix... > > > > > > [2] > > layout/browsertest: > > > https://codereview.chromium.org/2428263004/diff/200001/content/test/data/medi... > > Can you add a unittest in skcanvas_video_renderer_unittest.cc? Done. Good that you asked - testing with visible_rect offset identified write out of bounds.
Glad the test found real bugs. I usually don't trust my eyes and only trust tests :) (though test could have it's own bugs...) lgtm % nits https://codereview.chromium.org/2428263004/diff/260001/media/renderers/skcanv... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2428263004/diff/260001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:533: uint8_t* out = reinterpret_cast<uint8_t*>(argb_pixels); static_cast? https://codereview.chromium.org/2428263004/diff/260001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:540: *rgba++ = SkColorSetARGB(0xFF, gray_value, gray_value, gray_value); nit: There's a SkColorSetRGB() we can use. https://codereview.chromium.org/2428263004/diff/260001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:542: out += argb_row_bytes; shall we dcheck that |argb_row_bytes| is big enough? also, should it be a multiple of 4?
Thanks. https://codereview.chromium.org/2428263004/diff/120001/content/renderer/media... File content/renderer/media/video_capture_impl.cc (right): https://codereview.chromium.org/2428263004/diff/120001/content/renderer/media... content/renderer/media/video_capture_impl.cc:263: DCHECK_EQ(media::PIXEL_STORAGE_CPU, info->storage_type); On 2016/10/26 14:02:18, aleksandar.stojiljkovic wrote: > On 2016/10/25 22:44:43, mcasas wrote: > > This DCHECK and the one in l.261-262 and the if() clause > > below are redundant and we shouldn't handle (D)CHECK > > failures [1], but they were kept here for easier > > logging. You can safely remove them and add instead a > > NOTREACHED() << "Wrong state or pixel format or storage"; > > inside the if() body. > > > > [1] > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > One way is to leave the the DCHECK and remove the handling from if statement > (l.266). That would be a change in behavior adding risk to this patch - similar > issue handled yesterday for cc here [1] in separate patch: > if (!cond) {NOTREACHED(); return,} replaced by DCHECK(cond). > [1] > https://codereview.chromium.org/2428263004/diff/80001/cc/resources/video_reso... > > Otherwise, code in video_frame.cc is using LOG(DFATAL) for the same issue as > here inside the if body. > > I might have misunderstood the suggestion. Anyway, removed the DCHECK as > suggested and left the handled return (as it was in non debug mode) with > LOG(DFATAL) << "Wrong state or pixel format or storage..." inside the existing > if branch in l.269. mcasas@chromium.org, I had to modify this: state_ != VIDEO_CAPTURE_STATE_STARTED is legal and shouldn't trigger any LOG(DFATAL). Frame arrived after capture stop. There is a test checking this - VideoCaptureImplTest.BufferReceivedAfterStop. https://codereview.chromium.org/2428263004/diff/220001/media/capture/video/vi... File media/capture/video/video_capture_device_client.cc (right): https://codereview.chromium.org/2428263004/diff/220001/media/capture/video/vi... media/capture/video/video_capture_device_client.cc:117: if (frame_format.pixel_format == media::PIXEL_FORMAT_Y16) mcasas@: Online review: {} for bodies > 1 line. Done. https://codereview.chromium.org/2428263004/diff/260001/media/renderers/skcanv... File media/renderers/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/2428263004/diff/260001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:533: uint8_t* out = reinterpret_cast<uint8_t*>(argb_pixels); On 2016/10/28 16:54:17, xhwang wrote: > static_cast? Done. https://codereview.chromium.org/2428263004/diff/260001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:540: *rgba++ = SkColorSetARGB(0xFF, gray_value, gray_value, gray_value); On 2016/10/28 16:54:17, xhwang wrote: > nit: There's a SkColorSetRGB() we can use. Done. https://codereview.chromium.org/2428263004/diff/260001/media/renderers/skcanv... media/renderers/skcanvas_video_renderer.cc:542: out += argb_row_bytes; On 2016/10/28 16:54:17, xhwang wrote: > shall we dcheck that |argb_row_bytes| is big enough? also, should it be a > multiple of 4? I wouldn't check it in our generator (other SkImageGenerator implementations don't do it)- argb_row_bytes is SkBitmap row stride (SkBitmap::rowBytes()) in this case destination buffer stride. [1] https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkImageGenerat...
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 15 : Set FakeVCD count-devices=2 for webrtc_getusermedia_browsertest only. https://codereview.chromium.org/2428263004/diff/300001/content/browser/webrtc... File content/browser/webrtc/webrtc_getusermedia_browsertest.cc (right): https://codereview.chromium.org/2428263004/diff/300001/content/browser/webrtc... content/browser/webrtc/webrtc_getusermedia_browsertest.cc:132: void SetUpCommandLine(base::CommandLine* command_line) override { avi@chromium.org, This change came after LGTM when rebasing to split fake-video-capture-device CL . Initially, I thought it is good idea to set it for all in the file here [1], but after seeing that it also requires Android test changes here [2] which scope is wider, decided to control it and limit the affect to test here only. Is the LGTM still valid? Thanks. [1] Abandoned approach for Win/OSX, Linux: https://codereview.chromium.org/2428263004/patch/160001/170012 [2] Didn't think it is good idea to set "--use-fake-device-for-media-stream=device-count=2" for Android here: https://cs.chromium.org/chromium/src/testing/android/native_test/java/src/org...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
avi@chromium.org, I'll wait for your review on #56. The rest of the changes are reviewed and OK to land. Thanks.
aleksandar.stojiljkovic@intel.com changed reviewers: + tommi@chromium.org
tommi@, in case avi@ is busy and because it is in your domain, too, PTAL at #56. The code mentioned in #56 [1] is a change after Avi's LGTM so I thought to get it re-reviewed. You might also want to take a look at the other changes in your domain. Thanks. [1] https://codereview.chromium.org/2428263004/diff2/280001:300001/content/browse... Some more context: Initially, I thought it is good idea to set it for all in the file here [2], but after seeing that it also requires Android test changes here [3] which scope is wider, decided to control it and limit the affect to test here only. [1] Abandoned approach for Win/OSX, Linux: https://codereview.chromium.org/2428263004/patch/160001/170012 [2] Didn't think it is good idea to set "--use-fake-device-for-media-stream=device-count=2" for Android here: https://cs.chromium.org/chromium/src/testing/android/native_test/java/src/org...
aleksandar.stojiljkovic@intel.com changed reviewers: + phoglund@chromium.org
phoglund@, PTAL at [1]. The code mentioned in #56 [1] is a change after Avi's LGTM so good to get it reviewed. Thanks. [1] https://codereview.chromium.org/2428263004/diff2/280001:300001/content/browse... Similar approach (to remove the use-fake-device-for-media-stream from command line args) is already used in webrtc tests [2] - if you suggest, I could make a common method out of it in base class but would prefer to do it in follow up patcvh. This one is reviewed and would not like to grow it. [2] https://cs.chromium.org/chromium/src/content/browser/webrtc/webrtc_webcam_bro... Some more context: Initially, I thought it is good idea to set it for all in the file here [3], but after seeing that it also requires Android test changes here [4] which scope is wider, decided to control it and limit the affect to test here only. [3] Abandoned approach for Win/OSX, Linux: https://codereview.chromium.org/2428263004/patch/160001/170012 [4] Didn't think it was good idea to set "--use-fake-device-for-media-stream=device-count=2" for Android here as the scope is too wide. https://cs.chromium.org/chromium/src/testing/android/native_test/java/src/org...
https://codereview.chromium.org/2428263004/diff/300001/content/browser/webrtc... File content/browser/webrtc/webrtc_getusermedia_browsertest.cc (right): https://codereview.chromium.org/2428263004/diff/300001/content/browser/webrtc... content/browser/webrtc/webrtc_getusermedia_browsertest.cc:137: switches::kUseFakeDeviceForMediaStream; Well, your new test does appear to use getUserMedia, so you're in the right place. I dislike adding this much complexity to this file for just one test though; all the other tests in this file use normal fake devices. Could you extract this new test to a new test class for testing 16-bit devices? We've been trying to split browser tests into smaller files lately, to keep the individual tests simple. I think you get most of what you need through the base class anyway. Similarly, I think it's easy to create a new .html (getusermedia-16bit.html?) which includes webrtc_test_utilities.js and your new test utilities. You can see in https://cs.chromium.org/chromium/src/content/browser/webrtc/webrtc_ip_permiss... that we've done this before; if a test requires exotic flags, make a new test class instead of shoehorning into the main test class.
Patch Set 16 : Split webrtc_depth_capture_browsertest. Thanks phoglund@. https://codereview.chromium.org/2428263004/diff/300001/content/browser/webrtc... File content/browser/webrtc/webrtc_getusermedia_browsertest.cc (right): https://codereview.chromium.org/2428263004/diff/300001/content/browser/webrtc... content/browser/webrtc/webrtc_getusermedia_browsertest.cc:137: switches::kUseFakeDeviceForMediaStream; On 2016/11/02 10:56:57, phoglund_chrome wrote: > Well, your new test does appear to use getUserMedia, so you're in the right > place. I dislike adding this much complexity to this file for just one test > though; all the other tests in this file use normal fake devices. > > Could you extract this new test to a new test class for testing 16-bit devices? > We've been trying to split browser tests into smaller files lately, to keep the > individual tests simple. I think you get most of what you need through the base > class anyway. Similarly, I think it's easy to create a new .html > (getusermedia-16bit.html?) which includes webrtc_test_utilities.js and your new > test utilities. > > You can see in > https://cs.chromium.org/chromium/src/content/browser/webrtc/webrtc_ip_permiss... > that we've done this before; if a test requires exotic flags, make a new test > class instead of shoehorning into the main test class. Done. Good idea - there are more tests coming and better to split. Changed the name to depth-capture as there's also 8-bit format in pipeline.
lgtm w/ one suggestion https://codereview.chromium.org/2428263004/diff/320001/content/test/data/medi... File content/test/data/media/depth_stream_test_utilities.js (right): https://codereview.chromium.org/2428263004/diff/320001/content/test/data/medi... content/test/data/media/depth_stream_test_utilities.js:35: function testVideoToImageBitmap(videoElementName, success, error) I'd move this function, and the next, to the .html file since they lay out the test case itself, and are currently only used by one html file. The two functions above feel more like utility functions so they can stay though. Your call.
ehmaldonado@chromium.org changed reviewers: + ehmaldonado@chromium.org
https://codereview.chromium.org/2428263004/diff/320001/content/test/data/medi... File content/test/data/media/depth_stream_test_utilities.js (right): https://codereview.chromium.org/2428263004/diff/320001/content/test/data/medi... content/test/data/media/depth_stream_test_utilities.js:35: function testVideoToImageBitmap(videoElementName, success, error) On 2016/11/02 14:52:05, phoglund_chrome wrote: > I'd move this function, and the next, to the .html file since they lay out the > test case itself, and are currently only used by one html file. The two > functions above feel more like utility functions so they can stay though. Your > call. Thanks. Yes, it is good to do but let's not do it here - already extracted common code out of this in follow up patch while adding more tests.
The CQ bit was checked by aleksandar.stojiljkovic@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, dongseong.hwang@intel.com, danakj@chromium.org, mcasas@chromium.org, xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/2428263004/#ps320001 (title: "Split webrtc_depth_capture_browsertest. Thanks phoglund@,")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 16 bpp video stream capture, render and createImageBitmap(video) using (CPU) shared memory buffers This patch implements support for depth capture on Windows/Linux and ChromeOS using shared memory buffers. WebGL implementation with no precision loss and GPU memory buffer usage is split to separate patches from master patch: crrev.com/2121043002/ It Supports 16 bit depth video streams from R200 and SR300. Verified to work on Windows 10 and Ubuntu 16.04. Rendering video element: In cc, it is converted to RGBA format before glTexImage2D and rendered as TexturedQuad. 2D canvas + getImageData returns 8 bit color components (higher bit value) in RGBA. BUG=624436 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== 16 bpp video stream capture, render and createImageBitmap(video) using (CPU) shared memory buffers This patch implements support for depth capture on Windows/Linux and ChromeOS using shared memory buffers. WebGL implementation with no precision loss and GPU memory buffer usage is split to separate patches from master patch: crrev.com/2121043002/ It Supports 16 bit depth video streams from R200 and SR300. Verified to work on Windows 10 and Ubuntu 16.04. Rendering video element: In cc, it is converted to RGBA format before glTexImage2D and rendered as TexturedQuad. 2D canvas + getImageData returns 8 bit color components (higher bit value) in RGBA. BUG=624436 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #16 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== 16 bpp video stream capture, render and createImageBitmap(video) using (CPU) shared memory buffers This patch implements support for depth capture on Windows/Linux and ChromeOS using shared memory buffers. WebGL implementation with no precision loss and GPU memory buffer usage is split to separate patches from master patch: crrev.com/2121043002/ It Supports 16 bit depth video streams from R200 and SR300. Verified to work on Windows 10 and Ubuntu 16.04. Rendering video element: In cc, it is converted to RGBA format before glTexImage2D and rendered as TexturedQuad. 2D canvas + getImageData returns 8 bit color components (higher bit value) in RGBA. BUG=624436 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== 16 bpp video stream capture, render and createImageBitmap(video) using (CPU) shared memory buffers This patch implements support for depth capture on Windows/Linux and ChromeOS using shared memory buffers. WebGL implementation with no precision loss and GPU memory buffer usage is split to separate patches from master patch: crrev.com/2121043002/ It Supports 16 bit depth video streams from R200 and SR300. Verified to work on Windows 10 and Ubuntu 16.04. Rendering video element: In cc, it is converted to RGBA format before glTexImage2D and rendered as TexturedQuad. 2D canvas + getImageData returns 8 bit color components (higher bit value) in RGBA. BUG=624436 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/198b050b9fe647bf3799ab75df07b888c7adcf50 Cr-Commit-Position: refs/heads/master@{#429307} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/198b050b9fe647bf3799ab75df07b888c7adcf50 Cr-Commit-Position: refs/heads/master@{#429307} |
