From: Kirill A. Korinsky Subject: Re: sys/uvideo: don't skip frame immediately when buffer unavailable (regression from v1.256) To: marcus@nazgul.ch, tech@openbsd.org, ian@darwinsys.com Date: Wed, 16 Apr 2025 17:18:58 +0200 On Sun, 06 Apr 2025 21:44:24 +0200, Kirill A. Korinsky wrote: > > On Sun, 06 Apr 2025 15:54:03 +0200, > Kirill A. Korinsky wrote: > > > > On Sun, 06 Apr 2025 11:22:15 +0200, > > Marcus Glocker wrote: > > > > > > On Sun, Apr 06, 2025 at 10:44:36AM GMT, Kirill A. Korinsky wrote: > > > > > > > > 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? > > > > > > Fine for me to revert the bcopy() performance diff for the release. > > > > > > ok mglocker@ > > > > > > > Thanks, it is reverted. > > > > Meanwhile I had played with different screen size and frame rate and had > > found that some settings which triggers on my affected device some glitches > > with any tested version of uvideo when I had enabled UVIDEO_DEBUG and > > increases uvideo_debug to 2. As a base line to proof that it doesn't recent > > changes I've used 1.221. > > > > If I touches framerate or screen size, it behaves differently. Threshold is > > 10 FPS, and if I consume more than 10 FPS I haven't got major glitches. With > > 15 FPS or more I haven't got any kind of glitches on tested version. > > > > So, I have: > > > > - uvideo.c,v 1.221 and 1.257 some micro glitches time to time when I had > > enabled debug, and major glitches when I also increased uvideo_debug. > > > > - uvideo.c,v 1.256 major gliteches, screenshot: > > https://kirill.korins.ky/pub/uvideo-motion-regression/screenshot-2025-04-06_13-19-56.png > > > > - uvideo.c,v 1.256 + diff from a thread better, but still see some glitches > > with enabled debug. > > > > BTW I had keep samples of debug log and differences with motion.conf here: > > https://kirill.korins.ky/pub/uvideo-motion-regression/ > > > > Anyway, base on all that I said, I don't think that bcopy diff is a root > > cause of the issue. I think that that we already have the issue, but bcopy > > diff uncovers it on some device and settings as side effect, which allows us > > to notice it. > > > > Here a bit improved version of remove bcopy again. On my tests I haven't see > any differences with 1.257 with or without increased debug. I mean that > without debug it hasn't got any glitches, with it has, but amount matches > that I see on 1.221 and 1.257. > > May I ask you to try it? > The tree is unlocked, and I really would like to put it back. But before Ian, Markus, may I ask you to try this diff on your hardware? I don't see any differences with and without it on my devices. Index: sys/dev/usb/uvideo.c =================================================================== RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v diff -u -p -r1.257 uvideo.c --- sys/dev/usb/uvideo.c 6 Apr 2025 09:46:30 -0000 1.257 +++ sys/dev/usb/uvideo.c 6 Apr 2025 19:33:21 -0000 @@ -66,6 +66,7 @@ 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; @@ -101,7 +102,7 @@ struct uvideo_softc { const struct uvideo_devs *sc_quirk; void (*sc_decode_stream_header) (struct uvideo_softc *, - uint8_t *, int); + uint8_t *, int, uint8_t *); }; int uvideo_open(void *, int, int *, uint8_t *, void (*)(void *), @@ -167,11 +168,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 *, int, uint8_t *); void uvideo_vs_decode_stream_header_isight(struct uvideo_softc *, - uint8_t *, int); -int uvideo_get_buf(struct uvideo_softc *); -void uvideo_mmap_queue(struct uvideo_softc *, uint8_t *, int, int); + uint8_t *, int, uint8_t *); +uint8_t * uvideo_mmap_getbuf(struct uvideo_softc *); +void uvideo_mmap_queue(struct uvideo_softc *, 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); @@ -2219,6 +2220,7 @@ 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) { @@ -2245,7 +2247,13 @@ uvideo_vs_start_bulk_thread(void *arg) DPRINTF(2, "%s: *** buffer len = %d\n", DEVNAME(sc), size); - sc->sc_decode_stream_header(sc, sc->sc_vs_cur->bxfer.buf, size); + if (sc->sc_mmap_flag) + buf = uvideo_mmap_getbuf(sc); + else + buf = sc->sc_frame_buffer.buf; + + sc->sc_decode_stream_header(sc, sc->sc_vs_cur->bxfer.buf, size, + buf); } usbd_ref_decr(sc->sc_udev); @@ -2296,7 +2304,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; + uint8_t *frame, *buf; DPRINTF(2, "%s: %s\n", DEVNAME(sc), __func__); @@ -2319,7 +2327,12 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi /* frame is empty */ continue; - sc->sc_decode_stream_header(sc, frame, frame_size); + if (sc->sc_mmap_flag) + buf = uvideo_mmap_getbuf(sc); + else + buf = sc->sc_frame_buffer.buf; + + sc->sc_decode_stream_header(sc, frame, frame_size, buf); } skip: /* setup new transfer */ @@ -2328,7 +2341,7 @@ skip: /* setup new transfer */ void uvideo_vs_decode_stream_header(struct uvideo_softc *sc, uint8_t *frame, - int frame_size) + int frame_size, uint8_t *buf) { struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer; struct usb_video_stream_header *sh; @@ -2391,7 +2404,7 @@ uvideo_vs_decode_stream_header(struct uv fb->error = 1; } if (sample_len > 0) { - bcopy(frame + sh->bLength, fb->buf + fb->offset, sample_len); + bcopy(frame + sh->bLength, buf + fb->offset, sample_len); fb->offset += sample_len; } @@ -2409,7 +2422,7 @@ uvideo_vs_decode_stream_header(struct uv if (sc->sc_mmap_flag) { /* mmap */ - uvideo_mmap_queue(sc, fb->buf, fb->offset, fb->error); + uvideo_mmap_queue(sc, fb->offset, fb->error); } else if (fb->error) { DPRINTF(1, "%s: %s: error frame, skipped!\n", DEVNAME(sc), __func__); @@ -2442,7 +2455,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) + int frame_size, uint8_t *buf) { struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer; int sample_len, header = 0; @@ -2463,7 +2476,7 @@ uvideo_vs_decode_stream_header_isight(st if (header) { if (sc->sc_mmap_flag) { /* mmap */ - uvideo_mmap_queue(sc, fb->buf, fb->offset, 0); + uvideo_mmap_queue(sc, fb->offset, 0); } else { /* read */ uvideo_read(sc, fb->buf, fb->offset); @@ -2473,17 +2486,30 @@ uvideo_vs_decode_stream_header_isight(st /* save sample */ sample_len = frame_size; if ((fb->offset + sample_len) <= fb->buf_size) { - bcopy(frame, fb->buf + fb->offset, sample_len); + bcopy(frame, buf + fb->offset, sample_len); fb->offset += sample_len; } } } -int -uvideo_get_buf(struct uvideo_softc *sc) +uint8_t * +uvideo_mmap_getbuf(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_frame_buffer.offset > 0) + return sc->sc_frame_buffer.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 & @@ -2497,51 +2523,54 @@ uvideo_get_buf(struct uvideo_softc *sc) sc->sc_mmap_buffer_idx = 0; } - if (i == sc->sc_mmap_count) - return -1; + if (i == sc->sc_mmap_count) { + DPRINTF(1, "%s: %s: mmap queue is full!\n", + DEVNAME(sc), __func__); + return sc->sc_frame_buffer.buf; + } - return idx; + sc->sc_mmap_cur = &sc->sc_mmap[idx]; + + return sc->sc_mmap_cur->buf; } void -uvideo_mmap_queue(struct uvideo_softc *sc, uint8_t *buf, int len, int err) +uvideo_mmap_queue(struct uvideo_softc *sc, int len, int err) { - int i; + uint8_t *buf; - if (sc->sc_mmap_count == 0 || sc->sc_mmap_buffer == NULL) - panic("%s: mmap buffers not allocated", __func__); + if (sc->sc_mmap_cur == NULL) { + sc->sc_frame_buffer.offset = 0; + buf = uvideo_mmap_getbuf(sc); - /* 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; + if (buf == sc->sc_frame_buffer.buf) + return; + + bcopy(sc->sc_frame_buffer.buf, buf, 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; + /* report frame length */ + sc->sc_mmap_cur->v4l2_buf.bytesused = len; /* timestamp it */ - 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; + 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; /* forward error bit */ - sc->sc_mmap[i].v4l2_buf.flags &= ~V4L2_BUF_FLAG_ERROR; + sc->sc_mmap_cur->v4l2_buf.flags &= ~V4L2_BUF_FLAG_ERROR; if (err) - sc->sc_mmap[i].v4l2_buf.flags |= V4L2_BUF_FLAG_ERROR; + sc->sc_mmap_cur->v4l2_buf.flags |= V4L2_BUF_FLAG_ERROR; /* queue it */ - 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); + 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__); wakeup(sc); @@ -3507,6 +3536,7 @@ 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;