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@xxxxxxxxxx> 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@xxxxxxxxxx
> > Fixes: 650b95feee35 ("[media] uvcvideo: Generate discontinuous sequence numbers when frames are lost")
> > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx
> > ---
> >  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