[libcamera-devel] [PATCH v3 1/3] libcamera: formats: PixelFormatInfo: Add name lookup function

[Kieran Bingham]

Hi Kaaira,

On 27/07/2020 16:18, Kaaira Gupta wrote:
> Add a function which returns a format, given its name as a string.
> 
> Signed-off-by: Kaaira Gupta <kgupta at es.iitr.ac.in>
> ---
>  include/libcamera/internal/formats.h |  1 +
>  src/libcamera/formats.cpp            | 20 ++++++++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
> index 0bb1510..2589e88 100644
> --- a/include/libcamera/internal/formats.h
> +++ b/include/libcamera/internal/formats.h
> @@ -38,6 +38,7 @@ public:
>  
>  	static const PixelFormatInfo &info(const PixelFormat &format);
>  	static const PixelFormatInfo &info(const V4L2PixelFormat &format);
> +	static const PixelFormat &info(const std::string &name);

Very close, but I think this is a layering violation.

Note the return type of the other two info() calls, I would expect this
function to have the same type.

>  	unsigned int stride(unsigned int width, unsigned int plane,
>  			    unsigned int align = 1) const;
> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> index 11774b0..8ea5461 100644
> --- a/src/libcamera/formats.cpp
> +++ b/src/libcamera/formats.cpp
> @@ -23,6 +23,8 @@ namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(Formats)
>  
> +const PixelFormat invalidPixelFormat = PixelFormat();
> +
>  /**
>   * \class PixelFormatPlaneInfo
>   * \brief Information about a single plane of a pixel format
> @@ -663,6 +665,24 @@ const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format)
>  	return info->second;
>  }
>  
> +/**
> + * \brief Retrieve pixel format from its name
> + * \param[in] name The name of pixel format
> + * \return The pixel format corresponding to \a name if known, or an invalid
> + * pixel format otherwise
> + */
> +const PixelFormat &PixelFormatInfo::info(const std::string &name)
> +{
> +	auto it = pixelFormatInfo.begin();
> +	while (it != pixelFormatInfo.end()) {

Can this be written as:

 	for (const PixelFormatInfo &info : pixelFormatInfo) {
	...
	}

There's probably not much in it, except for not needing to use manual
iterators then.


> +		if (it->second.name == name) {
> +			return it->first;
> +		}
> +		it++;
> +	}
> +	return invalidPixelFormat;

This function should return either the correct PixelFormatInfo, or the
invalidPixelFormatInfo.

then it's up to the caller to extract the PixelFormat from that const
reference.

If invalidPixelFormatInfo is returned, it will contain an
'invalidPixelFormat'.

--
Kieran



> +}
> +
>  /**
>   * \brief Compute the stride
>   * \param[in] width The width of the line, in pixels
> 

-- 
Regards
--
Kieran