[PATCH v5 0/2] media: uvcvideo: Fixes for frame sequence number

[PATCH v5 0/2] media: uvcvideo: Fixes for frame sequence number

[Ricardo Ribalda]

This series fixes a couple of corner cases where the frame sequence
number is not properly handled.

Please note that the first patch has not been tested in a camera
without EOF. To emulate it I have used this:

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index b66d701f2582d..097bed2f7845f 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1360,6 +1360,8 @@ static void uvc_video_decode_end(struct uvc_streaming *stream,
 {
        /* Mark the buffer as done if the EOF marker is set. */
        if (data[1] & UVC_STREAM_EOF && buf->bytesused != 0) {
+               printk(KERN_ERR "Ignoring EOF\n");
+               return;
                uvc_dbg(stream->dev, FRAME, "Frame complete (EOF found)\n");
                if (data[0] == len)
                        uvc_dbg(stream->dev, FRAME, "EOF in empty payload\n");

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Changes in v5 (Thanks Hans):
- Swap order of patches
- Remove duplicated conditions.
- Link to v4: https://lore.kernel.org/r/20260320-uvc-fid-v4-0-f24f168ca2f9@chromium.org

Changes in v4 (Thanks Hans):
- Fix 2/2 logic.
- Link to v3: https://lore.kernel.org/r/20260316-uvc-fid-v3-0-c793354469b5@chromium.org

Changes in v3:
- Fix typo in commit message.
- Add new patch
- Link to v2: https://lore.kernel.org/r/20260313-uvc-fid-v2-1-3f7a996d9047@chromium.org

Changes in v2 (Thanks Laurent):
- Improve commit message.
- Remove original timestamp and sequence assignment. It is not neeed
- Link to v1: https://lore.kernel.org/r/20260310-uvc-fid-v1-1-5e37dc3c7024@chromium.org

---
Ricardo Ribalda (2):
      media: uvcvideo: Fix sequence number when no EOF
      media: uvcvideo: Fix buffer sequence in frame gaps

 drivers/media/usb/uvc/uvc_video.c | 110 +++++++++++++++++++++-----------------
 1 file changed, 61 insertions(+), 49 deletions(-)
---
base-commit: a7da7fb57f2a787412da1a62292a17fa00fbfbdf
change-id: 20260310-uvc-fid-e1e55447b6f1

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

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

[Ricardo Ribalda]

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) {
+			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;

-- 
2.53.0.959.g497ff81fa9-goog

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

[Laurent Pinchart]

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.

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>

> +			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;
> 

-- 
Regards,

Laurent Pinchart

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

[Ricardo Ribalda]

Hi Laurent


On Tue, 24 Mar 2026 at 01:20, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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.

SGTM. I will change this locally and upload it after someone reviews
2/2 to avoid bloating the list


Thanks!
>
> 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>
>
> > +                     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;
> >
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

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

[Hans de Goede]

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;
>>
> 

[PATCH v5 2/2] media: uvcvideo: Fix buffer sequence in frame gaps

[Ricardo Ribalda]

In UVC, the FID flips with every frame. For every FID flip, we increase
the stream sequence number.

Now, if a FID flips multiple times and there is no data transferred between
the flips, the buffer sequence number will be set to the value of the
stream sequence number after the first flip.

Userspace uses the buffer sequence number to determine if there have been
missing frames. With the current behaviour, userspace will think that the
gap is in the wrong location.

This patch modifies uvc_video_decode_start() to provide the correct buffer
sequence number and timestamp.

Cc: stable@kernel.org
Fixes: 650b95feee35 ("[media] uvcvideo: Generate discontinuous sequence numbers when frames are lost")
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_video.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index eddb4821b205..32c3469f26c6 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1223,6 +1223,20 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
 		stream->sequence++;
 		if (stream->sequence)
 			uvc_video_stats_update(stream);
+
+		/*
+		 * If there is a FID flip and the buffer has no data,
+		 * initialize its sequence number and timestamp.
+		 *
+		 * The driver already takes care of injecting FID flips for
+		 * UVC_QUIRK_STREAM_NO_FID and UVC_QUIRK_MJPEG_NO_EOF.
+		 */
+		if (buf) {
+			buf->buf.field = V4L2_FIELD_NONE;
+			buf->buf.sequence = stream->sequence;
+			buf->buf.vb2_buf.timestamp =
+					ktime_to_ns(uvc_video_get_time());
+		}
 	}
 
 	uvc_video_clock_decode(stream, buf, data, len);
@@ -1263,10 +1277,6 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
 			return -ENODATA;
 		}
 
-		buf->buf.field = V4L2_FIELD_NONE;
-		buf->buf.sequence = stream->sequence;
-		buf->buf.vb2_buf.timestamp = ktime_to_ns(uvc_video_get_time());
-
 		/* TODO: Handle PTS and SCR. */
 		buf->state = UVC_BUF_STATE_ACTIVE;
 	}

-- 
2.53.0.959.g497ff81fa9-goog

Re: [PATCH v5 2/2] media: uvcvideo: Fix buffer sequence in frame gaps

