Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(17)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1124723006: VideoCaptureDeviceLinux: Add support for SPLANE+DMABUF V4L2 type capture (Closed)

Created:
5 years, 7 months ago by mcasas
Modified:
5 years, 1 month ago
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

VideoCaptureDeviceLinux: Add support for SPLANE+DMABUF V4L2 type capture During V4L2CaptureDelegate::Create() method, check if DmaBuf capture is supported both in the device and in the platform, and instantiate a new V4L2CaptureDelegateSPlaneDmaBuf in that case. BUG=440843

Patch Set 1 : #

Total comments: 4

Patch Set 2 : emircan@s comments #

Patch Set 3 : Added V4L2CaptureDelegateSinglePlaneDmaBuf #

Patch Set 4 : Nuked V4L2CaptureDelegateSinglePlaneDmaBuf, merged into SPlane and its BufferTracker #

Patch Set 5 : Rebase #

Total comments: 34

Patch Set 6 : posciak@ review and rebase (AsPlatformFile(), ...BufferPoolUtilization...)). #

Unified diffs Side-by-side diffs Delta from patch set Stats (+186 lines, -42 lines) Patch
M content/browser/renderer_host/media/video_capture_device_client.cc View 1 2 3 4 5 1 chunk +2 lines, -1 line 0 comments Download
M media/video/capture/linux/v4l2_capture_delegate.h View 1 2 3 4 5 8 chunks +34 lines, -14 lines 0 comments Download
M media/video/capture/linux/v4l2_capture_delegate.cc View 1 2 3 4 5 9 chunks +49 lines, -10 lines 0 comments Download
M media/video/capture/linux/v4l2_capture_delegate_multi_plane.h View 1 2 3 4 5 1 chunk +2 lines, -1 line 0 comments Download
M media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc View 1 2 3 4 5 2 chunks +3 lines, -1 line 0 comments Download
M media/video/capture/linux/v4l2_capture_delegate_single_plane.h View 1 2 3 4 5 3 chunks +9 lines, -7 lines 0 comments Download
M media/video/capture/linux/v4l2_capture_delegate_single_plane.cc View 1 2 3 4 5 3 chunks +82 lines, -7 lines 0 comments Download
M media/video/capture/linux/video_capture_device_linux.cc View 1 2 3 4 5 1 chunk +5 lines, -1 line 0 comments Download

Messages

Total messages: 26 (18 generated)
mcasas
emircan@ PTAL
5 years, 7 months ago (2015-05-05 23:24:26 UTC) #5
emircan
lgtm % below. https://codereview.chromium.org/1124723006/diff/60001/media/video/capture/linux/video_capture_device_factory_linux.cc File media/video/capture/linux/video_capture_device_factory_linux.cc (right): https://codereview.chromium.org/1124723006/diff/60001/media/video/capture/linux/video_capture_device_factory_linux.cc#newcode21 media/video/capture/linux/video_capture_device_factory_linux.cc:21: //#include "content/common/gpu/gpu_memory_buffer_factory.h" Can be erased. https://codereview.chromium.org/1124723006/diff/60001/media/video/capture/linux/video_capture_device_factory_linux.cc#newcode201 ...
5 years, 7 months ago (2015-05-06 03:06:26 UTC) #6
mcasas
pawel@ PTAL if you want otherwise expect the next CL , with more meat, going ...
5 years, 7 months ago (2015-05-06 21:44:42 UTC) #8
mcasas
posciak@ please have a first look at the general design, see if it fits our ...
5 years, 6 months ago (2015-06-05 22:20:09 UTC) #15
mcasas
posciak@ ping.
5 years, 6 months ago (2015-06-10 22:22:11 UTC) #16
Pawel Osciak
https://chromiumcodereview.appspot.com/1124723006/diff/260001/media/video/capture/linux/v4l2_capture_delegate.cc File media/video/capture/linux/v4l2_capture_delegate.cc (right): https://chromiumcodereview.appspot.com/1124723006/diff/260001/media/video/capture/linux/v4l2_capture_delegate.cc#newcode67 media/video/capture/linux/v4l2_capture_delegate.cc:67: bool IsDmaBufCaptureSupported(const std::string& device_fd, static s/device_fd/device_name/ https://chromiumcodereview.appspot.com/1124723006/diff/260001/media/video/capture/linux/v4l2_capture_delegate.cc#newcode79 media/video/capture/linux/v4l2_capture_delegate.cc:79: if ...
5 years, 6 months ago (2015-06-15 10:34:55 UTC) #17
Pawel Osciak
+wuchengli@
5 years, 6 months ago (2015-06-15 10:35:24 UTC) #19
mcasas
5 years, 6 months ago (2015-06-17 01:30:54 UTC) #25 
guys PTAL.

