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

[Naushir Patuck]

Hi Jacopo,

You read my mind - I was going to respond to this next.

On Tue, 1 Oct 2024 at 08:48, Jacopo Mondi <jacopo.mondi at ideasonboard.com> 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>
>
> Do you know why RPi does instead
>
> bool PipelineHandlerBase::isRaw(const PixelFormat &pixFmt)
> {
>         /* This test works for both Bayer and raw mono formats. */
>         return BayerFormat::fromPixelFormat(pixFmt).isValid();
> }
>
> cc RPi which has upstreamed BayerFormat to know if there's a specfic reason
> why they do this instead of using PixelFormatInfo.

I assume RAW in this case means Bayer right?

I think looking at colourEncoding == ColourEncodingRAW might be incomplete as
mono formats (correctly) set colourEncoding to ColourEncodingYUV.

Regards,
Naush

>
> Seems like we have two maps of raw formants, one part of the canonical
> PixelFormatInfo and one in BayerFormat. They store more or less the
> same information, BayerFormat has a 'packing' field that
> PixelFormatInfo doesn't have, but I wonder why we have duplicated
> this instead of properly extending PixelFormatInfo
>
> PixelFormatInfo:
>
>         { formats::RGGB_PISP_COMP1, {
>                 .name = "RGGB_PISP_COMP1",
>                 .format = formats::RGGB_PISP_COMP1,
>                 .v4l2Formats = { V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB), },
>                 .bitsPerPixel = 8,
>                 .colourEncoding = PixelFormatInfo::ColourEncodingRAW,
>                 .packed = true,
>                 .pixelsPerGroup = 2,
>                 .planes = {{ { 2, 1 }, { 0, 0 }, { 0, 0 } }},
>         } },
>
> BayerFormat:
>         { { BayerFormat::RGGB, 16, BayerFormat::Packing::PISP1 },
>                 { formats::RGGB_PISP_COMP1, V4L2PixelFormat(V4L2_PIX_FMT_PISP_COMP1_RGGB) } },
>
> True, BayerFormat can be constructed from an mbusCode (something that
> has proven to be problematic in the past, as multiple codes map to the
> same BayerFormat), I wonder if that's the main reason.
>
> > ---
> >  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
> > + * \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
> > --
> > 2.45.2
> >