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

[Laurent Pinchart]

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.

> 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

-- 
Regards,

Laurent Pinchart