posciak@ please read first my reply in
v4l2_capture_delegate_single_plane.cc:60

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/li...
File media/video/capture/linux/v4l2_capture_delegate.cc (right):

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/li...
media/video/capture/linux/v4l2_capture_delegate.cc:67: bool
IsDmaBufCaptureSupported(const std::string& device_fd,
On 2015/06/15 10:34:54, Pawel Osciak wrote:
> static
> s/device_fd/device_name/

Done.

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/li...
media/video/capture/linux/v4l2_capture_delegate.cc:79: if
(HANDLE_EINTR(ioctl(fd.get(), VIDIOC_REQBUFS, &r_buffer)) < 0)
On 2015/06/15 10:34:55, Pawel Osciak wrote:
> Unfortunately, we check if the V4L2 capturer supports dmabufs, but we don't
> check whether the HW encoder does. Also, we don't know (and we can't know
here)
> if a HW encoder will be used at all (we wouldn't want to use dmabufs either
> then).
> 
> When we have dmabuf support in uvcvideo driver (and it's actually already
there
> in new upstream Linux kernels), we'll get dmabuf support in the V4L2 camera
> capturer on all CrOS platforms (and, well, all Linux systems with sufficiently
> new kernels). But some of them won't support dmabuf in HW encoder, and some
> won't have a HW encoder at all.
> 

This is true, and I think just landing this code "as-is" would
break current ToT, so in parallel I'm adding a command line
flag [1] to force Dmabuf usage. This flag boils down to a
bool in VideoCaptureParams passed on OnAllocateAndStart(). 
I think this flag will allow us to land all necessary code,
and then we can, if deemed necessary, use a Finch
experiment, as a step before having the decision
fully automatised like you say in l.84.

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

