Re: [PATCH v1 2/2] media: uvcvideo: Refactor frame parsing code into a uvc_parse_frame function

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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 &&

-- 
Regards,

Laurent Pinchart