Re: [PATCH v1 08/19] uvcvideo: Add UVC1.5 VP8 format support.

2013-11-10 Thread Laurent Pinchart 
Hi Pawel,

Thank you for the patch.

On Friday 30 August 2013 11:17:07 Pawel Osciak wrote:
 Add detection and parsing of VP8 format and frame descriptors and
 reorganize format parsing code.
 
 Signed-off-by: Pawel Osciak posc...@chromium.org
 ---
  drivers/media/usb/uvc/uvc_driver.c | 120 +-
  drivers/media/usb/uvc/uvcvideo.h   |   4 +-
  include/uapi/linux/usb/video.h |   8 +++
  3 files changed, 104 insertions(+), 28 deletions(-)
 
 diff --git a/drivers/media/usb/uvc/uvc_driver.c
 b/drivers/media/usb/uvc/uvc_driver.c index 936ddc7..27a7a11 100644
 --- a/drivers/media/usb/uvc/uvc_driver.c
 +++ b/drivers/media/usb/uvc/uvc_driver.c
 @@ -312,7 +312,7 @@ static int uvc_parse_format(struct uvc_device *dev,
   struct uvc_frame *frame;
   const unsigned char *start = buffer;
   unsigned int interval;
 - unsigned int i, n;
 + unsigned int i, n, intervals_off;

Could you please define the intervals_off variable on a new line, right above 
interval ?

   __u8 ftype;
 
   format-type = buffer[2];
 @@ -401,6 +401,18 @@ static int uvc_parse_format(struct uvc_device *dev,
   format-nframes = 1;
   break;
 
 + case UVC_VS_FORMAT_VP8:
 + if (buflen  13)
 + goto format_error;
 +
 + format-bpp = 0;
 + format-flags = UVC_FMT_FLAG_COMPRESSED;
 + ftype = UVC_VS_FRAME_VP8;

Nitpicking, could you please move this line after format-fcc, to keep 
statements initializing format together ?

 + strlcpy(format-name, VP8, sizeof(format-name));
 + format-fcc = V4L2_PIX_FMT_VP8;
 +
 + break;
 +
   case UVC_VS_FORMAT_MPEG2TS:
   case UVC_VS_FORMAT_STREAM_BASED:
   /* Not supported yet. */
 @@ -417,44 +429,83 @@ static int uvc_parse_format(struct uvc_device *dev,
   buflen -= buffer[0];
   buffer += buffer[0];
 
 - /* Parse the frame descriptors. Only uncompressed, MJPEG and frame
 -  * based formats have frame descriptors.
 + /* Parse the frame descriptors. Only uncompressed, MJPEG, temporally
 +  * encoded and frame based formats have frame descriptors.
*/
   while (buflen  2  buffer[1] == USB_DT_CS_INTERFACE 
  buffer[2] == ftype) {
   frame = format-frame[format-nframes];
 - if (ftype != UVC_VS_FRAME_FRAME_BASED)
 - n = buflen  25 ? buffer[25] : 0;
 - else
 - n = buflen  21 ? buffer[21] : 0;
 -
 - n = n ? n : 3;
 
 - if (buflen  26 + 4*n) {
 - uvc_trace(UVC_TRACE_DESCR, device %d videostreaming 
 -interface %d FRAME error\n, dev-udev-devnum,
 -alts-desc.bInterfaceNumber);
 - return -EINVAL;
 - }
 -
 - frame-bFrameIndex = buffer[3];
 - frame-bmCapabilities = buffer[4];
 - frame-wWidth = get_unaligned_le16(buffer[5]);
 - frame-wHeight = get_unaligned_le16(buffer[7]);
 - frame-dwMinBitRate = get_unaligned_le32(buffer[9]);
 - frame-dwMaxBitRate = get_unaligned_le32(buffer[13]);
 - if (ftype != UVC_VS_FRAME_FRAME_BASED) {

I'd like to create a uvc_parse_frame function for the code below. The function 
should be created in a new patch before this one. I can do it if you would 
like me to.

 + switch (ftype) {
 + case UVC_VS_FRAME_UNCOMPRESSED:
 + case UVC_VS_FRAME_MJPEG:
 + intervals_off = 26;
 + if (buflen  intervals_off)
 + goto frame_error;
 +
 + frame-bFrameIndex = buffer[3];
 + frame-bmCapabilities = buffer[4];
 + frame-wWidth = get_unaligned_le16(buffer[5]);
 + frame-wHeight = get_unaligned_le16(buffer[7]);
 + frame-dwMinBitRate = get_unaligned_le32(buffer[9]);
 + frame-dwMaxBitRate = get_unaligned_le32(buffer[13]);
   frame-dwMaxVideoFrameBufferSize =
   get_unaligned_le32(buffer[17]);
   frame-dwDefaultFrameInterval =
   get_unaligned_le32(buffer[21]);
 - frame-bFrameIntervalType = buffer[25];
 - } else {
 + frame-bFrameIntervalType = n = buffer[25];

One assignement per line please.

 + break;
 +
 + case UVC_VS_FRAME_FRAME_BASED:
 + intervals_off = 26;
 + if (buflen  intervals_off)
 + goto frame_error;
 +
 + frame-bFrameIndex = buffer[3];
 + frame-bmCapabilities = buffer[4];
 + frame-wWidth = get_unaligned_le16(buffer[5]);
 + frame-wHeight =

[PATCH v1 08/19] uvcvideo: Add UVC1.5 VP8 format support.

2013-08-29 Thread Pawel Osciak 
Add detection and parsing of VP8 format and frame descriptors and
reorganize format parsing code.

Signed-off-by: Pawel Osciak posc...@chromium.org
---
 drivers/media/usb/uvc/uvc_driver.c | 120 -
 drivers/media/usb/uvc/uvcvideo.h   |   4 +-
 include/uapi/linux/usb/video.h |   8 +++
 3 files changed, 104 insertions(+), 28 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c 
b/drivers/media/usb/uvc/uvc_driver.c
index 936ddc7..27a7a11 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -312,7 +312,7 @@ static int uvc_parse_format(struct uvc_device *dev,
struct uvc_frame *frame;
const unsigned char *start = buffer;
unsigned int interval;
-   unsigned int i, n;
+   unsigned int i, n, intervals_off;
__u8 ftype;
 
format-type = buffer[2];
@@ -401,6 +401,18 @@ static int uvc_parse_format(struct uvc_device *dev,
format-nframes = 1;
break;
 
+   case UVC_VS_FORMAT_VP8:
+   if (buflen  13)
+   goto format_error;
+
+   format-bpp = 0;
+   format-flags = UVC_FMT_FLAG_COMPRESSED;
+   ftype = UVC_VS_FRAME_VP8;
+   strlcpy(format-name, VP8, sizeof(format-name));
+   format-fcc = V4L2_PIX_FMT_VP8;
+
+   break;
+
case UVC_VS_FORMAT_MPEG2TS:
case UVC_VS_FORMAT_STREAM_BASED:
/* Not supported yet. */
@@ -417,44 +429,83 @@ static int uvc_parse_format(struct uvc_device *dev,
buflen -= buffer[0];
buffer += buffer[0];
 
-   /* Parse the frame descriptors. Only uncompressed, MJPEG and frame
-* based formats have frame descriptors.
+   /* Parse the frame descriptors. Only uncompressed, MJPEG, temporally
+* encoded and frame based formats have frame descriptors.
 */
while (buflen  2  buffer[1] == USB_DT_CS_INTERFACE 
   buffer[2] == ftype) {
frame = format-frame[format-nframes];
-   if (ftype != UVC_VS_FRAME_FRAME_BASED)
-   n = buflen  25 ? buffer[25] : 0;
-   else
-   n = buflen  21 ? buffer[21] : 0;
-
-   n = n ? n : 3;
 
-   if (buflen  26 + 4*n) {
-   uvc_trace(UVC_TRACE_DESCR, device %d videostreaming 
-  interface %d FRAME error\n, dev-udev-devnum,
-  alts-desc.bInterfaceNumber);
-   return -EINVAL;
-   }
-
-   frame-bFrameIndex = buffer[3];
-   frame-bmCapabilities = buffer[4];
-   frame-wWidth = get_unaligned_le16(buffer[5]);
-   frame-wHeight = get_unaligned_le16(buffer[7]);
-   frame-dwMinBitRate = get_unaligned_le32(buffer[9]);
-   frame-dwMaxBitRate = get_unaligned_le32(buffer[13]);
-   if (ftype != UVC_VS_FRAME_FRAME_BASED) {
+   switch (ftype) {
+   case UVC_VS_FRAME_UNCOMPRESSED:
+   case UVC_VS_FRAME_MJPEG:
+   intervals_off = 26;
+   if (buflen  intervals_off)
+   goto frame_error;
+
+   frame-bFrameIndex = buffer[3];
+   frame-bmCapabilities = buffer[4];
+   frame-wWidth = get_unaligned_le16(buffer[5]);
+   frame-wHeight = get_unaligned_le16(buffer[7]);
+   frame-dwMinBitRate = get_unaligned_le32(buffer[9]);
+   frame-dwMaxBitRate = get_unaligned_le32(buffer[13]);
frame-dwMaxVideoFrameBufferSize =
get_unaligned_le32(buffer[17]);
frame-dwDefaultFrameInterval =
get_unaligned_le32(buffer[21]);
-   frame-bFrameIntervalType = buffer[25];
-   } else {
+   frame-bFrameIntervalType = n = buffer[25];
+   break;
+
+   case UVC_VS_FRAME_FRAME_BASED:
+   intervals_off = 26;
+   if (buflen  intervals_off)
+   goto frame_error;
+
+   frame-bFrameIndex = buffer[3];
+   frame-bmCapabilities = buffer[4];
+   frame-wWidth = get_unaligned_le16(buffer[5]);
+   frame-wHeight = get_unaligned_le16(buffer[7]);
+   frame-dwMinBitRate = get_unaligned_le32(buffer[9]);
+   frame-dwMaxBitRate = get_unaligned_le32(buffer[13]);
frame-dwMaxVideoFrameBufferSize = 0;
frame-dwDefaultFrameInterval =
get_unaligned_le32(buffer[17]);
-   frame-bFrameIntervalType = buffer[21];
+