Hi Isaac
Reviewed-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
On Fri, 29 Nov 2024 at 11:36, Isaac Scott <isaac.scott@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Ricardo,
>
> I hope you are well!
>
> On Thu, 2024-11-28 at 17:16 +0100, Ricardo Ribalda wrote:
> > On Thu, 28 Nov 2024 at 15:53, 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..6d800a099749 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 = data + header_len;
> > > +
> > > + if (len >= header_len + 2 &&
> > > + packet[0] == 0xff && packet[1] ==
> > > JPEG_MARKER_SOI &&
> > > + buf->bytesused != 0) {
> > nit: !buf->bytesused (please ignore if you prefer your way)
> > > + buf->state = UVC_BUF_STATE_READY;
> > > + buf->error = 1;
> >
> > I have a question. Lets say that you have two frames: A and B, each
> > one has 4 packets:
> > A1A2A3A4B1B2B3B4
> > The last package of A is lost because the device is non-compliant.
> > A1A2A3B1B2B3B4
> >
> > You detect this by inspecting every packet, and checking for the
> > values 0xff, JPEG_MARKER_SOI at the beggining of the packet.
> >
> > Can't that value happen in the middle of the image, let's say in A2,
> > A3, B2, B3... ? If that happens, won't you be losing frames?
> >
>
> I have found that in MJPEG, it is required to have both an SOI (0xFFD8)
> and an EOI (0xFFD9) in every payload.
Thanks a lot for checking it out.
If you happen to make a new version, that would be a very nice info to
add to the comment.
>
> Source: p.16, USB Device Class Definition for Video Devices: MJPEG
> Payload
> (https://usb.org/document-library/video-class-v15-document-set))
>
> Furthermore, the JPEG standard also explicitly defines 0xFFD8 to be the
> start of image marker, meaning its usage outside that functionality
> would not adhere to the standard. If it appears in the middle of a
> payload, the payload should be marked as invalid.
>
> Source: p. 32, Digital Compression and Coding of Continuous-Tone Still
> Images - Requirements and Guidelines
> (https://www.w3.org/Graphics/JPEG/itu-t81.pdf)
>
> > Also, If I get it right, the device not only loses the packet A4, but
> > it sends the wrong fid for all the Bx packets?
>
> Before the patch, B would be joined into A, and gets delivered to user
> space as A1, A2, A3, A4, A5, A6, A7, A8, C1, C2, C3...
>
So it seems like the fid value does not change during all the A...
Thanks again and sorry for not raising the questions before.
>
> > Maybe the device is not losing A4 but sending wrong fids? Have you
> > tried not setting buf->error=1 and inspecting the "invalid" image?
> >
>
> I saw during the diagnosis of the issue by analysing the USB packets
> sent by the camera that the packet containing the EOF does not get sent
> whatsoever when the two streams are running simultaneously.
>
> > I am not saying that it is incorrect. I am just trying to understand
> > the patch better. :)
> >
> >
> > > + stream->last_fid ^= UVC_STREAM_FID;
> > It would be nice to have uvc_dbg() here, in case we want to debug
> > what
> > is going on.
> > > + 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
> > > --
> > > 2.43.0
> > >
> > >
> >
> >
>
> Best wishes,
>
> Isaac
--
Ricardo Ribalda
