[libcamera-devel,v7,4/8] libcamera: Add BayerFormat type

9494 diff mbox series

This type encodes BayerFormats in an explicit way, that makes them
easier to use than some of the other more opaque type formats. This
makes the BayerFormat useful for editing or manipulating Bayer types
more easily.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 include/libcamera/internal/bayer_format.h |  60 ++++++
 src/libcamera/bayer_format.cpp            | 231 ++++++++++++++++++++++
 src/libcamera/meson.build                 |   1 +
 3 files changed, 292 insertions(+)
 create mode 100644 include/libcamera/internal/bayer_format.h
 create mode 100644 src/libcamera/bayer_format.cpp

Hi David,

Thank you for the patch.

On Fri, Sep 04, 2020 at 11:30:38AM +0100, David Plowman wrote:
> This type encodes BayerFormats in an explicit way, that makes them
> easier to use than some of the other more opaque type formats. This
> makes the BayerFormat useful for editing or manipulating Bayer types
> more easily.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  include/libcamera/internal/bayer_format.h |  60 ++++++
>  src/libcamera/bayer_format.cpp            | 231 ++++++++++++++++++++++
>  src/libcamera/meson.build                 |   1 +
>  3 files changed, 292 insertions(+)
>  create mode 100644 include/libcamera/internal/bayer_format.h
>  create mode 100644 src/libcamera/bayer_format.cpp
> 
> diff --git a/include/libcamera/internal/bayer_format.h b/include/libcamera/internal/bayer_format.h
> new file mode 100644
> index 0000000..deeef5b
> --- /dev/null
> +++ b/include/libcamera/internal/bayer_format.h
> @@ -0,0 +1,60 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> + *
> + * bayer_format.h - Bayer Pixel Format
> + */
> +#ifndef __LIBCAMERA_INTERNAL_BAYER_FORMAT_H__
> +#define __LIBCAMERA_INTERNAL_BAYER_FORMAT_H__
> +
> +#include <stdint.h>
> +#include <string>
> +
> +#include "libcamera/internal/v4l2_pixelformat.h"
> +
> +namespace libcamera {
> +
> +enum class Transform;
> +
> +class BayerFormat
> +{
> +public:
> +	enum Order : uint8_t {
> +		BGGR = 0,
> +		GBRG = 1,
> +		GRBG = 2,
> +		RGGB = 3
> +	};
> +
> +	enum Modifier : uint16_t {
> +		None = 0,
> +		Packed = 1,

I'd name this CSI2Packed (or CSI2Packing), as other packing schemes
exist.

What other modifiers do we expect ? I'm wondering if we shouldn't rename
this to Packing, and turn the modifiers field into packing. Otherwise we
should define the modifiers as bitmasks (e.g. (1 << 0) for Packed), and
it may make it more difficult to handle the modifiers.

> +	};
> +
> +	constexpr BayerFormat()
> +		: order(Order::BGGR), bitDepth(0), modifiers(Modifier::None)
> +	{
> +	}
> +
> +	constexpr BayerFormat(Order o, uint8_t b, Modifier m)
> +		: order(o), bitDepth(b), modifiers(m)
> +	{
> +	}
> +
> +	explicit BayerFormat(V4L2PixelFormat v4l2Format);
> +	bool isValid() const { return bitDepth != 0; }
> +
> +	std::string toString() const;
> +
> +	V4L2PixelFormat toV4L2PixelFormat() const;
> +	BayerFormat transform(Transform t) const;
> +
> +	Order order;
> +	uint8_t bitDepth;
> +
> +	Modifier modifiers;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_BAYER_FORMAT_H__ */
> diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp
> new file mode 100644
> index 0000000..3780d7d
> --- /dev/null
> +++ b/src/libcamera/bayer_format.cpp
> @@ -0,0 +1,231 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Raspberry Pi (Trading) Limited
> + *
> + * bayer_format.cpp - class to represent Bayer formats

s/class/Class/ (or copy the description from the .h file)

> + */
> +
> +#include "libcamera/internal/bayer_format.h"
> +
> +#include <map>
> +
> +#include <libcamera/transform.h>
> +
> +/**
> + * \file bayer_format.h
> + * \brief Class to represent Bayer formats and manipulate them
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class BayerFormat
> + * \brief Class to represent a raw image Bayer format
> + *
> + * This class encodes the different Bayer formats in such a way that they can
> + * be easily manipulated. For example, the bit depth or Bayer order can be
> + * easily altered - the Bayer order can even be "transformed" in the same
> + * manner as happens in many sensors when their horizontal or vertical "flip"
> + * controls are set.
> + */
> +
> +/**
> + * \enum BayerFormat::Order
> + * \brief The order of the colour channels in the Bayer pattern
> + *
> + * \var BayerFormat::BGGR
> + * \brief B then G on the first row, G then R on the second row.
> + * \var BayerFormat::GBRG
> + * \brief G then B on the first row, R then G on the second row.
> + * \var BayerFormat::GRBG
> + * \brief G then R on the first row, B then G on the second row.
> + * \var BayerFormat::RGGB
> + * \brief T then G on the first row, G then B on the second row.

s/T/R/

> + */
> +
> +/**
> + * \enum BayerFormat::Modifier
> + * \brief Modifiers that can apply to a BayerFormat
> + *
> + * \var BayerFormat::None
> + * \brief No modifiers
> + * \var BayerFormat::Packed
> + * \brief Format uses MIPI CSI-2 style packing
> + */
> +
> +namespace {
> +
> +const std::map<V4L2PixelFormat, BayerFormat> v4l2ToBayer{
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8), { BayerFormat::BGGR, 8, BayerFormat::None } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SGBRG8), { BayerFormat::GBRG, 8, BayerFormat::None } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SGRBG8), { BayerFormat::GRBG, 8, BayerFormat::None } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SRGGB8), { BayerFormat::RGGB, 8, BayerFormat::None } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10), { BayerFormat::BGGR, 10, BayerFormat::None } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10), { BayerFormat::GBRG, 10, BayerFormat::None } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10), { BayerFormat::GRBG, 10, BayerFormat::None } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10), { BayerFormat::RGGB, 10, BayerFormat::None } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10P), { BayerFormat::BGGR, 10, BayerFormat::Packed } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10P), { BayerFormat::GBRG, 10, BayerFormat::Packed } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10P), { BayerFormat::GRBG, 10, BayerFormat::Packed } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10P), { BayerFormat::RGGB, 10, BayerFormat::Packed } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12), { BayerFormat::BGGR, 12, BayerFormat::None } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12), { BayerFormat::GBRG, 12, BayerFormat::None } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12), { BayerFormat::GRBG, 12, BayerFormat::None } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12), { BayerFormat::RGGB, 12, BayerFormat::None } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12P), { BayerFormat::BGGR, 12, BayerFormat::Packed } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12P), { BayerFormat::GBRG, 12, BayerFormat::Packed } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12P), { BayerFormat::GRBG, 12, BayerFormat::Packed } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12P), { BayerFormat::RGGB, 12, BayerFormat::Packed } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SBGGR16), { BayerFormat::BGGR, 16, BayerFormat::None } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SGBRG16), { BayerFormat::GBRG, 16, BayerFormat::None } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SGRBG16), { BayerFormat::GRBG, 16, BayerFormat::None } },
> +	{ V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16), { BayerFormat::RGGB, 16, BayerFormat::None } },
> +};
> +
> +/* Define a slightly arbitrary ordering so that we can use a std::map. */
> +struct BayerFormatComparator {
> +	bool operator()(const BayerFormat &left, const BayerFormat &right) const

The operands are commonly named lhs and rhs.

> +	{
> +		return left.bitDepth > right.bitDepth ||
> +		       (left.bitDepth == right.bitDepth && left.order > right.order) ||
> +		       (left.bitDepth == right.bitDepth && left.order == right.order &&
> +			left.modifiers > right.modifiers);

std::map used std::less as the default comparator. While your
implementation satisfies the Compare requirements, wouldn't it be good
to implement lhs < rhs instead of lhs > rhs ?

I was going to suggest that the following may be more readable

		if (lhs.bitDepth < rhs.bitDepth)
			return true;
		else if (lhs.bitDepth > rhs.bitDepth)
			return false;

		if (lhs.order < rhs.order)
			return true;
		else if (lhs.order > rhs.order)
			return false;

		if (lhs.modifiers < rhs.modifiers)
			return true;
		else
			return false;

but I'm actually not sure :-)

> +	}
> +};

