[libcamera-devel] [PATCH 1/2] libcamera: Add the fromV4L2PixelFormat method

[Laurent Pinchart]

Hi Sebastian,

Thank you for the patch.

On Wed, Dec 23, 2020 at 01:10:54PM +0100, Sebastian Fricke wrote:
> Add the method to get the corresponding bayer-format from a given
> V4L2-pixel-format. Uses the existing mapping table.
> 
> Signed-off-by: Sebastian Fricke <sebastian.fricke.linux at gmail.com>
> ---
>  include/libcamera/internal/bayer_format.h |  1 +
>  src/libcamera/bayer_format.cpp            | 14 ++++++++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h
> index 4280b76b..a8060029 100644
> --- a/include/libcamera/internal/bayer_format.h
> +++ b/include/libcamera/internal/bayer_format.h
> @@ -48,6 +48,7 @@ public:
>  	std::string toString() const;
>  
>  	V4L2PixelFormat toV4L2PixelFormat() const;
> +	BayerFormat fromV4L2PixelFormat(V4L2PixelFormat v4l2Format) const;

This should be a static function as it doesn't access any instance
member.

>  	BayerFormat transform(Transform t) const;
>  
>  	Order order;
> diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp
> index c42792ff..f4f0e8ca 100644
> --- a/src/libcamera/bayer_format.cpp
> +++ b/src/libcamera/bayer_format.cpp
> @@ -217,6 +217,20 @@ V4L2PixelFormat BayerFormat::toV4L2PixelFormat() const
>  	return V4L2PixelFormat();
>  }
>  
> +/**
> + * \brief Convert a V4L2PixelFormat into the corresponding BayerFormat
> + * \param[in] v4l2Format The raw format to convert into a BayerFormat
> + * \return The BayerFormat corresponding to the V4L2PixelFormat parameter
> + */
> +BayerFormat BayerFormat::fromV4L2PixelFormat(V4L2PixelFormat v4l2Format) const
> +{
> +	const auto it = v4l2ToBayer.find(v4l2Format);
> +	if (it != v4l2ToBayer.end())
> +	        return it->second;
> +
> +	return BayerFormat();
> +}

The same thing can be done through the constructor that takes a
V4L2PixelFormat argument. Could you explain why this function is needed
? Is this related to the discussion Jacopo and I had on the addition of
a constructor taking a media bus code ?

If we want to take the route of an explicit static function, I believe
we should drop the corresponding constructor as providing both would
have little value.

> +
>  /**
>   * \brief Apply a transform to this BayerFormat
>   * \param[in] t The transform to apply

-- 
Regards,

Laurent Pinchart