| Message ID | 20241108142310.19794-1-isaac.scott@ideasonboard.com |
|---|---|
| Headers | show |
| Series | Fix Sonix Technology MJPEG streams | expand |
Hi Isaac nit: If you have to redo the set I would recommend to squash this patch to the next one. On Fri, 8 Nov 2024 at 16:43, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Isaac, > > Thank you for the patch. > > On Fri, Nov 08, 2024 at 02:23:08PM +0000, Isaac Scott wrote: > > The uvc_video_decode_start function returns the first byte of the header, > > which is in fact the length of the header. Improve readability by using an > > appropriately named variable. > > > > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> > > --- > > drivers/media/usb/uvc/uvc_video.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > index e00f38dd07d9..2fb9f2b59afc 100644 > > --- a/drivers/media/usb/uvc/uvc_video.c > > +++ b/drivers/media/usb/uvc/uvc_video.c > > @@ -1117,6 +1117,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 = data[0]; > > Is there a guarantee at this point, before the sanity checks below, that > len is not zero, that is, that it's safe to read data[0] ? > > > > > /* > > * Sanity checks: > > @@ -1212,7 +1213,7 @@ static int uvc_video_decode_start(struct uvc_streaming *stream, > > > > stream->last_fid = fid; > > > > - return data[0]; > > + return header_len; > > } > > > > static inline enum dma_data_direction uvc_stream_dir( > > -- > Regards, > > Laurent Pinchart >
On Fri, 8 Nov 2024 at 15:24, 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, inverting the FID to make sure no frames are erroneously > dropped. > > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> > --- > 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. > + * > + * 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 > + buf->state = UVC_BUF_STATE_READY; > + buf->error = 1; > + stream->last_fid ^= UVC_STREAM_FID; > + return -EAGAIN; > + } > + } > + > stream->last_fid = fid; > > return header_len; > -- > 2.43.0 > >
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@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, inverting the FID to make sure no frames are erroneously > > dropped. > > > > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com> > > --- > > 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;
Some cameras, such as the Sonix Technology Co. 292A, allow multiple streams to be sent over the same USB device. It was found during testing that occasionally, when one H.264 stream sends a new keyframe during an MJPEG stream, a URB buffer would be lost, and a frame lost in the resulting capture. This was because some EOF markers in URB buffers are lost. This particular use case is not covered by the uvc_video driver, which causes two consecutive buffers to be erroneously considered to be as part of the same buffer, and lead to the whole erroneous buffer to be dropped. This can be seen when using the current driver, as frames occasionally are double the expected size, containing two images. Furthermore, the interval between the frames was double that of what it should be (~66ms as opposed to the usual ~33ms). Upon further investigation, it was found that the packets within the erroneous buffer were being dropped. The changes in this series supply a new quirk for the UVC Driver, which cause the buffers of MJPEG streams to be closed when a new JPEG SOI marker is discovered, and be treated as a new buffer. This works around the missing EOF marker and separates the buffers. Tested on v6.12-rc3 Isaac Scott (3): media: uvcvideo: Introduce header length media: uvcvideo: Add new quirk definition for the Sonix Technology Co. 292a camera media: uvcvideo: Implement dual stream quirk to fix loss of usb packets drivers/media/usb/uvc/uvc_driver.c | 9 +++++++++ drivers/media/usb/uvc/uvc_video.c | 27 ++++++++++++++++++++++++++- drivers/media/usb/uvc/uvcvideo.h | 1 + 3 files changed, 36 insertions(+), 1 deletion(-)