Re: [PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling
Hi Anton,
On Saturday 29 March 2014 09:28:02 Anton Leontiev wrote:
> 28.03.2014 20:12, Laurent Pinchart пишет:
> + * Set error flag for incomplete buffer.
> + */
> +static void uvc_buffer_check_bytesused(const struct uvc_streaming
> *const stream,
> >
> > No need for the second const keyword here.
> >
> > I would have used "uvc_video_" as a prefix, to be in sync with the
> > surrounding functions. What would you think of
> > uvc_video_validate_buffer() ?
> >
> +struct uvc_buffer *const buf)
> >
> > And no need for const at all here.
> >
> +{
> +if (buf->length != buf->bytesused &&
> +!(stream->cur_format->flags &
> UVC_FMT_FLAG_COMPRESSED))
> >
> > The indentation is wrong here, the ! on the second line should be aligned
> > to the first 'buf' of the first line.
> >
> > If you agree with these changes I can perform them while applying, there's
> > no need to resubmit the patch.
>
> Thank you for reviewing my first patch to Linux kernel.
Thank you for submitting it :-)
> I completely agree with your changes.
>
> Just want to ask why there is no need for the second 'const' after pointer
> character '*'? I thought it marks pointer itself as constant for type-
> checking opposite to first 'const', which marks memory it points to as
> constant for type-checking.
Your understanding is correct (as far as I know).
> I understand that the function is simple enough to verify it by hand but
> it's better to add more information for automatic checking.
>
> Is there any guidelines on 'const' keyword usage in Linux kernel code?
Marking the memory pointed to by the pointer as const ensures that the
function won't modify memory that the caller thought wouldn't be modified. It
thus serves as a contract between the caller and the callee, enforced by the
compiler.
Marking the pointer as const, on the other hand, only affects the function
implementation, without influencing its call contract. Its only use in this
case would be to prevent the function from modifying a pointer that the
function itself thought it shouldn't modify. While not useless in all cases,
this compile-time checked if usually not performed in the kernel, especially
for simple functions. I'm not aware of any explicit rule regarding const usage
though.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling
Hi Anton,
On Saturday 29 March 2014 09:28:02 Anton Leontiev wrote:
28.03.2014 20:12, Laurent Pinchart пишет:
+ * Set error flag for incomplete buffer.
+ */
+static void uvc_buffer_check_bytesused(const struct uvc_streaming
*const stream,
No need for the second const keyword here.
I would have used uvc_video_ as a prefix, to be in sync with the
surrounding functions. What would you think of
uvc_video_validate_buffer() ?
+struct uvc_buffer *const buf)
And no need for const at all here.
+{
+if (buf-length != buf-bytesused
+!(stream-cur_format-flags
UVC_FMT_FLAG_COMPRESSED))
The indentation is wrong here, the ! on the second line should be aligned
to the first 'buf' of the first line.
If you agree with these changes I can perform them while applying, there's
no need to resubmit the patch.
Thank you for reviewing my first patch to Linux kernel.
Thank you for submitting it :-)
I completely agree with your changes.
Just want to ask why there is no need for the second 'const' after pointer
character '*'? I thought it marks pointer itself as constant for type-
checking opposite to first 'const', which marks memory it points to as
constant for type-checking.
Your understanding is correct (as far as I know).
I understand that the function is simple enough to verify it by hand but
it's better to add more information for automatic checking.
Is there any guidelines on 'const' keyword usage in Linux kernel code?
Marking the memory pointed to by the pointer as const ensures that the
function won't modify memory that the caller thought wouldn't be modified. It
thus serves as a contract between the caller and the callee, enforced by the
compiler.
Marking the pointer as const, on the other hand, only affects the function
implementation, without influencing its call contract. Its only use in this
case would be to prevent the function from modifying a pointer that the
function itself thought it shouldn't modify. While not useless in all cases,
this compile-time checked if usually not performed in the kernel, especially
for simple functions. I'm not aware of any explicit rule regarding const usage
though.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling
28.03.2014 20:12, Laurent Pinchart пишет:
+ * Set error flag for incomplete buffer.
+ */
+static void uvc_buffer_check_bytesused(const struct uvc_streaming *const
stream,
>
> No need for the second const keyword here.
>
> I would have used "uvc_video_" as a prefix, to be in sync with the
> surrounding
> functions. What would you think of uvc_video_validate_buffer() ?
>
+ struct uvc_buffer *const buf)
>
> And no need for const at all here.
>
+{
+ if (buf->length != buf->bytesused &&
+ !(stream->cur_format->flags & UVC_FMT_FLAG_COMPRESSED))
>
> The indentation is wrong here, the ! on the second line should be aligned to
> the first 'buf' of the first line.
>
> If you agree with these changes I can perform them while applying, there's no
> need to resubmit the patch.
>
Thank you for reviewing my first patch to Linux kernel. I completely
agree with your changes.
Just want to ask why there is no need for the second 'const' after
pointer character '*'? I thought it marks pointer itself as constant for
type-checking opposite to first 'const', which marks memory it points to
as constant for type-checking. I understand that the function is simple
enough to verify it by hand but it's better to add more information for
automatic checking.
Is there any guidelines on 'const' keyword usage in Linux kernel code?
Regards,
--
Anton Leontiev
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling
Hi Anton,
On Friday 28 March 2014 07:58:00 Anton Leontiev wrote:
> 26.03.2014 21:41, Laurent Pinchart wrote:
> > On Tuesday 25 March 2014 08:40:57 Anton Leontiev wrote:
> >> Set error bit for incomplete buffers when end of buffer is detected by
> >> FID toggling (for example when last transaction with EOF is lost).
> >> This prevents passing incomplete buffers to the userspace.
> >
> > But this would also breaks buggy webcams that toggle the FID bit but don't
> > set the EOF bit. Support for this was added before the driver got merged
> > in the mainline kernel, and the SVN log is a bit terse I'm afraid:
> >
> > V 104
> > - Check both EOF and FID bits to detect end of frames.
> > - Updated disclaimer and general status comment.
> >
> > I don't remember which webcam(s) exhibit this behaviour.
> >
> > Your patch would also mark valid buffers as erroneous when the list EOF
> > bit is in a packet of its own with no data.
>
> If camera streams compressed video the patch doesn't affect it. It affects
> only uncompressed streams.
>
> If we get EOF bit in a packet of its own with no data we take second
> check under 'if (buf->state == UVC_BUF_STATE_READY)' that was before my
> patch. In this case if all data were received buffer is processed
> normally, but if some data is missing buffer is marked erroneous.
>
> > As the uvcvideo driver already marks buffers smaller than the expected
> > image size as erroneous, missing EOF packets that contain data should
> > already result in buffers with the error bit set.
>
> No, because there was no check for that in case then new frame is detected
> by FID toggling, it was there only for the case then new frame is detected
> by EOF bit.
Right, my bad, I had misread your patch. Please see below for a couple of
small comments.
> > Are you concerned about compressed formats only ?
>
> No, I'm concerning about uncompressed frames only.
>
> > While this patch would correctly detect the missing EOF packet in that
> > case, any other missing packet would still result in a corrupt image, so
> > I'm not sure if this would be worth it.
> >
> >> Signed-off-by: Anton Leontiev
> >> ---
> >>
> >> drivers/media/usb/uvc/uvc_video.c | 21 +++--
> >> 1 file changed, 15 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/media/usb/uvc/uvc_video.c
> >> b/drivers/media/usb/uvc/uvc_video.c index 8d52baf..57c9a4b 100644
> >> --- a/drivers/media/usb/uvc/uvc_video.c
> >> +++ b/drivers/media/usb/uvc/uvc_video.c
> >> @@ -1133,6 +1133,17 @@ static int uvc_video_encode_data(struct
> >> uvc_streaming *stream, */
> >>
> >> /*
> >>
> >> + * Set error flag for incomplete buffer.
> >> + */
> >> +static void uvc_buffer_check_bytesused(const struct uvc_streaming *const
> >> stream,
No need for the second const keyword here.
I would have used "uvc_video_" as a prefix, to be in sync with the surrounding
functions. What would you think of uvc_video_validate_buffer() ?
> >> + struct uvc_buffer *const buf)
And no need for const at all here.
> >> +{
> >> + if (buf->length != buf->bytesused &&
> >> + !(stream->cur_format->flags & UVC_FMT_FLAG_COMPRESSED))
The indentation is wrong here, the ! on the second line should be aligned to
the first 'buf' of the first line.
If you agree with these changes I can perform them while applying, there's no
need to resubmit the patch.
> >> + buf->error = 1;
> >> +}
> >> +
> >> +/*
> >> * Completion handler for video URBs.
> >> */
> >> static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming
> >> *stream,
> >> @@ -1156,9 +1167,11 @@ static void uvc_video_decode_isoc(struct
> >> urb *urb, struct uvc_streaming *stream,
> >>do {
> >>ret = uvc_video_decode_start(stream, buf, mem,
> >>urb->iso_frame_desc[i].actual_length);
> >> - if (ret == -EAGAIN)
> >> + if (ret == -EAGAIN) {
> >> + uvc_buffer_check_bytesused(stream, buf);
> >>buf = uvc_queue_next_buffer(>queue,
> >>buf);
> >> + }
> >>} while (ret == -EAGAIN);
> >>
> >>if (ret < 0)
> >> @@ -1173,11 +1186,7 @@ static void uvc_video_decode_isoc(struct urb *urb,
> >> struct uvc_streaming *stream, urb->iso_frame_desc[i].actual_length);
> >>if (buf->state == UVC_BUF_STATE_READY) {
> >> - if (buf->length != buf->bytesused &&
> >> - !(stream->cur_format->flags &
> >> -UVC_FMT_FLAG_COMPRESSED))
> >> - buf->error = 1;
> >> -
> >> + uvc_buffer_check_bytesused(stream, buf);
> >>buf = uvc_queue_next_buffer(>queue, buf);
> >>}
> >>}
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Re: [PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling
Hi Anton,
On Friday 28 March 2014 07:58:00 Anton Leontiev wrote:
26.03.2014 21:41, Laurent Pinchart wrote:
On Tuesday 25 March 2014 08:40:57 Anton Leontiev wrote:
Set error bit for incomplete buffers when end of buffer is detected by
FID toggling (for example when last transaction with EOF is lost).
This prevents passing incomplete buffers to the userspace.
But this would also breaks buggy webcams that toggle the FID bit but don't
set the EOF bit. Support for this was added before the driver got merged
in the mainline kernel, and the SVN log is a bit terse I'm afraid:
V 104
- Check both EOF and FID bits to detect end of frames.
- Updated disclaimer and general status comment.
I don't remember which webcam(s) exhibit this behaviour.
Your patch would also mark valid buffers as erroneous when the list EOF
bit is in a packet of its own with no data.
If camera streams compressed video the patch doesn't affect it. It affects
only uncompressed streams.
If we get EOF bit in a packet of its own with no data we take second
check under 'if (buf-state == UVC_BUF_STATE_READY)' that was before my
patch. In this case if all data were received buffer is processed
normally, but if some data is missing buffer is marked erroneous.
As the uvcvideo driver already marks buffers smaller than the expected
image size as erroneous, missing EOF packets that contain data should
already result in buffers with the error bit set.
No, because there was no check for that in case then new frame is detected
by FID toggling, it was there only for the case then new frame is detected
by EOF bit.
Right, my bad, I had misread your patch. Please see below for a couple of
small comments.
Are you concerned about compressed formats only ?
No, I'm concerning about uncompressed frames only.
While this patch would correctly detect the missing EOF packet in that
case, any other missing packet would still result in a corrupt image, so
I'm not sure if this would be worth it.
Signed-off-by: Anton Leontiev bun...@t-25.ru
---
drivers/media/usb/uvc/uvc_video.c | 21 +++--
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_video.c
b/drivers/media/usb/uvc/uvc_video.c index 8d52baf..57c9a4b 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1133,6 +1133,17 @@ static int uvc_video_encode_data(struct
uvc_streaming *stream, */
/*
+ * Set error flag for incomplete buffer.
+ */
+static void uvc_buffer_check_bytesused(const struct uvc_streaming *const
stream,
No need for the second const keyword here.
I would have used uvc_video_ as a prefix, to be in sync with the surrounding
functions. What would you think of uvc_video_validate_buffer() ?
+ struct uvc_buffer *const buf)
And no need for const at all here.
+{
+ if (buf-length != buf-bytesused
+ !(stream-cur_format-flags UVC_FMT_FLAG_COMPRESSED))
The indentation is wrong here, the ! on the second line should be aligned to
the first 'buf' of the first line.
If you agree with these changes I can perform them while applying, there's no
need to resubmit the patch.
+ buf-error = 1;
+}
+
+/*
* Completion handler for video URBs.
*/
static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming
*stream,
@@ -1156,9 +1167,11 @@ static void uvc_video_decode_isoc(struct
urb *urb, struct uvc_streaming *stream,
do {
ret = uvc_video_decode_start(stream, buf, mem,
urb-iso_frame_desc[i].actual_length);
- if (ret == -EAGAIN)
+ if (ret == -EAGAIN) {
+ uvc_buffer_check_bytesused(stream, buf);
buf = uvc_queue_next_buffer(stream-queue,
buf);
+ }
} while (ret == -EAGAIN);
if (ret 0)
@@ -1173,11 +1186,7 @@ static void uvc_video_decode_isoc(struct urb *urb,
struct uvc_streaming *stream, urb-iso_frame_desc[i].actual_length);
if (buf-state == UVC_BUF_STATE_READY) {
- if (buf-length != buf-bytesused
- !(stream-cur_format-flags
-UVC_FMT_FLAG_COMPRESSED))
- buf-error = 1;
-
+ uvc_buffer_check_bytesused(stream, buf);
buf = uvc_queue_next_buffer(stream-queue, buf);
}
}
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling
28.03.2014 20:12, Laurent Pinchart пишет:
+ * Set error flag for incomplete buffer.
+ */
+static void uvc_buffer_check_bytesused(const struct uvc_streaming *const
stream,
No need for the second const keyword here.
I would have used uvc_video_ as a prefix, to be in sync with the
surrounding
functions. What would you think of uvc_video_validate_buffer() ?
+ struct uvc_buffer *const buf)
And no need for const at all here.
+{
+ if (buf-length != buf-bytesused
+ !(stream-cur_format-flags UVC_FMT_FLAG_COMPRESSED))
The indentation is wrong here, the ! on the second line should be aligned to
the first 'buf' of the first line.
If you agree with these changes I can perform them while applying, there's no
need to resubmit the patch.
Thank you for reviewing my first patch to Linux kernel. I completely
agree with your changes.
Just want to ask why there is no need for the second 'const' after
pointer character '*'? I thought it marks pointer itself as constant for
type-checking opposite to first 'const', which marks memory it points to
as constant for type-checking. I understand that the function is simple
enough to verify it by hand but it's better to add more information for
automatic checking.
Is there any guidelines on 'const' keyword usage in Linux kernel code?
Regards,
--
Anton Leontiev
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling
26.03.2014 21:41, Laurent Pinchart wrote:
> Hi Anton,
>
> Thank you for the patch.
>
> On Tuesday 25 March 2014 08:40:57 Anton Leontiev wrote:
>> Set error bit for incomplete buffers when end of buffer is detected by
>> FID toggling (for example when last transaction with EOF is lost).
>> This prevents passing incomplete buffers to the userspace.
>
> But this would also breaks buggy webcams that toggle the FID bit but don't
> set
> the EOF bit. Support for this was added before the driver got merged in the
> mainline kernel, and the SVN log is a bit terse I'm afraid:
>
> V 104
> - Check both EOF and FID bits to detect end of frames.
> - Updated disclaimer and general status comment.
>
> I don't remember which webcam(s) exhibit this behaviour.
>
> Your patch would also mark valid buffers as erroneous when the list EOF bit
> is
> in a packet of its own with no data.
If camera streams compressed video the patch doesn't affect it. It
affects only uncompressed streams.
If we get EOF bit in a packet of its own with no data we take second
check under 'if (buf->state == UVC_BUF_STATE_READY)' that was before my
patch. In this case if all data were received buffer is processed
normally, but if some data is missing buffer is marked erroneous.
> As the uvcvideo driver already marks buffers smaller than the expected image
> size as erroneous, missing EOF packets that contain data should already
> result
> in buffers with the error bit set.
No, because there was no check for that in case then new frame is
detected by FID toggling, it was there only for the case then new frame
is detected by EOF bit.
> Are you concerned about compressed formats only ?
No, I'm concerning about uncompressed frames only.
> While this patch would correctly detect the missing EOF packet in that
> case, any other missing packet would still result in a corrupt image, so I'm
> not sure if this would be worth it.
>
>> Signed-off-by: Anton Leontiev
>> ---
>> drivers/media/usb/uvc/uvc_video.c | 21 +++--
>> 1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_video.c
>> b/drivers/media/usb/uvc/uvc_video.c index 8d52baf..57c9a4b 100644
>> --- a/drivers/media/usb/uvc/uvc_video.c
>> +++ b/drivers/media/usb/uvc/uvc_video.c
>> @@ -1133,6 +1133,17 @@ static int uvc_video_encode_data(struct uvc_streaming
>> *stream, */
>>
>> /*
>> + * Set error flag for incomplete buffer.
>> + */
>> +static void uvc_buffer_check_bytesused(const struct uvc_streaming *const
>> stream,
>> +struct uvc_buffer *const buf)
>> +{
>> +if (buf->length != buf->bytesused &&
>> +!(stream->cur_format->flags & UVC_FMT_FLAG_COMPRESSED))
>> +buf->error = 1;
>> +}
>> +
>> +/*
>> * Completion handler for video URBs.
>> */
>> static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming
>> *stream, @@ -1156,9 +1167,11 @@ static void uvc_video_decode_isoc(struct
>> urb *urb, struct uvc_streaming *stream, do {
>> ret = uvc_video_decode_start(stream, buf, mem,
>> urb->iso_frame_desc[i].actual_length);
>> -if (ret == -EAGAIN)
>> +if (ret == -EAGAIN) {
>> +uvc_buffer_check_bytesused(stream, buf);
>> buf = uvc_queue_next_buffer(>queue,
>> buf);
>> +}
>> } while (ret == -EAGAIN);
>>
>> if (ret < 0)
>> @@ -1173,11 +1186,7 @@ static void uvc_video_decode_isoc(struct urb *urb,
>> struct uvc_streaming *stream, urb->iso_frame_desc[i].actual_length);
>>
>> if (buf->state == UVC_BUF_STATE_READY) {
>> -if (buf->length != buf->bytesused &&
>> -!(stream->cur_format->flags &
>> - UVC_FMT_FLAG_COMPRESSED))
>> -buf->error = 1;
>> -
>> +uvc_buffer_check_bytesused(stream, buf);
>> buf = uvc_queue_next_buffer(>queue, buf);
>> }
>> }
>
--
Anton Leontiev
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling
26.03.2014 21:41, Laurent Pinchart wrote:
Hi Anton,
Thank you for the patch.
On Tuesday 25 March 2014 08:40:57 Anton Leontiev wrote:
Set error bit for incomplete buffers when end of buffer is detected by
FID toggling (for example when last transaction with EOF is lost).
This prevents passing incomplete buffers to the userspace.
But this would also breaks buggy webcams that toggle the FID bit but don't
set
the EOF bit. Support for this was added before the driver got merged in the
mainline kernel, and the SVN log is a bit terse I'm afraid:
V 104
- Check both EOF and FID bits to detect end of frames.
- Updated disclaimer and general status comment.
I don't remember which webcam(s) exhibit this behaviour.
Your patch would also mark valid buffers as erroneous when the list EOF bit
is
in a packet of its own with no data.
If camera streams compressed video the patch doesn't affect it. It
affects only uncompressed streams.
If we get EOF bit in a packet of its own with no data we take second
check under 'if (buf-state == UVC_BUF_STATE_READY)' that was before my
patch. In this case if all data were received buffer is processed
normally, but if some data is missing buffer is marked erroneous.
As the uvcvideo driver already marks buffers smaller than the expected image
size as erroneous, missing EOF packets that contain data should already
result
in buffers with the error bit set.
No, because there was no check for that in case then new frame is
detected by FID toggling, it was there only for the case then new frame
is detected by EOF bit.
Are you concerned about compressed formats only ?
No, I'm concerning about uncompressed frames only.
While this patch would correctly detect the missing EOF packet in that
case, any other missing packet would still result in a corrupt image, so I'm
not sure if this would be worth it.
Signed-off-by: Anton Leontiev bun...@t-25.ru
---
drivers/media/usb/uvc/uvc_video.c | 21 +++--
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_video.c
b/drivers/media/usb/uvc/uvc_video.c index 8d52baf..57c9a4b 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1133,6 +1133,17 @@ static int uvc_video_encode_data(struct uvc_streaming
*stream, */
/*
+ * Set error flag for incomplete buffer.
+ */
+static void uvc_buffer_check_bytesused(const struct uvc_streaming *const
stream,
+struct uvc_buffer *const buf)
+{
+if (buf-length != buf-bytesused
+!(stream-cur_format-flags UVC_FMT_FLAG_COMPRESSED))
+buf-error = 1;
+}
+
+/*
* Completion handler for video URBs.
*/
static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming
*stream, @@ -1156,9 +1167,11 @@ static void uvc_video_decode_isoc(struct
urb *urb, struct uvc_streaming *stream, do {
ret = uvc_video_decode_start(stream, buf, mem,
urb-iso_frame_desc[i].actual_length);
-if (ret == -EAGAIN)
+if (ret == -EAGAIN) {
+uvc_buffer_check_bytesused(stream, buf);
buf = uvc_queue_next_buffer(stream-queue,
buf);
+}
} while (ret == -EAGAIN);
if (ret 0)
@@ -1173,11 +1186,7 @@ static void uvc_video_decode_isoc(struct urb *urb,
struct uvc_streaming *stream, urb-iso_frame_desc[i].actual_length);
if (buf-state == UVC_BUF_STATE_READY) {
-if (buf-length != buf-bytesused
-!(stream-cur_format-flags
- UVC_FMT_FLAG_COMPRESSED))
-buf-error = 1;
-
+uvc_buffer_check_bytesused(stream, buf);
buf = uvc_queue_next_buffer(stream-queue, buf);
}
}
--
Anton Leontiev
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling
Hi Anton,
Thank you for the patch.
On Tuesday 25 March 2014 08:40:57 Anton Leontiev wrote:
> Set error bit for incomplete buffers when end of buffer is detected by
> FID toggling (for example when last transaction with EOF is lost).
> This prevents passing incomplete buffers to the userspace.
But this would also breaks buggy webcams that toggle the FID bit but don't set
the EOF bit. Support for this was added before the driver got merged in the
mainline kernel, and the SVN log is a bit terse I'm afraid:
V 104
- Check both EOF and FID bits to detect end of frames.
- Updated disclaimer and general status comment.
I don't remember which webcam(s) exhibit this behaviour.
Your patch would also mark valid buffers as erroneous when the list EOF bit is
in a packet of its own with no data.
As the uvcvideo driver already marks buffers smaller than the expected image
size as erroneous, missing EOF packets that contain data should already result
in buffers with the error bit set. Are you concerned about compressed formats
only ? While this patch would correctly detect the missing EOF packet in that
case, any other missing packet would still result in a corrupt image, so I'm
not sure if this would be worth it.
> Signed-off-by: Anton Leontiev
> ---
> drivers/media/usb/uvc/uvc_video.c | 21 +++--
> 1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index 8d52baf..57c9a4b 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1133,6 +1133,17 @@ static int uvc_video_encode_data(struct uvc_streaming
> *stream, */
>
> /*
> + * Set error flag for incomplete buffer.
> + */
> +static void uvc_buffer_check_bytesused(const struct uvc_streaming *const
> stream,
> + struct uvc_buffer *const buf)
> +{
> + if (buf->length != buf->bytesused &&
> + !(stream->cur_format->flags & UVC_FMT_FLAG_COMPRESSED))
> + buf->error = 1;
> +}
> +
> +/*
> * Completion handler for video URBs.
> */
> static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming
> *stream, @@ -1156,9 +1167,11 @@ static void uvc_video_decode_isoc(struct
> urb *urb, struct uvc_streaming *stream, do {
> ret = uvc_video_decode_start(stream, buf, mem,
> urb->iso_frame_desc[i].actual_length);
> - if (ret == -EAGAIN)
> + if (ret == -EAGAIN) {
> + uvc_buffer_check_bytesused(stream, buf);
> buf = uvc_queue_next_buffer(>queue,
> buf);
> + }
> } while (ret == -EAGAIN);
>
> if (ret < 0)
> @@ -1173,11 +1186,7 @@ static void uvc_video_decode_isoc(struct urb *urb,
> struct uvc_streaming *stream, urb->iso_frame_desc[i].actual_length);
>
> if (buf->state == UVC_BUF_STATE_READY) {
> - if (buf->length != buf->bytesused &&
> - !(stream->cur_format->flags &
> - UVC_FMT_FLAG_COMPRESSED))
> - buf->error = 1;
> -
> + uvc_buffer_check_bytesused(stream, buf);
> buf = uvc_queue_next_buffer(>queue, buf);
> }
> }
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling
Hi Anton,
Thank you for the patch.
On Tuesday 25 March 2014 08:40:57 Anton Leontiev wrote:
Set error bit for incomplete buffers when end of buffer is detected by
FID toggling (for example when last transaction with EOF is lost).
This prevents passing incomplete buffers to the userspace.
But this would also breaks buggy webcams that toggle the FID bit but don't set
the EOF bit. Support for this was added before the driver got merged in the
mainline kernel, and the SVN log is a bit terse I'm afraid:
V 104
- Check both EOF and FID bits to detect end of frames.
- Updated disclaimer and general status comment.
I don't remember which webcam(s) exhibit this behaviour.
Your patch would also mark valid buffers as erroneous when the list EOF bit is
in a packet of its own with no data.
As the uvcvideo driver already marks buffers smaller than the expected image
size as erroneous, missing EOF packets that contain data should already result
in buffers with the error bit set. Are you concerned about compressed formats
only ? While this patch would correctly detect the missing EOF packet in that
case, any other missing packet would still result in a corrupt image, so I'm
not sure if this would be worth it.
Signed-off-by: Anton Leontiev bun...@t-25.ru
---
drivers/media/usb/uvc/uvc_video.c | 21 +++--
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_video.c
b/drivers/media/usb/uvc/uvc_video.c index 8d52baf..57c9a4b 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1133,6 +1133,17 @@ static int uvc_video_encode_data(struct uvc_streaming
*stream, */
/*
+ * Set error flag for incomplete buffer.
+ */
+static void uvc_buffer_check_bytesused(const struct uvc_streaming *const
stream,
+ struct uvc_buffer *const buf)
+{
+ if (buf-length != buf-bytesused
+ !(stream-cur_format-flags UVC_FMT_FLAG_COMPRESSED))
+ buf-error = 1;
+}
+
+/*
* Completion handler for video URBs.
*/
static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming
*stream, @@ -1156,9 +1167,11 @@ static void uvc_video_decode_isoc(struct
urb *urb, struct uvc_streaming *stream, do {
ret = uvc_video_decode_start(stream, buf, mem,
urb-iso_frame_desc[i].actual_length);
- if (ret == -EAGAIN)
+ if (ret == -EAGAIN) {
+ uvc_buffer_check_bytesused(stream, buf);
buf = uvc_queue_next_buffer(stream-queue,
buf);
+ }
} while (ret == -EAGAIN);
if (ret 0)
@@ -1173,11 +1186,7 @@ static void uvc_video_decode_isoc(struct urb *urb,
struct uvc_streaming *stream, urb-iso_frame_desc[i].actual_length);
if (buf-state == UVC_BUF_STATE_READY) {
- if (buf-length != buf-bytesused
- !(stream-cur_format-flags
- UVC_FMT_FLAG_COMPRESSED))
- buf-error = 1;
-
+ uvc_buffer_check_bytesused(stream, buf);
buf = uvc_queue_next_buffer(stream-queue, buf);
}
}
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling
Set error bit for incomplete buffers when end of buffer is detected by
FID toggling (for example when last transaction with EOF is lost).
This prevents passing incomplete buffers to the userspace.
Signed-off-by: Anton Leontiev
---
drivers/media/usb/uvc/uvc_video.c | 21 +++--
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_video.c
b/drivers/media/usb/uvc/uvc_video.c
index 8d52baf..57c9a4b 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1133,6 +1133,17 @@ static int uvc_video_encode_data(struct uvc_streaming
*stream,
*/
/*
+ * Set error flag for incomplete buffer.
+ */
+static void uvc_buffer_check_bytesused(const struct uvc_streaming *const
stream,
+ struct uvc_buffer *const buf)
+{
+ if (buf->length != buf->bytesused &&
+ !(stream->cur_format->flags & UVC_FMT_FLAG_COMPRESSED))
+ buf->error = 1;
+}
+
+/*
* Completion handler for video URBs.
*/
static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming
*stream,
@@ -1156,9 +1167,11 @@ static void uvc_video_decode_isoc(struct urb *urb,
struct uvc_streaming *stream,
do {
ret = uvc_video_decode_start(stream, buf, mem,
urb->iso_frame_desc[i].actual_length);
- if (ret == -EAGAIN)
+ if (ret == -EAGAIN) {
+ uvc_buffer_check_bytesused(stream, buf);
buf = uvc_queue_next_buffer(>queue,
buf);
+ }
} while (ret == -EAGAIN);
if (ret < 0)
@@ -1173,11 +1186,7 @@ static void uvc_video_decode_isoc(struct urb *urb,
struct uvc_streaming *stream,
urb->iso_frame_desc[i].actual_length);
if (buf->state == UVC_BUF_STATE_READY) {
- if (buf->length != buf->bytesused &&
- !(stream->cur_format->flags &
- UVC_FMT_FLAG_COMPRESSED))
- buf->error = 1;
-
+ uvc_buffer_check_bytesused(stream, buf);
buf = uvc_queue_next_buffer(>queue, buf);
}
}
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling
Set error bit for incomplete buffers when end of buffer is detected by
FID toggling (for example when last transaction with EOF is lost).
This prevents passing incomplete buffers to the userspace.
Signed-off-by: Anton Leontiev bun...@t-25.ru
---
drivers/media/usb/uvc/uvc_video.c | 21 +++--
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_video.c
b/drivers/media/usb/uvc/uvc_video.c
index 8d52baf..57c9a4b 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1133,6 +1133,17 @@ static int uvc_video_encode_data(struct uvc_streaming
*stream,
*/
/*
+ * Set error flag for incomplete buffer.
+ */
+static void uvc_buffer_check_bytesused(const struct uvc_streaming *const
stream,
+ struct uvc_buffer *const buf)
+{
+ if (buf-length != buf-bytesused
+ !(stream-cur_format-flags UVC_FMT_FLAG_COMPRESSED))
+ buf-error = 1;
+}
+
+/*
* Completion handler for video URBs.
*/
static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming
*stream,
@@ -1156,9 +1167,11 @@ static void uvc_video_decode_isoc(struct urb *urb,
struct uvc_streaming *stream,
do {
ret = uvc_video_decode_start(stream, buf, mem,
urb-iso_frame_desc[i].actual_length);
- if (ret == -EAGAIN)
+ if (ret == -EAGAIN) {
+ uvc_buffer_check_bytesused(stream, buf);
buf = uvc_queue_next_buffer(stream-queue,
buf);
+ }
} while (ret == -EAGAIN);
if (ret 0)
@@ -1173,11 +1186,7 @@ static void uvc_video_decode_isoc(struct urb *urb,
struct uvc_streaming *stream,
urb-iso_frame_desc[i].actual_length);
if (buf-state == UVC_BUF_STATE_READY) {
- if (buf-length != buf-bytesused
- !(stream-cur_format-flags
- UVC_FMT_FLAG_COMPRESSED))
- buf-error = 1;
-
+ uvc_buffer_check_bytesused(stream, buf);
buf = uvc_queue_next_buffer(stream-queue, buf);
}
}
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
