Re: [PATCH v5 1/2] media: uvcvideo: Fix sequence number when no EOF

[Hans de Goede <hansg@kernel.org>]

Hi,

On 24-Mar-26 01:20, Laurent Pinchart wrote:
> On Mon, Mar 23, 2026 at 09:53:52AM +0000, Ricardo Ribalda wrote:
>> If the driver could not detect the EOF, the sequence number is increased
>> twice:
>>  1) When we enter uvc_video_decode_start() with the old buffer and FID has
>>    flipped => We return -EAGAIN and last_fid is not flipped
>>  2) When we enter uvc_video_decode_start() with the new buffer.
>>
>> Fix this issue by moving the new frame detection logic earlier in
>> uvc_video_decode_start().
>>
>> This also has some nice side affects:
>>
>> - The error status from the new packet will no longer get propagated
>>   to the previous frame-buffer.
>> - uvc_video_clock_decode() will no longer update the previous frame
>>   buf->stf with info from the new packet.
>> - uvc_video_clock_decode() and uvc_video_stats_decode() will no longer
>>   get called twice for the same packet.
>>
>> Cc: stable@kernel.org
>> Fixes: 650b95feee35 ("[media] uvcvideo: Generate discontinuous sequence numbers when frames are lost")
>> Reported-by: Hans de Goede <hansg@kernel.org>
>> Closes: https://lore.kernel.org/linux-media/CANiDSCuj4cPuB5_v2xyvAagA5FjoN8V5scXiFFOeD3aKDMqkCg@mail.gmail.com/T/#me39fb134e8c2c085567a31548c3403eb639625e4
>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>> ---
>>  drivers/media/usb/uvc/uvc_video.c | 92 ++++++++++++++++++++-------------------
>>  1 file changed, 47 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
>> index 40c76c051da2..eddb4821b205 100644
>> --- a/drivers/media/usb/uvc/uvc_video.c
>> +++ b/drivers/media/usb/uvc/uvc_video.c
>> @@ -1168,6 +1168,53 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
>>  	header_len = data[0];
>>  	fid = data[1] & UVC_STREAM_FID;
>>  
>> +	/*
>> +	 * Mark the buffer as done if we're at the beginning of a new frame.
>> +	 * End of frame detection is better implemented by checking the EOF
>> +	 * bit (FID bit toggling is delayed by one frame compared to the EOF
>> +	 * bit), but some devices don't set the bit at end of frame (and the
>> +	 * last payload can be lost anyway). We thus must check if the FID has
>> +	 * been toggled.
>> +	 *
>> +	 * stream->last_fid is initialized to -1, and buf->bytesused to 0,
>> +	 * so the first isochronous frame will never trigger an end of frame
>> +	 * detection.
>> +	 *
>> +	 * Empty buffers (bytesused == 0) don't trigger end of frame detection
>> +	 * as it doesn't make sense to return an empty buffer. This also
>> +	 * avoids detecting end of frame conditions at FID toggling if the
>> +	 * previous payload had the EOF bit set.
>> +	 */
>> +	if (fid != stream->last_fid && buf && buf->bytesused != 0) {
>> +		uvc_dbg(stream->dev, FRAME,
>> +			"Frame complete (FID bit toggled)\n");
>> +		buf->state = UVC_BUF_STATE_READY;
>> +
>> +		return -EAGAIN;
>> +	}
>> +
>> +	/*
>> +	 * Some cameras, when running two parallel streams (one MJPEG alongside
>> +	 * another non-MJPEG stream), are known to lose the EOF packet for a frame.
>> +	 * We can detect the end of a frame by checking for a new SOI marker, as
>> +	 * the SOI always lies on the packet boundary between two frames for
>> +	 * these devices.
>> +	 */
>> +	if (stream->dev->quirks & UVC_QUIRK_MJPEG_NO_EOF &&
>> +	    (stream->cur_format->fcc == V4L2_PIX_FMT_MJPEG ||
>> +	    stream->cur_format->fcc == V4L2_PIX_FMT_JPEG)) {
>> +		const u8 *packet = data + header_len;
>> +
>> +		if (len >= header_len + 2 &&
>> +		    packet[0] == 0xff && packet[1] == JPEG_MARKER_SOI &&
>> +		    buf && buf->bytesused != 0) {
> 
> How about moving the buf && buf->bytesused != 0 to the outer condition,
> so that we don't inspect the packet when we don't need to ? It will also
> make the two end of frame detection blocks more similar.

I've just merged this into my local uvc/for-next branch and
I've moved the "buf && buf->bytesused != 0" check to the outer condition
while applying, so there is no need to send a new version.

> The patch otherwise looks fine, I don't see anything below this code
> that would need to be performed before.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans



> 
>> +			buf->state = UVC_BUF_STATE_READY;
>> +			buf->error = 1;
>> +			stream->last_fid ^= UVC_STREAM_FID;
>> +			return -EAGAIN;
>> +		}
>> +	}
>> +
>>  	/*
>>  	 * Increase the sequence number regardless of any buffer states, so
>>  	 * that discontinuous sequence numbers always indicate lost frames.
>> @@ -1224,51 +1271,6 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
>>  		buf->state = UVC_BUF_STATE_ACTIVE;
>>  	}
>>  
>> -	/*
>> -	 * Mark the buffer as done if we're at the beginning of a new frame.
>> -	 * End of frame detection is better implemented by checking the EOF
>> -	 * bit (FID bit toggling is delayed by one frame compared to the EOF
>> -	 * bit), but some devices don't set the bit at end of frame (and the
>> -	 * last payload can be lost anyway). We thus must check if the FID has
>> -	 * been toggled.
>> -	 *
>> -	 * stream->last_fid is initialized to -1, so the first isochronous
>> -	 * frame will never trigger an end of frame detection.
>> -	 *
>> -	 * Empty buffers (bytesused == 0) don't trigger end of frame detection
>> -	 * as it doesn't make sense to return an empty buffer. This also
>> -	 * avoids detecting end of frame conditions at FID toggling if the
>> -	 * previous payload had the EOF bit set.
>> -	 */
>> -	if (fid != stream->last_fid && buf->bytesused != 0) {
>> -		uvc_dbg(stream->dev, FRAME,
>> -			"Frame complete (FID bit toggled)\n");
>> -		buf->state = UVC_BUF_STATE_READY;
>> -		return -EAGAIN;
>> -	}
>> -
>> -	/*
>> -	 * Some cameras, when running two parallel streams (one MJPEG alongside
>> -	 * another non-MJPEG stream), are known to lose the EOF packet for a frame.
>> -	 * We can detect the end of a frame by checking for a new SOI marker, as
>> -	 * the SOI always lies on the packet boundary between two frames for
>> -	 * these devices.
>> -	 */
>> -	if (stream->dev->quirks & UVC_QUIRK_MJPEG_NO_EOF &&
>> -	    (stream->cur_format->fcc == V4L2_PIX_FMT_MJPEG ||
>> -	    stream->cur_format->fcc == V4L2_PIX_FMT_JPEG)) {
>> -		const u8 *packet = data + header_len;
>> -
>> -		if (len >= header_len + 2 &&
>> -		    packet[0] == 0xff && packet[1] == JPEG_MARKER_SOI &&
>> -		    buf->bytesused != 0) {
>> -			buf->state = UVC_BUF_STATE_READY;
>> -			buf->error = 1;
>> -			stream->last_fid ^= UVC_STREAM_FID;
>> -			return -EAGAIN;
>> -		}
>> -	}
>> -
>>  	stream->last_fid = fid;
>>  
>>  	return header_len;
>>
>