[PATCH v2] media: uvcvideo: Fix buffer sequence in frame gaps

[PATCH v2] 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
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>
---
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
---
 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;
 	}

---
base-commit: a7da7fb57f2a787412da1a62292a17fa00fbfbdf
change-id: 20260310-uvc-fid-e1e55447b6f1

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

Re: [PATCH v2] media: uvcvideo: Fix buffer sequence in frame gaps

[Hans de Goede]

Hi,

On 13-Mar-26 7:21 PM, 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
> 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>

So I'm trying to understand this patch, but while looking at this I'm having
a hard time to figure out the current code.

Lets take the following scenario. We have an active current buffer with some
bytesused and uvc_video_decode_start() sees a fid flip.

Now the following happens:

First uvc_video_decode_start() run:

1. if (stream->last_fid != fid) check at line 1175 sees the flip does stream->sequence++;
2. if (fid != stream->last_fid && buf->bytesused != 0) check at line 1243 succeeds, marks
   buffer as ready and returns -EAGAIN.
3. Note stream->last_fid is not updated in this case.

Second uvc_video_decode_start() run because of -EGAIN with new fresh buffer

1. if (stream->last_fid != fid) check at line 1175 sees the flip does stream->sequence++;
2. if (buf->state != UVC_BUF_STATE_ACTIVE) check at line 1209 succeeds, updates
   buf->buf.sequence , timestamp
3. if (fid != stream->last_fid && buf->bytesused != 0) check at line 1243 fails because
   bytesused == 0
4. function exits normally doing:

        stream->last_fid = fid;

        return header_len;

Notice that step 1. happens in both uvc_video_decode_start() runs so we are doing
stream->sequence++ *twice* for a single fid flip. Am I missing something here or
are we indeed increasing sequence twice. And if we are indeed increasing sequence
twice, is that intentional and/or expected by userspace ?

Regards,

Hans




> ---
> 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
> ---
>  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;
>  	}
> 
> ---
> base-commit: a7da7fb57f2a787412da1a62292a17fa00fbfbdf
> change-id: 20260310-uvc-fid-e1e55447b6f1
> 
> Best regards,

Re: [PATCH v2] media: uvcvideo: Fix buffer sequence in frame gaps

[Ricardo Ribalda]

Hi Hans

Thanks for looking into this. uvc_video_decode_start() is not a
beautiful function (not saying that I could have made it better).

On Mon, 16 Mar 2026 at 12:45, Hans de Goede <hansg@kernel.org> wrote:
>
> Hi,
>
> On 13-Mar-26 7:21 PM, 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
> > 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>
>
> So I'm trying to understand this patch, but while looking at this I'm having
> a hard time to figure out the current code.
>
> Lets take the following scenario. We have an active current buffer with some
> bytesused and uvc_video_decode_start() sees a fid flip.
>
> Now the following happens:
>
> First uvc_video_decode_start() run:
>
> 1. if (stream->last_fid != fid) check at line 1175 sees the flip does stream->sequence++;
> 2. if (fid != stream->last_fid && buf->bytesused != 0) check at line 1243 succeeds, marks
>    buffer as ready and returns -EAGAIN.
> 3. Note stream->last_fid is not updated in this case.
>
> Second uvc_video_decode_start() run because of -EGAIN with new fresh buffer
>
> 1. if (stream->last_fid != fid) check at line 1175 sees the flip does stream->sequence++;
> 2. if (buf->state != UVC_BUF_STATE_ACTIVE) check at line 1209 succeeds, updates
>    buf->buf.sequence , timestamp
> 3. if (fid != stream->last_fid && buf->bytesused != 0) check at line 1243 fails because
>    bytesused == 0
> 4. function exits normally doing:
>
>         stream->last_fid = fid;
>
>         return header_len;
>
> Notice that step 1. happens in both uvc_video_decode_start() runs so we are doing
> stream->sequence++ *twice* for a single fid flip. Am I missing something here or
> are we indeed increasing sequence twice. And if we are indeed increasing sequence
> twice, is that intentional and/or expected by userspace ?

During normal operation the driver will not reach line 1243, instead
it will detect the new frame using the EOF in the function
uvc_video_decode_end().

Something like this:

****EOF_CHANGE
uvc_video_decode_end()
  - set buf->state to READY
uvc_video_decode_isoc()
  - calls uvc_video_next_buffers(), which provides buffes with bytesused=0
uvc_vieo_decode_start()
  - Loops in "Dropping payload (out of sync)"

**** FID FLIP
- uvc_video_decode_start()
  - sequence++
  - set buf->state to ACTIVE
  - last_fid = fid


If I understood the code correctly, line 1243 was introduced for
cameras that do not implement EOF properly. For those cameras I agree
that it looks like the code will increment the sequence twice per
frame... which is wrong. I can send a patch if you want but I have no
camera to test it. Basically I would set stream->last_fid = fid before
return -EAGAIN


Regards!!!

>
> Regards,
>
> Hans
>
>
>
>
> > ---
> > 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
> > ---
> >  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;
> >       }
> >
> > ---
> > base-commit: a7da7fb57f2a787412da1a62292a17fa00fbfbdf
> > change-id: 20260310-uvc-fid-e1e55447b6f1
> >
> > Best regards,
>


-- 
Ricardo Ribalda