From: Ian Darwin Subject: Re: sys/uvideo: don't skip frame immediately when buffer unavailable (regression from v1.256) To: tech@openbsd.org Date: Thu, 17 Apr 2025 10:45:21 -0400 On 4/17/25 4:00 AM, Kirill A. Korinsky wrote: > Markus, Ian, > > here a future improvment of this diff, this time I stricly requires that > mmap buffer is switched or "grab" only for the first sample of the frame. > > I can't reproduce glitches on my motion test case. May I ask you to try this > one? This one also works fine for me with motion, video(1), and luvcview. > > 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 17 Apr 2025 08:00:03 -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; > @@ -170,8 +171,8 @@ void uvideo_vs_decode_stream_header(str > uint8_t *, int); > 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 * 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); > @@ -2333,6 +2334,7 @@ uvideo_vs_decode_stream_header(struct uv > struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer; > struct usb_video_stream_header *sh; > int sample_len; > + uint8_t *buf; > > if (frame_size < UVIDEO_SH_MIN_LEN) > /* frame too small to contain a valid stream header */ > @@ -2364,16 +2366,17 @@ uvideo_vs_decode_stream_header(struct uv > fb->fid = sh->bFlags & UVIDEO_SH_FLAG_FID; > fb->offset = 0; > fb->error = 0; > + } else if (fb->fid != (sh->bFlags & UVIDEO_SH_FLAG_FID)) { > + /* first sample for a frame, due to flip FID */ > + DPRINTF(1, "%s: %s: wrong FID, ignore last frame!\n", > + DEVNAME(sc), __func__); > + 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)) { > - DPRINTF(1, "%s: %s: wrong FID, ignore last frame!\n", > - DEVNAME(sc), __func__); > - fb->sample = 1; > - fb->fid = sh->bFlags & UVIDEO_SH_FLAG_FID; > - fb->offset = 0; > - fb->error = 0; > - } > + /* continues sample for a frame */ > + fb->sample++; > } > > if (sh->bFlags & UVIDEO_SH_FLAG_ERR) { > @@ -2382,6 +2385,11 @@ uvideo_vs_decode_stream_header(struct uv > fb->error = 1; > } > > + if (sc->sc_mmap_flag) > + buf = uvideo_mmap_getbuf(sc); > + else > + buf = sc->sc_frame_buffer.buf; > + > /* save sample */ > sample_len = frame_size - sh->bLength; > if (sample_len > fb->buf_size - fb->offset) { > @@ -2390,8 +2398,8 @@ uvideo_vs_decode_stream_header(struct uv > sample_len = fb->buf_size - fb->offset; > fb->error = 1; > } > - if (sample_len > 0) { > - bcopy(frame + sh->bLength, fb->buf + fb->offset, sample_len); > + if (buf && sample_len > 0) { > + bcopy(frame + sh->bLength, buf + fb->offset, sample_len); > fb->offset += sample_len; > } > > @@ -2409,7 +2417,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__); > @@ -2446,6 +2454,7 @@ uvideo_vs_decode_stream_header_isight(st > { > struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer; > int sample_len, header = 0; > + uint8_t *buf; > uint8_t magic[] = { > 0x11, 0x22, 0x33, 0x44, > 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xfa, 0xce }; > @@ -2463,27 +2472,52 @@ 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); > } > fb->offset = 0; > } else { > + if (sc->sc_mmap_flag) > + buf = uvideo_mmap_getbuf(sc); > + else > + buf = sc->sc_frame_buffer.buf; > + > + if (buf == NULL) > + return; > + > /* 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; > + > + /* use the next buffer only for the first frame's sample */ > + if (sc->sc_frame_buffer.sample > 1) { > + DPRINTF(1, "%s: %s: not the first sample: %d\n", > + DEVNAME(sc), __func__, sc->sc_frame_buffer.sample); > + return NULL; > + } > + > + 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 +2531,45 @@ 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 NULL; > + } > + > + sc->sc_mmap_cur = &sc->sc_mmap[idx]; > > - return 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; > - > - 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__); > + if (sc->sc_mmap_cur == NULL) > return; > - } > > - /* 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 +3535,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; >