On Sun, 06 Apr 2025 15:54:03 +0200,
Kirill A. Korinsky <kirill@korins.ky> wrote:
> 
> On Sun, 06 Apr 2025 11:22:15 +0200,
> Marcus Glocker <marcus@nazgul.ch> wrote:
> >
> > On Sun, Apr 06, 2025 at 10:44:36AM GMT, Kirill A. Korinsky wrote:
> >
> > > > 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?
> >
> > Fine for me to revert the bcopy() performance diff for the release.
> >
> > ok mglocker@
> >
> 
> Thanks, it is reverted.
> 
> Meanwhile I had played with different screen size and frame rate and had
> found that some settings which triggers on my affected device some glitches
> with any tested version of uvideo when I had enabled UVIDEO_DEBUG and
> increases uvideo_debug to 2. As a base line to proof that it doesn't recent
> changes I've used 1.221.
> 
> If I touches framerate or screen size, it behaves differently. Threshold is
> 10 FPS, and if I consume more than 10 FPS I haven't got major glitches. With
> 15 FPS or more I haven't got any kind of glitches on tested version.
> 
> So, I have:
> 
>  - uvideo.c,v 1.221 and 1.257 some micro glitches time to time when I had
>    enabled debug, and major glitches when I also increased uvideo_debug.
> 
>  - uvideo.c,v 1.256 major gliteches, screenshot:
>    https://kirill.korins.ky/pub/uvideo-motion-regression/screenshot-2025-04-06_13-19-56.png
> 
>  - uvideo.c,v 1.256 + diff from a thread better, but still see some glitches
>    with enabled debug.
> 
> BTW I had keep samples of debug log and differences with motion.conf here:
> https://kirill.korins.ky/pub/uvideo-motion-regression/
> 
> Anyway, base on all that I said, I don't think that bcopy diff is a root
> cause of the issue. I think that that we already have the issue, but bcopy
> diff uncovers it on some device and settings as side effect, which allows us
> to notice it.
> 

Here a bit improved version of remove bcopy again. On my tests I haven't see
any differences with 1.257 with or without increased debug. I mean that
without debug it hasn't got any glitches, with it has, but amount matches
that I see on 1.221 and 1.257.

May I ask you to try it?

If it works fine, I suggest to commit it after release because.

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	6 Apr 2025 19:33:21 -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;
@@ -101,7 +102,7 @@ struct uvideo_softc {
 	const struct uvideo_devs		*sc_quirk;
 	void					(*sc_decode_stream_header)
 						    (struct uvideo_softc *,
-						    uint8_t *, int);
+						    uint8_t *, int, uint8_t *);
 };
 
 int		uvideo_open(void *, int, int *, uint8_t *, void (*)(void *),
@@ -167,11 +168,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 *, int, uint8_t *);
 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 *, int, uint8_t *);
+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);
@@ -2219,6 +2220,7 @@ 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) {
@@ -2245,7 +2247,13 @@ uvideo_vs_start_bulk_thread(void *arg)
 
 		DPRINTF(2, "%s: *** buffer len = %d\n", DEVNAME(sc), size);
 
-		sc->sc_decode_stream_header(sc, sc->sc_vs_cur->bxfer.buf, size);
+		if (sc->sc_mmap_flag)
+			buf = uvideo_mmap_getbuf(sc);
+		else
+			buf = sc->sc_frame_buffer.buf;
+
+		sc->sc_decode_stream_header(sc, sc->sc_vs_cur->bxfer.buf, size,
+		    buf);
 	}
 	usbd_ref_decr(sc->sc_udev);
 
@@ -2296,7 +2304,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;
+	uint8_t *frame, *buf;
 
 	DPRINTF(2, "%s: %s\n", DEVNAME(sc), __func__);
 
@@ -2319,7 +2327,12 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
 			/* frame is empty */
 			continue;
 
-		sc->sc_decode_stream_header(sc, frame, frame_size);
+		if (sc->sc_mmap_flag)
+			buf = uvideo_mmap_getbuf(sc);
+		else
+			buf = sc->sc_frame_buffer.buf;
+
+		sc->sc_decode_stream_header(sc, frame, frame_size, buf);
 	}
 
 skip:	/* setup new transfer */
@@ -2328,7 +2341,7 @@ skip:	/* setup new transfer */
 
 void
 uvideo_vs_decode_stream_header(struct uvideo_softc *sc, uint8_t *frame,
-    int frame_size)
+    int frame_size, uint8_t *buf)
 {
 	struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer;
 	struct usb_video_stream_header *sh;
@@ -2391,7 +2404,7 @@ uvideo_vs_decode_stream_header(struct uv
 		fb->error = 1;
 	}
 	if (sample_len > 0) {
-		bcopy(frame + sh->bLength, fb->buf + fb->offset, sample_len);
+		bcopy(frame + sh->bLength, buf + fb->offset, sample_len);
 		fb->offset += sample_len;
 	}
 
@@ -2409,7 +2422,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__);
@@ -2442,7 +2455,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)
+    int frame_size, uint8_t *buf)
 {
 	struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer;
 	int sample_len, header = 0;
@@ -2463,7 +2476,7 @@ 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);
@@ -2473,17 +2486,30 @@ uvideo_vs_decode_stream_header_isight(st
 		/* 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;
+
+	if (sc->sc_frame_buffer.offset > 0)
+		return sc->sc_frame_buffer.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 &
@@ -2497,51 +2523,54 @@ 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 sc->sc_frame_buffer.buf;
+	}
 
-	return idx;
+	sc->sc_mmap_cur = &sc->sc_mmap[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;
+	uint8_t *buf;
 
-	if (sc->sc_mmap_count == 0 || sc->sc_mmap_buffer == NULL)
-		panic("%s: mmap buffers not allocated", __func__);
+	if (sc->sc_mmap_cur == NULL) {
+		sc->sc_frame_buffer.offset = 0;
+		buf = uvideo_mmap_getbuf(sc);
 
-	/* 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;
+		if (buf == sc->sc_frame_buffer.buf)
+			return;
+
+		bcopy(sc->sc_frame_buffer.buf, buf, 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;
+	/* 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 +3536,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;