After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 720568 - v4l2: Various changes to allow using M2M decoders
v4l2: Various changes to allow using M2M decoders
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
http://cgit.collabora.com/git/user/ju...
: 719779 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-12-16 21:46 UTC by Nicolas Dufresne (ndufresne)
Modified: 2014-01-10 22:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH] v4l2object: Split caps in different categories (7.42 KB, patch)
2013-12-16 21:47 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2: Fix h264 caps (1.00 KB, patch)
2013-12-16 21:47 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2: better handle quirks activation (2.18 KB, patch)
2013-12-16 21:47 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2object: make IO_MODE enum public (1.25 KB, patch)
2013-12-16 21:47 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2object: Implement gst_v4l2_dup() (5.52 KB, patch)
2013-12-16 21:47 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2object: Add gst_v4l2_object_open_shared() (1.51 KB, patch)
2013-12-16 21:47 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2object: Quirks for dev without initial format (2.74 KB, patch)
2013-12-16 21:47 UTC, Nicolas Dufresne (ndufresne)
needs-work Details | Review
[PATCH] v4l2object: Don't validate dimension for encoded format (2.00 KB, patch)
2013-12-16 21:47 UTC, Nicolas Dufresne (ndufresne)
needs-work Details | Review
[PATCH] v4l2bufferpool: Request buffers only once (6.90 KB, patch)
2013-12-16 21:47 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2object: Ensure we assumes at least 1 plane (922 bytes, patch)
2013-12-16 21:47 UTC, Nicolas Dufresne (ndufresne)
reviewed Details | Review
[PATCH] v4l2object: Split _v4l2fourcc_to_video_format (6.73 KB, patch)
2013-12-16 21:48 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2object: Move mplane logic into gst_v4l2_object_get_caps_info() (5.93 KB, patch)
2013-12-16 21:48 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2object: Move back assertions where they should be (1.32 KB, patch)
2013-12-16 21:48 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2object: Move the extrapolation of stride at the right place (6.12 KB, patch)
2013-12-16 21:48 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2object: Don't check format specific information (1.82 KB, patch)
2013-12-16 21:48 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2object: Move code block where it belongs (1.77 KB, patch)
2013-12-16 21:48 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2object: Split out saving format from set_format() (4.65 KB, patch)
2013-12-16 21:48 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2object: Implement _setup_format() (4.49 KB, patch)
2013-12-16 21:48 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2: Move decide allocation into v4l2object (8.03 KB, patch)
2013-12-16 21:48 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2object: Allocate pool if needed in decide_allocation (1.03 KB, patch)
2013-12-16 21:48 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2object: Add V4L2_CID_MIN_BUFFERS_FOR_CAPTURE support (1.34 KB, patch)
2013-12-16 21:48 UTC, Nicolas Dufresne (ndufresne)
needs-work Details | Review
[PATCH] v4l2bufferpool: Respect the suggested min buffer (852 bytes, patch)
2013-12-16 21:48 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2object: Don't keep the max paramter when using our own pool (854 bytes, patch)
2013-12-16 21:48 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2object: Ensure max is not smaller then min in decide_allocation (813 bytes, patch)
2013-12-16 21:48 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2object: Validate returned dimensions (1.34 KB, patch)
2013-12-16 21:48 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2bufferpool: On warn on size change if n_planes > 1 (1.07 KB, patch)
2013-12-16 21:49 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2object: Ask for a decent buffer size when dealing with encoded formats (1.59 KB, patch)
2013-12-16 21:49 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2object: Add MPEG1/2 support (3.10 KB, patch)
2013-12-16 21:49 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2object: _v4l2fourcc_to_structure() can be static (1.33 KB, patch)
2013-12-16 21:49 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
[PATCH] v4l2object: Don't force framerate field for OUTPUT (1.50 KB, patch)
2013-12-16 21:49 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
v4l2object: Quirks for dev without initial format (3.02 KB, patch)
2013-12-30 12:08 UTC, Julien Isorce
committed Details | Review
[PATCH] v4l2object: Don't validate dimension for encoded format (2.45 KB, patch)
2013-12-30 12:11 UTC, Julien Isorce
committed Details | Review
[PATCH] v4l2object: Add vp8 support (1.57 KB, patch)
2013-12-30 15:45 UTC, Julien Isorce
committed Details | Review
[PATCH] v4l2bufferpool: add gst_v4l2_buffer_pool_flush (4.73 KB, patch)
2013-12-30 15:46 UTC, Julien Isorce
committed Details | Review
[PATCH] v4l2object: unref downstream pool (818 bytes, patch)
2013-12-30 15:47 UTC, Julien Isorce
committed Details | Review
[PATCH] v4l2object: set only one plane for encoded format (1.18 KB, patch)
2013-12-31 15:59 UTC, Julien Isorce
committed Details | Review
v4l2object: check if translated format is valid (1.84 KB, patch)
2013-12-31 16:48 UTC, Julien Isorce
committed Details | Review
[PATCH] v4l2object: Add V4L2_CID_MIN_BUFFERS_FOR_CAPTURE support (1.22 KB, patch)
2013-12-31 17:14 UTC, Julien Isorce
committed Details | Review

