On Thu, Nov 14, 2024 at 08:47:52PM +0100, Ricardo Ribalda wrote:
> Hi Isaac
>
> I am curious... How is this v7?
I see on the list v1 (implicit, it's not versioned), v5 and v7. I'm
curious to know if there's a pattern, we need a few more versions to
figure it out :-)
Jokes aside, Isaac, you should increment the version number only when
sending a new public version.
> Remember to run
> scripts/checkpatch.pl --strict -g HEAD
>
> It is complaining about an open parenthesis match here. Not the end of
> the world, but rather fix it locally than discussing :)
>
> Totally optional... maybe you can add to the cover-letter a run of
> yavta -c /dev/videoX
> with and without the patch. So people can see practically what changes.
>
> Regards!
>
> On Tue, 12 Nov 2024 at 12:33, 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.
> >
> > Signed-off-by: Isaac Scott <isaac.scott@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/media/usb/uvc/uvc_video.c | 27 ++++++++++++++++++++++++++-
> > drivers/media/usb/uvc/uvcvideo.h | 1 +
> > 2 files changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index e00f38dd07d9..9bb41362c48d 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -20,6 +20,7 @@
> > #include <linux/atomic.h>
> > #include <linux/unaligned.h>
> >
> > +#include <media/jpeg.h>
> > #include <media/v4l2-common.h>
> >
> > #include "uvcvideo.h"
> > @@ -1116,6 +1117,7 @@ static void uvc_video_stats_stop(struct uvc_streaming *stream)
> > static int uvc_video_decode_start(struct uvc_streaming *stream,
> > struct uvc_buffer *buf, const u8 *data, int len)
> > {
> > + u8 header_len;
> > u8 fid;
> >
> > /*
> > @@ -1129,6 +1131,7 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
> > return -EINVAL;
> > }
> >
> > + header_len = data[0];
> > fid = data[1] & UVC_STREAM_FID;
> >
> > /*
> > @@ -1210,9 +1213,31 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
> > 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 = ((const u8 *)data) + header_len;
>
> Do you need the (const u8 *) casting here? . I believe data has
> exactly that datatype
It doesn't seem to be needed indeed.
Can you send a new version that fixes these two issues (checkpatch
warning and unnecessary cast), as well as address the comment on 2/2 ?
> > +
> > + 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 data[0];
> > + return header_len;
> > }
> >
> > static inline enum dma_data_direction uvc_stream_dir(
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index b7d24a853ce4..040073326a24 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -76,6 +76,7 @@
> > #define UVC_QUIRK_NO_RESET_RESUME 0x00004000
> > #define UVC_QUIRK_DISABLE_AUTOSUSPEND 0x00008000
> > #define UVC_QUIRK_INVALID_DEVICE_SOF 0x00010000
> > +#define UVC_QUIRK_MJPEG_NO_EOF 0x00020000
> >
> > /* Format flags */
> > #define UVC_FMT_FLAG_COMPRESSED 0x00000001
--
Regards,
Laurent Pinchart
