lkml.org 
[lkml]   [2014]   [Mar]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling
Date
Hi Anton,

On Friday 28 March 2014 07:58:00 Anton Leontiev wrote:
> 26.03.2014 21:41, Laurent Pinchart wrote:
> > On Tuesday 25 March 2014 08:40:57 Anton Leontiev wrote:
> >> Set error bit for incomplete buffers when end of buffer is detected by
> >> FID toggling (for example when last transaction with EOF is lost).
> >> This prevents passing incomplete buffers to the userspace.
> >
> > But this would also breaks buggy webcams that toggle the FID bit but don't
> > set the EOF bit. Support for this was added before the driver got merged
> > in the mainline kernel, and the SVN log is a bit terse I'm afraid:
> >
> > V 104
> > - Check both EOF and FID bits to detect end of frames.
> > - Updated disclaimer and general status comment.
> >
> > I don't remember which webcam(s) exhibit this behaviour.
> >
> > Your patch would also mark valid buffers as erroneous when the list EOF
> > bit is in a packet of its own with no data.
>
> If camera streams compressed video the patch doesn't affect it. It affects
> only uncompressed streams.
>
> If we get EOF bit in a packet of its own with no data we take second
> check under 'if (buf->state == UVC_BUF_STATE_READY)' that was before my
> patch. In this case if all data were received buffer is processed
> normally, but if some data is missing buffer is marked erroneous.
>
> > As the uvcvideo driver already marks buffers smaller than the expected
> > image size as erroneous, missing EOF packets that contain data should
> > already result in buffers with the error bit set.
>
> No, because there was no check for that in case then new frame is detected
> by FID toggling, it was there only for the case then new frame is detected
> by EOF bit.

Right, my bad, I had misread your patch. Please see below for a couple of
small comments.

> > Are you concerned about compressed formats only ?
>
> No, I'm concerning about uncompressed frames only.
>
> > While this patch would correctly detect the missing EOF packet in that
> > case, any other missing packet would still result in a corrupt image, so
> > I'm not sure if this would be worth it.
> >
> >> Signed-off-by: Anton Leontiev <bunder@t-25.ru>
> >> ---
> >>
> >> drivers/media/usb/uvc/uvc_video.c | 21 +++++++++++++++------
> >> 1 file changed, 15 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/media/usb/uvc/uvc_video.c
> >> b/drivers/media/usb/uvc/uvc_video.c index 8d52baf..57c9a4b 100644
> >> --- a/drivers/media/usb/uvc/uvc_video.c
> >> +++ b/drivers/media/usb/uvc/uvc_video.c
> >> @@ -1133,6 +1133,17 @@ static int uvc_video_encode_data(struct
> >> uvc_streaming *stream, */
> >>
> >> /*
> >>
> >> + * Set error flag for incomplete buffer.
> >> + */
> >> +static void uvc_buffer_check_bytesused(const struct uvc_streaming *const
> >> stream,

No need for the second const keyword here.

I would have used "uvc_video_" as a prefix, to be in sync with the surrounding
functions. What would you think of uvc_video_validate_buffer() ?

> >> + struct uvc_buffer *const buf)

And no need for const at all here.

> >> +{
> >> + if (buf->length != buf->bytesused &&
> >> + !(stream->cur_format->flags & UVC_FMT_FLAG_COMPRESSED))

The indentation is wrong here, the ! on the second line should be aligned to
the first 'buf' of the first line.

If you agree with these changes I can perform them while applying, there's no
need to resubmit the patch.

> >> + buf->error = 1;
> >> +}
> >> +
> >> +/*
> >> * Completion handler for video URBs.
> >> */
> >> static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming
> >> *stream,
> >> @@ -1156,9 +1167,11 @@ static void uvc_video_decode_isoc(struct
> >> urb *urb, struct uvc_streaming *stream,
> >> do {
> >> ret = uvc_video_decode_start(stream, buf, mem,
> >> urb->iso_frame_desc[i].actual_length);
> >> - if (ret == -EAGAIN)
> >> + if (ret == -EAGAIN) {
> >> + uvc_buffer_check_bytesused(stream, buf);
> >> buf = uvc_queue_next_buffer(&stream->queue,
> >> buf);
> >> + }
> >> } while (ret == -EAGAIN);
> >>
> >> if (ret < 0)
> >> @@ -1173,11 +1186,7 @@ static void uvc_video_decode_isoc(struct urb *urb,
> >> struct uvc_streaming *stream, urb->iso_frame_desc[i].actual_length);
> >> if (buf->state == UVC_BUF_STATE_READY) {
> >> - if (buf->length != buf->bytesused &&
> >> - !(stream->cur_format->flags &
> >> - UVC_FMT_FLAG_COMPRESSED))
> >> - buf->error = 1;
> >> -
> >> + uvc_buffer_check_bytesused(stream, buf);
> >> buf = uvc_queue_next_buffer(&stream->queue, buf);
> >> }
> >> }

--
Regards,

Laurent Pinchart



\
 
 \ /
  Last update: 2014-03-28 17:41    [W:0.145 / U:2.412 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site