Hi Isaac,
Thank you for the patch.
On Mon, Nov 11, 2024 at 05:36:38PM +0000, Isaac Scott wrote:
> Some cameras, such as the Sonix Technology Co. 292A exhibit issues when
Nitpicking, s/ exhibit/, exhibit/
> 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..028f16dc189a 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -21,6 +21,7 @@
> #include <linux/unaligned.h>
>
> #include <media/v4l2-common.h>
> +#include <media/jpeg.h>
Alphabetical order please. This helps avoiding duplicates in long lists
of includes as they can be spotted more easily.
>
> #include "uvcvideo.h"
>
> @@ -1117,6 +1118,7 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
> struct uvc_buffer *buf, const u8 *data, int len)
> {
> u8 fid;
> + u8 header_len;
The kernel has a tendency to roughly sort variables by decreasing line
length, and the uvcvideo driver follows that rule when nothing else
makes it impractical.
>
> /*
> * Sanity checks:
> @@ -1129,6 +1131,8 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
> return -EINVAL;
> }
>
> + header_len = data[0];
> +
And I'd drop the blank line here.
> fid = data[1] & UVC_STREAM_FID;
>
> /*
> @@ -1210,9 +1214,30 @@ 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.
I would add "for these devices" at the end of the sentence. I expect it
to be universally true, but we haven't checked.
> + */
> + 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;
No need for the outer parentheses.
> +
> + if (len - header_len > 2 &&
I would have written
if (len > header_len + 2 &&
and shouldn't it actually be
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..116b1e383c25 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
Please use tabs for indentation, like in the previous lines.
>
> /* Format flags */
> #define UVC_FMT_FLAG_COMPRESSED 0x00000001
--
Regards,
Laurent Pinchart
