[PATCH/RFC 0/2] Fix DQBUF behavior for recoverable streaming errors

[PATCH/RFC 0/2] Fix DQBUF behavior for recoverable streaming errors

[Pawel Osciak]

Hello,

during the V4L2 brainstorm meeting in Norway we have concluded that streaming
error handling in dqbuf is lacking a bit and might result in the application
losing video buffers.

V4L2 specification states that DQBUF should set errno to EIO in such cases:


"EIO

VIDIOC_DQBUF failed due to an internal error. Can also indicate temporary
problems like signal loss. Note the driver might dequeue an (empty) buffer
despite returning an error, or even stop capturing."

There is a problem with this though. v4l2-ioctl.c code does not copy back
v4l2_buffer fields to userspace on a failed ioctl invocation, i.e. when
__video_do_ioctl() does not return 0, it jumps over the copy_to_user()
code:

/* ... */
err = __video_do_ioctl(file, cmd, parg);
/* ... */
if (err < 0)
	goto out;
/* ... */
	if (copy_to_user((void __user *)arg, parg, _IOC_SIZE(cmd)))
		err = -EFAULT;
/* ... */
out:


This is fine in general, but in the case of DQBUF errors, the v4l2_buffer
fields are not copied back. Because of that, the application does not have any
means of discovering on which buffer the operation failed. So it cannot reuse
that buffer, even despite the fact that the spec allows such behavior.


This RFC proposes a modification to the DQBUF behavior in cases of internal
(recoverable) errors to allow recovery from such situations.

We propose a new flag for the v4l2_buffer "flags" field, "V4L2_BUF_FLAG_ERROR".
There already exists a "V4L2_BUF_FLAG_DONE" flag, so to support older
applications, the new flag should always be set together with it.

Applications unaware of the new flag would simply display a corrupted frame, but
we believe it is still a better solution than failing altogether. Old EIO
behavior remains so the change is backwards compatible.

I will post relevant V4L2 documentation updates after (if) this change is 
accepted.


This series is rebased onto my recent videobuf clean-up and poll behavior
patches.

The series contains:
[PATCH 1/2] v4l: Add a new ERROR flag for DQBUF after recoverable streaming errors
[PATCH 2/2] v4l: videobuf: Add support for V4L2_BUF_FLAG_ERROR

Best regards
--
Pawel Osciak
Linux Platform Group
Samsung Poland R&D Center

[PATCH 1/2] v4l: Add a new ERROR flag for DQBUF after recoverable streaming errors

[Pawel Osciak]

This flag is to be set together with V4L2_BUF_FLAG_DONE. It is intended
to indicate streaming errors that might have resulted in corrupted video
data in the buffer, but the buffer can still be reused and the streaming
may continue.

Setting this flag and returning 0 is different from returning EIO. The
latter should now indicate more serious (unrecoverable) errors.

This patch also solves a problem with the ioctl handling code in
vl42-ioctl.c, which does not copy buffer identification data back to the
userspace when EIO is returned, so there is no way for applications
to discover on which buffer the operation failed.

