Re: [PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling

2014-03-28 Thread Anton Leontiev 
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

2014-03-28 Thread Anton Leontiev 
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

2014-03-27 Thread Anton Leontiev 
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

2014-03-27 Thread Anton Leontiev 
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/

[PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling

2014-03-24 Thread Anton Leontiev 
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

2014-03-24 Thread Anton Leontiev 
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/