[libcamera-devel] [PATCH v2 1/3] libcamera: Add ColorSpace class
David Plowman
david.plowman at raspberrypi.com
Mon Oct 11 17:05:13 CEST 2021
Hi again Laurent
On Mon, 11 Oct 2021 at 15:31, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David,
>
> On Mon, Oct 11, 2021 at 03:16:35PM +0100, David Plowman wrote:
> > Hi Laurent
> >
> > Thanks for the detailed review of all this. I probably won't comment
> > on some of the more "trivial" items (I'll just do a v3!) but some of
> > the points do indeed want some more discussion.
> >
> > On Tue, 5 Oct 2021 at 22:03, Laurent Pinchart wrote:
> > >
> > > Hi David,
> > >
> > > (CC'ing Hans, the colour space expert in V4L2)
> > >
> > > Thank you for the patch.
> > >
> > > Hans, if you have a bit of time to look at this, your feedback would be
> > > appreciated.
> > >
> > > On Mon, Sep 27, 2021 at 01:33:25PM +0100, David Plowman wrote:
> > > > This class represents a colour space by defining its YCbCr encoding,
> > > > the transfer (gamma) function is uses, and whether the output is full
> > >
> > > s/is/it/
> > >
> > > > or limited range.
> > > >
> > > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > > ---
> > > > include/libcamera/color_space.h | 83 +++++++++++++
> > > > include/libcamera/meson.build | 1 +
> > > > src/libcamera/color_space.cpp | 207 ++++++++++++++++++++++++++++++++
> > > > src/libcamera/meson.build | 1 +
> > > > 4 files changed, 292 insertions(+)
> > > > create mode 100644 include/libcamera/color_space.h
> > > > create mode 100644 src/libcamera/color_space.cpp
> > > >
> > > > diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h
> > > > new file mode 100644
> > > > index 00000000..9cd10503
> > > > --- /dev/null
> > > > +++ b/include/libcamera/color_space.h
> > > > @@ -0,0 +1,83 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
> > > > + *
> > > > + * color_space.h - color space definitions
> > > > + */
> > > > +
> > > > +#ifndef __LIBCAMERA_COLOR_SPACE_H__
> > > > +#define __LIBCAMERA_COLOR_SPACE_H__
> > > > +
> > > > +#include <string>
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +class ColorSpace
> > > > +{
> > > > +public:
> > > > + enum class Encoding : int {
> > > > + UNDEFINED,
> > >
> > > We typically use CamelCase for enumerators.
> > >
> > > > + RAW,
> > > > + REC601,
> > > > + REC709,
> > > > + REC2020,
> > > > + VIDEO,
> > > > + };
> > > > +
> > > > + enum class TransferFunction : int {
> > > > + UNDEFINED,
> > > > + IDENTITY,
> > > > + SRGB,
> > > > + REC709,
> > > > + };
> > > > +
> > > > + enum class Range : int {
> > > > + UNDEFINED,
> > > > + FULL,
> > > > + LIMITED,
> > > > + };
> > > > +
> > > > + constexpr ColorSpace(Encoding e, TransferFunction t, Range r)
> > > > + : encoding(e), transferFunction(t), range(r)
> > > > + {
> > > > + }
> > > > +
> > > > + constexpr ColorSpace()
> > > > + : ColorSpace(Encoding::UNDEFINED, TransferFunction::UNDEFINED, Range::UNDEFINED)
> > >
> > > Line wrap ?
> > >
> > > > + {
> > > > + }
> > >
> > > I'd move the default constructor furst.
> > >
> > > > +
> > > > + static const ColorSpace UNDEFINED;
> > > > + static const ColorSpace RAW;
> > > > + static const ColorSpace JFIF;
> > > > + static const ColorSpace SMPTE170M;
> > > > + static const ColorSpace REC709;
> > > > + static const ColorSpace REC2020;
> > > > + static const ColorSpace VIDEO;
> > >
> > > Shouldn't these be defined as static constexpr with a value here ?
> > >
> > > > +
> > > > + Encoding encoding;
> > > > + TransferFunction transferFunction;
> > > > + Range range;
> > > > +
> > > > + bool isFullyDefined() const;
> > > > +
> > > > + const std::string toString() const;
> > > > +};
> > > > +
> > > > +constexpr ColorSpace ColorSpace::UNDEFINED = { Encoding::UNDEFINED, TransferFunction::UNDEFINED, Range::UNDEFINED };
> > > > +constexpr ColorSpace ColorSpace::RAW = { Encoding::RAW, TransferFunction::IDENTITY, Range::FULL };
> > > > +constexpr ColorSpace ColorSpace::JFIF = { Encoding::REC601, TransferFunction::SRGB, Range::FULL };
> > > > +constexpr ColorSpace ColorSpace::SMPTE170M = { Encoding::REC601, TransferFunction::REC709, Range::LIMITED };
> > > > +constexpr ColorSpace ColorSpace::REC709 = { Encoding::REC709, TransferFunction::REC709, Range::LIMITED };
> > > > +constexpr ColorSpace ColorSpace::REC2020 = { Encoding::REC2020, TransferFunction::REC709, Range::LIMITED };
> > > > +constexpr ColorSpace ColorSpace::VIDEO = { Encoding::VIDEO, TransferFunction::REC709, Range::LIMITED };
> > >
> > > This could then be dropped, or possibly moved (without the initializers)
> > > to color_space.cpp if there's a need to bind a reference or take the
> > > address of these variables.
> > >
> > > > +
> > > > +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs);
> > > > +static inline bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)
> > > > +{
> > > > + return !(lhs == rhs);
> > > > +}
> > > > +
> > > > +} /* namespace libcamera */
> > > > +
> > > > +#endif /* __LIBCAMERA_COLOR_SPACE_H__ */
> > > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > > > index 5b25ef84..7a8a04e5 100644
> > > > --- a/include/libcamera/meson.build
> > > > +++ b/include/libcamera/meson.build
> > > > @@ -3,6 +3,7 @@
> > > > libcamera_public_headers = files([
> > > > 'camera.h',
> > > > 'camera_manager.h',
> > > > + 'color_space.h',
> > > > 'compiler.h',
> > > > 'controls.h',
> > > > 'file_descriptor.h',
> > > > diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp
> > > > new file mode 100644
> > > > index 00000000..d1ccb4cc
> > > > --- /dev/null
> > > > +++ b/src/libcamera/color_space.cpp
> > > > @@ -0,0 +1,207 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2021, Raspberry Pi (Trading) Limited
> > > > + *
> > > > + * color_space.cpp - color spaces.
> > > > + */
> > > > +
> > > > +#include <sstream>
> > > > +
> > > > +#include <libcamera/color_space.h>
> > >
> > > This should go first, to ensure that the header file is self-contained.
> > >
> > > > +
> > > > +/**
> > > > + * \file color_space.h
> > > > + * \brief Class and enums to represent colour spaces.
> > >
> > > No period at the end of briefs. Same below.
> > >
> > > > + */
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +/**
> > > > + * \class ColorSpace
> > > > + * \brief Class to describe a color space.
> > > > + *
> > > > + * The color space class defines the encodings of the color primaries, the
> > >
> > > s/color space/ColorSpace/
> > >
> > > You use both color and colour. For the class name, ColorSpace seems
> > > right (well, it seems wrong to me, but it seems to be the right choice
> > > :-)). For the text, we'll likely have to go through the documentation
> > > one day to make everything consistent, and will unfortunately likely
> > > have to pick the US spelling. This hasn't been completely decided yet,
> > > so I've fine with either for now, but let's make it consistent within
> > > the file.
> >
> > I tend to go American in the code and then UK in the text, but I can
> > go all-American, it's fine!
>
> My point was that the text mixes color and colour. I'm fine if you use
> the US spelling in code and UK in documentation for now (even though we
> will likely move everything to the US spelling at some point, to my
> dismay), as long as the documentation is consistent with itself.
>
> > > > + * transfer function associated with the color space, and the range (sometimes
> > > > + * also referred to as the quantisation) of the color space.
> > > > + *
> > > > + * Certain combinations of these fields form well-known standard color spaces,
> > > > + * such as "JFIF" or "REC709", though there is flexibility to leave some or all
> > > > + * of them undefined too.
> > > > + */
> > > > +
> > > > +/**
> > > > + * \enum ColorSpace::Encoding
> > > > + * \brief The encoding used for the color primaries.
> > >
> > > Can we name this Primaries ? Encoding is strongly tied to the Y'CbCr
> > > encoding from R'G'B' values, I think it would be confusing.
> > >
> > > Now that I've written this, I realize (after reading the rest of the
> > > series) that you're using this as the actual Y'CbCr encoding. The name
> > > is thus fine, but it shouldn't mention colour primaries then, it should
> > > be defined as Y'CbCr encoding. I would also move the definition of the
> > > Encoding enum after TransferFunction, as that's the order in which
> > > they're applied.
> > >
> > > This leads me to the next question: what should we do with the colour
> > > primaries (and white point) ? Those are part of the colour space
> > > definition.
> >
> > Yes, we do need a field for primaries, my bad for not including it. I
> > guess I'd imagined wrapping it up into the encoding as they often go
> > together, but that's not right.
> >
> > Looking at the colour spaces we have in mind, I think we'll need
> > values for Rec.709 (covers Rec.709, sRGB/JFIF), SMPTE (or "SMPTE-C"
> > for SMPTE170M, almost but not quite the same as Rec.709) and Rec.2020.
> >
> > I don't think we need to do anything about the white point, that's
> > determined for you once you have your primaries I believe.
>
> I'm not sure if the white point is a direct result of the chromaticity
> coordinates of the primaries, but it's defined in the above
> specifications, so if we go for presets it doesn't need to be handled
> separately.
In general we have a 3x3 RGB to XYZ matrix (for example
http://www.brucelindbloom.com/index.html?Eqn_RGB_XYZ_Matrix.html). The
primaries are what we get when we apply that matrix to red (1,0,0),
green (0,1,0) and blue (0,0,1). So the result of applying the matrix
to (1,1,1) is now pre-determined, and is the white point. Someone
please correct me if I understood that less well than I thought! :)
But I think going with presets is the thing to do, trying to do
on-the-fly conversions of CCMs to arbitrary colour spaces sounds quite
fraught... (and we certainly would want to opt out of implementing
anything like that)
>
> > > > + *
> > > > + * \var ColorSpace::Encoding::UNDEFINED
> > > > + * \brief The encoding for the colour primaries is not specified.
> > >
> > > I think we need to include more usage information. Can an application
> > > set the encoding to undefined ? What will happen then ? Can a camera
> > > return undefined ?
> > >
> > > > + * \var ColorSpace::Encoding::RAW
> > > > + * \brief These are raw colours from the sensor.
> > >
> > > Will this also apply to RGB formats ?
> > >
> > > > + * \var ColorSpace::Encoding::REC601
> > > > + * \brief REC601 colour primaries.
> > >
> > > s/REC601/Rec. 601/
> > >
> > > or possibly "ITU-R Rec. 601" or "ITU-R Rec. BT.601" if we want to be
> > > precise. Same below.
> > >
> > > Should we also include the encoding formula explicitly, or would that be
> > > too much details ?
> >
> > Maybe some web links instead, to avoid duplication?
>
> If you can find web links that are standard and stable enough, that's
> fine with me.
>
> > > > + * \var ColorSpace::Encoding::REC709
> > > > + * \brief Rec709 colour primaries.
> > > > + * \var ColorSpace::Encoding::REC2020
> > > > + * \brief REC2020 colour primaries.
> > > > + * \var ColorSpace::Encoding::VIDEO
> > > > + * \brief A place-holder for video streams which will be resolved to one
> > > > + * of REC601, REC709 or REC2020 once the video resolution is known.
> > >
> > > We also need more information here. What's your expected use case for
> > > this ? I'm concerned it may seem safe for applications to pick this as a
> > > default, but they won't care much about colour spaces then, and will
> > > likely get things wrong.
> >
> > So the idea was that generateConfiguration would use VIDEO when the
> > stream role is "video", so that it could be resolved later once the
> > video resolution is known. That way, application code would almost
> > never have to deal with colour spaces because you'd do
> >
> > configuration = camera->generateConfiguration(roles);
> > /* set up formats and resolutions *./
> > camera->configure(configuration);
> >
> > and for JPEG, mpeg/h26x video it would all work out correctly. The
> > only case you'd have to worry about would be something like mjpeg,
> > where you'd have to set the colour space explicitly to JPEG/JFIF.
> >
> > Note that we would have been able to leave it as UNDEFINED (rather
> > than inventing VIDEO) but for the fact that the configuration
> > generated doesn't record that it was intended for video.
> >
> > Slightly unhappily, we will need to do the same with the (proposed)
> > Primaries field, as that too can change depending on the video
> > resolution. Or perhaps it would be better for a stream configuration
> > to remember its "role"? I could go with that.
>
> Roles were not meant to be remembered, and may actually go away in their
> current form, so I'd rather not rely on them.
>
> One part that bothers me a bit is that the VIDEO encoding needs to be
> resolved by pipeline handlers, which will end up copying each other,
> with their own little bugs of course. Could we try to at least
> centralize that ?
Ah OK, didn't realise that roles might be changing. So what did you
have in mind here, maybe a PipelineHandler base class method that you
can run at the start of validate() which patches up any colour space
fields according to the stream's output resolution (if marked as
"VIDEO")?
David
>
> > > > + */
> > > > +
> > > > +/**
> > > > + * \enum ColorSpace::TransferFunction
> > > > + * \brief The transfer function used for this colour space.
> > > > + *
> > > > + * \var ColorSpace::TransferFunction::UNDEFINED
> > > > + * \brief The transfer function is not specified.
> > > > + * \var ColorSpace::TransferFunction::IDENTITY
> > > > + * \brief This color space uses an identity transfer function.
> > >
> > > Isn't "linear" a more common term than "identity" for the transfer
> > > function ?
> > >
> > > > + * \var ColorSpace::TransferFunction::SRGB
> > > > + * \brief sRGB transfer function.
> > > > + * \var ColorSpace::TransferFunction::REC709
> > > > + * \brief Rec709 transfer function.
> > > > + */
> > > > +
> > > > +/**
> > > > + * \enum ColorSpace::Range
> > > > + * \brief The range (sometimes "quantisation") for this color space.
> > >
> > > Technically speaking, the quantisation isn't part of the colour space,
> > > but I fear it would bring lots of complexity and little gain to try and
> > > keep it separate.
> > >
> > > > + *
> > > > + * \var ColorSpace::Range::UNDEFINED
> > > > + * \brief The range is not specified.
> > > > + * \var ColorSpace::Range::FULL
> > > > + * \brief This color space uses full range pixel values.
> > > > + * \var ColorSpace::Range::LIMITED
> > > > + * \brief This color space uses limited range pixel values.
> > >
> > > Should we define these in more details as well, with the exact range ?
> >
> > Yes, makes sense. Or possibly web links, will have a look!
> >
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn ColorSpace::ColorSpace(Encoding e, TransferFunction t, Range r)
> > > > + * \brief Construct a ColorSpace from explicit values
> > > > + * \param[in] e The encoding for the color primaries
> > > > + * \param[in] t The transfer function for the color space
> > > > + * \param[in] r The range of the pixel values in this color space
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn ColorSpace::ColorSpace()
> > > > + * \brief Construct a color space with undefined encoding, transfer function
> > > > + * and range
> > > > + */
> > > > +
> > > > +/**
> > > > + * \brief Return true if all the fields of the color space are defined, otherwise false.
> > >
> > > Line wrap. You also need a \return, so I'd write \brief as "Check if
> > > ...".
> > >
> > > > + */
> > > > +bool ColorSpace::isFullyDefined() const
> > > > +{
> > > > + return encoding != Encoding::UNDEFINED &&
> > > > + transferFunction != TransferFunction::UNDEFINED &&
> > > > + range != Range::UNDEFINED;
> > > > +}
> > > > +
> > > > +/**
> > > > + * \brief Assemble and return a readable string representation of the
> > > > + * ColorSpace
> > > > + * \return A string describing the ColorSpace
> > > > + */
> > > > +const std::string ColorSpace::toString() const
> > > > +{
> > > > + static const char *encodings[] = {
> > > > + "UNDEFINED",
> > > > + "RAW",
> > > > + "REC601",
> > > > + "REC709",
> > > > + "REC2020",
> > > > + };
> > > > + static const char *transferFunctions[] = {
> > > > + "UNDEFINED",
> > > > + "IDENTITY",
> > > > + "SRGB",
> > > > + "REC709",
> > > > + };
> > > > + static const char *ranges[] = {
> > > > + "UNDEFINED",
> > > > + "FULL",
> > > > + "LIMITED",
> > > > + };
> > > > +
> > > > + std::stringstream ss;
> > > > + ss << std::string(encodings[static_cast<int>(encoding)]) << "+"
> > >
> > > "+" seems a bit of a weird separator, I would have expected "/". Is
> > > there a specific reason ?
> > >
> > > > + << std::string(transferFunctions[static_cast<int>(transferFunction)]) << "+"
> > > > + << std::string(ranges[static_cast<int>(range)]);
> > > > +
> > > > + return ss.str();
> > > > +}
> > > > +
> > > > +/**
> > > > + * \var ColorSpace::encoding
> > > > + * \brief The encoding of the color primaries
> > > > + */
> > > > +
> > > > +/**
> > > > + * \var ColorSpace::transferFunction
> > > > + * \brief The transfer function for this color space.
> > > > + */
> > > > +
> > > > +/**
> > > > + * \var ColorSpace::range
> > > > + * \brief The pixel range used by this color space.
> > > > + */
> > > > +
> > > > +/**
> > > > + * \var ColorSpace::UNDEFINED
> > > > + * \brief A constant representing a fully undefined color space.
> > > > + */
> > > > +
> > > > +/**
> > > > + * \var ColorSpace::RAW
> > > > + * \brief A constant representing a raw color space (from a sensor).
> > > > + */
> > > > +
> > > > +/**
> > > > + * \var ColorSpace::JFIF
> > > > + * \brief A constant representing the JFIF color space usually used for
> > > > + * encoding JPEG images.
> > >
> > > Should this and the colour spaces below be defined more precisely in the
> > > documentation, or do you think users can just look at the values of the
> > > different members to figure out which is which ?
> > >
> > > > + */
> > > > +
> > > > +/**
> > > > + * \var ColorSpace::SMPTE170M
> > > > + * \brief A constant representing the SMPTE170M color space (sometimes also
> > > > + * referred to as "full range BT601").
> > >
> > > Even though quantization is limited range ?
> >
> > I shouldn't refer to SMPTE170M as a kind of "full range BT601", even
> > though rather lazily I sometimes think that. BT601 doesn't define
> > primaries, so it's misleading.
> >
> > > > + */
> > > > +
> > > > +/**
> > > > + * \var ColorSpace::REC709
> > > > + * \brief A constant representing the REC709 color space.
> > > > + */
> > > > +
> > > > +/**
> > > > + * \var ColorSpace::REC2020
> > > > + * \brief A constant representing the REC2020 color space.
> > > > + */
> > > > +
> > > > +/**
> > > > + * \var ColorSpace::VIDEO
> > > > + * \brief A constant that video streams can use to indicate the "default"
> > > > + * color space for a video of this resolution, once that is is known. For
> > > > + * exmample, SD streams would interpret this as SMPTE170M, HD streams as
> > >
> > > s/exmample/example/
> > >
> > > > + * REC709 and ultra HD as REC2020.
> > >
> > > As mentioned above, this sounds a bit dangerous to me, but maybe I worry
> > > too much. If we want to keep this, I think we need to make the
> > > definition stricter, by documenting the required behaviour (thus
> > > removing "for example", and defining SD and HD).
> >
> > Or maybe store the stream role in the configuration...?
> >
> > Let's continue the discussion, but in the meantime I'll put together a
> > v3 with most of the other points addressed.
> >
> > > > + */
> > > > +
> > > > +/**
> > > > + * \brief Compare color spaces for equality
> > >
> > > *
> > > * Color spaces are considered identical if they have the same
> > > * \ref transferFunction, \ref encoding and \ref range.
> > > *
> > >
> > > > + * \return True if the two color spaces are identical, false otherwise
> > > > + */
> > > > +bool operator==(const ColorSpace &lhs, const ColorSpace &rhs)
> > > > +{
> > > > + return lhs.encoding == rhs.encoding &&
> > > > + lhs.transferFunction == rhs.transferFunction &&
> > > > + lhs.range == rhs.range;
> > > > +}
> > > > +
> > > > +/**
> > > > + * \fn bool operator!=(const ColorSpace &lhs, const ColorSpace &rhs)
> > > > + * \brief Compare color spaces for inequality
> > > > + * \return True if the two color spaces are not identical, false otherwise
> > > > + */
> > > > +
> > > > +} /* namespace libcamera */
> > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > > index bf82d38b..88dfbf24 100644
> > > > --- a/src/libcamera/meson.build
> > > > +++ b/src/libcamera/meson.build
> > > > @@ -8,6 +8,7 @@ libcamera_sources = files([
> > > > 'camera_manager.cpp',
> > > > 'camera_sensor.cpp',
> > > > 'camera_sensor_properties.cpp',
> > > > + 'color_space.cpp',
> > > > 'controls.cpp',
> > > > 'control_serializer.cpp',
> > > > 'control_validator.cpp',
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list