Description Nicolas Dufresne (ndufresne) 2013-12-16 21:46:52 UTC
In order to introduce support for M2M decoders, I endup having to rework a lot of code in the v4l2 code base. As this patchset is growing, I decided to bring a first lot of patches to review. These patches includes some refactoring, new helpers functions and cleanup. There is also important change in the buffer pool to allow better handling of negotiation. This code is tested on top of a working, but yet incomplete decoder, but I'd like to start the review process as soon as possible.

(Patches will follow)
Comment 1 Nicolas Dufresne (ndufresne) 2013-12-16 21:47:23 UTC
Created attachment 264335 [details] [review]
[PATCH] v4l2object: Split caps in different categories


This is need to correctly expose capabilities on specialized devices
like decoders and encoders.
---
 sys/v4l2/gstv4l2object.c | 154 +++++++++++++++++++++++++++++------------------
 sys/v4l2/gstv4l2object.h |   4 ++
 2 files changed, 101 insertions(+), 57 deletions(-)
Comment 2 Nicolas Dufresne (ndufresne) 2013-12-16 21:47:27 UTC
Created attachment 264336 [details] [review]
[PATCH] v4l2: Fix h264 caps


V4L2_PIX_FMT_H264 is documentated as byte-stream (with start code). The ensure proper
negotiation with element like h264parse.
---
 sys/v4l2/gstv4l2object.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Comment 3 Nicolas Dufresne (ndufresne) 2013-12-16 21:47:30 UTC
Created attachment 264337 [details] [review]
[PATCH] v4l2: better handle quirks activation


This way we can activate deactivate those quirks all at once at one
place.
---
 sys/v4l2/gstv4l2object.c | 6 +-----
 sys/v4l2/gstv4l2object.h | 4 ++++
 sys/v4l2/v4l2_calls.c    | 7 +++++++
 3 files changed, 12 insertions(+), 5 deletions(-)
Comment 4 Nicolas Dufresne (ndufresne) 2013-12-16 21:47:34 UTC
Created attachment 264338 [details] [review]
[PATCH] v4l2object: make IO_MODE enum public


This is to allow adding a second io-mode property on M2M device like decoder so
input and output can be controlled separatly.
---
 sys/v4l2/gstv4l2object.c | 3 +--
 sys/v4l2/gstv4l2object.h | 3 +++
 2 files changed, 4 insertions(+), 2 deletions(-)
Comment 5 Nicolas Dufresne (ndufresne) 2013-12-16 21:47:38 UTC
Created attachment 264339 [details] [review]
[PATCH] v4l2object: Implement gst_v4l2_dup()


This will duplicated the FD from another object and copy over the probed result.
---
 sys/v4l2/v4l2_calls.c | 111 ++++++++++++++++++++++++++++++++++++++------------
 sys/v4l2/v4l2_calls.h |   1 +
 2 files changed, 86 insertions(+), 26 deletions(-)
Comment 6 Nicolas Dufresne (ndufresne) 2013-12-16 21:47:42 UTC
Created attachment 264340 [details] [review]
[PATCH] v4l2object: Add gst_v4l2_object_open_shared()

 sys/v4l2/gstv4l2object.c | 14 ++++++++++++++
 sys/v4l2/gstv4l2object.h |  1 +
 2 files changed, 15 insertions(+)
Comment 7 Nicolas Dufresne (ndufresne) 2013-12-16 21:47:46 UTC
Created attachment 264341 [details] [review]
[PATCH] v4l2object: Quirks for dev without initial format


Most M2M have undefined behaviour initially when VIDIOC_G_FMT is called.
---
 sys/v4l2/gstv4l2object.c | 18 +++++++++++++-----
 sys/v4l2/gstv4l2object.h |  2 ++
 2 files changed, 15 insertions(+), 5 deletions(-)
Comment 8 Nicolas Dufresne (ndufresne) 2013-12-16 21:47:50 UTC
Created attachment 264342 [details] [review]
[PATCH] v4l2object: Don't validate dimension for encoded format


Decoder don't care about the dimension when we set format as this information is
generally in the stream.
---
 sys/v4l2/gstv4l2object.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)
Comment 9 Nicolas Dufresne (ndufresne) 2013-12-16 21:47:53 UTC
Created attachment 264343 [details] [review]
[PATCH] v4l2bufferpool: Request buffers only once


VIDIOC_REQBUFS allocates buffer, it has no place inside set_config. Also, some driver do
no allow multiple calls to this ioctl.
---
 sys/v4l2/gstv4l2bufferpool.c | 180 ++++++++++++++++++++++++-------------------
 1 file changed, 101 insertions(+), 79 deletions(-)
Comment 10 Nicolas Dufresne (ndufresne) 2013-12-16 21:47:58 UTC
Created attachment 264344 [details] [review]
[PATCH] v4l2object: Ensure we assumes at least 1 plane

 sys/v4l2/gstv4l2object.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 11 Nicolas Dufresne (ndufresne) 2013-12-16 21:48:02 UTC
