Re: [PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling

[Anton Leontiev]

26.03.2014 21:41, Laurent Pinchart wrote:
> Hi Anton,
> 
> Thank you for the patch.
> 
> 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.

> 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,
>> +	struct uvc_buffer *const buf)
>> +{
>> +	if (buf->length != buf->bytesused &&
>> +			!(stream->cur_format->flags & UVC_FMT_FLAG_COMPRESSED))
>> +		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);
>>  		}
>>  	}
> 

-- 
Anton Leontiev
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/