From: Kirill A. Korinsky Subject: Re: sys/uvideo: forward error bit to consumer To: Marcus Glocker Cc: OpenBSD tech , sdk@openbsd.org Date: Sat, 21 Dec 2024 19:04:15 +0100 On Sat, 21 Dec 2024 18:27:41 +0100, Marcus Glocker wrote: > > 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. > Thanks for testing! So far I had confirmation that this diff fixes the integrated camera on: - ThinkPad T14 Gen 5 - ThinkPad X1 nano 2 - Lenovo x13 Also, this diff is required to make progress on the Elgato HD60 S+ support which was reported at https://marc.info/?l=openbsd-bugs&m=173419416326742&w=2 This diff isn't enough, some other hacks are required, and I need more time to find the right way and understand the issue. > > @@ -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? > ThinkPad T14 Gen 5 cam sends frames with bLength 176 bytes which is requied to stabilize the picture and prevent it from glithcing. > > - 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? > Yes, it does. But it sends header which may contain PTS or SCR. uvideo.c doesn't understand it, by consumer probably does. Thus, here a bit updated version of the diff where I send to consumer too small frame as well which seems required for NV12 stream for Elgato HD60 S+ I had tested on my camers and it works as before. BTW Linux does the same: marks frames as error and sends it to consumer. 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 20 Dec 2024 17:41:44 -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,31 +2206,34 @@ 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", + DPRINTF(1, "%s: %s: frame too small, marked as error\n", DEVNAME(sc), __func__); - } else { + fb->error = 1; + } + #ifdef UVIDEO_DUMP - /* do the file write in process context */ - usb_rem_task(sc->sc_udev, &sc->sc_task_write); - usb_add_task(sc->sc_udev, &sc->sc_task_write); + /* do the file write in process context */ + usb_rem_task(sc->sc_udev, &sc->sc_task_write); + usb_add_task(sc->sc_udev, &sc->sc_task_write); #endif - if (sc->sc_mmap_flag) { - /* mmap */ - if (uvideo_mmap_queue(sc, fb->buf, fb->offset)) - return (USBD_NOMEM); - } else { - /* read */ - uvideo_read(sc, fb->buf, fb->offset); - } + if (sc->sc_mmap_flag) { + /* mmap */ + 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); } fb->sample = 0; fb->fid = 0; + fb->error = 0; } return (USBD_NORMAL_COMPLETION); @@ -2270,7 +2278,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 +2298,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 +2322,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 20 Dec 2024 17:41:44 -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