[libcamera-devel] [PATCH 1/9] libcamera: bayer_format: Add toMbusCode method

[David Plowman]

Hi Jacopo

On Wed, 4 Jan 2023 at 13:12, Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi David
>    sorry to revive this, but I'm carrying this series in my tree and I
> just found a bug in this patches
>
>
> On Thu, Nov 24, 2022 at 01:12:12PM +0100, Jacopo Mondi wrote:
> > From: David Plowman <david.plowman at raspberrypi.com>
> >
> > This makes it easier to perform transformations on Bayer type mbus
> > codes by converting them to a BayerFormat, doing the transform, and
> > then converting them back again.
> >
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  include/libcamera/internal/bayer_format.h |  1 +
> >  src/libcamera/bayer_format.cpp            | 11 +++++++++++
> >  2 files changed, 12 insertions(+)
> >
> > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h
> > index 78ba3969913d..3601dccb7228 100644
> > --- a/include/libcamera/internal/bayer_format.h
> > +++ b/include/libcamera/internal/bayer_format.h
> > @@ -47,6 +47,7 @@ public:
> >       }
> >
> >       static const BayerFormat &fromMbusCode(unsigned int mbusCode);
> > +     unsigned int toMbusCode() const;
> >       bool isValid() const { return bitDepth != 0; }
> >
> >       std::string toString() const;
> > diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp
> > index f27cc1662d25..fdbc4af1dcc0 100644
> > --- a/src/libcamera/bayer_format.cpp
> > +++ b/src/libcamera/bayer_format.cpp
> > @@ -226,6 +226,17 @@ const BayerFormat &BayerFormat::fromMbusCode(unsigned int mbusCode)
> >               return it->second;
> >  }
> >
> > +/**
> > + * \brief Retrieve the media bus code corresponding this this BayerFormat
> > + * \return The corresponding media bus code, or zero if none was found
> > + */
> > +unsigned int BayerFormat::toMbusCode() const
> > +{
> > +     auto it = std::find_if(mbusCodeToBayer.begin(), mbusCodeToBayer.end(),
> > +                            [this](const auto &i) { return i.second == *this; });
> > +     return it != mbusCodeToBayer.end() ? it->first : 0;
>
> I'm working with a sensor that produces MEDIA_BUS_FMT_SBGGR10_1X10
> It doesn't register flips, so the bayer pattern should not be changed
>
> The code you added in the next patch for the CameraSensor class goes
> as
>
>         for (const auto &format : formats) {
>                 unsigned int mbusCode = format.first;
>                 BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);
>
>                 if (bayerFormat.isValid())
>                         mbusCode = bayerFormat.transform(transform).toMbusCode();
>
>                 if (mbusCode)
>                         formats_[mbusCode] = std::move(format.second);
>         }
>
>
> The first
>
>                 BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);
>
> returns  { BayerFormat::BGGR, 10, BayerFormat::Packing::None }
>
> we go throgh a transform() which does nothing as it receives Indentity
> and then call toMbusCode() on it
>
> The above implementation of toMbusCode() walks the mbusCodeToBayer map
> looking for a matching bayer. Look at this table section
>
>         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
>         { MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
>         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
>         { MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
>         { MEDIA_BUS_FMT_SBGGR10_1X10, { BayerFormat::BGGR, 10, BayerFormat::Packing::None } },
>
> So here we have that all the MEDIA_BUS_FMT_SBGGR10_2X8_PAD.. and
> MEDIA_BUS_FMT_SBGGR10_1X10 codes match on the same
> { BayerFormat::BGGR, 10, BayerFormat::Packing::None } and I suddenly
> get my original 0x3007 code transformed to 0x3003!
>
> I'm afraid the fact mbus_code-to-BayerFormat relationship is not
> unique is a blocker for this patch

Oh dear! Thanks for discovering this, I had no idea that these formats existed.

I wonder if the correct solution is to have 4 extra packing formats,
alongside the existing CSI2 and IPU3 ones. What do you think?

David

>
>
>
>
>
> > +}
> > +
> >  /**
> >   * \fn BayerFormat::isValid()
> >   * \brief Return whether a BayerFormat is valid
> > --
> > 2.38.1
> >