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 730200 - v4l2videodec: v4l2bufferpool must be updated according to driver requirement
v4l2videodec: v4l2bufferpool must be updated according to driver requirement
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.x
Other Linux
: Normal normal
: 1.3.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-05-15 15:14 UTC by Benjamin Gaignard
Modified: 2014-05-15 16:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[v4l2bufferpool] update pool min/max according to driver needs (1.57 KB, patch)
2014-05-15 15:41 UTC, Benjamin Gaignard
needs-work Details | Review

Description Benjamin Gaignard 2014-05-15 15:14:14 UTC
v4l2bufferpool min/max buffers must bo updated in case of the driver request more buffers than expected.
In gst_v4l2_buffer_pool_start() the value return by gst_v4l2_allocator_start() could be used to know how many buffers are needed.
Comment 1 Benjamin Gaignard 2014-05-15 15:41:52 UTC
Created attachment 276609 [details] [review]
[v4l2bufferpool] update pool min/max according to driver needs
Comment 2 Nicolas Dufresne (ndufresne) 2014-05-15 16:02:42 UTC
Review of attachment 276609 [details] [review]:

Small changes, but good otherwise.

::: sys/v4l2/gstv4l2bufferpool.c
@@ +670,3 @@
 
+        /* The initial minimum is could be provide either by GstBufferPool or
+         * driver needs. */

Some english typo here.

@@ +671,3 @@
+        /* The initial minimum is could be provide either by GstBufferPool or
+         * driver needs. */
+        min_buffers = MAX (min_buffers, count);

So now, if the driver gives less then expected, it will most likely fail in _start(). Previously it would at least start. This code was not initially expecting the count to be bigger then expected ;-P

I think we can just do that now:

min_buffers = count;

And that would be best effort. Even though there is a risk for buffer stall (in both cases). That's what the CID V4L2_CID_MIN_BUFFERS_FOR_CAPTURE and V4L2_CID_MIN_BUFFERS_FOR_OUTPUT are for.
Comment 3 Nicolas Dufresne (ndufresne) 2014-05-15 16:50:16 UTC
Tested by disabling CID V4L2_CID_MIN_BUFFERS_FOR_CAPTURE on my side. Seems to work for mmap, but fails stalls for dmabuf-import, as it can't having 1 buffer queued downstream (in the converter) is prevents more decoding. Would be the same with the video sink last-sample. But it's know issue, drivers need that CID to operate properly.
Comment 4 Nicolas Dufresne (ndufresne) 2014-05-15 16:58:29 UTC
I've just fixed it:

commit 0d852cf2353632b3d8d8d855fd84fe8eef7c1a62
Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Date:   Thu May 15 17:39:39 2014 +0200

    v4l2bufferpool: Update pool limit with hardware requiremenst
    
    If the driver need more buffers than requested by the config,
    update the pool min/max values. The minimum value for the pool
    could be provided either by the driver or by the pool. This is
    best effort for drivers that don't support
    CID V4L2_CID_MIN_BUFFERS_FOR_CAPTURE.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=730200