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

[David Plowman]

Hi Jacopo

Thanks for the review!

On Fri, 4 Nov 2022 at 11:34, Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi David,
>
> On Thu, Nov 03, 2022 at 10:40:26AM +0000, David Plowman via libcamera-devel wrote:
> > 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>
> > ---
> >  include/libcamera/internal/bayer_format.h |  1 +
> >  src/libcamera/bayer_format.cpp            | 12 ++++++++++++
> >  2 files changed, 13 insertions(+)
> >
> > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h
> > index 78ba3969..09f756c4 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(bool &valid) 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 f27cc166..a03c58c0 100644
> > --- a/src/libcamera/bayer_format.cpp
> > +++ b/src/libcamera/bayer_format.cpp
> > @@ -226,6 +226,18 @@ const BayerFormat &BayerFormat::fromMbusCode(unsigned int mbusCode)
> >               return it->second;
> >  }
> >
> > +/**
> > + * \brief Retrieve the media bus code corresponding this this BayerFormat
> > + * \param[out] valid Set to true if a matching media bus code was found, else false
> > + */
> > +unsigned int BayerFormat::toMbusCode(bool &valid) const
>
> Do we need valid ?
> Valid media bus code are > 0, can we return 0 in the case the
> BayerForamt is not supported ?

Yes I wasn't 100% sure. I think I read that 0 was "reserved" and so it
wasn't totally clear to me whether that guaranteed it was "invalid" or
not. But I guess it does, so that would simplify this a bit!

>
> > +{
> > +     auto it = std::find_if(mbusCodeToBayer.begin(), mbusCodeToBayer.end(),
> > +                            [this](const auto &i) { return i.second == *this; });
>
> To avoid iterating on the full map could store the media bus code as a BayerFormat
> class member at construction ? It will require a large update of the
> two maps though..
>
> const std::unordered_map<unsigned int, BayerFormat> mbusCodeToBayer{
>         { MEDIA_BUS_FMT_SBGGR8_1X8,
>                 { MEDIA_BUS_FMT_SBGGR8_1X8, BayerFormat::BGGR, 8, BayerFormat::Packing::None } },
>         ...
> }
> const std::map<BayerFormat, Formats, BayerFormatComparator> bayerToFormat{
>         { { MEDIA_BUS_FMT_SBGGR8_1X8, BayerFormat::BGGR, 8, BayerFormat::Packing::None },
>                 { formats::SBGGR8, V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8) } },
>         ...
> }
>
> Or maybe we can expand
>
> struct Formats {
>         PixelFormat pixelFormat;
>         V4L2PixelFormat v4l2Format;
> };
>
> to
>
> struct Formats {
>         PixelFormat pixelFormat;
>         V4L2PixelFormat v4l2Format;
>         uint32_t mbusCode;
> };
>
> which would require to expand
>
> const std::map<BayerFormat, Formats, BayerFormatComparator> bayerToFormat{
>         { { BayerFormat::BGGR, 8, BayerFormat::Packing::None },
>                 { formats::SBGGR8, V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8), MEDIA_BUS_FMT_SBGGR8_1X8 } },
>         ...
> }
>
> so that we can lookup in bayerToFormat map.
>
> Not a big deal, we already have a similar construct in other functions
> in the class...

I um-ed and er-ed here a bit too. I went with the "hunt through the
existing table" because there seemed to be a precedent for it. I'm
slightly inclined to leave it as it is just because it changes the
minimum amount of stuff, but don't really feel strongly about it...!

Thanks for the info above, and I'll post a v2 shortly.

David

>
> > +     valid = it != mbusCodeToBayer.end();
> > +     return valid ? it->first : 0;
> > +}
> > +
> >  /**
> >   * \fn BayerFormat::isValid()
> >   * \brief Return whether a BayerFormat is valid
> > --
> > 2.30.2
> >