Send bytes_used of v4l2 captured frame when video capture

Send bytes_used of v4l2 captured frame when video capture

There is no visible change by this patch because software JPEG decoder doesn't
care additional buffer size after the actual data. This is prepare for hardware
JPEG decoder which may copy data, so it needs to know actual size.

R=mcasas@chromium.org
BUG=335778
TEST=camera app (apprtc and hangout) with different resolutions works as usual

Committed: https://crrev.com/00ad11fc3cd0bf212013e74fbdd3a8a7f3547b6f
Cr-Commit-Position: refs/heads/master@{#331278}

[kcwu, 2015-05-18 14:04:04]

This is split from https://codereview.chromium.org/1016773002/

@mcasas, PTAL

[wuchengli, 2015-05-19 04:05:54]

wuchengli@chromium.org changed reviewers:
+ wuchengli@chromium.org

[wuchengli, 2015-05-19 04:05:54]

https://codereview.chromium.org/1147693002/diff/1/media/video/capture/linux/v...
File media/video/capture/linux/v4l2_capture_delegate.h (right):

https://codereview.chromium.org/1147693002/diff/1/media/video/capture/linux/v...
media/video/capture/linux/v4l2_capture_delegate.h:104: size_t bytes_used) const
= 0;
s/bytes_used/bytesused/ to be consistent with v4l2_buffer.

https://codereview.chromium.org/1147693002/diff/1/media/video/capture/linux/v...
File media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc (right):

