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 736072 - v4l2: set min_latency for output device according to required minimum number of buffers
v4l2: set min_latency for output device according to required minimum number ...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal enhancement
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-09-04 16:57 UTC by Aurélien Zanelli
Modified: 2014-09-09 22:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
v4l2: set min_latency for output device according to required minimum number of buffers (2.52 KB, patch)
2014-09-04 16:58 UTC, Aurélien Zanelli
committed Details | Review
[PATCH] v4l2: Add support for MIN_BUFFERS_FOR_OUTPUT (4.10 KB, patch)
2014-09-04 19:13 UTC, Nicolas Dufresne (ndufresne)
none Details | Review
[PATCH v2] v4l2: Add support for MIN_BUFFERS_FOR_OUTPUT (4.33 KB, patch)
2014-09-05 12:36 UTC, Nicolas Dufresne (ndufresne)
none Details | Review

Description Aurélien Zanelli 2014-09-04 16:57:18 UTC
Since we can get the minimum number of buffers needed by an output device to work, use it to set min_latency which will determine how many buffers are queued.


Device I use need at least 3 buffers to work correctly. Without this patch poo->min_latency is set to GST_V4L2_MIN_BUFFERS == 2.
At allocation time, V4L2 device doesn't complain because we request more than 3 buffers sinc v4l2 bufferpool min_buffers is set to 6.
But when streaming only 2 buffers will be queued at a time since we will dequeue a buffer when pool->num_queued reachs pool->min_latency.
Comment 1 Aurélien Zanelli 2014-09-04 16:58:25 UTC
Created attachment 285402 [details] [review]
v4l2: set min_latency for output device according to required minimum number of buffers

Proposed patch
Comment 2 Nicolas Dufresne (ndufresne) 2014-09-04 19:04:37 UTC
Review of attachment 285402 [details] [review]:

This patch seems outdated, have you look at master/1.4 ?

Although I agree with that this need to be added, the CID is different for the OUTPUT. That same object can't have both, but it means basically the same in both case, so we should rename it to simply, min_buffers.
Comment 3 Nicolas Dufresne (ndufresne) 2014-09-04 19:13:29 UTC
Created attachment 285405 [details] [review]
[PATCH] v4l2: Add support for MIN_BUFFERS_FOR_OUTPUT


This was already there for capture device, now also implement it for
output devices.

https://bugzilla.gnome.org/show_bug.cgi?id=73607
---
 sys/v4l2/gstv4l2bufferpool.c |  3 +--
 sys/v4l2/gstv4l2object.c     | 16 ++++++++++------
 sys/v4l2/gstv4l2object.h     |  4 ++--
 sys/v4l2/gstv4l2videodec.c   |  3 +--
 4 files changed, 14 insertions(+), 12 deletions(-)
