GNOME Bugzilla – Bug 736072
v4l2: set min_latency for output device according to required minimum number of buffers
Last modified: 2014-09-09 22:41:20 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.
Created attachment 285402 [details] [review] v4l2: set min_latency for output device according to required minimum number of buffers Proposed patch
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.
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(-)
(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.
(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.
Oh, was slightly confused. Basically all I would like is a single variable, min_buffers.
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.
(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.
(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
(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 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
Thanks for your contribution.