> Please also see the comment at l.84.

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/li...
media/video/capture/linux/v4l2_capture_delegate.cc:84:
client->ReserveOutputBuffer(media::PIXEL_FORMAT_GPUMEMORYBUFFER,
On 2015/06/15 10:34:55, Pawel Osciak wrote:
> This is not really a detail of V4L2 capture, so I would prefer not having this
> here. I understand this is a work in progress and some
> workarounds/simplifications like this could be acceptable, but in general, the
> decision about the whole pipeline can't be made in the V4L2 capturer, because
it
> has insufficient information about the platform. This should be queried in the
> client based on the information from all the devices/platform.
> 
> So with this and for the reasons in l.79 above, I'd suggest the model should
be:
> - we should have a common property on each type of device (capturer, GPU
> renderer, HW encoder) called SUPPORTS_NATIVE_BUFFERS, or something like that;
> - the above for V4L2 capturer should ideally be in VideoCaptureDeviceInfo I
> think. Then the client could query it very early.
> - it should be the client to query each device for this; if all support it, it
> just passes a bool or enum to each device, telling it to use native buffers;
> 
> Of course, within reasonable detail, I'm ok having some shortcuts and not a
> full-blown detection for all possible cases/platforms in the client, but as
long
> as we eliminate false positives.

That sounds reasonable. As you can see, at the level of
this particular file, CreateV4L2CaptureDelegate() is
instructed to use DmaBufs or not. The decision is taken
somewhere else, in a more intelligent part of Chromium.
So this function is reduced to just checking if V4L2
supports DmaBuf Capture.

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/li...
media/video/capture/linux/v4l2_capture_delegate.cc:194: plane.start = nullptr;
On 2015/06/15 10:34:55, Pawel Osciak wrote:
> Zero-initialization could be done in Plane::Plane().

Done.

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/li...
media/video/capture/linux/v4l2_capture_delegate.cc:332:
FinishFillingV4L2RequestBuffers(&r_buffer);
On 2015/06/15 10:34:55, Pawel Osciak wrote:
> I'd suggest just r_buffer.memory = memory(); if we do the getter.

Done.

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/li...
File media/video/capture/linux/v4l2_capture_delegate.h (right):

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/li...
media/video/capture/linux/v4l2_capture_delegate.h:70: int const GetFd(size_t
plane) const {
On 2015/06/15 10:34:55, Pawel Osciak wrote:
> GetPlaneFd()
> 
> Also, please add an empty line above.

Done.

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/li...
media/video/capture/linux/v4l2_capture_delegate.h:127: virtual void
FinishFillingV4L2RequestBuffers(
On 2015/06/15 10:34:55, Pawel Osciak wrote:
> This could just be a simple memory_type() const getter, especially since
> memory_type_ should be a member of V4L2CaptureDelegate (please see my comment
> later on).

Done.

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/li...
File media/video/capture/linux/v4l2_capture_delegate_single_plane.cc (right):

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/li...
media/video/capture/linux/v4l2_capture_delegate_single_plane.cc:55:
DCHECK(buffer->m.fd);  // For Dequeue, API should have filled |fd| in.
On 2015/06/15 10:34:55, Pawel Osciak wrote:
> We call this before calling DQBUF, so it couldn't have been filled yet?
> 
First time this method is called, it's with 
|for_enqueue|==true, the second time is called
with |for_enqueue|==false, see 
V4L2CaptureDelegate::MapAndQueueBuffer(...)



> Also, fd = 0 is a valid fd, while -1 is not. The former would fail, while the
> latter would not.

Done.

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/li...
media/video/capture/linux/v4l2_capture_delegate_single_plane.cc:60:
scoped_ptr<VideoCaptureDevice::Client::Buffer> capture_buffer =
On 2015/06/15 10:34:55, Pawel Osciak wrote:
> Do we need to allocate buffers on each QBUF? Could we allocate them once, when
> we REQBUFS?

We don't allocate a Buffers on each QBUF, we 
ReserveOutputBuffer(). The first time we call this
method, VideoCaptureBufferPool indeed allocates,
but subsequent calls, assuming same call params,
i.e. same format and resolution, do not allocate.

> 
> Further, I'd suggest adding Client::Buffers to Plane. Then
> we could look them up by buffer.index. With that, we could
> get rid of std::find, and also we could not fail on 
> SendBuffer. Moreover, we could just look at that common
> structure and wouldn't have to check memory type, just see
> if fd != -1 or if start != nullptr and make a decision
> based on that.
> 
> Also, we shouldn't need for_capture then, just fill it 
> always (cheap).

I see your points. I don't like the way BufferTracker can
be two things depending on the values of its Planes' 
members. I think it is already bad enough that it can
be either |fd| != -1 or |start| != nullptr (but not both).

The suggestion you made in l.128 seems to go in that
direction as well: have a BufferTracker that can be either
or but not both. I would suggest an abstract class 
BufferTracker with two implementations, MmapBufferTracker
and DmaBufBufferTracker, WDYT?

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/li...
media/video/capture/linux/v4l2_capture_delegate_single_plane.cc:69:
DLOG(WARNING) << "Uh oh, platform does not support Dma-Buf allocation, "
On 2015/06/15 10:34:55, Pawel Osciak wrote:
> We should propagate errors and SetErrorState() in higher layers.

This is a Warning, and it was meant to be to help
development, so I refactored the code to better
reflect the meaning.

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/li...
media/video/capture/linux/v4l2_capture_delegate_single_plane.cc:109:
[incoming_fd](const VideoCaptureDevice::Client::Buffer* b) {
On 2015/06/15 10:34:55, Pawel Osciak wrote:
> const Buffer& possible?

ScopedVector keeps pointers, from which you can
make_scoped_ptr() as in l.116.

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/li...
media/video/capture/linux/v4l2_capture_delegate_single_plane.cc:114: return;
On 2015/06/15 10:34:55, Pawel Osciak wrote:
> Need to propagate the error.

Same as before, this was meant to be for 
helping development, not for production code.
Using SetErrorState() comes with a series of
troubles on its own, as you can see it is a
private method, so we'll need to surface it etc.
Let me ask you the complementary question:
is an V4L2 driver able to capture in an |fd|
that has not been fed, given the configuration
in this CL?

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/li...
media/video/capture/linux/v4l2_capture_delegate_single_plane.cc:118:
allocated_buffers_.weak_erase(used_buffer);
On 2015/06/15 10:34:55, Pawel Osciak wrote:
> Perhaps we could use std::vector<scoped_refptr<>> to simplify this?

I wish! ReserveOutputBuffer() returns a scoped_ptr<>
and VideoCaptureDevice::Client::Buffer has no
ref counting semantics.

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/li...
media/video/capture/linux/v4l2_capture_delegate_single_plane.cc:128:
AddNonMmapedPlane(buffer.m.fd);
On 2015/06/15 10:34:55, Pawel Osciak wrote:
> Would it be possible to have one AddPlane() method, which could look at
> buffer.memory and decide what to use, to simplify and reduce the number of
> ifdefs?

Wouldn't it move this ifdef somewhere else, and
force every caller (including V4L2CaptureDelegateMultiPlane,
who is oblivious to DmaBufs so far) to indicate a
fd? And, in this method, we'd still need to avoid
mmap() for V4L2_MEMORY_DMABUFs.

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/li...
File media/video/capture/linux/v4l2_capture_delegate_single_plane.h (right):

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/li...
media/video/capture/linux/v4l2_capture_delegate_single_plane.h:25: bool
try_to_use_dma_buf);
On 2015/06/15 10:34:55, Pawel Osciak wrote:
> This is not really "try", but "use dmabuf or fail" I believe?

Is a "try" and if you can't, is fine.
Changed to |allow_using_dma_bufs|, please
instruct if not clear enough.

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/li...
media/video/capture/linux/v4l2_capture_delegate_single_plane.h:56: v4l2_memory
memory_type_;
On 2015/06/15 10:34:55, Pawel Osciak wrote:
> This should be a member of V4l2CaptureDelegate. We always have a memory type,
> for both single and multi plane. Using dmabuf should also be a property of
> V4l2CaptureDelegate and not an argument to constructor of this class.

Done.

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/vi...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/1124723006/diff/260001/media/video/capture/vi...
media/video/capture/video_capture_device.h:207: virtual base::PlatformFile
AsPlatformFile() const = 0;
On 2015/06/15 10:34:55, Pawel Osciak wrote:
> Are there platforms where it would not be a dmabuf (but would still be usable
in
> this context)? What would it be then?

Actually this is landed and needs rebase. But to answer
the question, yes, this can be a Shared Memory in any
non-Linux-DmaBuf platform.

Powered by Google App Engine
This is Rietveld 408576698