On Fri, Nov 08, 2024 at 07:22:01PM +0100, Ricardo Ribalda wrote:
> On Fri, 8 Nov 2024 at 15:24, Isaac Scott <isaac.scott@xxxxxxxxxxxxxxxx> wrote:
> >
> > Some cameras, such as the Sonix Technology Co. 292A exhibit issues when
> > running two parallel streams, causing USB packets to be dropped when an
> > H.264 stream posts a keyframe while an MJPEG stream is running
> > simultaneously. This occasionally causes the driver to erroneously
> > output two consecutive JPEG images as a single frame.
> >
> > To fix this, we inspect the buffer, and trigger a new frame when we
> > find an SOI, inverting the FID to make sure no frames are erroneously
> > dropped.
> >
> > Signed-off-by: Isaac Scott <isaac.scott@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/media/usb/uvc/uvc_video.c | 24 ++++++++++++++++++++++++
> > 1 file changed, 24 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index 2fb9f2b59afc..f754109f5e96 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -1211,6 +1211,30 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
> > return -EAGAIN;
> > }
> >
> > + /*
> > + * Some cameras, such as the Sonix Technology Co. 292A exhibit issues
> > + * when running two parallel streams, causing USB packets to be dropped
> > + * when an H.264 stream posts a keyframe while an MJPEG stream is
> > + * running simultaneously. This occasionally causes the driver to
> > + * erroneously output two consecutive JPEG images as a single frame.
The last sentence belongs to the commit message, not here, because once
the patch will be merged, this won't be true anymore.
Could you describe here what the device does in a bit more details ? I
think it's important to explain how the data is transferred, what
packets are lost, and why checking the first two bytes of the data is
the right quirk as opposed to having to search for the marker within the
whole packet.
> > + *
> > + * Check the buffer for a new SOI on JPEG streams and complete the
> > + * preceding buffer using EAGAIN, and invert the FID to make sure the
> > + * erroneous frame is not dropped.
> > + */
> > + 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;
>
> Don't you have to validate that data[header_len+1]
> can be read?
>
> > +
> > + if ((packet[0] == 0xff && packet[1] == 0xd8) && buf->bytesused != 0) {
>
> nit: maybe it would be nice to make a define for 0xd8 and say what it is
JPEG_MARKER_SOI in include/media/jpeg.h :-)
> > + 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