Comment 4 Nicolas Dufresne (ndufresne) 2014-09-04 19:21:04 UTC
(In reply to comment #3)
> Created an attachment (id=285405) [details] [review]
> [PATCH] v4l2: Add support for MIN_BUFFERS_FOR_OUTPUT
> 
> 
> This was already there for capture device, now also implement it for
> output devices.
> 
> https://bugzilla.gnome.org/show_bug.cgi?id=73607
> ---
>  sys/v4l2/gstv4l2bufferpool.c |  3 +--
>  sys/v4l2/gstv4l2object.c     | 16 ++++++++++------
>  sys/v4l2/gstv4l2object.h     |  4 ++--
>  sys/v4l2/gstv4l2videodec.c   |  3 +--
>  4 files changed, 14 insertions(+), 12 deletions(-)

Something approximately like this, not highly tested though.

(In reply to comment #0)
> Since we can get the minimum number of buffers needed by an output device to
> work, use it to set min_latency which will determine how many buffers are
> queued.
> 
> Device I use need at least 3 buffers to work correctly. Without this patch
> poo->min_latency is set to GST_V4L2_MIN_BUFFERS == 2.
> At allocation time, V4L2 device doesn't complain because we request more than 3
> buffers sinc v4l2 bufferpool min_buffers is set to 6.
> But when streaming only 2 buffers will be queued at a time since we will
> dequeue a buffer when pool->num_queued reachs pool->min_latency.

This underline some potential other issue, more complex to fix though. I think adding MIN_BUFFER_FOR_OUTPUT support to drivers is the most standard and straighforward approach. But in the long term, if we find too many driver without this CID, we could consider some loop around REQBUFS. So, e.g. we REQUEST N buffers, and the driver gives N+1, we know something wasn't quite right. If min_latency is already set, there is a driver bug, can't really do anything, but if min_latency is 2, We could update min_latency to be N+1, the minimum the driver asked, and do REQBUFS again with the new min_latency + N - 2. As we know we request 2 buffers normally, while the remaining, N - 2 is what downstream needs.
Comment 5 Aurélien Zanelli 2014-09-05 07:52:50 UTC
(In reply to comment #2)
> Review of attachment 285402 [details] [review]:
> 
> This patch seems outdated, have you look at master/1.4 ?
Yes, I wrote it ahead of the master branch.

It is likely intended for a an output device used by v4l2sink rather than a v4l2 filter/decoder, so we wont't trigger the decide_allocation method but only the propose_allocation. So I think your patch proposal in the v4l2sink case.

> Although I agree with that this need to be added, the CID is different for the
> OUTPUT. That same object can't have both, but it means basically the same in
> both case, so we should rename it to simply, min_buffers.
I agree with that.
Comment 6 Nicolas Dufresne (ndufresne) 2014-09-05 12:35:19 UTC
Oh, was slightly confused. Basically all I would like is a single variable, min_buffers.
Comment 7 Nicolas Dufresne (ndufresne) 2014-09-05 12:36:42 UTC
Created attachment 285486 [details] [review]
[PATCH v2] v4l2: Add support for MIN_BUFFERS_FOR_OUTPUT

Something like this. Let me know if that one is right, should be pretty to close to yours now.
Comment 8 Aurélien Zanelli 2014-09-05 12:56:05 UTC
(In reply to comment #7)
> Created an attachment (id=285486) [details] [review]
> [PATCH v2] v4l2: Add support for MIN_BUFFERS_FOR_OUTPUT
> 
> Something like this. Let me know if that one is right, should be pretty to
> close to yours now.

It's fine by me.
Comment 9 Nicolas Dufresne (ndufresne) 2014-09-05 13:02:03 UTC
(In reply to comment #8)
> (In reply to comment #7)
> > Created an attachment (id=285486) [details] [review] [details] [review]
> > [PATCH v2] v4l2: Add support for MIN_BUFFERS_FOR_OUTPUT
> > 
> > Something like this. Let me know if that one is right, should be pretty to
> > close to yours now.
> 
> It's fine by me.

Actually, I'll split this up, I'll merge yours, and then make unique variable, this way you get to keep the credit ;-P
Comment 10 Aurélien Zanelli 2014-09-05 13:15:53 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Created an attachment (id=285486) [details] [review] [details] [review] [details] [review]
> > > [PATCH v2] v4l2: Add support for MIN_BUFFERS_FOR_OUTPUT
> > > 
> > > Something like this. Let me know if that one is right, should be pretty to
> > > close to yours now.
> > 
> > It's fine by me.
> 
> Actually, I'll split this up, I'll merge yours, and then make unique variable,
> this way you get to keep the credit ;-P

As you wish :-)
Comment 11 Nicolas Dufresne (ndufresne) 2014-09-09 22:40:21 UTC
Comment on attachment 285402 [details] [review]
v4l2: set min_latency for output device according to required minimum number of buffers

commit 743c6a447592e841375daca5bd08f71d6ab209bb
Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk>
Date:   Thu Sep 4 15:11:40 2014 -0400

    v4l2: Merge min_buffers_for* variable into one
    
    Reuse the same min_buffers variable for both capture and output, this
    reduce the length of lines and make the code more readable.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=736072

commit 3afec4dd0167e6abd4aa145a043200885acc15f6
Author: Aurélien Zanelli <aurelien.zanelli@parrot.com>
Date:   Thu Sep 4 18:35:46 2014 +0200

    v4l2: set min_latency for output device according to required minimum number of buffers
    
    Since we can get the minimum number of buffers needed by an output
    device to work, use it to set min_latency which will determine how many
    buffers are queued.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=736072
Comment 12 Nicolas Dufresne (ndufresne) 2014-09-09 22:41:02 UTC
Thanks for your contribution.