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 Date: Fri, 18 Apr 2025 22:11:36 +0200 On Fri, 18 Apr 2025 21:52:50 +0200, Marcus Glocker wrote: > > On Thu, Apr 17, 2025 at 10:00:55AM GMT, 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? > > The diff looks for sure much better than the previous version. > > But still, I have a fundamental issue here; I'm just not able to > reproduce Ian's problem here with uvideo.c,1.256. I was trying three > different cams, with lucview, motion, video, ffplay, and there are > zero glitches. Hence, I'm still not sure what we are trying to fixing > here. Could this be more a timing/race condition issue other than how > the different applications are sequencing the buffer queuing/dequeuing? > > Kirill, are you able to reproduce Ian's issue with your setup? And if > yes, how? > Yes, I was abdle to reproduce Ian's issue with a webcam which is embeded into Huawei Matebook X which reads as 13d3:56f2 Azurewave, USB camera. But I wasn't able to reproduce it on anything else. It supports: Compressed: mjpeg : MJPEG : 1280x720 960x540 848x480 640x480 640x360 Raw : yuyv422 : YUYV : 1280x720 960x540 848x480 640x480 640x360 If I run motion with small screen and small frame rate, like this: video_device /dev/video0 video_params palette=15 width 848 height 480 framerate 6 I able to reproduce Ian's issue. The last changes addresses a kind of race condition. Let assume that a device streams a frame as series of samples which is splitted into two or more packets. If we have a flow: - the first package is processed, but it hasn't saved payload because no free buffers here; - motion releases one buffer; - the second packet is arrived and its payload is saved. - ... - the last packet triggers queue the buffer. before this diff, an application gets malformed frame. With this version it gets the next, well formed frame. > > 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; > > -- wbr, Kirill