Created attachment 264345 [details] [review]
[PATCH] v4l2object: Split _v4l2fourcc_to_video_format

 sys/v4l2/gstv4l2object.c | 153 +++++++++++++++++++++++++----------------------
 sys/v4l2/gstv4l2object.h |   1 +
 2 files changed, 83 insertions(+), 71 deletions(-)
Comment 12 Nicolas Dufresne (ndufresne) 2013-12-16 21:48:06 UTC
Created attachment 264346 [details] [review]
[PATCH] v4l2object: Move mplane logic into gst_v4l2_object_get_caps_info()


It makes the gst_v4l2_object_set_format() slightly simplier and will make that
logic reusable. Note that gst_v4l2_object_has_mplane() will always return the
same value for one device. There is no need to check against the caps as this
has already been done by _open.
---
 sys/v4l2/gstv4l2object.c | 95 ++++++++++++++++++++++--------------------------
 1 file changed, 44 insertions(+), 51 deletions(-)
Comment 13 Nicolas Dufresne (ndufresne) 2013-12-16 21:48:09 UTC
Created attachment 264347 [details] [review]
[PATCH] v4l2object: Move back assertions where they should be

 sys/v4l2/gstv4l2object.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Comment 14 Nicolas Dufresne (ndufresne) 2013-12-16 21:48:14 UTC
Created attachment 264348 [details] [review]
[PATCH] v4l2object: Move the extrapolation of stride at the right place


Now that we have a stride array, we should extrapolate only when
eeded (non multi-planar buffer).
---
 sys/v4l2/gstv4l2bufferpool.c | 21 +++------------------
 sys/v4l2/gstv4l2object.c     | 41 ++++++++++++++++++++++++++---------------
 2 files changed, 29 insertions(+), 33 deletions(-)
Comment 15 Nicolas Dufresne (ndufresne) 2013-12-16 21:48:17 UTC
Created attachment 264349 [details] [review]
[PATCH] v4l2object: Don't check format specific information


The number of plane, and the stride does not represent a capability change. Same caps
can have different stride from the default GstVideoInfo and the number of planes will
never change for 1 format.
---
 sys/v4l2/gstv4l2object.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
Comment 16 Nicolas Dufresne (ndufresne) 2013-12-16 21:48:21 UTC
Created attachment 264350 [details] [review]
[PATCH] v4l2object: Move code block where it belongs

 sys/v4l2/gstv4l2object.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)
Comment 17 Nicolas Dufresne (ndufresne) 2013-12-16 21:48:25 UTC
Created attachment 264351 [details] [review]
[PATCH] v4l2object: Split out saving format from set_format()

 sys/v4l2/gstv4l2object.c | 103 +++++++++++++++++++++++++++++------------------
 1 file changed, 63 insertions(+), 40 deletions(-)
Comment 18 Nicolas Dufresne (ndufresne) 2013-12-16 21:48:29 UTC
Created attachment 264352 [details] [review]
[PATCH] v4l2object: Implement _setup_format()


This method allow setting up the object from the currently configured format on the
device. This is useful for M2M element where input data decides the format that will
be set on capture side.
---
 sys/v4l2/gstv4l2object.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++
 sys/v4l2/gstv4l2object.h |  4 ++
 2 files changed, 100 insertions(+)
Comment 19 Nicolas Dufresne (ndufresne) 2013-12-16 21:48:33 UTC
Created attachment 264353 [details] [review]
[PATCH] v4l2: Move decide allocation into v4l2object

 sys/v4l2/gstv4l2object.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++
 sys/v4l2/gstv4l2object.h |   2 +
 sys/v4l2/gstv4l2src.c    |  91 +++---------------------------------------
 3 files changed, 108 insertions(+), 86 deletions(-)
Comment 20 Nicolas Dufresne (ndufresne) 2013-12-16 21:48:38 UTC
Created attachment 264354 [details] [review]
[PATCH] v4l2object: Allocate pool if needed in decide_allocation

 sys/v4l2/gstv4l2object.c | 8 ++++++++
 1 file changed, 8 insertions(+)
Comment 21 Nicolas Dufresne (ndufresne) 2013-12-16 21:48:42 UTC
Created attachment 264355 [details] [review]
[PATCH] v4l2object: Add V4L2_CID_MIN_BUFFERS_FOR_CAPTURE support

 sys/v4l2/gstv4l2object.c | 9 +++++++++
 1 file changed, 9 insertions(+)
Comment 22 Nicolas Dufresne (ndufresne) 2013-12-16 21:48:46 UTC
Created attachment 264356 [details] [review]
[PATCH] v4l2bufferpool: Respect the suggested min buffer

 sys/v4l2/gstv4l2bufferpool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 23 Nicolas Dufresne (ndufresne) 2013-12-16 21:48:50 UTC
Created attachment 264357 [details] [review]
[PATCH] v4l2object: Don't keep the max paramter when using our own pool

 sys/v4l2/gstv4l2object.c | 1 +
 1 file changed, 1 insertion(+)
