Download raw body.
sys/uvideo: don't skip frame immediately when buffer unavailable (regression from v1.256)
sys/uvideo: don't skip frame immediately when buffer unavailable (regression from v1.256)
sys/uvideo: don't skip frame immediately when buffer unavailable (regression from v1.256)
On Fri, 18 Apr 2025 21:52:50 +0200,
Marcus Glocker <marcus@nazgul.ch> 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
sys/uvideo: don't skip frame immediately when buffer unavailable (regression from v1.256)
sys/uvideo: don't skip frame immediately when buffer unavailable (regression from v1.256)
sys/uvideo: don't skip frame immediately when buffer unavailable (regression from v1.256)