[libcamera-devel] [PATCH v3 3/7] libcamera: v4l2_videodevice: Improve toColorSpace() readability

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Mon, Aug 29, 2022 at 10:07:38PM +0530, Umang Jain via libcamera-devel wrote:
> Abstract out usage of V4L2Device::toColorspace() to a separate
> namespace. Since it is separated out in an unnamed namespace,
> the V4L2Device::toColorSpace() function needs to made public.
> This is not an issue since V4L2Device is an internal class.
> 
> No functional changes intended.
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  include/libcamera/internal/v4l2_device.h | 14 +++++++-------
>  src/libcamera/v4l2_videodevice.cpp       | 23 +++++++++++++++--------
>  2 files changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index 75304be1..606332e1 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -49,6 +49,13 @@ public:
>  
>  	void updateControlInfo();
>  
> +	template<typename T>
> +	static std::optional<ColorSpace> toColorSpace(const T &v4l2Format,
> +						      PixelFormatInfo::ColourEncoding colourEncoding);
> +
> +	template<typename T>
> +	static int fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format);
> +
>  protected:
>  	V4L2Device(const std::string &deviceNode);
>  	~V4L2Device();
> @@ -60,13 +67,6 @@ protected:
>  
>  	int fd() const { return fd_.get(); }
>  
> -	template<typename T>
> -	static std::optional<ColorSpace> toColorSpace(const T &v4l2Format,
> -						      PixelFormatInfo::ColourEncoding colourEncoding);
> -
> -	template<typename T>
> -	static int fromColorSpace(const std::optional<ColorSpace> &colorSpace, T &v4l2Format);
> -
>  private:
>  	static ControlType v4l2CtrlType(uint32_t ctrlType);
>  	static std::unique_ptr<ControlId> v4l2ControlId(const v4l2_query_ext_ctrl &ctrl);
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 0e3f5436..b443253d 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -914,6 +914,17 @@ int V4L2VideoDevice::trySetFormatMeta(V4L2DeviceFormat *format, bool set)
>  	return 0;
>  }
>  
> +namespace {
> +
> +template<typename T>
> +std::optional<ColorSpace> getColorSpace(const T &v4l2Format)
> +{
> +	V4L2PixelFormat fourcc{ v4l2Format.pixelformat };
> +	return V4L2Device::toColorSpace(v4l2Format, PixelFormatInfo::info(fourcc).colourEncoding);
> +}

Have you considered making this function a (static and private) member
of the V4L2VideoDevice class ? That way you wouldn't have to make the
V4L2Device toColorSpace() function public. I'd then also call it
toColorSpace() instead of getColorSpace(). I think the resulting patch
would be simpler.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +
> +} /* namespace */
> +
>  int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)
>  {
>  	struct v4l2_format v4l2Format = {};
> @@ -931,8 +942,7 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)
>  	format->size.height = pix->height;
>  	format->fourcc = V4L2PixelFormat(pix->pixelformat);
>  	format->planesCount = pix->num_planes;
> -	format->colorSpace =
> -		toColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);
> +	format->colorSpace = getColorSpace(*pix);
>  
>  	for (unsigned int i = 0; i < format->planesCount; ++i) {
>  		format->planes[i].bpl = pix->plane_fmt[i].bytesperline;
> @@ -988,8 +998,7 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)
>  		format->planes[i].bpl = pix->plane_fmt[i].bytesperline;
>  		format->planes[i].size = pix->plane_fmt[i].sizeimage;
>  	}
> -	format->colorSpace =
> -		toColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);
> +	format->colorSpace = getColorSpace(*pix);
>  
>  	return 0;
>  }
> @@ -1013,8 +1022,7 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)
>  	format->planesCount = 1;
>  	format->planes[0].bpl = pix->bytesperline;
>  	format->planes[0].size = pix->sizeimage;
> -	format->colorSpace =
> -		toColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);
> +	format->colorSpace = getColorSpace(*pix);
>  
>  	return 0;
>  }
> @@ -1056,8 +1064,7 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)
>  	format->planesCount = 1;
>  	format->planes[0].bpl = pix->bytesperline;
>  	format->planes[0].size = pix->sizeimage;
> -	format->colorSpace =
> -		toColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);
> +	format->colorSpace = getColorSpace(*pix);
>  
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart