[PATCH 1/2] libcamera: pixel_format: Add isRaw() helper

[Umang Jain]

Hi Laurent,

On 03/10/24 8:57 pm, Laurent Pinchart wrote:
> On Tue, Oct 01, 2024 at 08:41:51PM +0200, Jacopo Mondi wrote:
>> Hi Umang
>>
>> On Mon, Sep 30, 2024 at 08:50:38PM GMT, Umang Jain wrote:
>>> Add a isRaw() helper to the PixelFormat class, to know whether the
>>> pixel format has RAW encoding.
>>>
>>> This will used by validation and configuration code paths in pipeline
>>> handlers, to know whether a pixel format is a raw format or not.
>>>
>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>>> ---
>>>   include/libcamera/pixel_format.h |  1 +
>>>   src/libcamera/pixel_format.cpp   | 11 +++++++++++
>>>   2 files changed, 12 insertions(+)
>>>
>>> diff --git a/include/libcamera/pixel_format.h b/include/libcamera/pixel_format.h
>>> index 1b4d8c7c..aed53ea6 100644
>>> --- a/include/libcamera/pixel_format.h
>>> +++ b/include/libcamera/pixel_format.h
>>> @@ -37,6 +37,7 @@ public:
>>>   	constexpr uint64_t modifier() const { return modifier_; }
>>>
>>>   	std::string toString() const;
>>> +	bool isRaw() const;
>>>
>>>   	static PixelFormat fromString(const std::string &name);
>>>
>>> diff --git a/src/libcamera/pixel_format.cpp b/src/libcamera/pixel_format.cpp
>>> index 314179a8..436ef5fb 100644
>>> --- a/src/libcamera/pixel_format.cpp
>>> +++ b/src/libcamera/pixel_format.cpp
>>> @@ -100,6 +100,17 @@ bool PixelFormat::operator<(const PixelFormat &other) const
>>>    * \return DRM modifier
>>>    */
>>>
>>> +/**
>>> + * \brief Checks if \a this is a RAW pixel format
>> Well, I'm not sure I want to go there, as talking about colour spaces
>> and colour encoding for RAW formats is a bit of a minefield. Indeed we
>> use the colour encoding of a format to identify it as RAW, so the
>> \brief here is somewhat correct.
> I'm also concerned, sorry :-(
>
> "RAW" is ill-defined here. It's actually not defined at all in this
> patch :-) The PixelFormat is a public class, so its API needs to be
> clearly defined, we can't just implement PixelFormat::isRaw() as "what
> libcamera does now internally in most place". And once you try to define
> the concept, you'll likely realize that it's not an easy exercize. A
> good example is the R8 format, which is used for 8-bit greyscale images.
> An image in that format can be a raw frame captured directly from the
> snesor, or a processed frame at the output of the ISP.

Thanks for the comment, it indeed is a difficult exercise to define 'raw'.

I will drop the series.
>> However I would mention briefly that we check if a pixel format is RAW
>> by checking its color encoding.
>>
>>
>> /**
>>   * \brief Check if \this is RAW pixel format
>>   *
>>   * Check if a pixel format is RAW by validating that its colour
>>   * encoding is ColourEncodingRAW.
>>   *
>>   * \return True if \a this has RAW colour encoding, false otherwise
>>   */
>>
>> With this, if you think it's appropriate:
>> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>>
>>> + * \return True if \a this is a RAW pixel format, false otherwise
>>> + */
>>> +bool PixelFormat::isRaw() const
>>> +{
>>> +	const PixelFormatInfo &info = PixelFormatInfo::info(*this);
>>> +
>>> +	return info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
>>> +}
>>> +
>>>   /**
>>>    * \brief Assemble and return a string describing the pixel format
>>>    * \return A string describing the pixel format