Comment 24 Nicolas Dufresne (ndufresne) 2013-12-16 21:48:54 UTC
Created attachment 264358 [details] [review]
[PATCH] v4l2object: Ensure max is not smaller then min in decide_allocation

 sys/v4l2/gstv4l2object.c | 4 ++++
 1 file changed, 4 insertions(+)
Comment 25 Nicolas Dufresne (ndufresne) 2013-12-16 21:48:58 UTC
Created attachment 264359 [details] [review]
[PATCH] v4l2object: Validate returned dimensions

 sys/v4l2/gstv4l2object.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
Comment 26 Nicolas Dufresne (ndufresne) 2013-12-16 21:49:02 UTC
Created attachment 264360 [details] [review]
[PATCH] v4l2bufferpool: On warn on size change if n_planes > 1

 sys/v4l2/gstv4l2bufferpool.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Comment 27 Nicolas Dufresne (ndufresne) 2013-12-16 21:49:06 UTC
Created attachment 264361 [details] [review]
[PATCH] v4l2object: Ask for a decent buffer size when dealing with encoded formats

 sys/v4l2/gstv4l2object.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
Comment 28 Nicolas Dufresne (ndufresne) 2013-12-16 21:49:11 UTC
Created attachment 264362 [details] [review]
[PATCH] v4l2object: Add MPEG1/2 support

 sys/v4l2/gstv4l2object.c | 42 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 7 deletions(-)
Comment 29 Nicolas Dufresne (ndufresne) 2013-12-16 21:49:16 UTC
Created attachment 264363 [details] [review]
[PATCH] v4l2object: _v4l2fourcc_to_structure() can be static


This function is not used anymore outside v4l2object.
---
 sys/v4l2/gstv4l2object.c | 2 +-
 sys/v4l2/gstv4l2object.h | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)
Comment 30 Nicolas Dufresne (ndufresne) 2013-12-16 21:49:20 UTC
Created attachment 264364 [details] [review]
[PATCH] v4l2object: Don't force framerate field for OUTPUT


If there is nothing that seems to force a certain framerate on output device, it is
preferable to simply not set that feild. This allow negotiation with tsdemux in a
decoder for example.
---
 sys/v4l2/gstv4l2object.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Comment 31 Sebastian Dröge (slomo) 2013-12-17 09:01:09 UTC
Comment on attachment 264341 [details] [review]
[PATCH] v4l2object: Quirks for dev without initial format

Add some more info to the comment in the header what this is about and why it is needed
Comment 32 Sebastian Dröge (slomo) 2013-12-17 09:03:49 UTC
Comment on attachment 264342 [details] [review]
[PATCH] v4l2object: Don't validate dimension for encoded format

Add some comments for this too, especially mention that we set the dimensions just in case but don't validate them afterwards. For some codecs the dimensions are *not* in the bitstream, IIRC VC1 in ASF mode for example.
Comment 33 Sebastian Dröge (slomo) 2013-12-17 15:16:32 UTC
(In reply to comment #8)
> Created an attachment (id=264342) [details] [review]
> [PATCH] v4l2object: Don't validate dimension for encoded format
> 
> 
> Decoder don't care about the dimension when we set format as this information
> is
> generally in the stream.
> ---
>  sys/v4l2/gstv4l2object.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)

Also see bug #604434 about this. I think this part of the code should in general be improved a bit
Comment 34 Julien Isorce 2013-12-30 12:08:59 UTC
Created attachment 265028 [details] [review]
v4l2object: Quirks for dev without initial format

add comments
Comment 35 Julien Isorce 2013-12-30 12:11:54 UTC
Created attachment 265029 [details] [review]
[PATCH] v4l2object: Don't validate dimension for encoded format

add comments
Comment 36 Julien Isorce 2013-12-30 15:45:19 UTC
Created attachment 265041 [details] [review]
[PATCH] v4l2object: Add vp8 support
Comment 37 Julien Isorce 2013-12-30 15:46:17 UTC
Created attachment 265042 [details] [review]
[PATCH] v4l2bufferpool: add gst_v4l2_buffer_pool_flush
Comment 38 Julien Isorce 2013-12-30 15:47:23 UTC
Created attachment 265043 [details] [review]
[PATCH] v4l2object: unref downstream pool
Comment 39 Sebastian Dröge (slomo) 2013-12-31 09:18:00 UTC
Comment on attachment 264362 [details] [review]
[PATCH] v4l2object: Add MPEG1/2 support

parsed=true maybe (for sinks and decoders, not for sources and encoders)? What about codec_data?
Comment 40 Sebastian Dröge (slomo) 2013-12-31 09:19:04 UTC
Comment on attachment 264361 [details] [review]
[PATCH] v4l2object: Ask for a decent buffer size when dealing with encoded formats