https://codereview.chromium.org/1147693002/diff/1/media/video/capture/linux/v...
media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:61: size_t
/*bytes_used*/) const {
From the v4l2 documentation, bytesused is ignored for multiplanar. So this value
doesn't make sense for 4L2CaptureDelegateMultiPlane. I think you can add
V4L2CaptureDelegate::BufferTracker::GetBytesused(size_t plane) and
V4L2CaptureDelegate::BufferTracker::Plane::bytesused. You won't need to add
|bytes_used| to SendBuffer.

http://linuxtv.org/downloads/v4l-dvb-apis/buffer.html

[wuchengli, 2015-05-19 04:18:52]

https://codereview.chromium.org/1147693002/diff/1/media/video/capture/linux/v...
File media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc (right):

https://codereview.chromium.org/1147693002/diff/1/media/video/capture/linux/v...
media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:61: size_t
/*bytes_used*/) const {
On 2015/05/19 04:05:54, wuchengli wrote:
> From the v4l2 documentation, bytesused is ignored for multiplanar. So this
value
> doesn't make sense for 4L2CaptureDelegateMultiPlane. I think you can add
> V4L2CaptureDelegate::BufferTracker::GetBytesused(size_t plane) and
> V4L2CaptureDelegate::BufferTracker::Plane::bytesused. You won't need to add
> |bytes_used| to SendBuffer.
> 
> http://linuxtv.org/downloads/v4l-dvb-apis/buffer.html
Kuang-che told me bytesused is different for every frame and BufferTracker is
initialized only once. So how about passing a |bytesused| array to SendBuffer()
in V4L2CaptureDelegate::DoCapture()? 
__u32 bytesused[VIDEO_MAX_PLANES];
if (video_fmt_.type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
  bytesused[0] = buffer.bytesused;
else
  for (i = 0; i < video_fmt_.pix_mp.num_planes; i++)
    bytesused[i] = buffer.m.planes[i].bytesused;

[wuchengli, 2015-05-20 05:19:24]

wuchengli@chromium.org changed reviewers:
+ posciak@chromium.org

[wuchengli, 2015-05-20 05:19:36]

Pawel. PTAL.

[wuchengli, 2015-05-20 05:22:48]

Please test this CL without the other two because the other two will probably
take longer for code review. Then add TEST in the change description.

[Pawel Osciak, 2015-05-20 07:29:02]

https://codereview.chromium.org/1147693002/diff/1/media/video/capture/linux/v...
File media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc (right):

https://codereview.chromium.org/1147693002/diff/1/media/video/capture/linux/v...
media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:61: size_t
/*bytes_used*/) const {
Personally I liked the idea to add this to BufferTracker. What do you think
about adding a plane_payload[] array to BufferTracker and a setter/getter
SetPlanePayload()/GetPlanePayload()?

[kcwu, 2015-05-20 12:14:53]

https://codereview.chromium.org/1147693002/diff/1/media/video/capture/linux/v...
File media/video/capture/linux/v4l2_capture_delegate.h (right):

https://codereview.chromium.org/1147693002/diff/1/media/video/capture/linux/v...
media/video/capture/linux/v4l2_capture_delegate.h:104: size_t bytes_used) const
= 0;
On 2015/05/19 04:05:54, wuchengli wrote:
> s/bytes_used/bytesused/ to be consistent with v4l2_buffer. 

Acknowledged.

https://codereview.chromium.org/1147693002/diff/1/media/video/capture/linux/v...
File media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc (right):

https://codereview.chromium.org/1147693002/diff/1/media/video/capture/linux/v...
media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:61: size_t
/*bytes_used*/) const {
On 2015/05/20 07:29:02, Pawel Osciak wrote:
> Personally I liked the idea to add this to BufferTracker. What do you think
> about adding a plane_payload[] array to BufferTracker and a setter/getter
> SetPlanePayload()/GetPlanePayload()?

Done.

https://codereview.chromium.org/1147693002/diff/20001/media/video/capture/lin...
File media/video/capture/linux/v4l2_capture_delegate.h (right):

https://codereview.chromium.org/1147693002/diff/20001/media/video/capture/lin...
media/video/capture/linux/v4l2_capture_delegate.h:114: virtual void
FetchBytesUsed(
any suggestion for the name? I feel it not good.

[wuchengli, 2015-05-25 07:22:19]

https://codereview.chromium.org/1147693002/diff/20001/media/video/capture/lin...
File media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc (right):

https://codereview.chromium.org/1147693002/diff/20001/media/video/capture/lin...
media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:58: void
V4L2CaptureDelegateMultiPlane::FetchBytesUsed(
s/FetchBytesUsed/SetBytesUsed/. The direction should be the other way around.

https://codereview.chromium.org/1147693002/diff/20001/media/video/capture/lin...
media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:60: const
v4l2_buffer* buffer) const {
Use const reference. This is input.

https://codereview.chromium.org/1147693002/diff/20001/media/video/capture/lin...
media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:62:
buffer_tracker->SetPlanePayload(i, buffer->m.planes[i].bytesused);
s/SetPlanePayload/SetPlanePayloadSize/. Payload usually refers to the data
array.

[kcwu, 2015-05-25 07:36:26]

https://codereview.chromium.org/1147693002/diff/20001/media/video/capture/lin...
File media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc (right):

https://codereview.chromium.org/1147693002/diff/20001/media/video/capture/lin...
media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:58: void
V4L2CaptureDelegateMultiPlane::FetchBytesUsed(
On 2015/05/25 07:22:19, wuchengli wrote:
> s/FetchBytesUsed/SetBytesUsed/. The direction should be the other way around.

Changed to SetPayloadSize per offline discussion.

https://codereview.chromium.org/1147693002/diff/20001/media/video/capture/lin...
media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:60: const
v4l2_buffer* buffer) const {
On 2015/05/25 07:22:19, wuchengli wrote:
> Use const reference. This is input.

Done.

https://codereview.chromium.org/1147693002/diff/20001/media/video/capture/lin...
media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:62:
buffer_tracker->SetPlanePayload(i, buffer->m.planes[i].bytesused);
On 2015/05/25 07:22:19, wuchengli wrote:
> s/SetPlanePayload/SetPlanePayloadSize/. Payload usually refers to the data
> array.

Done.

[wuchengli, 2015-05-25 07:51:01]

lgtm

https://codereview.chromium.org/1147693002/diff/40001/media/video/capture/lin...
File media/video/capture/linux/v4l2_capture_delegate.h (right):

https://codereview.chromium.org/1147693002/diff/40001/media/video/capture/lin...
media/video/capture/linux/v4l2_capture_delegate.h:75: DCHECK_LT(plane,
planes_.size());
add a DCHECK for |payload_size| <= |length|

[wuchengli, 2015-05-25 07:51:28]

Pawel. Need owner review. Thanks.

[kcwu, 2015-05-25 07:54:23]

https://codereview.chromium.org/1147693002/diff/40001/media/video/capture/lin...
File media/video/capture/linux/v4l2_capture_delegate.h (right):

https://codereview.chromium.org/1147693002/diff/40001/media/video/capture/lin...
media/video/capture/linux/v4l2_capture_delegate.h:75: DCHECK_LT(plane,
planes_.size());
On 2015/05/25 07:51:01, wuchengli wrote:
> add a DCHECK for |payload_size| <= |length|

Done.

[Pawel Osciak, 2015-05-25 10:10:55]

lgtm % nit

https://codereview.chromium.org/1147693002/diff/60001/media/video/capture/lin...
File media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc (right):

https://codereview.chromium.org/1147693002/diff/60001/media/video/capture/lin...
media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:61: for (size_t i
= 0; i < v4l2_planes_.size(); i++)
i < v4l2_planes_.size() && i < arraysize(buffer.m.planes)

[kcwu, 2015-05-25 10:33:32]

https://codereview.chromium.org/1147693002/diff/60001/media/video/capture/lin...
File media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc (right):

https://codereview.chromium.org/1147693002/diff/60001/media/video/capture/lin...
media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:61: for (size_t i
= 0; i < v4l2_planes_.size(); i++)
On 2015/05/25 10:10:55, Pawel Osciak wrote:
> i < v4l2_planes_.size() && i < arraysize(buffer.m.planes)

buffer.m.planes is not array.
can I use i < buffer.length here?

[Pawel Osciak, 2015-05-25 11:02:35]

https://codereview.chromium.org/1147693002/diff/60001/media/video/capture/lin...
File media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc (right):

https://codereview.chromium.org/1147693002/diff/60001/media/video/capture/lin...
media/video/capture/linux/v4l2_capture_delegate_multi_plane.cc:61: for (size_t i
= 0; i < v4l2_planes_.size(); i++)
On 2015/05/25 10:33:32, kcwu wrote:
> On 2015/05/25 10:10:55, Pawel Osciak wrote:
> > i < v4l2_planes_.size() && i < arraysize(buffer.m.planes)
> 
> buffer.m.planes is not array.
> can I use i < buffer.length here?

Yes please, kernel verifies this.

[kcwu, 2015-05-25 11:45:52]

The CQ bit was checked by kcwu@chromium.org

[kcwu, 2015-05-25 11:45:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
wuchengli@chromium.org, posciak@chromium.org
Link to the patchset: https://codereview.chromium.org/1147693002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-05-25 11:45:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1147693002/80001

[commit-bot: I haz the power, 2015-05-25 12:23:34]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-25 12:24:26]

Patchset 5 (id:??) landed as
https://crrev.com/00ad11fc3cd0bf212013e74fbdd3a8a7f3547b6f
Cr-Commit-Position: refs/heads/master@{#331278}