[libcamera-devel] [PATCH] libcamera: v4l2_subdevice: Silence warning for unknown metadata formats

[Umang Jain]

Hi Laurent,

Thank you for the patch.

On 9/5/22 10:16 PM, Laurent Pinchart via libcamera-devel wrote:
> Commit e297673e7686 ("libcamera: v4l2_device: Adjust colorspace based on
> pixel format") has introduced a warning when trying to convert a color
> space from V4L2 to libcamera if the media bus code is unknown. This was
> meant to catch unknown image formats, but turned out to be also
> triggered for metadata formats.

err...
>
> Color spaces are not applicable to metadata formats, there should thus
> be no warning. Fix it by skipping the color space translation and
> returning std::nullopt directly if the kernel reports
> V4L2_COLORSPACE_DEFAULT. This doesn't introduce any change in behaviour
> other than getting rid of the warning, as the V4L2Device::toColorSpace()
> function returns std::nullopt alread in that case.

s/alread/already
>
> Fixes: e297673e7686 ("libcamera: v4l2_device: Adjust colorspace based on pixel format")
> Reported-by: Naushir Patuck <naush at raspberrypi.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>   include/libcamera/internal/v4l2_subdevice.h |  3 ++
>   src/libcamera/v4l2_subdevice.cpp            | 51 ++++++++++-----------
>   2 files changed, 26 insertions(+), 28 deletions(-)
>
> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> index 00be17bb1465..69862de0585a 100644
> --- a/include/libcamera/internal/v4l2_subdevice.h
> +++ b/include/libcamera/internal/v4l2_subdevice.h
> @@ -101,6 +101,9 @@ protected:
>   private:
>   	LIBCAMERA_DISABLE_COPY(V4L2Subdevice)
>   
> +	std::optional<ColorSpace>
> +	toColorSpace(const v4l2_mbus_framefmt &format) const;
> +
>   	std::vector<unsigned int> enumPadCodes(unsigned int pad);
>   	std::vector<SizeRange> enumPadSizes(unsigned int pad,
>   					    unsigned int code);
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 95bfde340d8c..f3a9a0096c6c 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -477,6 +477,27 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
>   	return formats;
>   }
>   
> +std::optional<ColorSpace> V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &format) const
> +{

Comment about metadata formats <>  V4L2_COLORSPACE_DEFAULT please :-)

Other than that, looks good to me.

Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> +	if (format.colorspace == V4L2_COLORSPACE_DEFAULT)
> +		return std::nullopt;
> +
> +	PixelFormatInfo::ColourEncoding colourEncoding;
> +	auto iter = formatInfoMap.find(format.code);
> +	if (iter != formatInfoMap.end()) {
> +		colourEncoding = iter->second.colourEncoding;
> +	} else {
> +		LOG(V4L2, Warning)
> +			<< "Unknown subdev format "
> +			<< utils::hex(format.code, 4)
> +			<< ", defaulting to RGB encoding";
> +
> +		colourEncoding = PixelFormatInfo::ColourEncodingRGB;
> +	}
> +
> +	return V4L2Device::toColorSpace(format, colourEncoding);
> +}
> +
>   /**
>    * \brief Retrieve the image format set on one of the V4L2 subdevice pads
>    * \param[in] pad The 0-indexed pad number the format is to be retrieved from
> @@ -503,20 +524,7 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>   	format->size.width = subdevFmt.format.width;
>   	format->size.height = subdevFmt.format.height;
>   	format->mbus_code = subdevFmt.format.code;
> -
> -	PixelFormatInfo::ColourEncoding colourEncoding;
> -	auto iter = formatInfoMap.find(subdevFmt.format.code);
> -	if (iter != formatInfoMap.end()) {
> -		colourEncoding = iter->second.colourEncoding;
> -	} else {
> -		LOG(V4L2, Warning)
> -			<< "Unknown subdev format "
> -			<< utils::hex(subdevFmt.format.code, 4)
> -			<< ", defaulting to RGB encoding";
> -
> -		colourEncoding = PixelFormatInfo::ColourEncodingRGB;
> -	}
> -	format->colorSpace = toColorSpace(subdevFmt.format, colourEncoding);
> +	format->colorSpace = toColorSpace(subdevFmt.format);
>   
>   	return 0;
>   }
> @@ -562,20 +570,7 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>   	format->size.width = subdevFmt.format.width;
>   	format->size.height = subdevFmt.format.height;
>   	format->mbus_code = subdevFmt.format.code;
> -
> -	PixelFormatInfo::ColourEncoding colourEncoding;
> -	auto iter = formatInfoMap.find(subdevFmt.format.code);
> -	if (iter != formatInfoMap.end()) {
> -		colourEncoding = iter->second.colourEncoding;
> -	} else {
> -		LOG(V4L2, Warning)
> -			<< "Unknown subdev format "
> -			<< utils::hex(subdevFmt.format.code, 4)
> -			<< ", defaulting to RGB encoding";
> -
> -		colourEncoding = PixelFormatInfo::ColourEncodingRGB;
> -	}
> -	format->colorSpace = toColorSpace(subdevFmt.format, colourEncoding);
> +	format->colorSpace = toColorSpace(subdevFmt.format);
>   
>   	return 0;
>   }
>
> base-commit: da9bb8dea6821edcfd3464e8cba37ad4a91087d6