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

[Laurent Pinchart]

Hi Jacopo,

On Tue, Dec 22, 2020 at 01:54:38PM +0100, Jacopo Mondi wrote:
> On Tue, Dec 22, 2020 at 01:35:11PM +0200, Laurent Pinchart wrote:
> > On Fri, Dec 18, 2020 at 05:47:48PM +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 constructor.
> > >
> > > Expand the number of supported compression and expand the documentation.
> > >
> > > Do not provide support for converting a BayerFormat to a media bus code
> > > as the feature is currently not required.
> >
> > And the mapping would be 1:1 anyway. That's the issue with media bus
> > codes, they don't have a 1:1 mapping to pixel formats.
> >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > >  include/libcamera/internal/bayer_format.h |  3 ++
> > >  src/libcamera/bayer_format.cpp            | 66 ++++++++++++++++++++++-
> > >  2 files changed, 68 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h
> > > index 4280b76b016f..7d7539e275ff 100644
> > > --- a/include/libcamera/internal/bayer_format.h
> > > +++ b/include/libcamera/internal/bayer_format.h
> > > @@ -30,6 +30,8 @@ public:
> > >  		None = 0,
> > >  		CSI2Packed = 1,
> > >  		IPU3Packed = 2,
> > > +		ALAW8Compression = 3,
> > > +		DPCM8Compression = 4,
> >
> > Compression and packing are not mutually exclusive, this would need to
> > become flags, or possibly get moved to a separate enum.
> >
> 
> I'll make these flags

Actually, you don't seem to use these at the moment. Do you plan to use
those flags, or should we leave them out for now ?

> > >  	};
> > >
> > >  	constexpr BayerFormat()
> > > @@ -43,6 +45,7 @@ public:
> > >  	}
> > >
> > >  	explicit BayerFormat(V4L2PixelFormat v4l2Format);
> > > +	explicit BayerFormat(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..a4eba4592bfe 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>
> > >
> > > @@ -45,7 +48,8 @@ namespace libcamera {
> > >
> > >  /**
> > >   * \enum BayerFormat::Packing
> > > - * \brief Different types of packing that can be applied to a BayerFormat
> > > + * \brief Different types of packing or compressions that can be applied to a
> > > + * BayerFormat
> > >   *
> > >   * \var BayerFormat::None
> > >   * \brief No packing
> > > @@ -53,6 +57,10 @@ namespace libcamera {
> > >   * \brief Format uses MIPI CSI-2 style packing
> > >   * \var BayerFormat::IPU3Packed
> > >   * \brief Format uses IPU3 style packing
> > > + * \var BayerFormat::ALAW8Compression
> > > + * \brief Format uses ALAW8 compression
> > > + * \var BayerFormat::DPCM8Compression
> > > + * \brief Format uses DPCM8 compression
> > >   */
> > >
> > >  namespace {
> > > @@ -140,6 +148,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::ALAW8Compression } },
> > > +	{ MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8, { BayerFormat::GBRG, 8, BayerFormat::ALAW8Compression } },
> > > +	{ MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8, { BayerFormat::GRBG, 8, BayerFormat::ALAW8Compression } },
> > > +	{ MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8, { BayerFormat::RGGB, 8, BayerFormat::ALAW8Compression } },
> > > +	{ MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8, { BayerFormat::BGGR, 8, BayerFormat::DPCM8Compression } },
> > > +	{ MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8, { BayerFormat::GBRG, 8, BayerFormat::DPCM8Compression } },
> > > +	{ MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8, { BayerFormat::GRBG, 8, BayerFormat::DPCM8Compression } },
> > > +	{ MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8, { BayerFormat::RGGB, 8, BayerFormat::DPCM8Compression } },
> > > +	{ 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 } },
> > > +};
> > > +
> > >  } /* namespace */
> > >
> > >  /**
> > > @@ -169,6 +212,22 @@ BayerFormat::BayerFormat(V4L2PixelFormat v4l2Format)
> > >  		*this = it->second;
> > >  }
> > >
> > > +/**
> > > + * \brief Construct a BayerFormat from a media bus code
> > > + * \param[in] mbusCode The media bus code to convert into a BayerFormat
> > > + *
> > > + * The media bus code numeric identifiers are defined by the V4L2 specification.
> > > + */
> > > +BayerFormat::BayerFormat(unsigned int mbusCode)
> > > +	: order(BGGR), packing(None)
> > > +{
> > > +	const auto it = mbusCodeToBayer.find(mbusCode);
> > > +	if (it == mbusCodeToBayer.end())
> > > +		bitDepth = 0;
> > > +	else
> > > +		*this = it->second;
> > > +}
> >
> > Instead of a constructor, wouldn't a static
> > BayerFormat::fromMediaBusCode() function be better ? The trouble with a
> > constructor is that, as it has to take an unsigned int argument (we
> > don't have a media bus code class), one could write
> >
> > 	BayerFormat format(V4L2_PIX_FMT_YUYV);
> >
> > and the compiler wouldn't complain. Worse, PixelFormat::operator
> > uint32_t() isn't explicit, so
> >
> > 	PixelFormat pf(...);
> > 	BayerFormat format(pf);
> >
> > will also compile.
> >
> > Obviously,
> >
> > 	PixelFormat pf(...);
> > 	BayerFormat format = BayerFormat::fromMediaBusCode(pf);
> >
> > will also compile, but it will be easier to spot from the code that
> > something is wrong.
> 
> I went with a constructor for simmetry with the existing
> 	explicit BayerFormat(V4L2PixelFormat v4l2Format);
> 
> I feel like the whole class should be better reworked to be similar to
> V4L2PixelFormat that provides a 'toPixelFormat' and one
> 'fromPixelFormat'.

Seems like a a good idea. We already have
BayerFormat::toV4L2PixelFormat(), it would make sense to add
BayerFormat::fromV4L2PixelFormat(). It's not a prerequisite for this
series though.

> In the meantime, if a static methods makes it harder to get the
> constuctor wrong, I'll use that.
> 
> > > +
> > >  /**
> > >   * \fn BayerFormat::isValid()
> > >   * \brief Return whether a BayerFormat is valid
> > > @@ -258,6 +317,11 @@ BayerFormat BayerFormat::transform(Transform t) const
> > >  /**
> > >   * \var BayerFormat::bitDepth
> > >   * \brief The bit depth of the samples in the Bayer pattern
> > > + *
> > > + * For formats using compression schemes such as Packing::ALAW8Compression or
> > > + * Packing::DPCM8Compression, the bitDepth value refers to the size of the
> > > + * sample when transmitted on the wire or stored into memory. The resulting
> > > + * images need to be de-compressed to be viewed properly.
> > >   */
> > >
> > >  /**

-- 
Regards,

Laurent Pinchart