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@xxxxxxx>
> >> ---
> >>
> >> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
