[libcamera-devel] [PATCH v7 4/8] libcamera: Add BayerFormat type
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Sep 6 02:41:10 CEST 2020
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 at raspberrypi.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at 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',
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list