[libcamera-devel] [PATCH v2 1/6] libcamera: bayer_format: Add a toPixelFormat() helpers to the BayerFormat class

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Fri, Oct 22, 2021 at 03:39:02PM +0100, Naushir Patuck wrote:
> Add a BayerFormat::toPixelFormat() member function to convert a BayerFormat to a
> PixelFormat type. This conversion uses the existing bayerToV4l2 conversion
> table.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  include/libcamera/internal/bayer_format.h |  3 +++
>  src/libcamera/bayer_format.cpp            | 13 +++++++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h
> index 723382d4168d..247a60cf0e36 100644
> --- a/include/libcamera/internal/bayer_format.h
> +++ b/include/libcamera/internal/bayer_format.h
> @@ -10,6 +10,8 @@
>  #include <stdint.h>
>  #include <string>
>  
> +#include <libcamera/pixel_format.h>
> +
>  #include "libcamera/internal/v4l2_pixelformat.h"
>  
>  namespace libcamera {
> @@ -50,6 +52,7 @@ public:
>  
>  	V4L2PixelFormat toV4L2PixelFormat() const;
>  	static BayerFormat fromV4L2PixelFormat(V4L2PixelFormat v4l2Format);
> +	PixelFormat toPixelFormat() const;
>  	BayerFormat transform(Transform t) const;
>  
>  	Order order;
> diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp
> index 11355f144f66..c662ba36604c 100644
> --- a/src/libcamera/bayer_format.cpp
> +++ b/src/libcamera/bayer_format.cpp
> @@ -269,6 +269,19 @@ BayerFormat BayerFormat::fromV4L2PixelFormat(V4L2PixelFormat v4l2Format)
>  	return BayerFormat();
>  }
>  
> +/**
> + * \brief Convert a BayerFormat into the corresponding PixelFormat
> + * \return The PixelFormat corresponding to this BayerFormat
> + */
> +PixelFormat BayerFormat::toPixelFormat() const
> +{
> +	const auto it = bayerToV4l2.find(*this);
> +	if (it != bayerToV4l2.end())
> +		return PixelFormat(it->second);

Should we store the PixelFormat in the bayerToV4l2 map to avoid a double
looking (one here, one in the PixelFormat constructor) ? The map should
be renamed to bayerToFormat, and be stored as either

std::map<BayerFormat, std::pair<PixelFormat, V4L2PixelFormat>, BayerFormatComparator>

or

struct Formats {
	PixelFormat pixelFormat;
	V4L2PixelFormat v4l2Format;
};

std::map<BayerFormat, Formats, BayerFormatComparator>

It could of course be argued that we'll then duplicate data, and that it
could get out of sync.

I'm concerned about how the BayerFormat class is used to convert a media
bus code to a pixel format or V4L2 pixel format, but I'll comment on
that where applicable in the rest of the series.

> +
> +	return PixelFormat();
> +}
> +
>  /**
>   * \brief Apply a transform to this BayerFormat
>   * \param[in] t The transform to apply

-- 
Regards,

Laurent Pinchart