On Wed, Mar 12, 2025 at 11:30:03AM GMT, Marcus Glocker wrote:
> On Wed, Mar 12, 2025 at 10:24:04AM GMT, Landry Breuil wrote:
>
> > Le Wed, Mar 12, 2025 at 10:05:30AM +0100, Landry Breuil a ?crit :
> > > hi,
> > >
> > > updated my t470s from '#564: Tue Feb 25 21:20:18 MST 2025' to '#591: Tue
> > > Mar 11 20:48:18 MDT 2025' and my webcam now shows weird 'stripes' on top
> > > of the image (that works).
> >
> > the 'weird stripes' only show up from some webrtc clients (ive noticed
> > it with jitsi in firefox 137.0b4), using video(1) i havent been able to
> > replicate (yet)..
> >
> > no issue in https://mozilla.github.io/webrtc-landing/gum_test.html so
> > might be the way jitsi queries the video device.
> >
> > Landry
>
> I have the same cam in my X1 Carbon. I can't reproduce the issue on
> video(1) either, because it's using YUYV422 as the default input format.
> When using ffplay with MJPEG as input format, I can reproduce the issue.
>
> I'll try to figure out which of the recent commits has introduced the
> regression.
The issue was introduced by:
***
Revision 1.247 / (download) - annotate - [select for diffs], Sat Mar 1 12:30:19
2025 UTC (11 days ago) by mglocker
Branch: MAIN
Changes since 1.246: +55 -32 lines
Diff to previous 1.246 (colored)
Copy frames directly in to the mmap'ed buffer. This saves us one bcopy()
down the frame queuing path.
Original idea and diff from kirill@, with some modifications.
ok kirill@
***
Unfortunately, I couldn't figure out yet what the issue is with this
commit. Hence, following the back out diff for uvideo.c 1.247. It
fixes the issue for me. What about you?
Index: sys/dev/usb/uvideo.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
diff -u -p -u -p -r1.251 uvideo.c
--- sys/dev/usb/uvideo.c 10 Mar 2025 07:38:12 -0000 1.251
+++ sys/dev/usb/uvideo.c 12 Mar 2025 12:39:01 -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;
q_mmap sc_mmap_q;
@@ -104,7 +103,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 *),
@@ -170,11 +169,10 @@ 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);
+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);
@@ -2232,7 +2230,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) {
@@ -2259,14 +2256,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);
@@ -2320,7 +2310,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__);
@@ -2335,12 +2325,6 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
if (len == 0)
goto skip;
- if (sc->sc_mmap_flag) {
- if ((buf = uvideo_mmap_getbuf(sc)) == NULL)
- goto skip;
- } else
- buf = sc->sc_frame_buffer.buf;
-
for (i = 0; i < sc->sc_nframes; i++) {
frame = ixfer->buf + (i * sc->sc_vs_cur->psize);
frame_size = ixfer->size[i];
@@ -2349,7 +2333,7 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
/* frame is empty */
continue;
- sc->sc_decode_stream_header(sc, frame, frame_size, buf);
+ sc->sc_decode_stream_header(sc, frame, frame_size);
}
skip: /* setup new transfer */
@@ -2358,7 +2342,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;
@@ -2421,7 +2405,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;
}
@@ -2444,7 +2428,7 @@ uvideo_vs_decode_stream_header(struct uv
#endif
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__);
@@ -2477,7 +2461,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;
@@ -2498,7 +2482,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);
@@ -2508,14 +2492,14 @@ 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)
+void
+uvideo_mmap_queue(struct uvideo_softc *sc, uint8_t *buf, int len, int err)
{
int i;
@@ -2530,38 +2514,32 @@ uvideo_mmap_getbuf(struct uvideo_softc *
if (i == sc->sc_mmap_count) {
DPRINTF(1, "%s: %s: mmap queue is full!\n",
DEVNAME(sc), __func__);
- return NULL;
+ return;
}
- sc->sc_mmap_cur = &sc->sc_mmap[i];
-
- return sc->sc_mmap_cur->buf;
-}
-
-void
-uvideo_mmap_queue(struct uvideo_softc *sc, int len, int err)
-{
/* copy frame to mmap buffer and report length */
- sc->sc_mmap_cur->v4l2_buf.bytesused = len;
+ 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);
- 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);