Note that we could also inject a custom specialization of less<> in the
std namespace:

namespace std {

/* Define a slightly arbitrary ordering so that we can use a std::map. */
template<> struct less<libcamera::BayerFormat> {
	constexpr bool operator()(const libcamera::BayerFormat &lhs,
				  const libcamera::BayerFormat &rhs) const
	{
		return lhs.bitDepth > rhs.bitDepth ||
		       (lhs.bitDepth == rhs.bitDepth && lhs.order > rhs.order) ||
		       (lhs.bitDepth == rhs.bitDepth && lhs.order == rhs.order &&
			lhs.modifiers > rhs.modifiers);
	}
};

} /* namespace std */

This is allowed by C++ (see
https://en.cppreference.com/w/cpp/language/extending_std). I'll let you
pick the option you like best. The map would then be declared as
std::map<BayerFormat, V4L2PixelFormat>.

I was going to say that we could avoid the need for an ordering function
by switching to std::unordered_map, but then we'll need a custom hash
function which may not be better. For the record (and because I wanted
to learn how it would be done),

namespace std {

/* Define a slightly arbitrary ordering so that we can use a std::map. */
template<> struct hash<libcamera::BayerFormat> {
private:
	template<typename T>
	void combine(size_t &seed, const T &v) const
	{
		std::hash<T> hasher;
		seed ^= hasher(v) + 0x9e3779b9 + (seed << 6) + (seed >> 2);
	}

public:
	size_t operator()(const libcamera::BayerFormat &format) const
	{
		size_t seed = 0;
		combine(seed, format.bitDepth);
		combine(seed, format.order);
		combine(seed, format.modifiers);
		return seed;
	}
};

} /* namespace std */

(we would need a BayerFormat::operator==() too)

> +
> +const std::map<BayerFormat, V4L2PixelFormat, BayerFormatComparator> bayerToV4l2{
> +	{ { BayerFormat::BGGR, 8, BayerFormat::None }, V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8) },
> +	{ { BayerFormat::GBRG, 8, BayerFormat::None }, V4L2PixelFormat(V4L2_PIX_FMT_SGBRG8) },
> +	{ { BayerFormat::GRBG, 8, BayerFormat::None }, V4L2PixelFormat(V4L2_PIX_FMT_SGRBG8) },
> +	{ { BayerFormat::RGGB, 8, BayerFormat::None }, V4L2PixelFormat(V4L2_PIX_FMT_SRGGB8) },
> +	{ { BayerFormat::BGGR, 10, BayerFormat::None }, V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10) },
> +	{ { BayerFormat::GBRG, 10, BayerFormat::None }, V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10) },
> +	{ { BayerFormat::GRBG, 10, BayerFormat::None }, V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10) },
> +	{ { BayerFormat::RGGB, 10, BayerFormat::None }, V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10) },
> +	{ { BayerFormat::BGGR, 10, BayerFormat::Packed }, V4L2PixelFormat(V4L2_PIX_FMT_SBGGR10P) },
> +	{ { BayerFormat::GBRG, 10, BayerFormat::Packed }, V4L2PixelFormat(V4L2_PIX_FMT_SGBRG10P) },
> +	{ { BayerFormat::GRBG, 10, BayerFormat::Packed }, V4L2PixelFormat(V4L2_PIX_FMT_SGRBG10P) },
> +	{ { BayerFormat::RGGB, 10, BayerFormat::Packed }, V4L2PixelFormat(V4L2_PIX_FMT_SRGGB10P) },
> +	{ { BayerFormat::BGGR, 12, BayerFormat::None }, V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12) },
> +	{ { BayerFormat::GBRG, 12, BayerFormat::None }, V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12) },
> +	{ { BayerFormat::GRBG, 12, BayerFormat::None }, V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12) },
> +	{ { BayerFormat::RGGB, 12, BayerFormat::None }, V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12) },
> +	{ { BayerFormat::BGGR, 12, BayerFormat::Packed }, V4L2PixelFormat(V4L2_PIX_FMT_SBGGR12P) },
> +	{ { BayerFormat::GBRG, 12, BayerFormat::Packed }, V4L2PixelFormat(V4L2_PIX_FMT_SGBRG12P) },
> +	{ { BayerFormat::GRBG, 12, BayerFormat::Packed }, V4L2PixelFormat(V4L2_PIX_FMT_SGRBG12P) },
> +	{ { BayerFormat::RGGB, 12, BayerFormat::Packed }, V4L2PixelFormat(V4L2_PIX_FMT_SRGGB12P) },
> +	{ { BayerFormat::BGGR, 16, BayerFormat::None }, V4L2PixelFormat(V4L2_PIX_FMT_SBGGR16) },
> +	{ { BayerFormat::GBRG, 16, BayerFormat::None }, V4L2PixelFormat(V4L2_PIX_FMT_SGBRG16) },
> +	{ { BayerFormat::GRBG, 16, BayerFormat::None }, V4L2PixelFormat(V4L2_PIX_FMT_SGRBG16) },
> +	{ { BayerFormat::RGGB, 16, BayerFormat::None }, V4L2PixelFormat(V4L2_PIX_FMT_SRGGB16) },

