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?
 
> 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;
>