| Message ID | 20241128145144.61475-2-isaac.scott@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix Sonix Technology MJPEG streams | expand |
On Thu, 28 Nov 2024 at 15:53, Isaac Scott <isaac.scott@ideasonboard.com> 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@ideasonboard.com> > --- > 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? 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? 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 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 > >
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@ideasonboard.com> 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@ideasonboard.com> > > --- > > 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. 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... > 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
Hi Isaac Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> On Fri, 29 Nov 2024 at 11:36, Isaac Scott <isaac.scott@ideasonboard.com> 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@ideasonboard.com> 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@ideasonboard.com> > > > --- > > > 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
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) { + 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
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@ideasonboard.com> --- drivers/media/usb/uvc/uvc_video.c | 27 ++++++++++++++++++++++++++- drivers/media/usb/uvc/uvcvideo.h | 1 + 2 files changed, 27 insertions(+), 1 deletion(-)