[libcamera-devel] [PATCH 3/6] libcamera: bayer_format: Add support for mbus codes

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Dec 28, 2020 at 10:42:57AM +0100, Jacopo Mondi wrote:
> On Sun, Dec 27, 2020 at 12:09:48PM +0100, Niklas Söderlund wrote:
> > On 2020-12-23 18:47:06 +0100, Jacopo Mondi wrote:
> > > The existing implementation of the BayerFormat class supports
> > > converting a V4L2PixelFormat to a BayerFormat and vice-versa.
> > >
> > > Expand the class by adding support for converting a media bus code
> > > to a BayerFormat instance, by providing a conversion table and a
> > > dedicated static methods.
> > >
> > > Do not provide support for converting a BayerFormat to a media bus code
> > > as the feature is currently not required.

And it's also no possible, there's no 1:1 mapping between the two. I'd
reword this last sentence to explain this.

> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > >  include/libcamera/internal/bayer_format.h |  1 +
> > >  src/libcamera/bayer_format.cpp            | 55 +++++++++++++++++++++++
> > >  2 files changed, 56 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h
> > > index 4280b76b016f..dc86f6ee3aca 100644
> > > --- a/include/libcamera/internal/bayer_format.h
> > > +++ b/include/libcamera/internal/bayer_format.h
> > > @@ -43,6 +43,7 @@ public:
> > >  	}
> > >
> > >  	explicit BayerFormat(V4L2PixelFormat v4l2Format);
> > > +	static const BayerFormat &fromMbusCode(unsigned int mbusCode);
> > >  	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 c42792ff1948..c2c91ce8b6da 100644
> > > --- a/src/libcamera/bayer_format.cpp
> > > +++ b/src/libcamera/bayer_format.cpp
> > > @@ -8,6 +8,9 @@
> > >  #include "libcamera/internal/bayer_format.h"
> > >
> > >  #include <map>
> > > +#include <unordered_map>
> > > +
> > > +#include <linux/media-bus-format.h>
> > >
> > >  #include <libcamera/transform.h>
> > >
> > > @@ -140,6 +143,41 @@ const std::map<BayerFormat, V4L2PixelFormat, BayerFormatComparator> bayerToV4l2{
> > >  	{ { BayerFormat::RGGB, 16, BayerFormat::None }, V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16) },
> > >  };
> > >
> > > +const std::unordered_map<unsigned int, BayerFormat> mbusCodeToBayer{
> > > +	{ MEDIA_BUS_FMT_SBGGR8_1X8, { BayerFormat::BGGR, 8, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SGBRG8_1X8, { BayerFormat::GBRG, 8, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SGRBG8_1X8, { BayerFormat::GRBG, 8, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SRGGB8_1X8, { BayerFormat::RGGB, 8, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8, { BayerFormat::BGGR, 8, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8, { BayerFormat::GBRG, 8, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8, { BayerFormat::GRBG, 8, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8, { BayerFormat::RGGB, 8, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8, { BayerFormat::BGGR, 8, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8, { BayerFormat::GBRG, 8, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8, { BayerFormat::GRBG, 8, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8, { BayerFormat::RGGB, 8, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE, { BayerFormat::BGGR, 10, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, { BayerFormat::BGGR, 10, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE, { BayerFormat::BGGR, 10, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE, { BayerFormat::BGGR, 10, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SBGGR10_1X10, { BayerFormat::BGGR, 10, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SGBRG10_1X10, { BayerFormat::GBRG, 10, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SGRBG10_1X10, { BayerFormat::GRBG, 10, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SRGGB10_1X10, { BayerFormat::RGGB, 10, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SBGGR12_1X12, { BayerFormat::BGGR, 12, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SGBRG12_1X12, { BayerFormat::GBRG, 12, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SGRBG12_1X12, { BayerFormat::GRBG, 12, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SRGGB12_1X12, { BayerFormat::RGGB, 12, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SBGGR14_1X14, { BayerFormat::BGGR, 14, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SGBRG14_1X14, { BayerFormat::GBRG, 14, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SGRBG14_1X14, { BayerFormat::GRBG, 14, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SRGGB14_1X14, { BayerFormat::RGGB, 14, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SBGGR16_1X16, { BayerFormat::BGGR, 16, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SGBRG16_1X16, { BayerFormat::GBRG, 16, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SGRBG16_1X16, { BayerFormat::GRBG, 16, BayerFormat::None } },
> > > +	{ MEDIA_BUS_FMT_SRGGB16_1X16, { BayerFormat::RGGB, 16, BayerFormat::None } },
> > > +};
> >
> > nit: Would it make sens to use BayerFormat::Packing::None to ease
> > reading of the table?
> 
> Are you suggesting
>         s/BayerFormat::None/BayerFormat::Packing::None/
> right ?
> 
> We could, but the existing tables use BayerFormat::None, I didn't want
> to make this one different!

Turning Packing into an enum class would indeed make sense, on top of
this.

> > In either case,
> >
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >
> > > +
> > >  } /* namespace */
> > >
> > >  /**
> > > @@ -169,6 +207,23 @@ BayerFormat::BayerFormat(V4L2PixelFormat v4l2Format)
> > >  		*this = it->second;
> > >  }
> > >
> > > +/**
> > > + * \brief Retrieve the BayerFormat associated to a media bus code

s/associated to/associated with/

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > > + * \param[in] mbusCode The media bus code to convert into a BayerFormat
> > > + *
> > > + * The media bus code numeric identifiers are defined by the V4L2 specification.
> > > + */
> > > +const BayerFormat &BayerFormat::fromMbusCode(unsigned int mbusCode)
> > > +{
> > > +	static BayerFormat empty;
> > > +
> > > +	const auto it = mbusCodeToBayer.find(mbusCode);
> > > +	if (it == mbusCodeToBayer.end())
> > > +		return empty;
> > > +	else
> > > +		return it->second;
> > > +}
> > > +
> > >  /**
> > >   * \fn BayerFormat::isValid()
> > >   * \brief Return whether a BayerFormat is valid

-- 
Regards,

Laurent Pinchart