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

[Hans de Goede <hansg@kernel.org>]

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,