Download raw body.
sys/uvideo: forward error bit to consumer
On Thu, Dec 19, 2024 at 11:49:34PM GMT, Kirill A. Korinsky wrote:
> tech@,
>
> Some invalid frames or frames with an error flag from the camera might be
> essential for a stream consumer for synchronization and other purposes.
> Without this, the consumer may be unable to synchronize the stream or play
> it at all.
>
> Instead of skipping frames that the driver considers incorrect, this change
> forwards them to the V4L consumer with the V4L2_BUF_FLAG_ERROR flag set,
> allowing the consumer to decide whether to skip the frame or use parts of it.
>
> This change fixes the embedded webcam on the ThinkPad T14 Gen 5, which was
> tested by sdk@.
>
> The behavior for non-mmap consumers remains unchanged, and error frames
> are still skipped by the driver.
>
> I had tested this diff with the following webcams and no regressions
> were noticed:
> - Azurewave, USB camera
> - LG UltraFine Display Camera
> - Jabra PanaCast 20
>
> Feedback? Tests? Ok?
Makes basically sense to me, and it seems to fix a couple of cams what
I could read on the feedback's. Also it didn't cause any regression
with my cams. Though, two questions in-line.
> Index: sys/dev/usb/uvideo.c
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
> diff -u -p -r1.228 uvideo.c
> --- sys/dev/usb/uvideo.c 14 Dec 2024 09:58:04 -0000 1.228
> +++ sys/dev/usb/uvideo.c 18 Dec 2024 09:49:15 -0000
> @@ -168,7 +168,7 @@ usbd_status uvideo_vs_decode_stream_head
> uint8_t *, int);
> usbd_status uvideo_vs_decode_stream_header_isight(struct uvideo_softc *,
> uint8_t *, int);
> -int uvideo_mmap_queue(struct uvideo_softc *, uint8_t *, int);
> +int uvideo_mmap_queue(struct uvideo_softc *, uint8_t *, int, int);
> void uvideo_read(struct uvideo_softc *, uint8_t *, int);
> usbd_status uvideo_usb_control(struct uvideo_softc *, uint8_t, uint8_t,
> uint16_t, uint8_t *, size_t);
> @@ -2148,18 +2148,9 @@ uvideo_vs_decode_stream_header(struct uv
>
> DPRINTF(2, "%s: stream header len = %d\n", DEVNAME(sc), sh->bLength);
>
> - if (sh->bLength > UVIDEO_SH_MAX_LEN || sh->bLength < UVIDEO_SH_MIN_LEN)
> + if (sh->bLength > frame_size || sh->bLength < UVIDEO_SH_MIN_LEN)
> /* invalid header size */
> return (USBD_INVAL);
This check is there to make sure that the frame header isn't exceeding
the max frame header size defined in the specs. Now when changing that
to check if it exceeds the entire frame size, what use case does this
cover?
> - if (sh->bLength == frame_size && !(sh->bFlags & UVIDEO_SH_FLAG_EOF)) {
> - /* stream header without payload and no EOF */
> - return (USBD_INVAL);
> - }
If I remember correctly, this check was introduced since some cams did
occasionally just sent a frame header without payload. You remove it
because we also want to pass this case to the consumer?
> - if (sh->bFlags & UVIDEO_SH_FLAG_ERR) {
> - /* stream error, skip xfer */
> - DPRINTF(1, "%s: %s: stream error!\n", DEVNAME(sc), __func__);
> - return (USBD_CANCELLED);
> - }
>
> DPRINTF(2, "%s: frame_size = %d\n", DEVNAME(sc), frame_size);
>
> @@ -2178,6 +2169,7 @@ uvideo_vs_decode_stream_header(struct uv
> fb->sample = 1;
> fb->fid = sh->bFlags & UVIDEO_SH_FLAG_FID;
> fb->offset = 0;
> + fb->error = 0;
> } else {
> /* continues sample for a frame, check consistency */
> if (fb->fid != (sh->bFlags & UVIDEO_SH_FLAG_FID)) {
> @@ -2186,12 +2178,25 @@ uvideo_vs_decode_stream_header(struct uv
> fb->sample = 1;
> fb->fid = sh->bFlags & UVIDEO_SH_FLAG_FID;
> fb->offset = 0;
> + fb->error = 0;
> }
> }
>
> + if (sh->bFlags & UVIDEO_SH_FLAG_ERR) {
> + /* stream error, skip xfer */
> + DPRINTF(1, "%s: %s: stream error!\n", DEVNAME(sc), __func__);
> + fb->error = 1;
> + }
> +
> /* save sample */
> sample_len = frame_size - sh->bLength;
> - if ((fb->offset + sample_len) <= fb->buf_size) {
> + if (sample_len > fb->buf_size - fb->offset) {
> + DPRINTF(1, "%s: %s: frame too large, marked as error\n",
> + DEVNAME(sc), __func__);
> + sample_len = fb->buf_size - fb->offset;
> + fb->error = 1;
> + }
> + if (sample_len > 0) {
> bcopy(frame + sh->bLength, fb->buf + fb->offset, sample_len);
> fb->offset += sample_len;
> }
> @@ -2201,10 +2206,7 @@ uvideo_vs_decode_stream_header(struct uv
> DPRINTF(2, "%s: %s: EOF (frame size = %d bytes)\n",
> DEVNAME(sc), __func__, fb->offset);
>
> - if (fb->offset > fb->buf_size) {
> - DPRINTF(1, "%s: %s: frame too large, skipped!\n",
> - DEVNAME(sc), __func__);
> - } else if (fb->offset < fb->buf_size &&
> + if (fb->offset < fb->buf_size &&
> !(fb->fmt_flags & V4L2_FMT_FLAG_COMPRESSED)) {
> DPRINTF(1, "%s: %s: frame too small, skipped!\n",
> DEVNAME(sc), __func__);
> @@ -2216,8 +2218,12 @@ uvideo_vs_decode_stream_header(struct uv
> #endif
> if (sc->sc_mmap_flag) {
> /* mmap */
> - if (uvideo_mmap_queue(sc, fb->buf, fb->offset))
> + if (uvideo_mmap_queue(sc, fb->buf, fb->offset,
> + fb->error))
> return (USBD_NOMEM);
> + } else if (fb->error) {
> + DPRINTF(1, "%s: %s: error frame, skipped!\n",
> + DEVNAME(sc), __func__);
> } else {
> /* read */
> uvideo_read(sc, fb->buf, fb->offset);
> @@ -2226,6 +2232,7 @@ uvideo_vs_decode_stream_header(struct uv
>
> fb->sample = 0;
> fb->fid = 0;
> + fb->error = 0;
> }
>
> return (USBD_NORMAL_COMPLETION);
> @@ -2270,7 +2277,7 @@ uvideo_vs_decode_stream_header_isight(st
> if (header) {
> if (sc->sc_mmap_flag) {
> /* mmap */
> - if (uvideo_mmap_queue(sc, fb->buf, fb->offset))
> + if (uvideo_mmap_queue(sc, fb->buf, fb->offset, 0))
> return (USBD_NOMEM);
> } else {
> /* read */
> @@ -2290,7 +2297,7 @@ uvideo_vs_decode_stream_header_isight(st
> }
>
> int
> -uvideo_mmap_queue(struct uvideo_softc *sc, uint8_t *buf, int len)
> +uvideo_mmap_queue(struct uvideo_softc *sc, uint8_t *buf, int len, int err)
> {
> int i;
>
> @@ -2314,6 +2321,11 @@ uvideo_mmap_queue(struct uvideo_softc *s
>
> /* timestamp it */
> getmicrotime(&sc->sc_mmap[i].v4l2_buf.timestamp);
> +
> + /* forward error bit */
> + sc->sc_mmap[i].v4l2_buf.flags &= ~V4L2_BUF_FLAG_ERROR;
> + if (err)
> + sc->sc_mmap[i].v4l2_buf.flags |= V4L2_BUF_FLAG_ERROR;
>
> /* queue it */
> sc->sc_mmap[i].v4l2_buf.flags |= V4L2_BUF_FLAG_DONE;
> Index: sys/dev/usb/uvideo.h
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/usb/uvideo.h,v
> diff -u -p -r1.60 uvideo.h
> --- sys/dev/usb/uvideo.h 8 Dec 2019 13:21:21 -0000 1.60
> +++ sys/dev/usb/uvideo.h 18 Dec 2024 09:30:26 -0000
> @@ -449,6 +449,7 @@ struct uvideo_vs_iface {
> struct uvideo_frame_buffer {
> int sample;
> uint8_t fid;
> + uint8_t error;
> int offset;
> int buf_size;
> uint8_t *buf;
>
>
> --
> wbr, Kirill
>
sys/uvideo: forward error bit to consumer