On Sun, 06 Apr 2025 09:12:42 +0200,
Marcus Glocker <marcus@nazgul.ch> wrote:
> 
> On Fri, Apr 04, 2025 at 09:24:51AM GMT, Kirill A. Korinsky wrote:
> 
> > tech@,
> > 
> > this diff fixes a regression that was introduced by v1.256 and reported
> > by ian@.
> > 
> > The original change (v1.256), which avoided one bcopy, was essential to
> > improve performance sufficiently for supporting 4K 60fps devices.
> > 
> > However, some applications (e.g., multimedia/motion) release a buffer
> > back to the driver just before the read. If this release happens after a
> > frame packet has arrived, the logic introduced in v1.256 drops the
> > frame. This dropped frame can be critical, especially for devices with
> > low framerates, causing the stream to glitch.
> > 
> > Therefore, instead of dropping the frame immediately, this fix parses
> > the incoming frame into sc->sc_frame_buffer.buf. The data is then bcopy
> > into shared memory after parsing is complete if buffers was released,
> > restoring the behavior from before v1.256.
> > 
> > Notably, this issue is application-dependent; for example, ffplay works
> > correctly, while motion does not.
> > 
> > ian@ has confirmed that this fix the issue on his affected device.
> > 
> > Ok?
> 
> As already discussed a bit on ICB:
> 
> I don't exactly understand ian@ issue, and I also didn't see a bug
> report on bugs@ for it which would explain it well.  I can half way
> imagine the issue based on your explanation.
> 
> I also don't understand how your diff would fix what I did understand
> from your issue explanation.
> 
> In any case, since I'm also using motion;  I don't see glitches with
> my setup using motion with uvideo.c,1.256.  And if I apply your diff,
> *then* I'm seeing glitches, almost every 5-8 seconds.
> 
> So based on the current information, it's pretty hard to discuss
> about a possible solution.  Maybe ian@ could provide a detailed bug
> report on bugs@, showing a dmesg, 'ubsdevs -v' output, and his
> motion.conf.  Based on that, by any chance we could try to reproduce
> the issue locally, and then discuss about a possible solution.
>  

Well, I do not understand your issue but I can reproduce ian@ one on one of
my webcam, and only one. So, I will prepare two debug outputs later today.

But we're short in the time, this changes needed only to support new devices
with high FPS... So, I suggest to revert it for release.

Ok?

Index: sys/dev/usb/uvideo.c
===================================================================
RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
diff -u -p -r1.256 uvideo.c
--- sys/dev/usb/uvideo.c	24 Mar 2025 18:56:34 -0000	1.256
+++ sys/dev/usb/uvideo.c	6 Apr 2025 08:41:40 -0000
@@ -66,7 +66,6 @@ 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;
@@ -102,7 +101,7 @@ struct uvideo_softc {
 	const struct uvideo_devs		*sc_quirk;
 	void					(*sc_decode_stream_header)
 						    (struct uvideo_softc *,
-						    uint8_t *, int, uint8_t *);
+						    uint8_t *, int);
 };
 
 int		uvideo_open(void *, int, int *, uint8_t *, void (*)(void *),
@@ -168,11 +167,11 @@ void		uvideo_vs_start_isoc_ixfer(struct 
 void		uvideo_vs_cb(struct usbd_xfer *, void *,
 		    usbd_status);
 void		uvideo_vs_decode_stream_header(struct uvideo_softc *,
-		    uint8_t *, int, uint8_t *);
+		    uint8_t *, int);
 void		uvideo_vs_decode_stream_header_isight(struct uvideo_softc *,
-		    uint8_t *, int, uint8_t *);
-uint8_t *	uvideo_mmap_getbuf(struct uvideo_softc *);
-void		uvideo_mmap_queue(struct uvideo_softc *, int, int);
+		    uint8_t *, int);
+int		uvideo_get_buf(struct uvideo_softc *);
+void		uvideo_mmap_queue(struct uvideo_softc *, uint8_t *, 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);
@@ -2220,7 +2219,6 @@ uvideo_vs_start_bulk_thread(void *arg)
 	struct uvideo_softc *sc = arg;
 	usbd_status error;
 	int size;
-	uint8_t *buf;
 
 	usbd_ref_incr(sc->sc_udev);
 	while (sc->sc_vs_cur->bulk_running) {
@@ -2247,14 +2245,7 @@ uvideo_vs_start_bulk_thread(void *arg)
 
 		DPRINTF(2, "%s: *** buffer len = %d\n", DEVNAME(sc), size);
 
-		if (sc->sc_mmap_flag) {
-			if ((buf = uvideo_mmap_getbuf(sc)) == NULL)
-				continue;
-		} else
-			buf = sc->sc_frame_buffer.buf;
-
-		sc->sc_decode_stream_header(sc, sc->sc_vs_cur->bxfer.buf, size,
-		    buf);
+		sc->sc_decode_stream_header(sc, sc->sc_vs_cur->bxfer.buf, size);
 	}
 	usbd_ref_decr(sc->sc_udev);
 
