Download raw body.
sys/uvideo: forward error bit to consumer
On Sat, 21 Dec 2024 18:27:41 +0100,
Marcus Glocker <marcus@nazgul.ch> wrote:
>
> Makes basically sense to me, and it seems to fix a couple of cams what
> I could read on the feedback's. Also it didn't cause any regression
> with my cams. Though, two questions in-line.
>
Thanks for testing!
So far I had confirmation that this diff fixes the integrated camera on:
- ThinkPad T14 Gen 5
- ThinkPad X1 nano 2
- Lenovo x13
Also, this diff is required to make progress on the Elgato HD60 S+ support
which was reported at https://marc.info/?l=openbsd-bugs&m=173419416326742&w=2
This diff isn't enough, some other hacks are required, and I need more time
to find the right way and understand the issue.
> > @@ -2148,18 +2148,9 @@ uvideo_vs_decode_stream_header(struct uv
> >
> > DPRINTF(2, "%s: stream header len = %d\n", DEVNAME(sc), sh->bLength);
> >
> > - if (sh->bLength > UVIDEO_SH_MAX_LEN || sh->bLength < UVIDEO_SH_MIN_LEN)
> > + if (sh->bLength > frame_size || sh->bLength < UVIDEO_SH_MIN_LEN)
> > /* invalid header size */
> > return (USBD_INVAL);
>
> This check is there to make sure that the frame header isn't exceeding
> the max frame header size defined in the specs. Now when changing that
> to check if it exceeds the entire frame size, what use case does this
> cover?
>
ThinkPad T14 Gen 5 cam sends frames with bLength 176 bytes which is requied
to stabilize the picture and prevent it from glithcing.
> > - if (sh->bLength == frame_size && !(sh->bFlags & UVIDEO_SH_FLAG_EOF)) {
> > - /* stream header without payload and no EOF */
> > - return (USBD_INVAL);
> > - }
>
> If I remember correctly, this check was introduced since some cams did
> occasionally just sent a frame header without payload. You remove it
> because we also want to pass this case to the consumer?
>
Yes, it does. But it sends header which may contain PTS or SCR. uvideo.c
doesn't understand it, by consumer probably does.
Thus, here a bit updated version of the diff where I send to consumer too
small frame as well which seems required for NV12 stream for Elgato HD60 S+
I had tested on my camers and it works as before.
BTW Linux does the same: marks frames as error and sends it to consumer.
Index: sys/dev/usb/uvideo.c
===================================================================
RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
diff -u -p -r1.228 uvideo.c
--- sys/dev/usb/uvideo.c 14 Dec 2024 09:58:04 -0000 1.228
+++ sys/dev/usb/uvideo.c 20 Dec 2024 17:41:44 -0000
@@ -168,7 +168,7 @@ usbd_status uvideo_vs_decode_stream_head
uint8_t *, int);
usbd_status uvideo_vs_decode_stream_header_isight(struct uvideo_softc *,
uint8_t *, int);
-int uvideo_mmap_queue(struct uvideo_softc *, uint8_t *, int);
+int 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);
@@ -2148,18 +2148,9 @@ uvideo_vs_decode_stream_header(struct uv
DPRINTF(2, "%s: stream header len = %d\n", DEVNAME(sc), sh->bLength);
- if (sh->bLength > UVIDEO_SH_MAX_LEN || sh->bLength < UVIDEO_SH_MIN_LEN)
+ if (sh->bLength > frame_size || sh->bLength < UVIDEO_SH_MIN_LEN)
/* invalid header size */
return (USBD_INVAL);
- if (sh->bLength == frame_size && !(sh->bFlags & UVIDEO_SH_FLAG_EOF)) {
- /* stream header without payload and no EOF */
- return (USBD_INVAL);
- }
- if (sh->bFlags & UVIDEO_SH_FLAG_ERR) {
- /* stream error, skip xfer */
- DPRINTF(1, "%s: %s: stream error!\n", DEVNAME(sc), __func__);
- return (USBD_CANCELLED);
- }
DPRINTF(2, "%s: frame_size = %d\n", DEVNAME(sc), frame_size);
@@ -2178,6 +2169,7 @@ uvideo_vs_decode_stream_header(struct uv
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)) {
@@ -2186,12 +2178,25 @@ uvideo_vs_decode_stream_header(struct uv
fb->sample = 1;
fb->fid = sh->bFlags & UVIDEO_SH_FLAG_FID;
fb->offset = 0;
+ fb->error = 0;
}
}
+ if (sh->bFlags & UVIDEO_SH_FLAG_ERR) {
+ /* stream error, skip xfer */
+ DPRINTF(1, "%s: %s: stream error!\n", DEVNAME(sc), __func__);
+ fb->error = 1;
+ }
+
/* save sample */
sample_len = frame_size - sh->bLength;
- if ((fb->offset + sample_len) <= fb->buf_size) {
+ if (sample_len > fb->buf_size - fb->offset) {
+ DPRINTF(1, "%s: %s: frame too large, marked as error\n",
+ DEVNAME(sc), __func__);
+ sample_len = fb->buf_size - fb->offset;
+ fb->error = 1;
+ }
+ if (sample_len > 0) {
bcopy(frame + sh->bLength, fb->buf + fb->offset, sample_len);
fb->offset += sample_len;
}
@@ -2201,31 +2206,34 @@ uvideo_vs_decode_stream_header(struct uv
DPRINTF(2, "%s: %s: EOF (frame size = %d bytes)\n",
DEVNAME(sc), __func__, fb->offset);
- if (fb->offset > fb->buf_size) {
- DPRINTF(1, "%s: %s: frame too large, skipped!\n",
- DEVNAME(sc), __func__);
- } else if (fb->offset < fb->buf_size &&
+ if (fb->offset < fb->buf_size &&
!(fb->fmt_flags & V4L2_FMT_FLAG_COMPRESSED)) {
- DPRINTF(1, "%s: %s: frame too small, skipped!\n",
+ DPRINTF(1, "%s: %s: frame too small, marked as error\n",
DEVNAME(sc), __func__);
- } else {
+ fb->error = 1;
+ }
+
#ifdef UVIDEO_DUMP
- /* do the file write in process context */
- usb_rem_task(sc->sc_udev, &sc->sc_task_write);
- usb_add_task(sc->sc_udev, &sc->sc_task_write);
+ /* do the file write in process context */
+ usb_rem_task(sc->sc_udev, &sc->sc_task_write);
+ usb_add_task(sc->sc_udev, &sc->sc_task_write);
#endif
- if (sc->sc_mmap_flag) {
- /* mmap */
- if (uvideo_mmap_queue(sc, fb->buf, fb->offset))
- return (USBD_NOMEM);
- } else {
- /* read */
- uvideo_read(sc, fb->buf, fb->offset);
- }
+ if (sc->sc_mmap_flag) {
+ /* mmap */
+ if (uvideo_mmap_queue(sc, fb->buf, fb->offset,
+ fb->error))
+ return (USBD_NOMEM);
+ } else if (fb->error) {
+ DPRINTF(1, "%s: %s: error frame, skipped!\n",
+ DEVNAME(sc), __func__);
+ } else {
+ /* read */
+ uvideo_read(sc, fb->buf, fb->offset);
}
fb->sample = 0;
fb->fid = 0;
+ fb->error = 0;
}
return (USBD_NORMAL_COMPLETION);
@@ -2270,7 +2278,7 @@ uvideo_vs_decode_stream_header_isight(st
if (header) {
if (sc->sc_mmap_flag) {
/* mmap */
- if (uvideo_mmap_queue(sc, fb->buf, fb->offset))
+ if (uvideo_mmap_queue(sc, fb->buf, fb->offset, 0))
return (USBD_NOMEM);
} else {
/* read */
@@ -2290,7 +2298,7 @@ uvideo_vs_decode_stream_header_isight(st
}
int
-uvideo_mmap_queue(struct uvideo_softc *sc, uint8_t *buf, int len)
+uvideo_mmap_queue(struct uvideo_softc *sc, uint8_t *buf, int len, int err)
{
int i;
@@ -2314,6 +2322,11 @@ uvideo_mmap_queue(struct uvideo_softc *s
/* timestamp it */
getmicrotime(&sc->sc_mmap[i].v4l2_buf.timestamp);
+
+ /* forward error bit */
+ sc->sc_mmap[i].v4l2_buf.flags &= ~V4L2_BUF_FLAG_ERROR;
+ if (err)
+ sc->sc_mmap[i].v4l2_buf.flags |= V4L2_BUF_FLAG_ERROR;
/* queue it */
sc->sc_mmap[i].v4l2_buf.flags |= V4L2_BUF_FLAG_DONE;
Index: sys/dev/usb/uvideo.h
===================================================================
RCS file: /home/cvs/src/sys/dev/usb/uvideo.h,v
diff -u -p -r1.60 uvideo.h
--- sys/dev/usb/uvideo.h 8 Dec 2019 13:21:21 -0000 1.60
+++ sys/dev/usb/uvideo.h 20 Dec 2024 17:41:44 -0000
@@ -449,6 +449,7 @@ struct uvideo_vs_iface {
struct uvideo_frame_buffer {
int sample;
uint8_t fid;
+ uint8_t error;
int offset;
int buf_size;
uint8_t *buf;
--
wbr, Kirill
sys/uvideo: forward error bit to consumer