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

[Jacopo Mondi]

Hi Laurent,

On Tue, Dec 22, 2020 at 01:35:11PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> 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

> >  	};
> >
> >  	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'.

In the meantime, if a static methods makes it harder to get the
constuctor wrong, I'll use that.

Thanks
   j


> > +
> >  /**
> >   * \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