Signed-off-by: Pawel Osciak <p.osciak@samsung.com>
---
 include/linux/videodev2.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 3c26560..1ae1568 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -550,6 +550,9 @@ struct v4l2_buffer {
 #define V4L2_BUF_FLAG_KEYFRAME	0x0008	/* Image is a keyframe (I-frame) */
 #define V4L2_BUF_FLAG_PFRAME	0x0010	/* Image is a P-frame */
 #define V4L2_BUF_FLAG_BFRAME	0x0020	/* Image is a B-frame */
+/* Buffer is ready, but the data contained within is corrupted.
+ * Always set together with V4L2_BUF_FLAG_DONE (for backward compatibility). */
+#define V4L2_BUF_FLAG_ERROR	0x0040
 #define V4L2_BUF_FLAG_TIMECODE	0x0100	/* timecode field is valid */
 #define V4L2_BUF_FLAG_INPUT     0x0200  /* input field is valid */
 
-- 
1.7.0.31.g1df487

[PATCH 2/2] v4l: videobuf: Add support for V4L2_BUF_FLAG_ERROR

[Pawel Osciak]

On dqbuf videobuf will now return 0 and an error flag will be set, instead
of returning EIO in case of recoverable streaming errors.
---
 drivers/media/video/videobuf-core.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/videobuf-core.c b/drivers/media/video/videobuf-core.c
index e93672a..aa361d3 100644
--- a/drivers/media/video/videobuf-core.c
+++ b/drivers/media/video/videobuf-core.c
@@ -286,8 +286,10 @@ static void videobuf_status(struct videobuf_queue *q, struct v4l2_buffer *b,
 	case VIDEOBUF_ACTIVE:
 		b->flags |= V4L2_BUF_FLAG_QUEUED;
 		break;
-	case VIDEOBUF_DONE:
 	case VIDEOBUF_ERROR:
+		b->flags |= V4L2_BUF_FLAG_ERROR;
+		/* fall through */
+	case VIDEOBUF_DONE:
 		b->flags |= V4L2_BUF_FLAG_DONE;
 		break;
 	case VIDEOBUF_NEEDS_INIT:
@@ -679,7 +681,6 @@ int videobuf_dqbuf(struct videobuf_queue *q,
 	switch (buf->state) {
 	case VIDEOBUF_ERROR:
 		dprintk(1, "dqbuf: state is error\n");
-		retval = -EIO;
 		CALL(q, sync, q, buf);
 		buf->state = VIDEOBUF_IDLE;
 		break;
-- 
1.7.0.31.g1df487

Re: [PATCH/RFC 0/2] Fix DQBUF behavior for recoverable streaming errors

[Hans Verkuil]

On Wednesday 17 March 2010 15:29:48 Pawel Osciak wrote:
> Hello,
> 
> during the V4L2 brainstorm meeting in Norway we have concluded that streaming
> error handling in dqbuf is lacking a bit and might result in the application
> losing video buffers.
> 
> V4L2 specification states that DQBUF should set errno to EIO in such cases:
> 
> 
> "EIO
> 
> VIDIOC_DQBUF failed due to an internal error. Can also indicate temporary
> problems like signal loss. Note the driver might dequeue an (empty) buffer
> despite returning an error, or even stop capturing."
> 
> There is a problem with this though. v4l2-ioctl.c code does not copy back
> v4l2_buffer fields to userspace on a failed ioctl invocation, i.e. when
> __video_do_ioctl() does not return 0, it jumps over the copy_to_user()
> code:
> 
> /* ... */
> err = __video_do_ioctl(file, cmd, parg);
> /* ... */
> if (err < 0)
> 	goto out;
> /* ... */
> 	if (copy_to_user((void __user *)arg, parg, _IOC_SIZE(cmd)))
> 		err = -EFAULT;
> /* ... */
> out:
> 
> 
> This is fine in general, but in the case of DQBUF errors, the v4l2_buffer
> fields are not copied back. Because of that, the application does not have any
> means of discovering on which buffer the operation failed. So it cannot reuse
> that buffer, even despite the fact that the spec allows such behavior.
> 
> 
> This RFC proposes a modification to the DQBUF behavior in cases of internal
> (recoverable) errors to allow recovery from such situations.
> 
> We propose a new flag for the v4l2_buffer "flags" field, "V4L2_BUF_FLAG_ERROR".
> There already exists a "V4L2_BUF_FLAG_DONE" flag, so to support older
> applications, the new flag should always be set together with it.
> 
> Applications unaware of the new flag would simply display a corrupted frame, but
> we believe it is still a better solution than failing altogether. Old EIO
> behavior remains so the change is backwards compatible.
> 
> I will post relevant V4L2 documentation updates after (if) this change is 
> accepted.
> 
> 
> This series is rebased onto my recent videobuf clean-up and poll behavior
> patches.
> 
> The series contains:
> [PATCH 1/2] v4l: Add a new ERROR flag for DQBUF after recoverable streaming errors
> [PATCH 2/2] v4l: videobuf: Add support for V4L2_BUF_FLAG_ERROR

Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>

I think this is a very sensible change. After all, DQBUF succeeds, even though the
buffer itself contains errors. But that is not the fault of DQBUF. It is enough to
flag that the buffer does have an error. Without this you actually loose the
buffer completely from the point of view of the application. And that's really
nasty.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH/RFC 0/2] Fix DQBUF behavior for recoverable streaming errors

[Laurent Pinchart]

Hi Hans, Pavel,

On Wednesday 17 March 2010 21:05:19 Hans Verkuil wrote:
> On Wednesday 17 March 2010 15:29:48 Pawel Osciak wrote:
> > Hello,
> > 
> > during the V4L2 brainstorm meeting in Norway we have concluded that
> > streaming error handling in dqbuf is lacking a bit and might result in
> > the application losing video buffers.
> > 
> > V4L2 specification states that DQBUF should set errno to EIO in such
> > cases:
> > 
> > 
> > "EIO
> > 
> > VIDIOC_DQBUF failed due to an internal error. Can also indicate temporary
> > problems like signal loss. Note the driver might dequeue an (empty)
> > buffer despite returning an error, or even stop capturing."
> > 
> > There is a problem with this though. v4l2-ioctl.c code does not copy back
> > v4l2_buffer fields to userspace on a failed ioctl invocation, i.e. when
> > __video_do_ioctl() does not return 0, it jumps over the copy_to_user()
> > code:
> > 
> > /* ... */
> > err = __video_do_ioctl(file, cmd, parg);
> > /* ... */
> > if (err < 0)
> > 
> > 	goto out;
> > 
> > /* ... */
> > 
> > 	if (copy_to_user((void __user *)arg, parg, _IOC_SIZE(cmd)))
> > 	
> > 		err = -EFAULT;
> > 
> > /* ... */
> > out:
> > 
> > 
> > This is fine in general, but in the case of DQBUF errors, the v4l2_buffer
> > fields are not copied back. Because of that, the application does not
> > have any means of discovering on which buffer the operation failed. So
> > it cannot reuse that buffer, even despite the fact that the spec allows
> > such behavior.
> > 
> > 
> > This RFC proposes a modification to the DQBUF behavior in cases of
> > internal (recoverable) errors to allow recovery from such situations.
> > 
> > We propose a new flag for the v4l2_buffer "flags" field,
> > "V4L2_BUF_FLAG_ERROR". There already exists a "V4L2_BUF_FLAG_DONE" flag,
> > so to support older applications, the new flag should always be set
> > together with it.
> > 
> > Applications unaware of the new flag would simply display a corrupted
> > frame, but we believe it is still a better solution than failing
> > altogether. Old EIO behavior remains so the change is backwards
> > compatible.
> > 
> > I will post relevant V4L2 documentation updates after (if) this change is
> > accepted.
> > 
> > 
> > This series is rebased onto my recent videobuf clean-up and poll behavior
> > patches.
> > 
> > The series contains:
> > [PATCH 1/2] v4l: Add a new ERROR flag for DQBUF after recoverable
> > streaming errors [PATCH 2/2] v4l: videobuf: Add support for
> > V4L2_BUF_FLAG_ERROR
> 
> Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>
> 
> I think this is a very sensible change. After all, DQBUF succeeds, even
> though the buffer itself contains errors. But that is not the fault of
> DQBUF. It is enough to flag that the buffer does have an error. Without
> this you actually loose the buffer completely from the point of view of
> the application. And that's really nasty.

Especially with the current wording of the spec:

"Note the driver might dequeue an (empty) buffer despite returning an error, 
or even stop capturing."

That's pretty bad. Because of video_ioctl2 the application won't know which 
buffer has been dequeued, if any, and will thus have no way to requeue it.

Pavel, could you update the Docbook documentation as well in your patch set ? 
The behaviour of DQBUF needs to be properly documented.

-- 
Regards,

Laurent Pinchart

RE: [PATCH/RFC 0/2] Fix DQBUF behavior for recoverable streaming errors

[Pawel Osciak]

>Laurent Pinchart wrote:
>On Wednesday 17 March 2010 21:05:19 Hans Verkuil wrote:
>> On Wednesday 17 March 2010 15:29:48 Pawel Osciak wrote:
>> > Hello,
>> >
>> > during the V4L2 brainstorm meeting in Norway we have concluded that
>> > streaming error handling in dqbuf is lacking a bit and might result in
>> > the application losing video buffers.
>> >
>> > V4L2 specification states that DQBUF should set errno to EIO in such
>> > cases:
>> >
>> >
>> > "EIO
>> >
>> > VIDIOC_DQBUF failed due to an internal error. Can also indicate temporary
>> > problems like signal loss. Note the driver might dequeue an (empty)
>> > buffer despite returning an error, or even stop capturing."
>> >
>> > There is a problem with this though. v4l2-ioctl.c code does not copy back
>> > v4l2_buffer fields to userspace on a failed ioctl invocation, i.e. when
>> > __video_do_ioctl() does not return 0, it jumps over the copy_to_user()
>> > code:
>> >
>> > /* ... */
>> > err = __video_do_ioctl(file, cmd, parg);
>> > /* ... */
>> > if (err < 0)
>> >
>> > 	goto out;
>> >
>> > /* ... */
>> >
>> > 	if (copy_to_user((void __user *)arg, parg, _IOC_SIZE(cmd)))
>> >
>> > 		err = -EFAULT;
>> >
>> > /* ... */
>> > out:
>> >
>> >
>> > This is fine in general, but in the case of DQBUF errors, the v4l2_buffer
>> > fields are not copied back. Because of that, the application does not
>> > have any means of discovering on which buffer the operation failed. So
>> > it cannot reuse that buffer, even despite the fact that the spec allows
>> > such behavior.
>> >
>> >
>> > This RFC proposes a modification to the DQBUF behavior in cases of
>> > internal (recoverable) errors to allow recovery from such situations.
>> >
>> > We propose a new flag for the v4l2_buffer "flags" field,
>> > "V4L2_BUF_FLAG_ERROR". There already exists a "V4L2_BUF_FLAG_DONE" flag,
>> > so to support older applications, the new flag should always be set
>> > together with it.
>> >
>> > Applications unaware of the new flag would simply display a corrupted
>> > frame, but we believe it is still a better solution than failing
>> > altogether. Old EIO behavior remains so the change is backwards
>> > compatible.
>> >
>> > I will post relevant V4L2 documentation updates after (if) this change is
>> > accepted.
>> >
>> >
>> > This series is rebased onto my recent videobuf clean-up and poll behavior
>> > patches.
>> >
>> > The series contains:
>> > [PATCH 1/2] v4l: Add a new ERROR flag for DQBUF after recoverable
>> > streaming errors [PATCH 2/2] v4l: videobuf: Add support for
>> > V4L2_BUF_FLAG_ERROR
>>
>> Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>
>>
>> I think this is a very sensible change. After all, DQBUF succeeds, even
>> though the buffer itself contains errors. But that is not the fault of
>> DQBUF. It is enough to flag that the buffer does have an error. Without
>> this you actually loose the buffer completely from the point of view of
>> the application. And that's really nasty.
>
>Especially with the current wording of the spec:
>
>"Note the driver might dequeue an (empty) buffer despite returning an error,
>or even stop capturing."
>
>That's pretty bad. Because of video_ioctl2 the application won't know which
>buffer has been dequeued, if any, and will thus have no way to requeue it.
>
>Pavel, could you update the Docbook documentation as well in your patch set ?
>The behaviour of DQBUF needs to be properly documented.


Sure. Although I just noticed something. The spec for V4L2_BUF_FLAG_DONE says:

"When this flag is set, the buffer is currently on the outgoing queue,
ready to be dequeued from the driver. Drivers set or clear this flag when
the VIDIOC_QUERYBUF ioctl is called. After calling the VIDIOC_QBUF or
VIDIOC_DQBUF it is always cleared. Of course a buffer cannot be on both queues
at the same time, the V4L2_BUF_FLAG_QUEUED and V4L2_BUF_FLAG_DONE flag are
mutually exclusive. They can be both cleared however, then the buffer is in
"dequeued" state, in the application domain to say so."


So according to the spec, "DONE" means done but not yet returned to userspace.
So it should be cleared during dequeueing. But videobuf actually sets that
flag in dqbuf. And frankly, it seems more sensible to me.

Could you confirm that this is how we want it and I should follow the specs?
If so, I will "fix" videobuf to stop setting that flag.


Best regards
--
Pawel Osciak
Linux Platform Group
Samsung Poland R&D Center

Re: [PATCH/RFC 0/2] Fix DQBUF behavior for recoverable streaming errors

[Hans Verkuil]

On Thursday 18 March 2010 16:37:18 Pawel Osciak wrote:
> >Laurent Pinchart wrote:
> >On Wednesday 17 March 2010 21:05:19 Hans Verkuil wrote:
> >> On Wednesday 17 March 2010 15:29:48 Pawel Osciak wrote:
> >> > Hello,
> >> >
> >> > during the V4L2 brainstorm meeting in Norway we have concluded that
> >> > streaming error handling in dqbuf is lacking a bit and might result in
> >> > the application losing video buffers.
> >> >
> >> > V4L2 specification states that DQBUF should set errno to EIO in such
> >> > cases:
> >> >
> >> >
> >> > "EIO
> >> >
> >> > VIDIOC_DQBUF failed due to an internal error. Can also indicate temporary
> >> > problems like signal loss. Note the driver might dequeue an (empty)
> >> > buffer despite returning an error, or even stop capturing."
> >> >
> >> > There is a problem with this though. v4l2-ioctl.c code does not copy back
> >> > v4l2_buffer fields to userspace on a failed ioctl invocation, i.e. when
> >> > __video_do_ioctl() does not return 0, it jumps over the copy_to_user()
> >> > code:
> >> >
> >> > /* ... */
> >> > err = __video_do_ioctl(file, cmd, parg);
> >> > /* ... */
> >> > if (err < 0)
> >> >
> >> > 	goto out;
> >> >
> >> > /* ... */
> >> >
> >> > 	if (copy_to_user((void __user *)arg, parg, _IOC_SIZE(cmd)))
> >> >
> >> > 		err = -EFAULT;
> >> >
> >> > /* ... */
> >> > out:
> >> >
> >> >
> >> > This is fine in general, but in the case of DQBUF errors, the v4l2_buffer
> >> > fields are not copied back. Because of that, the application does not
> >> > have any means of discovering on which buffer the operation failed. So
> >> > it cannot reuse that buffer, even despite the fact that the spec allows
> >> > such behavior.
> >> >
> >> >
> >> > This RFC proposes a modification to the DQBUF behavior in cases of
> >> > internal (recoverable) errors to allow recovery from such situations.
> >> >
> >> > We propose a new flag for the v4l2_buffer "flags" field,
> >> > "V4L2_BUF_FLAG_ERROR". There already exists a "V4L2_BUF_FLAG_DONE" flag,
> >> > so to support older applications, the new flag should always be set
> >> > together with it.
> >> >
> >> > Applications unaware of the new flag would simply display a corrupted
> >> > frame, but we believe it is still a better solution than failing
> >> > altogether. Old EIO behavior remains so the change is backwards
> >> > compatible.
> >> >
> >> > I will post relevant V4L2 documentation updates after (if) this change is
> >> > accepted.
> >> >
> >> >
> >> > This series is rebased onto my recent videobuf clean-up and poll behavior
> >> > patches.
> >> >
> >> > The series contains:
> >> > [PATCH 1/2] v4l: Add a new ERROR flag for DQBUF after recoverable
> >> > streaming errors [PATCH 2/2] v4l: videobuf: Add support for
> >> > V4L2_BUF_FLAG_ERROR
> >>
> >> Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>
> >>
> >> I think this is a very sensible change. After all, DQBUF succeeds, even
> >> though the buffer itself contains errors. But that is not the fault of
> >> DQBUF. It is enough to flag that the buffer does have an error. Without
> >> this you actually loose the buffer completely from the point of view of
> >> the application. And that's really nasty.
> >
> >Especially with the current wording of the spec:
> >
> >"Note the driver might dequeue an (empty) buffer despite returning an error,
> >or even stop capturing."
> >
> >That's pretty bad. Because of video_ioctl2 the application won't know which
> >buffer has been dequeued, if any, and will thus have no way to requeue it.
> >
> >Pavel, could you update the Docbook documentation as well in your patch set ?
> >The behaviour of DQBUF needs to be properly documented.
> 
> 
> Sure. Although I just noticed something. The spec for V4L2_BUF_FLAG_DONE says:
> 
> "When this flag is set, the buffer is currently on the outgoing queue,
> ready to be dequeued from the driver. Drivers set or clear this flag when
> the VIDIOC_QUERYBUF ioctl is called. After calling the VIDIOC_QBUF or
> VIDIOC_DQBUF it is always cleared. Of course a buffer cannot be on both queues
> at the same time, the V4L2_BUF_FLAG_QUEUED and V4L2_BUF_FLAG_DONE flag are
> mutually exclusive. They can be both cleared however, then the buffer is in
> "dequeued" state, in the application domain to say so."
> 
> 
> So according to the spec, "DONE" means done but not yet returned to userspace.
> So it should be cleared during dequeueing. But videobuf actually sets that
> flag in dqbuf. And frankly, it seems more sensible to me.
> 
> Could you confirm that this is how we want it and I should follow the specs?
> If so, I will "fix" videobuf to stop setting that flag.

I think the spec makes sense, actually. But doesn't videobuf clear this already?

On ERROR and DONE videobuf_dqbuf will change the state to IDLE. videobuf_status
then won't set the DONE flag.

So as far as I can tell there is nothing that needs to change here.

Regards,

          Hans

> 
> 
> Best regards
> --
> Pawel Osciak
> Linux Platform Group
> Samsung Poland R&D Center
> 
> 
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG