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

[Dave Stevenson]

Hi David and Jacopo

On Fri, 6 Jan 2023 at 16:21, David Plowman via libcamera-devel
<libcamera-devel at lists.libcamera.org> wrote:
>
> 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.

Why do those formats exist, and why only the BGGR ordering versions?
A partial implementation of something, and presumably on a parallel
interface as formats such as YUYV are mandated to use the 1X16
versions (not 2X8) on CSI.

Are they actually relevant, or legacy cruft?

  Dave

> 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
> > >