From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0C1CD202997; Tue, 24 Mar 2026 00:20:25 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=213.167.242.64 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774311627; cv=none; b=UtLVItVsFGx2JoF6sWLHwss1ocWkob8GUR0CLIfUiQPg8LnkO/GA9fbrLK5576OjL+zkxik1c5I6P2hgpAAVW5z+6TBuIqePJ8OWVKgNlbpP+oKPCoNNEkgWgv1/YJ8WBWBgg0fGkZUtd2ofzW+zuhbIuCljNdx8+pu4KRZ1t/c= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774311627; c=relaxed/simple; bh=3dosacfdbH6jC2r+obvt3527gGj3wloWAca625hnbzk=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ssEJP8tUILGNcoZ84hnMWzpnD5JCqqMN7MGNWToNynUao6GJs9nhpRXOtWkHZZerfcQH/xaXb9zxqIflkNSLTfDM88nLTDrL735xc4ta6pqlRdaog6sUODQerFYXavrG0Wz865YRswGkKIz8o6jN7SBQ7YEIVErtZ0IBO1oo8M8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=ideasonboard.com; spf=pass smtp.mailfrom=ideasonboard.com; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b=QxC5qfu7; arc=none smtp.client-ip=213.167.242.64 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="QxC5qfu7" Received: from killaraus.ideasonboard.com (2001-14ba-703d-e500--2a1.rev.dnainternet.fi [IPv6:2001:14ba:703d:e500::2a1]) by perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 41AC9225; Tue, 24 Mar 2026 01:19:07 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1774311547; bh=3dosacfdbH6jC2r+obvt3527gGj3wloWAca625hnbzk=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=QxC5qfu77/bbG5bMDYgEFxOZU7C/EgRAm0Sy2vfLaUe+h9UtfH8hxop0k8RJsYAQ8 3qiUFbyw/JHuN4yR4iaO4QPwzEzbQkkrIz4yi5RIR+5gYYdBqTxcbvfpJGPTOO3r2O DFj8Ff/MRqGNu/g73/9OliQdOoBiTggsZSWJrUbo= Date: Tue, 24 Mar 2026 02:20:22 +0200 From: Laurent Pinchart To: Ricardo Ribalda Cc: Hans de Goede , Mauro Carvalho Chehab , linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, Yunke Cao , stable@kernel.org Subject: Re: [PATCH v5 1/2] media: uvcvideo: Fix sequence number when no EOF Message-ID: <20260324002022.GC2334070@killaraus.ideasonboard.com> References: <20260323-uvc-fid-v5-0-e2858b657aac@chromium.org> <20260323-uvc-fid-v5-1-e2858b657aac@chromium.org> Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20260323-uvc-fid-v5-1-e2858b657aac@chromium.org> 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 > Closes: https://lore.kernel.org/linux-media/CANiDSCuj4cPuB5_v2xyvAagA5FjoN8V5scXiFFOeD3aKDMqkCg@mail.gmail.com/T/#me39fb134e8c2c085567a31548c3403eb639625e4 > Signed-off-by: Ricardo Ribalda > --- > 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. The patch otherwise looks fine, I don't see anything below this code that would need to be performed before. Reviewed-by: Laurent Pinchart > + 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