On Fri, Apr 18, 2025 at 10:11:36PM GMT, Kirill A. Korinsky wrote:

> 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.

All right.  Thanks for the explanation.  I understand the issue now, and
how your diff tries to fix it.  We are basically collecting the rest of
an anyway already corrupted frame, so that at the end we can cleanly
skip it, and get the next valid frame.
 
> before this diff, an application gets malformed frame. With this version it
> gets the next, well formed frame.

One other question, just that I have asked it;  Did we consider to set
the V4L2_BUF_FLAG_ERROR bit in such a case, and still pass the corrupted
frame to the application?  I'm guessing applications like luvcview and
motion don't credit this flag though ... 

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