| Message ID | 0d3ee0aa-0f1f-4670-a5cc-8dd982e2e3b0@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | uvc: fix sequence counting | expand |
Hello Hans What if we make something like: #define UVC_STREAM_FID_UNINITIALISED (UVC_STREAM_FID + 1) and then use that define at the initialization and in decode_start() ? I think it will be clearer than the current comparison. Also you might want to wait to assign "stream->last_fid = fid;" until line 1106, because otherwise the "Dropping payload" will be triggered (I believe) Thanks! PS: You will get better response time if you email me at ribalda@chromium.org , not much time recently for the personal email :( On Wed, Nov 24, 2021 at 11:49 AM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > When you start streaming from uvc, then the first buffer will > have sequence number 0 and the second buffer has sequence number > 2. Fix the logic to ensure proper monotonically increasing sequence > numbers. > > The root cause is not setting last_fid when you start streaming > and a new fid is found for the first time. > > This patch also changes the initial last_fid value from -1 to 0xff. > Since last_fid is unsigned, it is better to stick to unsigned values. > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > drivers/media/usb/uvc/uvc_video.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > index 9f37eaf28ce7..8ba8d25e2c4a 100644 > --- a/drivers/media/usb/uvc/uvc_video.c > +++ b/drivers/media/usb/uvc/uvc_video.c > @@ -1055,7 +1055,10 @@ static int uvc_video_decode_start(struct uvc_streaming *stream, > * that discontinuous sequence numbers always indicate lost frames. > */ > if (stream->last_fid != fid) { > - stream->sequence++; > + if (stream->last_fid > UVC_STREAM_FID) > + stream->last_fid = fid; > + else > + stream->sequence++; > if (stream->sequence) > uvc_video_stats_update(stream); > } > @@ -1080,7 +1083,7 @@ static int uvc_video_decode_start(struct uvc_streaming *stream, > > /* Synchronize to the input stream by waiting for the FID bit to be > * toggled when the the buffer state is not UVC_BUF_STATE_ACTIVE. > - * stream->last_fid is initialized to -1, so the first isochronous > + * stream->last_fid is initialized to 0xff, so the first isochronous > * frame will always be in sync. > * > * If the device doesn't toggle the FID bit, invert stream->last_fid > @@ -1111,7 +1114,7 @@ static int uvc_video_decode_start(struct uvc_streaming *stream, > * last payload can be lost anyway). We thus must check if the FID has > * been toggled. > * > - * stream->last_fid is initialized to -1, so the first isochronous > + * stream->last_fid is initialized to 0xff, so the first isochronous > * frame will never trigger an end of frame detection. > * > * Empty buffers (bytesused == 0) don't trigger end of frame detection > @@ -1895,7 +1898,7 @@ static int uvc_video_start_transfer(struct uvc_streaming *stream, > int ret; > > stream->sequence = -1; > - stream->last_fid = -1; > + stream->last_fid = 0xff; > stream->bulk.header_size = 0; > stream->bulk.skip_payload = 0; > stream->bulk.payload_size = 0; > -- > 2.33.0 > -- Ricardo Ribalda
Hello, On Fri, Nov 26, 2021 at 05:14:31PM +0100, Ricardo Ribalda Delgado wrote: > Hello Hans > > What if we make something like: > > #define UVC_STREAM_FID_UNINITIALISED (UVC_STREAM_FID + 1) > > and then use that define at the initialization and in decode_start() ? > I think it will be clearer than the current comparison. > > > Also you might want to wait to assign "stream->last_fid = fid;" until > line 1106, because otherwise the "Dropping payload" will be triggered > (I believe) > > Thanks! > > PS: You will get better response time if you email me at > ribalda@chromium.org , not much time recently for the personal email > :( > > On Wed, Nov 24, 2021 at 11:49 AM Hans Verkuil wrote: > > > > When you start streaming from uvc, then the first buffer will > > have sequence number 0 and the second buffer has sequence number > > 2. Fix the logic to ensure proper monotonically increasing sequence > > numbers. > > > > The root cause is not setting last_fid when you start streaming > > and a new fid is found for the first time. I can confirm the issue with my device, but this short explanation doesn't really tell me where the problem comes from. Could you elaborate on this, maybe with a detailed sequence ? > > This patch also changes the initial last_fid value from -1 to 0xff. > > Since last_fid is unsigned, it is better to stick to unsigned values. Maybe U8_MAX then ? > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > > --- > > drivers/media/usb/uvc/uvc_video.c | 11 +++++++---- > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > > index 9f37eaf28ce7..8ba8d25e2c4a 100644 > > --- a/drivers/media/usb/uvc/uvc_video.c > > +++ b/drivers/media/usb/uvc/uvc_video.c > > @@ -1055,7 +1055,10 @@ static int uvc_video_decode_start(struct uvc_streaming *stream, > > * that discontinuous sequence numbers always indicate lost frames. > > */ > > if (stream->last_fid != fid) { > > - stream->sequence++; > > + if (stream->last_fid > UVC_STREAM_FID) > > + stream->last_fid = fid; > > + else > > + stream->sequence++; > > if (stream->sequence) > > uvc_video_stats_update(stream); > > } > > @@ -1080,7 +1083,7 @@ static int uvc_video_decode_start(struct uvc_streaming *stream, > > > > /* Synchronize to the input stream by waiting for the FID bit to be > > * toggled when the the buffer state is not UVC_BUF_STATE_ACTIVE. > > - * stream->last_fid is initialized to -1, so the first isochronous > > + * stream->last_fid is initialized to 0xff, so the first isochronous > > * frame will always be in sync. > > * > > * If the device doesn't toggle the FID bit, invert stream->last_fid > > @@ -1111,7 +1114,7 @@ static int uvc_video_decode_start(struct uvc_streaming *stream, > > * last payload can be lost anyway). We thus must check if the FID has > > * been toggled. > > * > > - * stream->last_fid is initialized to -1, so the first isochronous > > + * stream->last_fid is initialized to 0xff, so the first isochronous > > * frame will never trigger an end of frame detection. > > * > > * Empty buffers (bytesused == 0) don't trigger end of frame detection > > @@ -1895,7 +1898,7 @@ static int uvc_video_start_transfer(struct uvc_streaming *stream, > > int ret; > > > > stream->sequence = -1; > > - stream->last_fid = -1; > > + stream->last_fid = 0xff; > > stream->bulk.header_size = 0; > > stream->bulk.skip_payload = 0; > > stream->bulk.payload_size = 0;
diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c index 9f37eaf28ce7..8ba8d25e2c4a 100644 --- a/drivers/media/usb/uvc/uvc_video.c +++ b/drivers/media/usb/uvc/uvc_video.c @@ -1055,7 +1055,10 @@ static int uvc_video_decode_start(struct uvc_streaming *stream, * that discontinuous sequence numbers always indicate lost frames. */ if (stream->last_fid != fid) { - stream->sequence++; + if (stream->last_fid > UVC_STREAM_FID) + stream->last_fid = fid; + else + stream->sequence++; if (stream->sequence) uvc_video_stats_update(stream); } @@ -1080,7 +1083,7 @@ static int uvc_video_decode_start(struct uvc_streaming *stream, /* Synchronize to the input stream by waiting for the FID bit to be * toggled when the the buffer state is not UVC_BUF_STATE_ACTIVE. - * stream->last_fid is initialized to -1, so the first isochronous + * stream->last_fid is initialized to 0xff, so the first isochronous * frame will always be in sync. * * If the device doesn't toggle the FID bit, invert stream->last_fid @@ -1111,7 +1114,7 @@ static int uvc_video_decode_start(struct uvc_streaming *stream, * last payload can be lost anyway). We thus must check if the FID has * been toggled. * - * stream->last_fid is initialized to -1, so the first isochronous + * stream->last_fid is initialized to 0xff, so the first isochronous * frame will never trigger an end of frame detection. * * Empty buffers (bytesused == 0) don't trigger end of frame detection @@ -1895,7 +1898,7 @@ static int uvc_video_start_transfer(struct uvc_streaming *stream, int ret; stream->sequence = -1; - stream->last_fid = -1; + stream->last_fid = 0xff; stream->bulk.header_size = 0; stream->bulk.skip_payload = 0; stream->bulk.payload_size = 0;
When you start streaming from uvc, then the first buffer will have sequence number 0 and the second buffer has sequence number 2. Fix the logic to ensure proper monotonically increasing sequence numbers. The root cause is not setting last_fid when you start streaming and a new fid is found for the first time. This patch also changes the initial last_fid value from -1 to 0xff. Since last_fid is unsigned, it is better to stick to unsigned values. Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> --- drivers/media/usb/uvc/uvc_video.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)