[Hans de Goede]

Hi,

On 23-Mar-26 10:53, Ricardo Ribalda wrote:
> In UVC, the FID flips with every frame. For every FID flip, we increase
> the stream sequence number.
> 
> Now, if a FID flips multiple times and there is no data transferred between
> the flips, the buffer sequence number will be set to the value of the
> stream sequence number after the first flip.
> 
> Userspace uses the buffer sequence number to determine if there have been
> missing frames. With the current behaviour, userspace will think that the
> gap is in the wrong location.
> 
> This patch modifies uvc_video_decode_start() to provide the correct buffer
> sequence number and timestamp.
> 
> Cc: stable@kernel.org
> Fixes: 650b95feee35 ("[media] uvcvideo: Generate discontinuous sequence numbers when frames are lost")
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index eddb4821b205..32c3469f26c6 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1223,6 +1223,20 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
>  		stream->sequence++;
>  		if (stream->sequence)
>  			uvc_video_stats_update(stream);
> +
> +		/*
> +		 * If there is a FID flip and the buffer has no data,
> +		 * initialize its sequence number and timestamp.

The "and the buffer has no data" part of the comment is a bit weird
now that the buf->bytes_used == 0 check is dropped. I'll drop
this part of the comment while merging this.

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

Regards,

Hans


> +		 *
> +		 * The driver already takes care of injecting FID flips for
> +		 * UVC_QUIRK_STREAM_NO_FID and UVC_QUIRK_MJPEG_NO_EOF.
> +		 */
> +		if (buf) {
> +			buf->buf.field = V4L2_FIELD_NONE;
> +			buf->buf.sequence = stream->sequence;
> +			buf->buf.vb2_buf.timestamp =
> +					ktime_to_ns(uvc_video_get_time());
> +		}
>  	}
>  
>  	uvc_video_clock_decode(stream, buf, data, len);
> @@ -1263,10 +1277,6 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
>  			return -ENODATA;
>  		}
>  
> -		buf->buf.field = V4L2_FIELD_NONE;
> -		buf->buf.sequence = stream->sequence;
> -		buf->buf.vb2_buf.timestamp = ktime_to_ns(uvc_video_get_time());
> -
>  		/* TODO: Handle PTS and SCR. */
>  		buf->state = UVC_BUF_STATE_ACTIVE;
>  	}
> 

Re: [PATCH v5 0/2] media: uvcvideo: Fixes for frame sequence number

[Hans de Goede]

Hi,

On 23-Mar-26 10:53, Ricardo Ribalda wrote:
> This series fixes a couple of corner cases where the frame sequence
> number is not properly handled.

Thanks, I've applied this to my local uvc/for-next branch and will
push this out later today.

As mentioned in replies to the individual patches I did squash in
some small fixes.

Regards,

Hans



> 
> Please note that the first patch has not been tested in a camera
> without EOF. To emulate it I have used this:
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index b66d701f2582d..097bed2f7845f 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1360,6 +1360,8 @@ static void uvc_video_decode_end(struct uvc_streaming *stream,
>  {
>         /* Mark the buffer as done if the EOF marker is set. */
>         if (data[1] & UVC_STREAM_EOF && buf->bytesused != 0) {
> +               printk(KERN_ERR "Ignoring EOF\n");
> +               return;
>                 uvc_dbg(stream->dev, FRAME, "Frame complete (EOF found)\n");
>                 if (data[0] == len)
>                         uvc_dbg(stream->dev, FRAME, "EOF in empty payload\n");
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> Changes in v5 (Thanks Hans):
> - Swap order of patches
> - Remove duplicated conditions.
> - Link to v4: https://lore.kernel.org/r/20260320-uvc-fid-v4-0-f24f168ca2f9@chromium.org
> 
> Changes in v4 (Thanks Hans):
> - Fix 2/2 logic.
> - Link to v3: https://lore.kernel.org/r/20260316-uvc-fid-v3-0-c793354469b5@chromium.org
> 
> Changes in v3:
> - Fix typo in commit message.
> - Add new patch
> - Link to v2: https://lore.kernel.org/r/20260313-uvc-fid-v2-1-3f7a996d9047@chromium.org
> 
> Changes in v2 (Thanks Laurent):
> - Improve commit message.
> - Remove original timestamp and sequence assignment. It is not neeed
> - Link to v1: https://lore.kernel.org/r/20260310-uvc-fid-v1-1-5e37dc3c7024@chromium.org
> 
> ---
> Ricardo Ribalda (2):
>       media: uvcvideo: Fix sequence number when no EOF
>       media: uvcvideo: Fix buffer sequence in frame gaps
> 
>  drivers/media/usb/uvc/uvc_video.c | 110 +++++++++++++++++++++-----------------
>  1 file changed, 61 insertions(+), 49 deletions(-)
> ---
> base-commit: a7da7fb57f2a787412da1a62292a17fa00fbfbdf
> change-id: 20260310-uvc-fid-e1e55447b6f1
> 
> Best regards,