Which size would it use by default otherwise? Maybe the devices will give use reasonable values already and values that work better with this specific device than 1MB?
Comment 41 Sebastian Dröge (slomo) 2013-12-31 09:21:19 UTC
Comment on attachment 264355 [details] [review]
[PATCH] v4l2object: Add V4L2_CID_MIN_BUFFERS_FOR_CAPTURE support

This diff does not make any sense, the V4L2_CID_MIN_BUFFERS_FOR_CAPTURE usage is already elsewhere and this one only adds the declaration of the ctl variable and the min variable setting.

Otherwise this looks correct
Comment 42 Sebastian Dröge (slomo) 2013-12-31 09:24:35 UTC
Comment on attachment 264359 [details] [review]
[PATCH] v4l2object: Validate returned dimensions

Is this maybe valid for encoded streams?
Comment 43 Sebastian Dröge (slomo) 2013-12-31 09:26:40 UTC
Comment on attachment 265029 [details] [review]
[PATCH] v4l2object: Don't validate dimension for encoded format

Also please consider fixing bug #604434 while you're at it
Comment 44 Sebastian Dröge (slomo) 2013-12-31 09:30:55 UTC
Comment on attachment 264343 [details] [review]
[PATCH] v4l2bufferpool: Request buffers only once

Isn't it potentially necessary to release and re-request/reallocate all buffers when the config changes?
Comment 45 Sebastian Dröge (slomo) 2013-12-31 09:31:48 UTC
Comment on attachment 264344 [details] [review]
[PATCH] v4l2object: Ensure we assumes at least 1 plane

Shouldn't we instead error out if this is <= 0? The broken value must come from somewhere and most likely things are not as they should at this point then.
Comment 46 Sebastian Dröge (slomo) 2013-12-31 09:36:13 UTC
Comment on attachment 264351 [details] [review]
[PATCH] v4l2object: Split out saving format from set_format()

