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

[PATCH v4 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 second 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 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 buffer sequence in frame gaps
      media: uvcvideo: Fix sequence number when no EOF

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

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

[PATCH v4 1/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 has 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 40c76c051da2..9e06b1d0f0f9 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1176,6 +1176,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->bytesused) {
+			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);
@@ -1216,10 +1230,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 v4 1/2] media: uvcvideo: Fix buffer sequence in frame gaps

[Hans de Goede]

Hi,

On 20-Mar-26 14:35, 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 has 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 40c76c051da2..9e06b1d0f0f9 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1176,6 +1176,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->bytesused) {
> +			buf->buf.field = V4L2_FIELD_NONE;
> +			buf->buf.sequence = stream->sequence;
> +			buf->buf.vb2_buf.timestamp =
> +					ktime_to_ns(uvc_video_get_time());
> +		}

If you move this patch to after patch 2/2 then you can drop the 
!buf->bytesused check since if the fid changed and
buf->bytesused != 0 uvc_video_decode_start() will hit:

	if (stream->last_fid != (u8) -1 && fid != stream->last_fid &&
	    buf && buf->bytesused != 0) {

First and return -EAGAIN, so if we get here (with here being
inside a fid-changed check) then we know that buf->bytesused == 0
(or there is no buffer at all).

Regards,

Hans




>  	}
>  
>  	uvc_video_clock_decode(stream, buf, data, len);
> @@ -1216,10 +1230,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;
>  	}
> 

[PATCH v4 2/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
   fliped => 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().

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 | 48 ++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 9e06b1d0f0f9..2218e4d8e564 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1168,6 +1168,31 @@ 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, 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 (stream->last_fid != -1 && 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;
+	}
+
 	/*
 	 * Increase the sequence number regardless of any buffer states, so
 	 * that discontinuous sequence numbers always indicate lost frames.
@@ -1234,29 +1259,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.

-- 
2.53.0.959.g497ff81fa9-goog

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

[Ricardo Ribalda]

On Fri, 20 Mar 2026 at 14:35, Ricardo Ribalda <ribalda@chromium.org> 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
>    fliped => 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().
>
> 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 | 48 ++++++++++++++++++++-------------------
>  1 file changed, 25 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 9e06b1d0f0f9..2218e4d8e564 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1168,6 +1168,31 @@ 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, 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 (stream->last_fid != -1 && fid != stream->last_fid &&
> +           buf && buf->bytesused != 0) {

This should be
if (stream->last_fid != (u8) -1 && fid != stream->last_fid &&
    buf && buf->bytesused != 0) {


I will wait a bit for more comments before sending a v5

Have a good weekend

> +               uvc_dbg(stream->dev, FRAME,
> +                       "Frame complete (FID bit toggled)\n");
> +               buf->state = UVC_BUF_STATE_READY;
> +
> +               return -EAGAIN;
> +       }
> +
>         /*
>          * Increase the sequence number regardless of any buffer states, so
>          * that discontinuous sequence numbers always indicate lost frames.
> @@ -1234,29 +1259,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.
>
> --
> 2.53.0.959.g497ff81fa9-goog
>


-- 
Ricardo Ribalda

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

[Hans de Goede]

Hi,

On 20-Mar-26 14:35, 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
>    fliped => 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().
> 
> 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 | 48 ++++++++++++++++++++-------------------
>  1 file changed, 25 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 9e06b1d0f0f9..2218e4d8e564 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1168,6 +1168,31 @@ 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, 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 (stream->last_fid != -1 && 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;
> +	}
> +
>  	/*
>  	 * Increase the sequence number regardless of any buffer states, so
>  	 * that discontinuous sequence numbers always indicate lost frames.

Nice, since this compensates for the previous packet being parsed missing
the EOF flag doing it as the first thing makes a lot of sense.

Note this has a number of extra advantages which would be good to list
in the commit message for v5:

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

I wonder if we should move the JPEG SOI marker detect also up for
all the same reasons? That seems to be for a camera not toggling FID
itself *and* sometimes missing the packet with the EOF flag ?

Regards,

Hans






> @@ -1234,29 +1259,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.
>