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

[David Plowman]

Hi again

I was thinking about this a bit more. One thing I wanted to clarify
was what media bus formats really mean. For example, if I have
MEDIA_BUS_FMT_SBGGR10_1X10, presumably that means Bayer BGGR with CSI2
packing. Does that sound right?

Under those circumstances it seems reasonable to me that the
mbusCodeToBayer table (bayer_format.cpp) should specify Packing::CSI2P
for most of the formats there, but with new values (e.g.
Packing::PADHI_BE) for the ones now under discussion.

I don't think such a change would bother Raspberry Pi (it might well
"just work", or if not require only very minor tweaks), but someone
else would have to comment on ipu3 or rkisp1.

Dave also mentions that all the non-BGGR versions are missing (it
seems a bit disappointing that they weren't added all at the same
time), but for the moment I suggest we ignore that.

I also note there's a potential problem with the "ALAW8" and "DPCM8"
formats. I don't know if folks think those can be handled by more
"packing" formats, or whether we'd rather have a new "modifiers"
field?

Thoughts?

David

On Fri, 6 Jan 2023 at 17:28, Dave Stevenson
<dave.stevenson at raspberrypi.com> wrote:
>
> 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
> > > >