From: Antoine Jacoutot Subject: Re: sys/uvideo: forward error bit to consumer To: tech@openbsd.org, sdk@openbsd.org, mglocker@openbsd.org Date: Fri, 20 Dec 2024 15:30:49 +0100 On Thu, Dec 19, 2024 at 11:49:34PM +0100, 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? Oh you rock! The webcam on my Lenovo x13 is finally working! :-) addr 02: 174f:1812 , Integrated Camera high speed, power 500 mA, config 1, rev 10.22, iSerial 0001 driver: uvideo0 driver: uvideo1 driver: ugen0 uvideo0 at uhub1 port 4 configuration 1 interface 0 " Integrated Camera" rev 2.01/10.22 addr 2 video0 at uvideo0 Thanks a lot. > 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); > - if (sh->bLength == frame_size && !(sh->bFlags & UVIDEO_SH_FLAG_EOF)) { > - /* stream header without payload and no EOF */ > - return (USBD_INVAL); > - } > - 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 > -- Antoine