From: Kirill A. Korinsky Subject: Re: sys/uvideo: don't skip frame immediately when buffer unavailable (regression from v1.256) To: Marcus Glocker Cc: tech@openbsd.org, Ian Darwin Date: Sun, 06 Apr 2025 10:44:36 +0200 On Sun, 06 Apr 2025 09:12:42 +0200, Marcus Glocker wrote: > > On Fri, Apr 04, 2025 at 09:24:51AM GMT, Kirill A. Korinsky wrote: > > > tech@, > > > > this diff fixes a regression that was introduced by v1.256 and reported > > by ian@. > > > > The original change (v1.256), which avoided one bcopy, was essential to > > improve performance sufficiently for supporting 4K 60fps devices. > > > > However, some applications (e.g., multimedia/motion) release a buffer > > back to the driver just before the read. If this release happens after a > > frame packet has arrived, the logic introduced in v1.256 drops the > > frame. This dropped frame can be critical, especially for devices with > > low framerates, causing the stream to glitch. > > > > Therefore, instead of dropping the frame immediately, this fix parses > > the incoming frame into sc->sc_frame_buffer.buf. The data is then bcopy > > into shared memory after parsing is complete if buffers was released, > > restoring the behavior from before v1.256. > > > > Notably, this issue is application-dependent; for example, ffplay works > > correctly, while motion does not. > > > > ian@ has confirmed that this fix the issue on his affected device. > > > > Ok? > > As already discussed a bit on ICB: > > I don't exactly understand ian@ issue, and I also didn't see a bug > report on bugs@ for it which would explain it well. I can half way > imagine the issue based on your explanation. > > I also don't understand how your diff would fix what I did understand > from your issue explanation. > > In any case, since I'm also using motion; I don't see glitches with > my setup using motion with uvideo.c,1.256. And if I apply your diff, > *then* I'm seeing glitches, almost every 5-8 seconds. > > So based on the current information, it's pretty hard to discuss > about a possible solution. Maybe ian@ could provide a detailed bug > report on bugs@, showing a dmesg, 'ubsdevs -v' output, and his > motion.conf. Based on that, by any chance we could try to reproduce > the issue locally, and then discuss about a possible solution. > Well, I do not understand your issue but I can reproduce ian@ one on one of my webcam, and only one. So, I will prepare two debug outputs later today. But we're short in the time, this changes needed only to support new devices with high FPS... So, I suggest to revert it for release. Ok? Index: sys/dev/usb/uvideo.c =================================================================== RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v diff -u -p -r1.256 uvideo.c --- sys/dev/usb/uvideo.c 24 Mar 2025 18:56:34 -0000 1.256 +++ sys/dev/usb/uvideo.c 6 Apr 2025 08:41:40 -0000 @@ -66,7 +66,6 @@ struct uvideo_softc { struct uvideo_frame_buffer sc_frame_buffer; struct uvideo_mmap sc_mmap[UVIDEO_MAX_BUFFERS]; - struct uvideo_mmap *sc_mmap_cur; uint8_t *sc_mmap_buffer; size_t sc_mmap_buffer_size; int sc_mmap_buffer_idx; @@ -102,7 +101,7 @@ struct uvideo_softc { const struct uvideo_devs *sc_quirk; void (*sc_decode_stream_header) (struct uvideo_softc *, - uint8_t *, int, uint8_t *); + uint8_t *, int); }; int uvideo_open(void *, int, int *, uint8_t *, void (*)(void *), @@ -168,11 +167,11 @@ void uvideo_vs_start_isoc_ixfer(struct void uvideo_vs_cb(struct usbd_xfer *, void *, usbd_status); void uvideo_vs_decode_stream_header(struct uvideo_softc *, - uint8_t *, int, uint8_t *); + uint8_t *, int); void uvideo_vs_decode_stream_header_isight(struct uvideo_softc *, - uint8_t *, int, uint8_t *); -uint8_t * uvideo_mmap_getbuf(struct uvideo_softc *); -void uvideo_mmap_queue(struct uvideo_softc *, int, int); + uint8_t *, int); +int uvideo_get_buf(struct uvideo_softc *); +void 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); @@ -2220,7 +2219,6 @@ uvideo_vs_start_bulk_thread(void *arg) struct uvideo_softc *sc = arg; usbd_status error; int size; - uint8_t *buf; usbd_ref_incr(sc->sc_udev); while (sc->sc_vs_cur->bulk_running) { @@ -2247,14 +2245,7 @@ uvideo_vs_start_bulk_thread(void *arg) DPRINTF(2, "%s: *** buffer len = %d\n", DEVNAME(sc), size); - if (sc->sc_mmap_flag) { - if ((buf = uvideo_mmap_getbuf(sc)) == NULL) - continue; - } else - buf = sc->sc_frame_buffer.buf; - - sc->sc_decode_stream_header(sc, sc->sc_vs_cur->bxfer.buf, size, - buf); + sc->sc_decode_stream_header(sc, sc->sc_vs_cur->bxfer.buf, size); } usbd_ref_decr(sc->sc_udev); @@ -2305,7 +2296,7 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi struct uvideo_isoc_xfer *ixfer = priv; struct uvideo_softc *sc = ixfer->sc; int len, i, frame_size; - uint8_t *frame, *buf; + uint8_t *frame; DPRINTF(2, "%s: %s\n", DEVNAME(sc), __func__); @@ -2328,13 +2319,7 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi /* frame is empty */ continue; - if (sc->sc_mmap_flag) { - if ((buf = uvideo_mmap_getbuf(sc)) == NULL) - goto skip; - } else - buf = sc->sc_frame_buffer.buf; - - sc->sc_decode_stream_header(sc, frame, frame_size, buf); + sc->sc_decode_stream_header(sc, frame, frame_size); } skip: /* setup new transfer */ @@ -2343,7 +2328,7 @@ skip: /* setup new transfer */ void uvideo_vs_decode_stream_header(struct uvideo_softc *sc, uint8_t *frame, - int frame_size, uint8_t *buf) + int frame_size) { struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer; struct usb_video_stream_header *sh; @@ -2406,7 +2391,7 @@ uvideo_vs_decode_stream_header(struct uv fb->error = 1; } if (sample_len > 0) { - bcopy(frame + sh->bLength, buf + fb->offset, sample_len); + bcopy(frame + sh->bLength, fb->buf + fb->offset, sample_len); fb->offset += sample_len; } @@ -2424,7 +2409,7 @@ uvideo_vs_decode_stream_header(struct uv if (sc->sc_mmap_flag) { /* mmap */ - uvideo_mmap_queue(sc, fb->offset, fb->error); + uvideo_mmap_queue(sc, fb->buf, fb->offset, fb->error); } else if (fb->error) { DPRINTF(1, "%s: %s: error frame, skipped!\n", DEVNAME(sc), __func__); @@ -2457,7 +2442,7 @@ uvideo_vs_decode_stream_header(struct uv */ void uvideo_vs_decode_stream_header_isight(struct uvideo_softc *sc, uint8_t *frame, - int frame_size, uint8_t *buf) + int frame_size) { struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer; int sample_len, header = 0; @@ -2478,7 +2463,7 @@ uvideo_vs_decode_stream_header_isight(st if (header) { if (sc->sc_mmap_flag) { /* mmap */ - uvideo_mmap_queue(sc, fb->offset, 0); + uvideo_mmap_queue(sc, fb->buf, fb->offset, 0); } else { /* read */ uvideo_read(sc, fb->buf, fb->offset); @@ -2488,27 +2473,17 @@ uvideo_vs_decode_stream_header_isight(st /* save sample */ sample_len = frame_size; if ((fb->offset + sample_len) <= fb->buf_size) { - bcopy(frame, buf + fb->offset, sample_len); + bcopy(frame, fb->buf + fb->offset, sample_len); fb->offset += sample_len; } } } -uint8_t * -uvideo_mmap_getbuf(struct uvideo_softc *sc) +int +uvideo_get_buf(struct uvideo_softc *sc) { int i, idx = sc->sc_mmap_buffer_idx; - /* - * Section 2.4.3.2 explicitly allows multiple frames per one - * transfer and multiple transfers per one frame. - */ - if (sc->sc_mmap_cur != NULL) - return sc->sc_mmap_cur->buf; - - if (sc->sc_mmap_count == 0 || sc->sc_mmap_buffer == NULL) - panic("%s: mmap buffers not allocated", __func__); - /* find a buffer which is ready for queueing */ for (i = 0; i < sc->sc_mmap_count; i++) { if (sc->sc_mmap[sc->sc_mmap_buffer_idx].v4l2_buf.flags & @@ -2522,45 +2497,51 @@ uvideo_mmap_getbuf(struct uvideo_softc * sc->sc_mmap_buffer_idx = 0; } - if (i == sc->sc_mmap_count) { - DPRINTF(1, "%s: %s: mmap queue is full!\n", - DEVNAME(sc), __func__); - return NULL; - } - - sc->sc_mmap_cur = &sc->sc_mmap[idx]; + if (i == sc->sc_mmap_count) + return -1; - return sc->sc_mmap_cur->buf; + return idx; } void -uvideo_mmap_queue(struct uvideo_softc *sc, int len, int err) +uvideo_mmap_queue(struct uvideo_softc *sc, uint8_t *buf, int len, int err) { - if (sc->sc_mmap_cur == NULL) - panic("uvideo_mmap_queue: NULL pointer!"); + int i; + + if (sc->sc_mmap_count == 0 || sc->sc_mmap_buffer == NULL) + panic("%s: mmap buffers not allocated", __func__); + + /* find a buffer which is ready for queueing */ + i = uvideo_get_buf(sc); + if (i == -1) { + DPRINTF(1, "%s: %s: mmap queue is full!\n", + DEVNAME(sc), __func__); + return; + } - /* report frame length */ - sc->sc_mmap_cur->v4l2_buf.bytesused = len; + /* copy frame to mmap buffer and report length */ + bcopy(buf, sc->sc_mmap[i].buf, len); + sc->sc_mmap[i].v4l2_buf.bytesused = len; /* timestamp it */ - getmicrouptime(&sc->sc_mmap_cur->v4l2_buf.timestamp); - sc->sc_mmap_cur->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TIMESTAMP_MASK; - sc->sc_mmap_cur->v4l2_buf.flags |= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; - sc->sc_mmap_cur->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK; - sc->sc_mmap_cur->v4l2_buf.flags |= V4L2_BUF_FLAG_TSTAMP_SRC_EOF; - sc->sc_mmap_cur->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TIMECODE; + getmicrouptime(&sc->sc_mmap[i].v4l2_buf.timestamp); + sc->sc_mmap[i].v4l2_buf.flags &= ~V4L2_BUF_FLAG_TIMESTAMP_MASK; + sc->sc_mmap[i].v4l2_buf.flags |= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; + sc->sc_mmap[i].v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK; + sc->sc_mmap[i].v4l2_buf.flags |= V4L2_BUF_FLAG_TSTAMP_SRC_EOF; + sc->sc_mmap[i].v4l2_buf.flags &= ~V4L2_BUF_FLAG_TIMECODE; /* forward error bit */ - sc->sc_mmap_cur->v4l2_buf.flags &= ~V4L2_BUF_FLAG_ERROR; + sc->sc_mmap[i].v4l2_buf.flags &= ~V4L2_BUF_FLAG_ERROR; if (err) - sc->sc_mmap_cur->v4l2_buf.flags |= V4L2_BUF_FLAG_ERROR; + sc->sc_mmap[i].v4l2_buf.flags |= V4L2_BUF_FLAG_ERROR; /* queue it */ - sc->sc_mmap_cur->v4l2_buf.flags |= V4L2_BUF_FLAG_DONE; - sc->sc_mmap_cur->v4l2_buf.flags &= ~V4L2_BUF_FLAG_QUEUED; - SIMPLEQ_INSERT_TAIL(&sc->sc_mmap_q, sc->sc_mmap_cur, q_frames); - sc->sc_mmap_cur = NULL; - DPRINTF(2, "%s: %s: frame queued\n", DEVNAME(sc), __func__); + sc->sc_mmap[i].v4l2_buf.flags |= V4L2_BUF_FLAG_DONE; + sc->sc_mmap[i].v4l2_buf.flags &= ~V4L2_BUF_FLAG_QUEUED; + SIMPLEQ_INSERT_TAIL(&sc->sc_mmap_q, &sc->sc_mmap[i], q_frames); + DPRINTF(2, "%s: %s: frame queued on index %d\n", + DEVNAME(sc), __func__, i); wakeup(sc); @@ -3526,7 +3507,6 @@ uvideo_reqbufs(void *v, struct v4l2_requ } sc->sc_mmap_buffer_idx = 0; - sc->sc_mmap_cur = NULL; /* tell how many buffers we have really allocated */ rb->count = sc->sc_mmap_count;