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

[Ricardo Ribalda <ribalda@chromium.org>]

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