Chromium Code Reviews

Issue 967793002: Linux Video Capture: Add V4L2VideoCaptureDelegate{Single,Multi}Plane. (Closed)

Created:
5 years, 9 months ago by mcasas
Modified:
5 years, 9 months ago
Reviewers:
perkj_chrome, emircan, magjed_chromium, Pawel Osciak, DaleCurtis
CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Linux Video Capture: Add V4L2VideoCaptureDelegate{Single,Multi}Plane. This CL adds support for V4L2 MPLANE Capture Api. Only supported format is YUV420M triplanar. A new method is added to VideoCaptureDeviceClient, namely OnIncomingCapturedYuvData(...), which forces adding MOCKing here and there, and its own implementation. V4L2 MMAP API works via user mmap()ing a number of buffer allocated by V4L2 capture device. If those buffers are not correctly munmap()ed, bad things (c) happen. In light of this, the manual buffer lifetime management is changed to automatic one. Construction (mmap()ing) of those called BufferTracker is planarity specific (i.e. there's one such ctor in each of BufferTracker{S,M}Plane), while the dtor is generic and the same. ToT class diagram: +------------------------------------+ | VideoCaptureDeviceLinux | | +----------------------+| | <<ref>> -->| V4L2CaptureDelegate || | cnt | (struct Buffer) || | +----------------------+| +------------------------------------+ This CL class scheme: +--------------------------+ | VideoCaptureDeviceLinux | | | | <<ref_cnt>> ---+ | +----------------|---------+ +----------------v-----------+ v4l2_capture_delegate.{cc,h} | +-----------------------+ | | |V4L2CaptureDelegate | | | | (class BufferTracker)| | | +-----------------------+ | +-------^------------------^-+ | | +----|-------+ +--------|--+ v4l2_capture_delegate_multi_plane.{cc,h} | SPlane | | MPlane | | (BTSplane) | | (BTMPlane)| | | +-----------+ +------------+ v4l2_capture_delegate_single_plane.{cc,h} - VCDevice works on the premise that its calls into VCDevice::Client::OnIncomingWhatever() are synchronous. That assumption is respected here. - A bit of cleanup is done in OnIncomingCaptureData(), in what regards rotation/crop/odd sizes. A unit test is subsequently added. - VideoCaptureDeviceFactory labels the devices as Single or Multi Planar. That labeling capture_api_type() needs to ripple through a bunch of files, causing some otherwise uninteresting changes in the patchsets. BUG=441836 TEST= Compile and insmod vivid.ko into a kernel, with options for supporting MPLANE api (multiplanar=2) then capture using patched Chromium. Current vivid does _not_ support any common Mplane format, needs a patch: https://github.com/miguelao/linux/tree/adding_yu12_yv12_nv12_nv21___mplane_formats___with_mods_for_kernel_3_13 that needs to be compiled against ubuntu sources 3.13 etc. For even better coverage, use a normal WebCam and navigate to http://goo.gl/fUcIiP, then open both the MPlane camera mentioned in the previous paragraph and the "normal" webcam (this is,partially, how I try it). Committed: https://crrev.com/d3d37d0f3493dd28cd687fe9285b0aac81a61dae Cr-Commit-Position: refs/heads/master@{#321612}

Patch Set 1 : #

Total comments: 55

Patch Set 2 : Rebased (VideoCaptureControllerClient splitted from VCController) #

Patch Set 3 : emircan@ and posciak@ comments #

Total comments: 10

Patch Set 4 : magjed@ comments #

Total comments: 48

Patch Set 5 : posciak@ comments #

Total comments: 27

Patch Set 6 : Extracted V4L2CaptureDelegate{Single,Multi}Plane into separated files, Multi not compiled for OpenB… #

Patch Set 7 : posciak@ comments on PS5. Minor rebase of video_capture_controller_unittest.cc and BUILD.gn #

Total comments: 28

Patch Set 8 : posciak@ nits perkj@,magjed@ and dalecurtis@ comments. Rebase. #

Total comments: 4

Patch Set 9 : DCHECK correction. Rebase. #

Total comments: 4

Patch Set 10 : s/V4L2VideoCaptureDelegate/V4L2CaptureDelegate/; using v4l2_format for strides. #

Total comments: 4

Patch Set 11 : magjed@ comments #

Unified diffs Side-by-side diffs Stats (+1032 lines, -838 lines)
M content/browser/media/capture/desktop_capture_device_aura_unittest.cc View 1 chunk +10 lines, -0 lines 0 comments
M content/browser/media/capture/desktop_capture_device_unittest.cc View 1 chunk +10 lines, -0 lines 0 comments
M content/browser/media/capture/web_contents_video_capture_device_unittest.cc View 1 chunk +12 lines, -0 lines 0 comments
M content/browser/media/media_internals.cc View 1 chunk +1 line, -1 line 0 comments
M content/browser/media/media_internals_unittest.cc View 4 chunks +14 lines, -7 lines 0 comments
M content/browser/renderer_host/media/video_capture_controller_unittest.cc View 9 chunks +89 lines, -26 lines 0 comments
M content/browser/renderer_host/media/video_capture_device_client.h View 1 chunk +9 lines, -0 lines 0 comments
M content/browser/renderer_host/media/video_capture_device_client.cc View 4 chunks +87 lines, -23 lines 0 comments
M media/BUILD.gn View 2 chunks +13 lines, -0 lines 0 comments
M media/media.gyp View 2 chunks +11 lines, -0 lines 0 comments
M media/video/capture/fake_video_capture_device_factory.cc View 1 chunk +3 lines, -1 line 0 comments
M media/video/capture/fake_video_capture_device_unittest.cc View 2 chunks +12 lines, -0 lines 0 comments
A media/video/capture/linux/v4l2_capture_delegate.h View 1 chunk +150 lines, -0 lines 0 comments
A + media/video/capture/linux/v4l2_capture_delegate.cc View 8 chunks +224 lines, -339 lines 0 comments
A media/video/capture/linux/v4l2_capture_delegate_multi_plane.h View 1 chunk +59 lines, -0 lines 0 comments
A media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc View 1 chunk +99 lines, -0 lines 0 comments
A media/video/capture/linux/v4l2_capture_delegate_single_plane.h View 1 chunk +54 lines, -0 lines 0 comments
A media/video/capture/linux/v4l2_capture_delegate_single_plane.cc View 1 chunk +60 lines, -0 lines 0 comments
M media/video/capture/linux/video_capture_device_factory_linux.cc View 4 chunks +15 lines, -8 lines 0 comments
M media/video/capture/linux/video_capture_device_linux.h View 2 chunks +5 lines, -2 lines 0 comments
M media/video/capture/linux/video_capture_device_linux.cc View 4 chunks +33 lines, -418 lines 0 comments
M media/video/capture/video_capture_device.h View 6 chunks +26 lines, -7 lines 0 comments
M media/video/capture/video_capture_device.cc View 3 chunks +22 lines, -5 lines 0 comments
M media/video/capture/video_capture_device_unittest.cc View 3 chunks +14 lines, -1 line 0 comments

Messages

Total messages: 61 (31 generated)
mcasas
emircan@ PTAL
5 years, 9 months ago (2015-03-04 01:00:21 UTC) #8
emircan
I'll need more time to understand general workflow of this. https://codereview.chromium.org/967793002/diff/100001/content/browser/renderer_host/media/video_capture_controller.cc File content/browser/renderer_host/media/video_capture_controller.cc (right): https://codereview.chromium.org/967793002/diff/100001/content/browser/renderer_host/media/video_capture_controller.cc#newcode434 ...
5 years, 9 months ago (2015-03-04 02:47:48 UTC) #9
mcasas
posciak@ PTAL (emircan@ I'll take on your comments , since they are few, together with ...
5 years, 9 months ago (2015-03-04 15:44:08 UTC) #11
Pawel Osciak
I only had enough steam to finish Delegates for now. https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capture/linux/v4l2_video_capture_delegate.cc File media/video/capture/linux/v4l2_video_capture_delegate.cc (right): https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capture/linux/v4l2_video_capture_delegate.cc#newcode95 ...
5 years, 9 months ago (2015-03-06 10:43:55 UTC) #13
perkj_chrome
Adding magjed. I will be ooo for a week. Sorry, I have not had time ...
5 years, 9 months ago (2015-03-06 16:12:47 UTC) #15
mcasas
guys PTAL. I had to do a rebase, but luckily not big changes happened. https://codereview.chromium.org/967793002/diff/100001/content/browser/renderer_host/media/video_capture_controller.cc ...
5 years, 9 months ago (2015-03-09 21:23:57 UTC) #18
magjed_chromium
https://codereview.chromium.org/967793002/diff/180001/content/browser/renderer_host/media/video_capture_controller_unittest.cc File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/967793002/diff/180001/content/browser/renderer_host/media/video_capture_controller_unittest.cc#newcode672 content/browser/renderer_host/media/video_capture_controller_unittest.cc:672: unsigned char data[kScratchpadSizeInBytes]; You need to initialize this memory ...
5 years, 9 months ago (2015-03-10 16:32:48 UTC) #19
mcasas
posciak@ PTAL magjed@ PTAL if you wish. https://codereview.chromium.org/967793002/diff/180001/content/browser/renderer_host/media/video_capture_controller_unittest.cc File content/browser/renderer_host/media/video_capture_controller_unittest.cc (right): https://codereview.chromium.org/967793002/diff/180001/content/browser/renderer_host/media/video_capture_controller_unittest.cc#newcode672 content/browser/renderer_host/media/video_capture_controller_unittest.cc:672: unsigned char ...
5 years, 9 months ago (2015-03-10 20:10:32 UTC) #21
Pawel Osciak
https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capture/linux/v4l2_video_capture_delegate.cc File media/video/capture/linux/v4l2_video_capture_delegate.cc (right): https://chromiumcodereview.appspot.com/967793002/diff/100001/media/video/capture/linux/v4l2_video_capture_delegate.cc#newcode172 media/video/capture/linux/v4l2_video_capture_delegate.cc:172: if (pixel_format != PIXEL_FORMAT_I420) On 2015/03/09 21:23:56, mcasas wrote: ...
5 years, 9 months ago (2015-03-13 09:52:53 UTC) #23
mcasas
posciak@ PTAL. Note please the 2 Patchset 5 and 6. PS5 represents all your comments ...
5 years, 9 months ago (2015-03-14 03:36:12 UTC) #29
Pawel Osciak
Well, looking pretty good! I reviewed the 4->5 diff. Just some final remaining nits in ...
5 years, 9 months ago (2015-03-17 11:05:26 UTC) #30
mcasas
posciak@ PTAL. https://codereview.chromium.org/967793002/diff/300001/media/video/capture/linux/v4l2_video_capture_delegate.cc File media/video/capture/linux/v4l2_video_capture_delegate.cc (right): https://codereview.chromium.org/967793002/diff/300001/media/video/capture/linux/v4l2_video_capture_delegate.cc#newcode34 media/video/capture/linux/v4l2_video_capture_delegate.cc:34: // (depending on |num_planes|). This list is ...
5 years, 9 months ago (2015-03-17 22:01:57 UTC) #34
Pawel Osciak
media/video/capture/linux/ lgtm % nits Perhaps please grab a copy of videoio.h, change your GYP_DEFINES to ...
5 years, 9 months ago (2015-03-19 07:52:43 UTC) #35
magjed_chromium
lgtm % nits https://codereview.chromium.org/967793002/diff/430001/content/browser/renderer_host/media/video_capture_device_client.cc File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/967793002/diff/430001/content/browser/renderer_host/media/video_capture_device_client.cc#newcode251 content/browser/renderer_host/media/video_capture_device_client.cc:251: const size_t u_stride = u_length / ...
5 years, 9 months ago (2015-03-19 18:13:29 UTC) #36
mcasas
dalecurtis@: media/media.gyp/BUILD.gn please
5 years, 9 months ago (2015-03-19 18:58:24 UTC) #38
DaleCurtis
https://codereview.chromium.org/967793002/diff/430001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/967793002/diff/430001/media/BUILD.gn#newcode404 media/BUILD.gn:404: if (is_openbsd) { I think this is always false. ...
5 years, 9 months ago (2015-03-19 19:07:17 UTC) #39
perkj_chrome
lgtm if all coments are addressed. https://codereview.chromium.org/967793002/diff/430001/content/browser/renderer_host/media/video_capture_device_client.cc File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/967793002/diff/430001/content/browser/renderer_host/media/video_capture_device_client.cc#newcode90 content/browser/renderer_host/media/video_capture_device_client.cc:90: DLOG_IF(WARNING, (rotation % ...
5 years, 9 months ago (2015-03-19 20:38:18 UTC) #40
mcasas
dalecurtis@ PTAL. (posciak@ I try with SPlane Webcams all the time, see updated TEST= section). ...
5 years, 9 months ago (2015-03-19 22:57:01 UTC) #42
Pawel Osciak
https://codereview.chromium.org/967793002/diff/430001/media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc File media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc (right): https://codereview.chromium.org/967793002/diff/430001/media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc#newcode52 media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:52: buffer->m.planes = const_cast<struct v4l2_plane*>(v4l2_planes_.data()); On 2015/03/19 22:57:00, mcasas wrote: ...
5 years, 9 months ago (2015-03-20 00:56:07 UTC) #43
mcasas
posciak@ PTAL. As spoken offline, there was action in SendBuffer()s, V4L2CaptureDelegate holds on to v4l2_format ...
5 years, 9 months ago (2015-03-20 03:01:20 UTC) #47
magjed_chromium
https://codereview.chromium.org/967793002/diff/570001/content/browser/renderer_host/media/video_capture_device_client.cc File content/browser/renderer_host/media/video_capture_device_client.cc (right): https://codereview.chromium.org/967793002/diff/570001/content/browser/renderer_host/media/video_capture_device_client.cc#newcode241 content/browser/renderer_host/media/video_capture_device_client.cc:241: DCHECK_GE(u_stride, 1u * frame_format.frame_size.width() / 2); It would still ...
5 years, 9 months ago (2015-03-20 07:53:45 UTC) #48
magjed_chromium
lgtm
5 years, 9 months ago (2015-03-20 17:13:29 UTC) #50
DaleCurtis
I'm not sure openbsd support even compiles anymore, certainly ffmpeg doesn't -- but lgtm for ...
5 years, 9 months ago (2015-03-20 18:05:52 UTC) #52
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/967793002/630001
5 years, 9 months ago (2015-03-20 18:34:45 UTC) #55
commit-bot: I haz the power
Committed patchset #11 (id:630001)
5 years, 9 months ago (2015-03-20 19:15:22 UTC) #56
commit-bot: I haz the power
Patchset 11 (id:??) landed as https://crrev.com/d3d37d0f3493dd28cd687fe9285b0aac81a61dae Cr-Commit-Position: refs/heads/master@{#321612}
5 years, 9 months ago (2015-03-20 19:16:42 UTC) #57
perkj_chrome
A revert of this CL (patchset #11 id:630001) has been created in https://codereview.chromium.org/1031583002/ by perkj@chromium.org. ...
5 years, 9 months ago (2015-03-23 08:53:03 UTC) #58
phoglund_chromium
A revert of this CL (patchset #11 id:630001) has been created in https://codereview.chromium.org/1024093003/ by phoglund@chromium.org. ...
5 years, 9 months ago (2015-03-23 09:38:12 UTC) #59
mcasas
On 2015/03/23 09:38:12, phoglund wrote: > A revert of this CL (patchset #11 id:630001) has ...
5 years, 9 months ago (2015-03-23 16:25:36 UTC) #60
mcasas
5 years, 9 months ago (2015-03-24 14:24:50 UTC) #61
Message was sent while issue was closed.
Old comments. Ignore.

https://codereview.chromium.org/967793002/diff/570001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_device_client.cc (right):

https://codereview.chromium.org/967793002/diff/570001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_device_client.cc:241:
DCHECK_GE(u_stride, 1u * frame_format.frame_size.width() / 2);
On 2015/03/20 07:53:45, magjed wrote:
> It would still be nice to have height + 1 here. For example, a 31x31 image
needs
> u/v strides at least 16, not 15.

Done.

https://codereview.chromium.org/967793002/diff/570001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_device_client.cc:262: dst_y,
y_stride,
On 2015/03/20 07:53:45, magjed wrote:
> The destination strides are wrong. You calculate |dst_u| and |dst_v| using
> VideoFrame::PlaneAllocationSize which implies tightly packed strides, but you
> use the src strides here.

Changed to using those from VideoFrame::RowBytes().

Powered by Google App Engine