| Message ID | 20241107142204.1182969-3-bsevens@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | Skip parsing frames of type UVC_VS_UNDEFINED in | expand |
Hi Benoît, Thank you for the patch. On Thu, Nov 07, 2024 at 02:22:03PM +0000, Benoit Sevens wrote: > The ftype value does not change in the while loop so we can check it > before entering the while loop. Refactoring the frame parsing code into > a dedicated uvc_parse_frame function makes this more readable. > > Signed-off-by: Benoit Sevens <bsevens@google.com> > --- > drivers/media/usb/uvc/uvc_driver.c | 228 ++++++++++++++++------------- > 1 file changed, 123 insertions(+), 105 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index 13db0026dc1a..99f811ace5d6 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -220,6 +220,117 @@ static struct uvc_streaming *uvc_stream_new(struct uvc_device *dev, > * Descriptors parsing > */ > > +static int uvc_parse_frame(struct uvc_device *dev, struct uvc_streaming *streaming, > + struct uvc_format *format, struct uvc_frame *frame, u32 **intervals, > + u8 ftype, int width_multiplier, const unsigned char *buffer, int buflen) > +{ > + struct usb_interface *intf = streaming->intf; > + struct usb_host_interface *alts = intf->cur_altsetting; The intf variable is only used here, so you can write struct usb_host_interface *alts = streaming->intf->cur_altsetting; > + unsigned int i, n; > + unsigned int interval; > + unsigned int maxIntervalIndex; We usually sort variables in (more or less) decreasing line length order if nothing makes that inpractical. > + > + 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_dbg(dev, DESCR, > + "device %d videostreaming interface %d FRAME error\n", > + dev->udev->devnum, > + alts->desc.bInterfaceNumber); We can now reflow the code as the indentation level has decreased, for instance dev->udev->devnum, alts->desc.bInterfaceNumber); > + return -EINVAL; > + } > + > + frame->bFrameIndex = buffer[3]; > + frame->bmCapabilities = buffer[4]; > + frame->wWidth = get_unaligned_le16(&buffer[5]) > + * width_multiplier; > + 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) { > + frame->dwMaxVideoFrameBufferSize = > + get_unaligned_le32(&buffer[17]); > + frame->dwDefaultFrameInterval = > + get_unaligned_le32(&buffer[21]); > + frame->bFrameIntervalType = buffer[25]; > + } else { > + frame->dwMaxVideoFrameBufferSize = 0; > + frame->dwDefaultFrameInterval = > + get_unaligned_le32(&buffer[17]); > + frame->bFrameIntervalType = buffer[21]; > + } > + > + /* > + * Copy the frame intervals. > + * > + * Some bogus devices report dwMinFrameInterval equal to > + * dwMaxFrameInterval and have dwFrameIntervalStep set to > + * zero. Setting all null intervals to 1 fixes the problem and > + * some other divisions by zero that could happen. > + */ > + frame->dwFrameInterval = *intervals; > + > + for (i = 0; i < n; ++i) { > + interval = get_unaligned_le32(&buffer[26+4*i]); > + (*intervals)[i] = interval ? interval : 1; > + } > + > + /* > + * Apply more fixes, quirks and workarounds to handle incorrect > + * or broken descriptors. > + */ > + > + /* > + * Several UVC chipsets screw up dwMaxVideoFrameBufferSize > + * completely. Observed behaviours range from setting the > + * value to 1.1x the actual frame size to hardwiring the > + * 16 low bits to 0. This results in a higher than necessary > + * memory usage as well as a wrong image size information. For > + * uncompressed formats this can be fixed by computing the > + * value from the frame size. > + */ > + if (!(format->flags & UVC_FMT_FLAG_COMPRESSED)) > + frame->dwMaxVideoFrameBufferSize = format->bpp > + * frame->wWidth * frame->wHeight / 8; > + > + /* > + * Clamp the default frame interval to the boundaries. A zero > + * bFrameIntervalType value indicates a continuous frame > + * interval range, with dwFrameInterval[0] storing the minimum > + * value and dwFrameInterval[1] storing the maximum value. > + */ > + maxIntervalIndex = frame->bFrameIntervalType ? n - 1 : 1; > + frame->dwDefaultFrameInterval = > + clamp(frame->dwDefaultFrameInterval, > + frame->dwFrameInterval[0], > + frame->dwFrameInterval[maxIntervalIndex]); > + > + /* > + * Some devices report frame intervals that are not functional. > + * If the corresponding quirk is set, restrict operation to the > + * first interval only. > + */ > + if (dev->quirks & UVC_QUIRK_RESTRICT_FRAME_RATE) { > + frame->bFrameIntervalType = 1; > + (*intervals)[0] = frame->dwDefaultFrameInterval; > + } > + > + uvc_dbg(dev, DESCR, "- %ux%u (%u.%u fps)\n", > + frame->wWidth, frame->wHeight, > + 10000000 / frame->dwDefaultFrameInterval, > + (100000000 / frame->dwDefaultFrameInterval) % 10); > + > + *intervals += n; > + > + return buffer[0]; > +} > + > + > static int uvc_parse_format(struct uvc_device *dev, > struct uvc_streaming *streaming, struct uvc_format *format, > struct uvc_frame *frames, u32 **intervals, const unsigned char *buffer, > @@ -231,9 +342,9 @@ static int uvc_parse_format(struct uvc_device *dev, While at it, we can also merge the intf and alts variables here too. With this addressed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I can make all those small changes locally when applying the patch, unless you prefer submitting a new version yourself. > struct uvc_frame *frame; > const unsigned char *start = buffer; > unsigned int width_multiplier = 1; > - unsigned int interval; > unsigned int i, n; > u8 ftype; > + int ret; > > format->type = buffer[2]; > format->index = buffer[3]; > @@ -371,111 +482,18 @@ static int uvc_parse_format(struct uvc_device *dev, > * Parse the frame descriptors. Only uncompressed, MJPEG and frame > * based formats have frame descriptors. > */ > - while (ftype && buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE && > - buffer[2] == ftype) { > - unsigned int maxIntervalIndex; > - > - frame = &frames[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_dbg(dev, 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]) > - * width_multiplier; > - 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) { > - frame->dwMaxVideoFrameBufferSize = > - get_unaligned_le32(&buffer[17]); > - frame->dwDefaultFrameInterval = > - get_unaligned_le32(&buffer[21]); > - frame->bFrameIntervalType = buffer[25]; > - } else { > - frame->dwMaxVideoFrameBufferSize = 0; > - frame->dwDefaultFrameInterval = > - get_unaligned_le32(&buffer[17]); > - frame->bFrameIntervalType = buffer[21]; > - } > - > - /* > - * Copy the frame intervals. > - * > - * Some bogus devices report dwMinFrameInterval equal to > - * dwMaxFrameInterval and have dwFrameIntervalStep set to > - * zero. Setting all null intervals to 1 fixes the problem and > - * some other divisions by zero that could happen. > - */ > - frame->dwFrameInterval = *intervals; > - > - for (i = 0; i < n; ++i) { > - interval = get_unaligned_le32(&buffer[26+4*i]); > - (*intervals)[i] = interval ? interval : 1; > - } > - > - /* > - * Apply more fixes, quirks and workarounds to handle incorrect > - * or broken descriptors. > - */ > - > - /* > - * Several UVC chipsets screw up dwMaxVideoFrameBufferSize > - * completely. Observed behaviours range from setting the > - * value to 1.1x the actual frame size to hardwiring the > - * 16 low bits to 0. This results in a higher than necessary > - * memory usage as well as a wrong image size information. For > - * uncompressed formats this can be fixed by computing the > - * value from the frame size. > - */ > - if (!(format->flags & UVC_FMT_FLAG_COMPRESSED)) > - frame->dwMaxVideoFrameBufferSize = format->bpp > - * frame->wWidth * frame->wHeight / 8; > - > - /* > - * Clamp the default frame interval to the boundaries. A zero > - * bFrameIntervalType value indicates a continuous frame > - * interval range, with dwFrameInterval[0] storing the minimum > - * value and dwFrameInterval[1] storing the maximum value. > - */ > - maxIntervalIndex = frame->bFrameIntervalType ? n - 1 : 1; > - frame->dwDefaultFrameInterval = > - clamp(frame->dwDefaultFrameInterval, > - frame->dwFrameInterval[0], > - frame->dwFrameInterval[maxIntervalIndex]); > - > - /* > - * Some devices report frame intervals that are not functional. > - * If the corresponding quirk is set, restrict operation to the > - * first interval only. > - */ > - if (dev->quirks & UVC_QUIRK_RESTRICT_FRAME_RATE) { > - frame->bFrameIntervalType = 1; > - (*intervals)[0] = frame->dwDefaultFrameInterval; > + if (ftype) { > + while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE && > + buffer[2] == ftype) { > + frame = &frames[format->nframes]; > + ret = uvc_parse_frame(dev, streaming, format, frame, intervals, ftype, > + width_multiplier, buffer, buflen); > + if (ret < 0) > + return ret; > + format->nframes++; > + buflen -= ret; > + buffer += ret; > } > - > - uvc_dbg(dev, DESCR, "- %ux%u (%u.%u fps)\n", > - frame->wWidth, frame->wHeight, > - 10000000 / frame->dwDefaultFrameInterval, > - (100000000 / frame->dwDefaultFrameInterval) % 10); > - > - format->nframes++; > - *intervals += n; > - > - buflen -= buffer[0]; > - buffer += buffer[0]; > } > > if (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index 13db0026dc1a..99f811ace5d6 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -220,6 +220,117 @@ static struct uvc_streaming *uvc_stream_new(struct uvc_device *dev, * Descriptors parsing */ +static int uvc_parse_frame(struct uvc_device *dev, struct uvc_streaming *streaming, + struct uvc_format *format, struct uvc_frame *frame, u32 **intervals, + u8 ftype, int width_multiplier, const unsigned char *buffer, int buflen) +{ + struct usb_interface *intf = streaming->intf; + struct usb_host_interface *alts = intf->cur_altsetting; + unsigned int i, n; + unsigned int interval; + unsigned int maxIntervalIndex; + + 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_dbg(dev, 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]) + * width_multiplier; + 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) { + frame->dwMaxVideoFrameBufferSize = + get_unaligned_le32(&buffer[17]); + frame->dwDefaultFrameInterval = + get_unaligned_le32(&buffer[21]); + frame->bFrameIntervalType = buffer[25]; + } else { + frame->dwMaxVideoFrameBufferSize = 0; + frame->dwDefaultFrameInterval = + get_unaligned_le32(&buffer[17]); + frame->bFrameIntervalType = buffer[21]; + } + + /* + * Copy the frame intervals. + * + * Some bogus devices report dwMinFrameInterval equal to + * dwMaxFrameInterval and have dwFrameIntervalStep set to + * zero. Setting all null intervals to 1 fixes the problem and + * some other divisions by zero that could happen. + */ + frame->dwFrameInterval = *intervals; + + for (i = 0; i < n; ++i) { + interval = get_unaligned_le32(&buffer[26+4*i]); + (*intervals)[i] = interval ? interval : 1; + } + + /* + * Apply more fixes, quirks and workarounds to handle incorrect + * or broken descriptors. + */ + + /* + * Several UVC chipsets screw up dwMaxVideoFrameBufferSize + * completely. Observed behaviours range from setting the + * value to 1.1x the actual frame size to hardwiring the + * 16 low bits to 0. This results in a higher than necessary + * memory usage as well as a wrong image size information. For + * uncompressed formats this can be fixed by computing the + * value from the frame size. + */ + if (!(format->flags & UVC_FMT_FLAG_COMPRESSED)) + frame->dwMaxVideoFrameBufferSize = format->bpp + * frame->wWidth * frame->wHeight / 8; + + /* + * Clamp the default frame interval to the boundaries. A zero + * bFrameIntervalType value indicates a continuous frame + * interval range, with dwFrameInterval[0] storing the minimum + * value and dwFrameInterval[1] storing the maximum value. + */ + maxIntervalIndex = frame->bFrameIntervalType ? n - 1 : 1; + frame->dwDefaultFrameInterval = + clamp(frame->dwDefaultFrameInterval, + frame->dwFrameInterval[0], + frame->dwFrameInterval[maxIntervalIndex]); + + /* + * Some devices report frame intervals that are not functional. + * If the corresponding quirk is set, restrict operation to the + * first interval only. + */ + if (dev->quirks & UVC_QUIRK_RESTRICT_FRAME_RATE) { + frame->bFrameIntervalType = 1; + (*intervals)[0] = frame->dwDefaultFrameInterval; + } + + uvc_dbg(dev, DESCR, "- %ux%u (%u.%u fps)\n", + frame->wWidth, frame->wHeight, + 10000000 / frame->dwDefaultFrameInterval, + (100000000 / frame->dwDefaultFrameInterval) % 10); + + *intervals += n; + + return buffer[0]; +} + + static int uvc_parse_format(struct uvc_device *dev, struct uvc_streaming *streaming, struct uvc_format *format, struct uvc_frame *frames, u32 **intervals, const unsigned char *buffer, @@ -231,9 +342,9 @@ static int uvc_parse_format(struct uvc_device *dev, struct uvc_frame *frame; const unsigned char *start = buffer; unsigned int width_multiplier = 1; - unsigned int interval; unsigned int i, n; u8 ftype; + int ret; format->type = buffer[2]; format->index = buffer[3]; @@ -371,111 +482,18 @@ static int uvc_parse_format(struct uvc_device *dev, * Parse the frame descriptors. Only uncompressed, MJPEG and frame * based formats have frame descriptors. */ - while (ftype && buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE && - buffer[2] == ftype) { - unsigned int maxIntervalIndex; - - frame = &frames[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_dbg(dev, 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]) - * width_multiplier; - 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) { - frame->dwMaxVideoFrameBufferSize = - get_unaligned_le32(&buffer[17]); - frame->dwDefaultFrameInterval = - get_unaligned_le32(&buffer[21]); - frame->bFrameIntervalType = buffer[25]; - } else { - frame->dwMaxVideoFrameBufferSize = 0; - frame->dwDefaultFrameInterval = - get_unaligned_le32(&buffer[17]); - frame->bFrameIntervalType = buffer[21]; - } - - /* - * Copy the frame intervals. - * - * Some bogus devices report dwMinFrameInterval equal to - * dwMaxFrameInterval and have dwFrameIntervalStep set to - * zero. Setting all null intervals to 1 fixes the problem and - * some other divisions by zero that could happen. - */ - frame->dwFrameInterval = *intervals; - - for (i = 0; i < n; ++i) { - interval = get_unaligned_le32(&buffer[26+4*i]); - (*intervals)[i] = interval ? interval : 1; - } - - /* - * Apply more fixes, quirks and workarounds to handle incorrect - * or broken descriptors. - */ - - /* - * Several UVC chipsets screw up dwMaxVideoFrameBufferSize - * completely. Observed behaviours range from setting the - * value to 1.1x the actual frame size to hardwiring the - * 16 low bits to 0. This results in a higher than necessary - * memory usage as well as a wrong image size information. For - * uncompressed formats this can be fixed by computing the - * value from the frame size. - */ - if (!(format->flags & UVC_FMT_FLAG_COMPRESSED)) - frame->dwMaxVideoFrameBufferSize = format->bpp - * frame->wWidth * frame->wHeight / 8; - - /* - * Clamp the default frame interval to the boundaries. A zero - * bFrameIntervalType value indicates a continuous frame - * interval range, with dwFrameInterval[0] storing the minimum - * value and dwFrameInterval[1] storing the maximum value. - */ - maxIntervalIndex = frame->bFrameIntervalType ? n - 1 : 1; - frame->dwDefaultFrameInterval = - clamp(frame->dwDefaultFrameInterval, - frame->dwFrameInterval[0], - frame->dwFrameInterval[maxIntervalIndex]); - - /* - * Some devices report frame intervals that are not functional. - * If the corresponding quirk is set, restrict operation to the - * first interval only. - */ - if (dev->quirks & UVC_QUIRK_RESTRICT_FRAME_RATE) { - frame->bFrameIntervalType = 1; - (*intervals)[0] = frame->dwDefaultFrameInterval; + if (ftype) { + while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE && + buffer[2] == ftype) { + frame = &frames[format->nframes]; + ret = uvc_parse_frame(dev, streaming, format, frame, intervals, ftype, + width_multiplier, buffer, buflen); + if (ret < 0) + return ret; + format->nframes++; + buflen -= ret; + buffer += ret; } - - uvc_dbg(dev, DESCR, "- %ux%u (%u.%u fps)\n", - frame->wWidth, frame->wHeight, - 10000000 / frame->dwDefaultFrameInterval, - (100000000 / frame->dwDefaultFrameInterval) % 10); - - format->nframes++; - *intervals += n; - - buflen -= buffer[0]; - buffer += buffer[0]; } if (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
The ftype value does not change in the while loop so we can check it before entering the while loop. Refactoring the frame parsing code into a dedicated uvc_parse_frame function makes this more readable. Signed-off-by: Benoit Sevens <bsevens@google.com> --- drivers/media/usb/uvc/uvc_driver.c | 228 ++++++++++++++++------------- 1 file changed, 123 insertions(+), 105 deletions(-)