Could you already add the IPU3 packed formats to these tables ? They can
be found in formats.cpp.

> +};
> +
> +} // namespace
> +
> +/**
> + * \fn BayerFormat::BayerFormat()
> + * \brief Construct an empty (and invalid) BayerFormat
> + */
> +
> +/**
> + * \fn BayerFormat::BayerFormat(Order o, uint8_t b, Modifier m)
> + * \brief Construct a BayerFormat from explicit values

Missing \param[in] for the three arguments.

> + */
> +
> +/**
> + * \brief Construct a BayerFormat from a V4L2PixelFormat
> + * \param[in] v4l2Format The raw format to convert into a BayerFormat
> + */
> +BayerFormat::BayerFormat(V4L2PixelFormat v4l2Format)
> +{
> +	const auto it = v4l2ToBayer.find(v4l2Format);
> +	if (it == v4l2ToBayer.end())
> +		bitDepth = 0;

This will leave order and modifiers uninitialized. This should be an
issue in most cases as the caller should call isValid() first, but it
could become a problem if the resulting BayerFormat was compared with
BayerFormatComparator (or hashed) as it would generate uninitialized
read warnings from valgrind. I think setting the other members would be
best.

> +	else
> +		*this = it->second;
> +}
> +
> +/**
> + * \fn BayerFormat::isValid()
> + * \brief Return whether a BayerFormat is valid
> + */
> +
> +/**
> + * \brief Returns a readable string representation of the BayerFormat

For consistency with the rest of the code base,
s/Returns a readable string/Assemble and return a string/

and

 * \return A string describing the BayerFormat

> + */
> +std::string BayerFormat::toString() const
> +{
> +	std::string result;
> +
> +	static const char *orderStrings[] = {
> +		"BGGR",
> +		"GBRG",
> +		"GRBG",
> +		"RGGB"
> +	};
> +	if (order <= RGGB)
> +		result = orderStrings[order];
> +	else
> +		result = "unknown";

As this should never happen, how about just

	return "INVALID";

or a similar string ? We should also return the same if !isValid().

> +
> +	result += "-" + std::to_string(bitDepth);
> +
> +	if (modifiers & Packed)
> +		result += "-P";
> +
> +	return result;
> +}
> +
> +/**
> + * \brief Convert a BayerFormat into the corresponding V4L2PixelFormat

Missing \return

> + */
> +V4L2PixelFormat BayerFormat::toV4L2PixelFormat() const
> +{
> +	const auto it = bayerToV4l2.find(*this);
> +	if (it != bayerToV4l2.end())
> +		return it->second;
> +
> +	return V4L2PixelFormat();
> +}
> +
> +/**
> + * \brief Apply a transform to this BayerFormat.

s/.$//

> + * \param[in] t The transform to apply
> + *
> + * For example, performing a horizontal flip on the Bayer pattern RGGB causes
> + * the RG rows of pixels to become GR, and the GB rows become BG. So the final
> + * result in this case would be GRBG.

Let's make clear that this only affects the Bayer order.

 * Appplying a transform to an image stored in a Bayer format affects the Bayer
 * order. For example, performing a horizontal flip on the Bayer pattern
 * RGGB causes the RG rows of pixels to become GR, and the GB rows to become BG.
 * The transformed image would have a GRBG order. The bit depth and modifiers
 * are not affected.
 *
 * \return The transformed Bayer format

> + */
> +BayerFormat BayerFormat::transform(Transform t) const
> +{
> +	BayerFormat result = *this;
> +
> +	/*
> +	 * Observe that flipping bit 0 of the Order enum performs a horizontal
> +	 * mirror on the Bayer pattern (e.g. RGGB goes to GRBG). Similarly,
> +	 * flipping bit 1 performs a vertical mirror operation on it. Hence:
> +	 */
> +	if (!!(t & Transform::HFlip))
> +		result.order = static_cast<Order>(result.order ^ 1);
> +	if (!!(t & Transform::VFlip))
> +		result.order = static_cast<Order>(result.order ^ 2);

Shouldn't we also support Transform::Transpose to be complete ?

> +
> +	return result;
> +}
> +
> +/**
> + * \var BayerFormat::order
> + * \brief The order of the colour channels in the Bayer pattern
> + */
> +
> +/**
> + * \var BayerFormat::bitDepth
> + * \brief The bit depth of the samples in the Bayer pattern
> + */
> +
> +/**
> + * \var BayerFormat::modifiers
> + * \brief Any modifier flags applied to this BayerFormat
> + */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index edec55e..86c225f 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
>  libcamera_sources = files([
> +    'bayer_format.cpp',
>      'bound_method.cpp',
>      'buffer.cpp',
>      'byte_stream_buffer.cpp',