[libcamera-devel] [PATCH v2 3/6] libcamera: formats: PixelFormatInfo: Add v4l2 lookup function

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Tue, Jun 30, 2020 at 11:58:05PM +0900, Paul Elder wrote:
> Add a lookup function for PixelFormatInfo that takes a V4L2PixelFormat.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> ---
> Changes in v2:
> - move invalid PixelFormatInfo instance to anonymous namespace
> - add documentation
> ---
>  include/libcamera/internal/formats.h |  1 +
>  src/libcamera/formats.cpp            | 23 +++++++++++++++++++++--
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
> index dc19492..3c7440a 100644
> --- a/include/libcamera/internal/formats.h
> +++ b/include/libcamera/internal/formats.h
> @@ -44,6 +44,7 @@ public:
>  	bool isValid() const { return format.isValid(); }
>  
>  	static const PixelFormatInfo &info(const PixelFormat &format);
> +	static const PixelFormatInfo &info(const V4L2PixelFormat &format);
>  
>  	const char *name;
>  	PixelFormat format;
> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> index 8076c39..4d18825 100644
> --- a/src/libcamera/formats.cpp
> +++ b/src/libcamera/formats.cpp
> @@ -7,6 +7,7 @@
>  
>  #include "libcamera/internal/formats.h"
>  
> +#include <algorithm>
>  #include <errno.h>
>  
>  #include <libcamera/formats.h>
> @@ -190,6 +191,8 @@ const std::map<unsigned int, std::vector<SizeRange>> &ImageFormats::data() const
>  
>  namespace {
>  
> +const PixelFormatInfo invalid{};

This is a possibly candidate for a later namespace clash. I would either
call it pixelFormatInfoInvalid, or add it as an entry in the
pixelFormatInfo map.

> +
>  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>  	/* RGB formats. */
>  	{ formats::BGR888, {
> @@ -661,8 +664,6 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>   */
>  const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format)
>  {
> -	static const PixelFormatInfo invalid{};
> -
>  	const auto iter = pixelFormatInfo.find(format);
>  	if (iter == pixelFormatInfo.end()) {
>  		LOG(Formats, Warning)
> @@ -674,4 +675,22 @@ const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format)
>  	return iter->second;
>  }
>  
> +/**
> + * \brief Retrieve information about a pixel format
> + * \param[in] format The V4L2 pixel format
> + * \return The PixelFormatInfo describing the V4L2 \a format if known, or an
> + * invalid PixelFormatInfo otherwise
> + */
> +const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format)
> +{
> +	auto info = std::find_if(pixelFormatInfo.begin(), pixelFormatInfo.end(),

	const auto &info = ...

to ensure that it will be a const reference and not a copy.

With these two changes,

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

> +				 [format](auto pair) {
> +					 return pair.second.v4l2Format == format;
> +				 });
> +	if (info == pixelFormatInfo.end())
> +		return invalid;
> +
> +	return info->second;
> +}
> +
>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart