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

[Jacopo Mondi]

Hi Naush

On Tue, Oct 01, 2024 at 08:53:09AM GMT, Naushir Patuck wrote:
> 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.

Ok, thanks for clarifying it.

We should then replace all current usages of  colourEncoding ==
ColourEncodingRAW and leave BayerFormat be. That's what Umang did
already and there won't be regressions introduced this way.
>
> 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
> > >