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

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;
>  	}
>