@@ -2305,7 +2296,7 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
 	struct uvideo_isoc_xfer *ixfer = priv;
 	struct uvideo_softc *sc = ixfer->sc;
 	int len, i, frame_size;
-	uint8_t *frame, *buf;
+	uint8_t *frame;
 
 	DPRINTF(2, "%s: %s\n", DEVNAME(sc), __func__);
 
@@ -2328,13 +2319,7 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
 			/* frame is empty */
 			continue;
 
-		if (sc->sc_mmap_flag) {
-			if ((buf = uvideo_mmap_getbuf(sc)) == NULL)
-				goto skip;
-		} else
-			buf = sc->sc_frame_buffer.buf;
-
-		sc->sc_decode_stream_header(sc, frame, frame_size, buf);
+		sc->sc_decode_stream_header(sc, frame, frame_size);
 	}
 
 skip:	/* setup new transfer */
@@ -2343,7 +2328,7 @@ skip:	/* setup new transfer */
 
 void
 uvideo_vs_decode_stream_header(struct uvideo_softc *sc, uint8_t *frame,
-    int frame_size, uint8_t *buf)
+    int frame_size)
 {
 	struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer;
 	struct usb_video_stream_header *sh;
@@ -2406,7 +2391,7 @@ uvideo_vs_decode_stream_header(struct uv
 		fb->error = 1;
 	}
 	if (sample_len > 0) {
-		bcopy(frame + sh->bLength, buf + fb->offset, sample_len);
+		bcopy(frame + sh->bLength, fb->buf + fb->offset, sample_len);
 		fb->offset += sample_len;
 	}
 
@@ -2424,7 +2409,7 @@ uvideo_vs_decode_stream_header(struct uv
 
 		if (sc->sc_mmap_flag) {
 			/* mmap */
-			uvideo_mmap_queue(sc, fb->offset, fb->error);
+			uvideo_mmap_queue(sc, fb->buf, fb->offset, fb->error);
 		} else if (fb->error) {
 			DPRINTF(1, "%s: %s: error frame, skipped!\n",
 			    DEVNAME(sc), __func__);
@@ -2457,7 +2442,7 @@ uvideo_vs_decode_stream_header(struct uv
  */
 void
 uvideo_vs_decode_stream_header_isight(struct uvideo_softc *sc, uint8_t *frame,
-    int frame_size, uint8_t *buf)
+    int frame_size)
 {
 	struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer;
 	int sample_len, header = 0;
@@ -2478,7 +2463,7 @@ uvideo_vs_decode_stream_header_isight(st
 	if (header) {
 		if (sc->sc_mmap_flag) {
 			/* mmap */
-			uvideo_mmap_queue(sc, fb->offset, 0);
+			uvideo_mmap_queue(sc, fb->buf, fb->offset, 0);
 		} else {
 			/* read */
 			uvideo_read(sc, fb->buf, fb->offset);
@@ -2488,27 +2473,17 @@ uvideo_vs_decode_stream_header_isight(st
 		/* save sample */
 		sample_len = frame_size;
 		if ((fb->offset + sample_len) <= fb->buf_size) {
-			bcopy(frame, buf + fb->offset, sample_len);
+			bcopy(frame, fb->buf + fb->offset, sample_len);
 			fb->offset += sample_len;
 		}
 	}
 }
 
-uint8_t *
-uvideo_mmap_getbuf(struct uvideo_softc *sc)
+int
+uvideo_get_buf(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;
-
-	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 &
@@ -2522,45 +2497,51 @@ uvideo_mmap_getbuf(struct uvideo_softc *
 			sc->sc_mmap_buffer_idx = 0;
 	}
 
-	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];
+	if (i == sc->sc_mmap_count)
+		return -1;
 
-	return sc->sc_mmap_cur->buf;
+	return idx;
 }
 
 void
-uvideo_mmap_queue(struct uvideo_softc *sc, int len, int err)
+uvideo_mmap_queue(struct uvideo_softc *sc, uint8_t *buf, int len, int err)
 {
-	if (sc->sc_mmap_cur == NULL)
-		panic("uvideo_mmap_queue: NULL pointer!");
+	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__);
+		return;
+	}
 
-	/* report frame length */
-	sc->sc_mmap_cur->v4l2_buf.bytesused = len;
+	/* copy frame to mmap buffer and report length */
+	bcopy(buf, sc->sc_mmap[i].buf, len);
+	sc->sc_mmap[i].v4l2_buf.bytesused = len;
 
 	/* timestamp it */
-	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;
+	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;
 
 	/* forward error bit */
-	sc->sc_mmap_cur->v4l2_buf.flags &= ~V4L2_BUF_FLAG_ERROR;
+	sc->sc_mmap[i].v4l2_buf.flags &= ~V4L2_BUF_FLAG_ERROR;
 	if (err)
-		sc->sc_mmap_cur->v4l2_buf.flags |= V4L2_BUF_FLAG_ERROR;
+		sc->sc_mmap[i].v4l2_buf.flags |= V4L2_BUF_FLAG_ERROR;
 
 	/* queue it */
-	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__);
+	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);
 
 	wakeup(sc);
 
@@ -3526,7 +3507,6 @@ 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;