From: Marcus Glocker <marcus@nazgul.ch>
Subject: Re: sys/uvideo: don't skip frame immediately when buffer unavailable (regression from v1.256)
To: "Kirill A. Korinsky" <kirill@korins.ky>
Cc: tech@openbsd.org, Ian Darwin <ian@darwinsys.com>
Date: Sun, 6 Apr 2025 09:12:42 +0200

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.
 
> 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	4 Apr 2025 07:10:32 -0000
> @@ -2249,7 +2249,7 @@ uvideo_vs_start_bulk_thread(void *arg)
>  
>  		if (sc->sc_mmap_flag) {
>  			if ((buf = uvideo_mmap_getbuf(sc)) == NULL)
> -				continue;
> +				buf = sc->sc_frame_buffer.buf;
>  		} else
>  			buf = sc->sc_frame_buffer.buf;
>  
> @@ -2330,7 +2330,7 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
>  
>  		if (sc->sc_mmap_flag) {
>  			if ((buf = uvideo_mmap_getbuf(sc)) == NULL)
> -				goto skip;
> +				buf = sc->sc_frame_buffer.buf;
>  		} else
>  			buf = sc->sc_frame_buffer.buf;
>  
> @@ -2536,8 +2536,14 @@ uvideo_mmap_getbuf(struct uvideo_softc *
>  void
>  uvideo_mmap_queue(struct uvideo_softc *sc, int len, int err)
>  {
> -	if (sc->sc_mmap_cur == NULL)
> -		panic("uvideo_mmap_queue: NULL pointer!");
> +	uint8_t *buf;
> +
> +	if (sc->sc_mmap_cur == NULL) {
> +		if ((buf = uvideo_mmap_getbuf(sc)) == NULL)
> +			return;
> +
> +		bcopy(sc->sc_frame_buffer.buf, buf, len);
> +	}
>  
>  	/* report frame length */
>  	sc->sc_mmap_cur->v4l2_buf.bytesused = len;
>