The duration calculation should probably use the rounding variant of uint64_scale_int(). Also how is this supposed to work, it will cause accumulating rounding errors? Not a bug in this patch but looks wrong
Comment 47 Julien Isorce 2013-12-31 11:15:49 UTC
(In reply to comment #44)
> (From update of attachment 264343 [details] [review])
> Isn't it potentially necessary to release and re-request/reallocate all buffers
> when the config changes?

Now pool->buffers[] allocation and VIDIOC_REQBUFS call (with .count != 0) are done in pool_start() instead of pool_set_config().

pool_stop() is left unchanged, i.e. it releases pool->buffers[] and call VIDIOC_REQBUFS with .count equal to 0.

So that now it's more symmetric, and now pool_set_config() allocates nothing. So that it can be called several times with few managements.
As it's not possible to change the config when the pool is active (pool_start() succeeded), you have to have inactivate it first (so pool_stop() will be called).
Comment 48 Julien Isorce 2013-12-31 15:59:45 UTC
Created attachment 265075 [details] [review]
[PATCH] v4l2object: set only one plane for encoded format

(In reply to comment #45)
> (From update of attachment 264344 [details] [review])
> Shouldn't we instead error out if this is <= 0? The broken value must come from
> somewhere and most likely things are not as they should at this point then.

You right there was a mistake, we are asking for a number of planes equal to GST_VIDEO_INFO_N_PLANES but this returns 0 for encoded format. So just ask for 1 plane in this case.
Comment 49 Julien Isorce 2013-12-31 16:48:29 UTC
Created attachment 265077 [details] [review]
 v4l2object: check if translated format is valid

(In reply to comment #42)
> (From update of attachment 264359 [details] [review])
> Is this maybe valid for encoded streams?

Yup this is not valid so I added a patch to at least check the return value. Also a FIXME for encoded case because it requires more work and mainly an encoder device to test it.
Comment 50 Julien Isorce 2013-12-31 17:14:09 UTC
Created attachment 265079 [details] [review]
[PATCH] v4l2object: Add V4L2_CID_MIN_BUFFERS_FOR_CAPTURE support

(In reply to comment #41)
> (From update of attachment 264355 [details] [review])
> This diff does not make any sense, the V4L2_CID_MIN_BUFFERS_FOR_CAPTURE usage
> is already elsewhere and this one only adds the declaration of the ctl variable
> and the min variable setting.
> 
> Otherwise this looks correct

Indeed :)
Comment 51 Julien Isorce 2013-12-31 17:18:20 UTC
Just for note, the patchs are maintained here: http://cgit.collabora.com/git/user/julien/gst-plugins-good.git/log/?h=v4l2videodec-all
Comment 52 Nicolas Dufresne (ndufresne) 2014-01-06 14:39:18 UTC
(In reply to comment #39)
> (From update of attachment 264362 [details] [review])
> parsed=true maybe (for sinks and decoders, not for sources and encoders)? What
> about codec_data?

Good point, we will reduce the scope when adding parsed=. Not sure about codec_data, I don't remember negotiating these ? Though for codec_data mechanics I'm cannot guarranty it's 100% functionnal since the HW we work on don't use it so far.
Comment 53 Nicolas Dufresne (ndufresne) 2014-01-06 14:46:49 UTC
(In reply to comment #40)
> (From update of attachment 264361 [details] [review])
> Which size would it use by default otherwise? Maybe the devices will give use
> reasonable values already and values that work better with this specific device
> than 1MB?

Currently the drivers sets this size to about 200kb, regardless of the input. V4L2 documentation does not say anything about it. We should probably ask Benjamin to see if other driver do something smarter. In the end we probably need some minimum base on the codec + video size.
Comment 54 Sebastian Dröge (slomo) 2014-01-06 17:11:58 UTC
(In reply to comment #52)
> (In reply to comment #39)
> > (From update of attachment 264362 [details] [review] [details])
> > parsed=true maybe (for sinks and decoders, not for sources and encoders)? What
> > about codec_data?
> 
> Good point, we will reduce the scope when adding parsed=. Not sure about
> codec_data, I don't remember negotiating these ? Though for codec_data
> mechanics I'm cannot guarranty it's 100% functionnal since the HW we work on
> don't use it so far.

Right, codec_data can't be negotiated. You should just fail if it's not there but needed :) parsed=true and/or stream-format and/or alignment in the caps should be a good idea though. You don't handle unparsed streams properly most likely
Comment 55 Nicolas Dufresne (ndufresne) 2014-01-07 20:50:29 UTC
(In reply to comment #46)
> (From update of attachment 264351 [details] [review])
> The duration calculation should probably use the rounding variant of
> uint64_scale_int(). Also how is this supposed to work, it will cause
> accumulating rounding errors? Not a bug in this patch but looks wrong

This value is set at the buffer duration and for control bindings, same math is used for latency calculation. We know that UVC camera lies on the framerate, we can't really do anything about that. For latency we probably want a roundup version, For buffer duration it will be wrong anyway, so unless we find another solution there is nothing to do. For the controls bindings, I think we should simply do this:

-      v4l2src->ctrl_time += v4l2src->duration;
+      v4l2src->ctrl_time = timestamp + v4l2src->duration;

But clearly this bug should be solved separately.
Comment 56 Nicolas Dufresne (ndufresne) 2014-01-07 21:03:46 UTC
(In reply to comment #42)
> (From update of attachment 264359 [details] [review])
> Is this maybe valid for encoded streams?

gst_v4l2_object_setup_format() is for when we let the driver decides the format. It's currently only tested for decoders, which decides a raw format. I've added this check to catch earlier a race in the driver I'm working on. I think it's good enough, I could probably also check that the format is raw, as reported by Micheal, his decoder does not make any decision and report an encoded default format. I would suggest to revisit these check later if we have more use cases and a way to test them.
Comment 57 Nicolas Dufresne (ndufresne) 2014-01-07 21:15:28 UTC
(In reply to comment #52)
> (In reply to comment #39)
> > (From update of attachment 264362 [details] [review] [details])
> > parsed=true maybe (for sinks and decoders, not for sources and encoders)? What
> > about codec_data?
> 
> Good point, we will reduce the scope when adding parsed=. Not sure about
> codec_data, I don't remember negotiating these ? Though for codec_data
> mechanics I'm cannot guarranty it's 100% functionnal since the HW we work on
> don't use it so far.

Sorry, I was confused. I do add parsed=True, but it's being done in the decoder code (should be submitted soon. I agree it might also be useful for sink the decoders, though I don't own such a device myself. We could probably add this field more generically as you describe, though I think the current approach is valid. Let me know.
Comment 58 Sebastian Dröge (slomo) 2014-01-08 08:43:29 UTC
Ok for comment 55 and comment 56 (I assume that function is not used for sources or sinks?)

For comment 57, I would add parsed=TRUE for sinks too. Can be a separate patch but generalize the caps generation please.
Comment 59 Víctor Manuel Jáquez Leal 2014-01-08 08:50:23 UTC
Review of attachment 264339 [details] [review]:

::: sys/v4l2/v4l2_calls.c
@@ +673,3 @@
+
+  g_free (v4l2object->videodev);
+  v4l2object->videodev = g_strdup (v4l2object->videodev);

nitpick: assigning to a freed string, this should be

v4l2object->videodev = g_strdup (other->videodev);
Comment 60 Nicolas Dufresne (ndufresne) 2014-01-10 21:59:34 UTC
(In reply to comment #59)
> Review of attachment 264339 [details] [review]:
> 
> ::: sys/v4l2/v4l2_calls.c
> @@ +673,3 @@
> +
> +  g_free (v4l2object->videodev);
> +  v4l2object->videodev = g_strdup (v4l2object->videodev);
> 
> nitpick: assigning to a freed string, this should be
> 
> v4l2object->videodev = g_strdup (other->videodev);

Thanks, fixed in my branch.
Comment 61 Nicolas Dufresne (ndufresne) 2014-01-10 22:05:37 UTC
*** Bug 719779 has been marked as a duplicate of this bug. ***
Comment 62 Nicolas Dufresne (ndufresne) 2014-01-10 22:44:41 UTC
commit 55da3bc8850c0af826067ac31ae42b13681b4acf
Author: Julien Isorce <julien.isorce@collabora.co.uk>
Date:   Tue Jan 7 11:58:23 2014 +0000

    v4l2bufferpool: check set_config return value in gst_v4l2_buffer_pool_new
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit b39c838eceb32e76323c04040048ab298817a276
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Fri Jan 10 12:40:31 2014 -0500

    v4l2object: Add parsed=1 field for encoded output
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 5be7d39a3f53671184818237d90cecb6a076085d
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Fri Jan 10 12:39:16 2014 -0500

    v4l2object: Don't leak empty caps
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit a54b34121f77f809642f9e24be9d8777eaadf09f
Author: Julien Isorce <julien.isorce@collabora.co.uk>
Date:   Wed Jan 8 16:51:21 2014 +0000

    v4l2bufferpool: do not stop a stream not previously started
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 986e5b460d5174db22d238248485c7559203a1cf
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Dec 12 16:27:21 2013 -0500

    v4l2object: Don't enforce dimension field on encoded formats
    
    Don't enforce having width, height and framerate in template caps for encoded
    formats. These don't always need to be exposed and may break negotiation for
    decoder and decoding sink. If needed, these field will be automatically added
    when probed caps are known.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit ba81eeb16bea849ab0d1383de2376f38c0e6bb54
Author: Julien Isorce <julien.isorce@collabora.co.uk>
Date:   Thu Dec 12 17:09:59 2013 +0000

    v4l2object: unref downstream pool
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit c701dcd16c2d4eb4ddc05f62897ea5d55ff6a558
Author: Julien Isorce <julien.isorce@collabora.co.uk>
Date:   Wed Dec 18 13:37:23 2013 -0500

    v4l2bufferpool: add gst_v4l2_buffer_pool_flush
    
    STREAMOFF set all v4l2buffers to DEQUEUE state.
    
    Then for CAPTURE we call QBUF on each buffer.
    For OUTPUT the buffers are just push back in the GstBufferPool
    base class 's queue.
    But the loop actually looks like the same.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 062f4f87107c7870dc2dce021c1fb8117c98923d
Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Date:   Mon Dec 16 17:29:30 2013 -0500

    v4l2object: Add vp8 support
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit e01766664522aa10f0d7ca4ecc780010ef49784d
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Dec 12 16:46:09 2013 -0500

    v4l2object: Don't force framerate field for OUTPUT
    
    If there is nothing that seems to force a certain framerate on output device, it is
    preferable to simply not set that feild. This allow negotiation with tsdemux in a
    decoder for example.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 97cf8f4d170767f4f83ee24d19572cee7192c9b6
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Dec 12 14:07:03 2013 -0500

    v4l2object: _v4l2fourcc_to_structure() can be static
    
    This function is not used anymore outside v4l2object.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit fdd7bcc78c8ba97f482c259ef5aae81efafa824b
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Dec 12 14:22:26 2013 -0500

    v4l2object: Add MPEG1/2 support
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 0c8ec43a121ab35392a45d7f0dc5bb481aeca8ad
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Dec 12 12:18:45 2013 -0500

    v4l2object: Ask for a decent buffer size when dealing with encoded formats
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 86646ce03b3dcff6dce2617ee51598c92aa428b1
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Sat Dec 7 14:03:53 2013 -0500

    v4l2bufferpool: On warn on size change if n_planes > 1
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 79f2c06883701dcdfeb4f4fdf92d504973caf3aa
Author: Julien Isorce <julien.isorce@collabora.co.uk>
Date:   Tue Dec 31 16:38:09 2013 +0000

    v4l2object: check if translated format is valid
    
    Also add a FIXME in gst_v4l2_object_setup_format
    to note that the whole function has to be improved
    in order to support ENCODED formats.
    It requires to have an encoder device which we do not
    have right now.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 520dda092a0c6aaf749bca32499ee38220cc8812
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Sat Dec 7 10:31:15 2013 -0500

    v4l2object: Validate returned dimensions
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 61ce7b1a62795c438f41e0a02dbeb7ea555abda4
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Dec 5 19:36:25 2013 -0500

    v4l2object: Ensure max is not smaller then min in decide_allocation
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit b261287745e92d2387039d7934961cbfe1e3037c
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Dec 5 19:36:06 2013 -0500

    v4l2object: Don't keep the max paramter when using our own pool
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 994c5d2c936cfbc79e6c24bdb47318303edc5918
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Dec 5 19:34:44 2013 -0500

    v4l2bufferpool: Respect the suggested min buffer
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit a77d2a64c1f2251fb8afcd041c7d37815fcb3ed1
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Dec 5 18:48:44 2013 -0500

    v4l2object: Allocate pool if needed in decide_allocation
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit d79eea15fa958783bddabe584b6a2b113098d816
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Dec 5 18:49:19 2013 -0500

    v4l2object: Add V4L2_CID_MIN_BUFFERS_FOR_CAPTURE support
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 3cf85c9083291d89eace8a3968e79043bd687024
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Dec 5 18:48:15 2013 -0500

    v4l2: Move decide allocation into v4l2object
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 34a8cb09c82867a8d9a4d0a2f46d1160664dff68
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Dec 5 13:51:13 2013 -0500

    v4l2object: Implement _setup_format()
    
    This method allow setting up the object from the currently configured format on the
    device. This is useful for M2M element where input data decides the format that will
    be set on capture side.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 41c37a2c6c4568c419c6c9e575c9ddcad01505f9
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Tue Dec 10 14:34:17 2013 -0500

    v4l2object: Split out saving format from set_format()
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 582f91366781117d6554e8df2ba06432d29e8bec
Author: Julien Isorce <julien.isorce@collabora.co.uk>
Date:   Tue Dec 31 15:37:26 2013 +0000

    v4l2object: set only one plane for encoded format
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit e2c32ec99750ca6e2a0bce3e1153d772588ca8aa
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Wed Dec 4 16:49:13 2013 -0500

    v4l2object: Move code block where it belongs
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 689672ef17d7ee41c079b11be5f8c5acc426791f
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Wed Dec 4 16:26:12 2013 -0500

    v4l2object: Don't check format specific information
    
    The number of plane, and the stride does not represent a capability change. Same caps
    can have different stride from the default GstVideoInfo and the number of planes will
    never change for 1 format.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 5faa20b044f2f4ca2206c0d0eaa8fc8a65ca5a4d
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Wed Dec 4 16:23:18 2013 -0500

    v4l2object: Move the extrapolation of stride at the right place
    
    Now that we have a stride array, we should extrapolate only when
    eeded (non multi-planar buffer).
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 5f1e47da246d48feff9ec4811d9a45aaebce7353
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Wed Dec 4 15:09:44 2013 -0500

    v4l2object: Move back assertions where they should be
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit a64de44a0cf3b99285851bae02e1f9e7f5093050
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Wed Dec 4 15:09:10 2013 -0500

    v4l2object: Move mplane logic into gst_v4l2_object_get_caps_info()
    
    It makes the gst_v4l2_object_set_format() slightly simplier and will make that
    logic reusable. Note that gst_v4l2_object_has_mplane() will always return the
    same value for one device. There is no need to check against the caps as this
    has already been done by _open.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 4956b46bab4c896d0ba5bf1264071cdc94506318
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Tue Dec 3 18:27:47 2013 -0500

    v4l2object: Split _v4l2fourcc_to_video_format
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit e1be685c34255e9da761bc265aff9cc98406d674
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Mon Dec 2 18:05:11 2013 -0500

    v4l2bufferpool: Request buffers only once
    
    VIDIOC_REQBUFS allocates buffer, it has no place inside set_config. Also, some driver do
    no allow multiple calls to this ioctl.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit b80169a16abb8526e5d766fa3e7cc04c6667db3e
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Mon Dec 2 15:26:50 2013 -0500

    v4l2object: Don't validate dimension for encoded format
    
    We set the dimensions just in case but don't validate them
    afterwards. For some codecs the dimensions are *not* in the
    bitstream, IIRC VC1 in ASF mode for example.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 4a85f90c7949e5ef0ec1dfd3ac502cba8d5b3aa7
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Nov 28 17:10:29 2013 -0500

    v4l2object: Quirks for dev without initial format
    
    Most M2M have undefined behaviour initially when VIDIOC_G_FMT is called.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 080f58166e6f14e10cd9c2680a58aaa326c09efb
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Nov 28 17:09:26 2013 -0500

    v4l2object: Add gst_v4l2_object_open_shared()
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit cf32d6ec4344ead1da7f56373b5805c5477e7ded
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Nov 28 17:07:05 2013 -0500

    v4l2object: Implement gst_v4l2_dup()
    
    This will duplicated the FD from another object and copy over the probed result.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 7fd6dc08b975233c61c8f84b025f4b5f4c7edf22
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Nov 28 16:59:59 2013 -0500

    v4l2object: make IO_MODE enum public
    
    This is to allow adding a second io-mode property on M2M device like decoder so
    input and output can be controlled separatly.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 203e2451346d1161d95ee318479fd8e1ea892ab1
Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk>
Date:   Tue Jun 4 23:42:24 2013 -0400

    v4l2: better handle quirks activation
    
    This way we can activate deactivate those quirks all at once at one
    place.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit 092e7605d3b534510291a4dbf3ff9930afa756b3
Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk>
Date:   Tue Jun 4 23:34:04 2013 -0400

    v4l2: Fix h264 caps
    
    V4L2_PIX_FMT_H264 is documentated as byte-stream (with start code). The ensure proper
    negotiation with element like h264parse.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568

commit e821de34940a529d920267e6249a668b31b3c4c9
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Fri Dec 6 14:44:51 2013 -0500

    v4l2object: Split caps in different categories
    
    This is need to correctly expose capabilities on specialized devices
    like decoders and encoders.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=720568
Comment 63 Nicolas Dufresne (ndufresne) 2014-01-10 22:55:29 UTC
Everything is in now, sorry for the spam. If that breaks anything let me know, we'll track case-by-case in separate bug report.