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

[Kieran Bingham]

Hi Laurent,

On 27/07/2020 16:38, Laurent Pinchart wrote:
> On Mon, Jul 27, 2020 at 06:33:41PM +0300, Laurent Pinchart wrote:
>> Hi Kieran,
>>
>> On Mon, Jul 27, 2020 at 04:30:01PM +0100, Kieran Bingham wrote:
>>> 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.
>>
>> I would expect this function to be part of the PixelFormat class
>>
>> 	static PixelFormat fromString(const std::string &name);
>>
>> and be called with
>>
>> 	PixelFormat format = PixelFormat::fromString("YUYV");
> 
> I see that's what patch 2/2 implements :-) I wonder if we need a
> name-based lookup in the PixelFormatInfo class, it should be enough to
> implement it in PixelFormat only. However, as the PixelFormat class
> doesn't have access to the pixelFormatInfo map, that's not something we
> could implement without some refactoring. I'm thus fine with a lookup
> function here.

Indeed, that's why I think we should have a PixelFormatInfo
  static const PixelFormatInfo &info(const std::string &name);


and a separate
 static PixelFormat fromString(const std::string &name);

as is implemented in 2/3 indeed.

--
Kieran


>>>>  	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.
>>
>> And it looks nicer (to me at least :-)).
>>
>>>> +		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'.
>>>
>>>> +}
>>>> +
>>>>  /**
>>>>   * \brief Compute the stride
>>>>   * \param[in] width The width of the line, in pixels
> 

-- 
Regards
--
Kieran