Re: [PATCH] [media] BZ#84401: Revert "[media] v4l: vb2: Don't return POLLERR during transient buffer underruns"

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Mauro,

On Tuesday 16 September 2014 07:58:12 Mauro Carvalho Chehab wrote:
> Em Tue, 16 Sep 2014 13:15:27 +0300 Laurent Pinchart escreveu:
> > On Tuesday 16 September 2014 07:01:29 Mauro Carvalho Chehab wrote:
> > > Em Tue, 16 Sep 2014 12:09:01 +0300 Laurent Pinchart escreveu:
> > > > On Monday 15 September 2014 21:10:55 Mauro Carvalho Chehab wrote:
> > > > > This reverts commit 9241650d62f79a3da01f1d5e8ebd195083330b75.
> > > > > 
> > > > > The commit 9241650d62f7 was meant to solve an issue with Gstreamer
> > > > > version 0.10 with libv4l 1.2, where a fixup patch for DQBUF exposed
> > > > > a bad behavior ag Gstreamer.
> > > > 
> > > > That's not correct. The patch was created to solve an issue observed
> > > > with the Gstreamer 0.10 v4l2src element accessing the video device
> > > > directly, *without* libv4l.
> > > 
> > > Ok. From the discussions we took yesterday on the thread, I got the
> > > wrong impression from Nicolas comments that this happens only with
> > > gst < 1.4 and libv4l >= 1.2.
> > 
> > My understanding is that recent gst versions worked around the problem,
> > and the above combination of versions might be problematic, but gst 0.10
> > is definitely affected.
> > 
> > > > The V4L2 specification documents poll() as follows.
> > > > 
> > > > "When the application did not call VIDIOC_QBUF or VIDIOC_STREAMON yet
> > > > the poll() function succeeds, but sets the POLLERR flag in the revents
> > > > field."
> > > > 
> > > > The vb2 poll implementation didn't conform with that, as it returned
> > > > POLLERR when the buffer list was empty due to a transient buffer
> > > > underrun, even if both VIDIOC_STREAMON and VIDIOC_QBUF have been
> > > > called.
> > > > 
> > > > The commit thus brought the vb2 poll implementation in line with the
> > > > specification. If we really want to revert it to its broken behaviour,
> > > > then it would be fair to explain this in the revert message,
> > > 
> > > Ok, I'll rewrite the text. We likely want to fix the documentation too,
> > > in order to reflect the way it is.
> > > 
> > > > and I want to know how you propose fixing this properly, as the revert
> > > > really causes issues for userspace.
> > > 
> > > This patch simply broke all VBI applications. So, it should be reverted.
> > > 
> > > From what you're saying, using Gst 0.10 with a kernel before 3.16 and
> > > VB2 was always broken, right?
> > 
> > Correct. And not only gst 0.10, any userspace application that doesn't
> > specifically handles transient buffer underruns will be affected.
> > 
> > vb2 doesn't conform to the V4L2 specification, and I believe the
> > specification is right in this case. Reverting this patch will push the
> > problem to userspace, where all applications will have to handle buffer
> > underruns manually.
> 
> What happens with VB1? How is it solved there?
> 
> I don't generally use gst 0.10, but I don't remember a single error
> report about gst 0.10 and VB1-based drivers.

I haven't tried VB1, but a quick look at the source code shows it to be 
affected as well.

The problem with gst 0.10 is only noticeable when a buffer underrun occurs 
(when you don't requeue buffers fast enough and the queue buffers list becomes 
temporarily empty), so it can very well go unnoticed for a long time.

> > > And with VB1, is it also broken? If so, then this is a Gst 0.10 bug,
> > > and the fix should be a patch for it, or a recommendation to upgrade
> > > to a newer version without such bug.
> > 
> > As explained above, this isn't a gst bug.
> > 
> > > If, otherwise, it works with VB1, then we need to patch VB2 to have
> > > exactly the same behavior as VB1 with that regards, as VBI works
> > > with VB1.
> > 
> > One option would be to have implement a different poll behaviour for VBI
> > and video.
> 
> That would be a nightmare.

I don't like it much either. Hans has posted a proposal to fix the problem an 
hour ago, let's discuss it.

-- 
Regards,

Laurent Pinchart