[libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a default colorspace
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jul 27 02:01:58 CEST 2022
Hello,
(CC'ing Hans Verkuil)
On Tue, Jul 26, 2022 at 09:17:01PM +0530, Umang Jain via libcamera-devel wrote:
> On 7/26/22 17:42, Umang Jain via libcamera-devel wrote:
> > On 7/26/22 15:42, David Plowman wrote:
> >> On Mon, 25 Jul 2022 at 12:15, Umang Jain wrote:
> >>> On 7/25/22 16:16, David Plowman wrote:
> >>>> Hi Umang
> >>>>
> >>>> Thanks for this patch. I was wondering if I could ask for a little
> >>>> more information on how it would be used.
> >>>
> >>> Yes, so the idea here is to provide a colorspace where one(or more)
> >>> identifiers are specified by the user whereas others can be left upto
> >>> the driver to fill in.
> >>>
> >>> I wasn't aware of this use case until very recently. In gstreamer, it
> >>> seems possible that the user wants to use(enforce?) a particular
> >>> transfer function whereas other identifier are left upto the driver
> >>> to fill.
> >>>
> >>> I am not very sure how would this be helpful (since colorspace
> >>> identifiers depend on each other I think), and mis-matches between them
> >>> might not make any sense at all.
> >>>
> >>> But such a use case as has been identified it seems, atleast in the
> >>> gstreamer case - so to satisfy that, I went ahead with this approach. I
> >>> couldn't get a better solution where the user specifies a set of
> >>> identifiers to use for colorspace, at the same time depending on the
> >>> driver to fill unknown or default values for remaining identifier(s).
> >>>
> >>>> In the original libcamera ColorSpace implementation I was very keen to
> >>>> avoid "unknown" or "default" values inside ColorSpaces. I was taking
> >>>> the view that applications really should say what ColorSpace they
> >>>> want, but at the same time I knew there will always be (bad)
> >>>> applications that don't think about colour spaces and so I let them
> >>>> use std::optional to allow the ColorSpace to be completely
> >>>> unspecified.
> >>>
> >>> Right, so when colorspace is completely unspecified, all values are
> >>> default-ed (V4L2Device::fromColorSpace()) in the v4l2_format.
Right, so when std::optional<ColorSpace> is std::nullopt, we set all the
colorspace-related fields in V4L2VideoDevice::setFormat() (and in the
equivalent implementation for v4L2Subdevice) to *_DEFAULT. The V4L2
driver then has to return a non-DEFAULT .colorspace, and possibly
non-DEFAULT values for .xfer_func, .ycbcr_enc and .quantization. This
defines without any ambiguity the exact colorspace information, and we
can create a fully defined ColorSpace out of that (if any of the
.xfer_func, .ycbcr_enc or .quantization fields are returned as DEFAULT,
we pick the default value that corresponds to the .colorspace, which
V4L2 guarantee will never be returned as DEFAULT by drivers).
So far I don't see an issue.
> >>> If I am asked about what's the goal of this patch, I would say that it
> >>> enables the use case where a sub-set of colorspace identifiers are
> >>> known, while others need to use defaults. Currently there is no default
> >>> std::optional<libcamera::ColorSpace> hence I added one, to facilitate
> >>> the application layer. The application should ideally copy the
> >>> libcamera::ColorSpace::Default as base, swap the identifiers it wants to
> >>> use in the copy, and send the custom colorspace for validation.
Our current API supports the following well-defined cases, where an
application can request
- A well-defined colorspace (e.g. ColorSpace::Rec709), with everything
it implies (the primaries, transfer function, YCbCr encoding and
quantization range defined by the colorspace). For this, an
application simply uses one of the presets.
- A well-defined colorspace, with one or multiple fields overridden to
different values (e.g. ColorSpace::Rec709 with a Full quantization
range). For this, an application copies one of the presets and
modifies one or multiple fields to well-defined values.
- A completely custom colorspace (e.g. Rec709 primaries, Srgb transfer
unction, Rec2020 YCbCr encoding and Full quantization range). For this
an application can construct the colorspace manually.
What we don't support today, and this is what I understand you want to
do with this patch, is for an application to tell libcamera "give me any
colorspace, as long as the quantization is full range". Is this right ?
If that's the use case you're after, would it be an issue if the
application used
ColorSpace cs = ColorSpace::Srgb;
cs.range = ColorSpace::Range::Full;
StreamConfiguration &cfg = ...;
cfg.colorSpace = cs;
? The first two lines will be equivalent to
ColorSpace cs(Primaries::Rec709, TransferFunction::Srgb,
YcbcrEncoding::Rec601, Range::Full);
which would then be translated to the V4L2 format
.colorspace = V4L2_COLORSPACE_REC709;
.xfer_func = V4L2_XFER_FUNC_SRGB;
.ycbcr_enc = V4L2_YCBCR_ENC_601;
.quantization = V4L2_QUANTIZATION_FULL_RANGE;
How a driver will handle this if this particular combination isn't
supported is unfortunately not clearly documented by V4L2, se we may
well get back
.colorspace = V4L2_COLORSPACE_REC709;
.xfer_func = V4L2_XFER_FUNC_709;
.ycbcr_enc = V4L2_YCBCR_ENC_709;
.quantization = V4L2_QUANTIZATION_LIM_RANGE;
even if the driver would support
.colorspace = V4L2_COLORSPACE_REC709;
.xfer_func = V4L2_XFER_FUNC_709;
.ycbcr_enc = V4L2_YCBCR_ENC_709;
.quantization = V4L2_QUANTIZATION_FULL_RANGE;
if asked directly, given that V4L2 doesn't describe how to adjust a
colorspace that isn't supported.
Requesting
.colorspace = V4L2_COLORSPACE_DEFAULT;
.xfer_func = V4L2_XFER_FUNC_DEFAULT;
.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
.quantization = V4L2_QUANTIZATION_FULL_RANGE;
could possibly help, except that we have only two drivers that even try
to honour the *_CSC flags, let alone implement correct colorspace
behaviour.
On a side note, I have a small feeling we may be trying to solve
problems that will never appear in practice.
> >>> Also note, the mapping is only one way (from libcamera to V4L2)
> >>> intentionally. It shouldn't
> >>> https://github.com/raspberrypi/picamera2#installationbe the case
> >>> where driver itself returns
> >>> "::Default" back to user.
> >>>
> >>>> In V4L2, my understanding is that "_DEFAULT" normally means that you
> >>>> infer the value from the overall V4L2_COLORSPACE (someone please
> >>>> correct me if that's wrong!). In libcamera we don't have an "overall
> >>>
> >>> If we could infer all the identifier values from
> >>> V4L2_COLORSPACE_DEFAULT, there shouldn't ideally be:
> >>>
> >>> V4L2_XFER_FUNC_DEFAULT, V4L2_YCBCR_ENC_DEFAULT,
> >>> V4L2_QUANTIZATION_DEFAULT defaults, atleast to my understanding.
> >>>
> >>>> colour space" in this way, so what would it then mean? For example, is
> >>>> "ColorSpace::Range::Default" ever the same as
> >>>> "ColorSpace::Range::Full", or is it always different? How do we (can
> >>>> we even) document what it means?
> >>>
> >>> Maybe we should look at V4L2 level first, before libcamera for e.g.
> >>>
> >>> Would V4L2_QUANTIZATION_DEFAULT be ever same as
> >>> V4L2_QUANTIZATION_FULL_RANGE ?
We need to make sure we can implement the behaviour defined in the
libcamera public API using the V4L2-based pipeline handlers, but the
V4L2 API doesn't dictate how we should handle colorspaces. If anything,
we should influence V4L2 with real world use cases.
> >>>> I'd be interested to hear what other folks think. Possibly I'm being a
> >>>
> >>> I am all ears here as well, so let' see
> >>>
> >>>> bit paranoid, but then I have been bitten by "wrong colour space"
> >>>> problems often enough in the past (usually just before a product
> >>>> ships!).
> >>>
> >>> ouch :-S
> >>
> >> My overall feeling is that I don't like having "default" things
> >> because I'm worried that they'll start appearing when you don't expect
> >> them. And then how would you know what they meant? As you know by now,
> >> I'm a bit paranoid about colour spaces going wrong...
> >>
> >> However, I see the point about these values possibly being convenient
> >> for negotiating colour spaces with (for example) gstreamer. But
> >> perhaps we could reduce the risk of them spreading, maybe we might
> >> consider:
> >>
> >> * Documenting clearly that "default" values are only "inputs" (what
> >> you want), and are never returned (what you got).
We should indeed never return any of the colorspace fields set to
"Default".
> > I've already made it impossible to return "default" values in this
> > patch. That's the reasoning behind you see the mappings going from
> > libcamera to V4L2 /only/ and not other way around.
> >
> > Yes we can clarify the document on that front.
> >
> >> * Adding a valid() method that complains if a ColorSpace has any
> >> undefined/default values in it?
> >> * When a function returns a ColorSpace that has been set, can we make
> >> them check that the returned value is "valid()"?
> >
> > The driver always return a colorspace which is supported right? I
> > believe a supported colorspace would not _DEFAULT identifier.
>
> To answer this, I have been clarified on #linux-media that a driver can
> report _DEFAULT identifiers back to the userspace.
Only for .xfer_func, .ycbcr_enc and .quantization, never for
.colorspace, so we can always map a V4L2 *_DEFAULT value that we get
back from a driver to well-defined values for all fields.
> But libcamera doesn't map to _DEFAULT identifiers in
> V4L2Device::toColorSpace() either, so we should be good here? Or still
> we need additional valid()?
On a side note, we have bugs in our colorspace handling. V4L2 considers
the colorspace fields as read-only from an application point of view,
both on video devices and on subdevs, unless the
V4L2_PIX_FMT_FLAG_SET_CSC or V4L2_MBUS_FRAMEFMT_SET_CSC flags
(respectively) are set when calling the S_FMT ioctl. Only vivid
implements this correctly for video nodes, and only the rkisp1 driver
handles the V4L2_MBUS_FRAMEFMT_SET_CSC on subdevs.
Furthermore, how the *_DEFAULT values operate when setting them is not
very well defined, I've tried to clarify that in #linux-media on OFTC
today, and I've been told by Hans that setting
.flags = V4L2_MBUS_FRAMEFMT_SET_CSC;
.colorspace = REC709;
.xfer_func = DEFAULT;
.ycbcr_enc = DEFAULT;
.quantization = DEFAULT;
on a subdev source pad means that the application wants to override the
color primaries and nothing else, while I interpret the documentation
([1]) as saying that the application wants to set the .xfer_func,
.ycbcr_enc and .quantization to the default values specified by REC709.
Discussions are still ongoing.
https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/subdev-formats.html#id1
> > valid() on the returned colorspace means we don't trust the driver? It
> > might have set one of the fields as _DEFAULT ?
> >
> > I am not sure what valid() will try to achieve here. Provided there is
> > already handled case where any the mappings is missing in
> > libcamera(colorspace will be std::nullopt) [1]
> >
> > [1] https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n816
> >
> >> * I also wasn't quite sure why a ColorSpace::Default is needed, would
> >
> > Convenience
> >
> >> a ColorSpace be regarded as being "default" if every field is
> >> "default", or just some of them? Or could an unset std::optional be
> >> used instead?
> >
> > If some fields are specific values than the "default" identifiers,
> > it's not a default colorspace. A default colorspace is when "all"
> > fields are default and is equal to libcamera::ColorSpace::Default.
> >
> > A std::optional is initialised with std::nullopt. How would I set a
> > colorspace where I want a specific transfer function to be used in
> > that? Will it still remain "unset" ?
I do see an overlap between setting the colorspace to nullopt and
setting it to a Default colorspace, which isn't very nice (in general
multiple ways to do the same thing is a bad idea).
> >> What do you think?
> >>
> >>>> On Sun, 24 Jul 2022 at 15:44, Umang Jain via libcamera-devel wrote:
> >>>>> Add a Colorspace::Default which corresponds to default V4L2
> >>>>> colorspace identifiers.
> >>>>>
> >>>>> \todo Add defaults to python colorspace bindings
> >>>>>
> >>>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >>>>> ---
> >>>>> include/libcamera/color_space.h | 5 +++++
> >>>>> src/libcamera/color_space.cpp | 29 ++++++++++++++++++++++++++++-
> >>>>> src/libcamera/v4l2_device.cpp | 5 +++++
> >>>>> 3 files changed, 38 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/include/libcamera/color_space.h b/include/libcamera/color_space.h
> >>>>> index 086c56c1..0ad8da17 100644
> >>>>> --- a/include/libcamera/color_space.h
> >>>>> +++ b/include/libcamera/color_space.h
> >>>>> @@ -16,6 +16,7 @@ class ColorSpace
> >>>>> {
> >>>>> public:
> >>>>> enum class Primaries {
> >>>>> + Default,
> >>>>> Raw,
> >>>>> Smpte170m,
> >>>>> Rec709,
> >>>>> @@ -23,12 +24,14 @@ public:
> >>>>> };
> >>>>>
> >>>>> enum class TransferFunction {
> >>>>> + Default,
> >>>>> Linear,
> >>>>> Srgb,
> >>>>> Rec709,
> >>>>> };
> >>>>>
> >>>>> enum class YcbcrEncoding {
> >>>>> + Default,
> >>>>> None,
> >>>>> Rec601,
> >>>>> Rec709,
> >>>>> @@ -36,6 +39,7 @@ public:
> >>>>> };
> >>>>>
> >>>>> enum class Range {
> >>>>> + Default,
> >>>>> Full,
> >>>>> Limited,
> >>>>> };
> >>>>> @@ -45,6 +49,7 @@ public:
> >>>>> {
> >>>>> }
> >>>>>
> >>>>> + static const ColorSpace Default;
> >>>>> static const ColorSpace Raw;
> >>>>> static const ColorSpace Jpeg;
> >>>>> static const ColorSpace Srgb;
> >>>>> diff --git a/src/libcamera/color_space.cpp b/src/libcamera/color_space.cpp
> >>>>> index 895e5c8e..d52f58cf 100644
> >>>>> --- a/src/libcamera/color_space.cpp
> >>>>> +++ b/src/libcamera/color_space.cpp
> >>>>> @@ -50,6 +50,9 @@ namespace libcamera {
> >>>>> * \enum ColorSpace::Primaries
> >>>>> * \brief The color primaries for this color space
> >>>>> *
> >>>>> + * \var ColorSpace::Primaries::Default
> >>>>> + * \brief Use the default primaries as defined by the driver
> >>>>> + *
> >>>>> * \var ColorSpace::Primaries::Raw
> >>>>> * \brief These are raw colors directly from a sensor, the primaries are
> >>>>> * unspecified
> >>>>> @@ -68,6 +71,9 @@ namespace libcamera {
> >>>>> * \enum ColorSpace::TransferFunction
> >>>>> * \brief The transfer function used for this color space
> >>>>> *
> >>>>> + * \var ColorSpace::TransferFunction::Default
> >>>>> + * \brief Use the default transfer function as defined by the driver
> >>>>> + *
> >>>>> * \var ColorSpace::TransferFunction::Linear
> >>>>> * \brief This color space uses a linear (identity) transfer function
> >>>>> *
> >>>>> @@ -82,6 +88,9 @@ namespace libcamera {
> >>>>> * \enum ColorSpace::YcbcrEncoding
> >>>>> * \brief The Y'CbCr encoding
> >>>>> *
> >>>>> + * \var ColorSpace::YcbcrEncoding::Default
> >>>>> + * \brief Use the default Y'CbCr encoding as defined by the driver
> >>>>> + *
> >>>>> * \var ColorSpace::YcbcrEncoding::None
> >>>>> * \brief There is no defined Y'CbCr encoding (used for non-YUV formats)
> >>>>> *
> >>>>> @@ -99,6 +108,9 @@ namespace libcamera {
> >>>>> * \enum ColorSpace::Range
> >>>>> * \brief The range (sometimes "quantisation") for this color space
> >>>>> *
> >>>>> + * \var ColorSpace::Range::Default
> >>>>> + * Use the default range as defined by the driver
> >>>>> + *
> >>>>> * \var ColorSpace::Range::Full
> >>>>> * \brief This color space uses full range pixel values
> >>>>> *
> >>>>> @@ -132,7 +144,8 @@ std::string ColorSpace::toString() const
> >>>>> {
> >>>>> /* Print out a brief name only for standard color spaces. */
> >>>>>
> >>>>> - static const std::array<std::pair<ColorSpace, const char *>, 6> colorSpaceNames = { {
> >>>>> + static const std::array<std::pair<ColorSpace, const char *>, 7> colorSpaceNames = { {
> >>>>> + { ColorSpace::Default, "Default" },
> >>>>> { ColorSpace::Raw, "RAW" },
> >>>>> { ColorSpace::Jpeg, "JPEG" },
> >>>>> { ColorSpace::Srgb, "sRGB" },
> >>>>> @@ -150,23 +163,27 @@ std::string ColorSpace::toString() const
> >>>>> /* Assemble a name made of the constituent fields. */
> >>>>>
> >>>>> static const std::map<Primaries, std::string> primariesNames = {
> >>>>> + { Primaries::Default, "Default" },
> >>>>> { Primaries::Raw, "RAW" },
> >>>>> { Primaries::Smpte170m, "SMPTE170M" },
> >>>>> { Primaries::Rec709, "Rec709" },
> >>>>> { Primaries::Rec2020, "Rec2020" },
> >>>>> };
> >>>>> static const std::map<TransferFunction, std::string> transferNames = {
> >>>>> + { TransferFunction::Default, "Default" },
> >>>>> { TransferFunction::Linear, "Linear" },
> >>>>> { TransferFunction::Srgb, "sRGB" },
> >>>>> { TransferFunction::Rec709, "Rec709" },
> >>>>> };
> >>>>> static const std::map<YcbcrEncoding, std::string> encodingNames = {
> >>>>> + { YcbcrEncoding::Default, "Default" },
> >>>>> { YcbcrEncoding::None, "None" },
> >>>>> { YcbcrEncoding::Rec601, "Rec601" },
> >>>>> { YcbcrEncoding::Rec709, "Rec709" },
> >>>>> { YcbcrEncoding::Rec2020, "Rec2020" },
> >>>>> };
> >>>>> static const std::map<Range, std::string> rangeNames = {
> >>>>> + { Range::Default, "Default" },
> >>>>> { Range::Full, "Full" },
> >>>>> { Range::Limited, "Limited" },
> >>>>> };
> >>>>> @@ -232,6 +249,16 @@ std::string ColorSpace::toString(const std::optional<ColorSpace> &colorSpace)
> >>>>> * \brief The pixel range used with by color space
> >>>>> */
> >>>>>
> >>>>> +/**
> >>>>> + * \brief A constant representing a default color space
> >>>>> + */
> >>>>> +const ColorSpace ColorSpace::Default = {
> >>>>> + Primaries::Default,
> >>>>> + TransferFunction::Default,
> >>>>> + YcbcrEncoding::Default,
> >>>>> + Range::Default
> >>>>> +};
> >>>>> +
> >>>>> /**
> >>>>> * \brief A constant representing a raw color space (from a sensor)
> >>>>> */
> >>>>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> >>>>> index 3fc8438f..ecfcf337 100644
> >>>>> --- a/src/libcamera/v4l2_device.cpp
> >>>>> +++ b/src/libcamera/v4l2_device.cpp
> >>>>> @@ -770,6 +770,7 @@ static const std::map<uint32_t, ColorSpace::Range> v4l2ToRange = {
> >>>>> };
> >>>>>
> >>>>> static const std::vector<std::pair<ColorSpace, v4l2_colorspace>> colorSpaceToV4l2 = {
> >>>>> + { ColorSpace::Default, V4L2_COLORSPACE_DEFAULT },
> >>>>> { ColorSpace::Raw, V4L2_COLORSPACE_RAW },
> >>>>> { ColorSpace::Jpeg, V4L2_COLORSPACE_JPEG },
> >>>>> { ColorSpace::Srgb, V4L2_COLORSPACE_SRGB },
> >>>>> @@ -779,6 +780,7 @@ static const std::vector<std::pair<ColorSpace, v4l2_colorspace>> colorSpaceToV4l
> >>>>> };
> >>>>>
> >>>>> static const std::map<ColorSpace::Primaries, v4l2_colorspace> primariesToV4l2 = {
> >>>>> + { ColorSpace::Primaries::Default, V4L2_COLORSPACE_DEFAULT },
> >>>>> { ColorSpace::Primaries::Raw, V4L2_COLORSPACE_RAW },
> >>>>> { ColorSpace::Primaries::Smpte170m, V4L2_COLORSPACE_SMPTE170M },
> >>>>> { ColorSpace::Primaries::Rec709, V4L2_COLORSPACE_REC709 },
> >>>>> @@ -786,18 +788,21 @@ static const std::map<ColorSpace::Primaries, v4l2_colorspace> primariesToV4l2 =
> >>>>> };
> >>>>>
> >>>>> static const std::map<ColorSpace::TransferFunction, v4l2_xfer_func> transferFunctionToV4l2 = {
> >>>>> + { ColorSpace::TransferFunction::Default, V4L2_XFER_FUNC_DEFAULT },
> >>>>> { ColorSpace::TransferFunction::Linear, V4L2_XFER_FUNC_NONE },
> >>>>> { ColorSpace::TransferFunction::Srgb, V4L2_XFER_FUNC_SRGB },
> >>>>> { ColorSpace::TransferFunction::Rec709, V4L2_XFER_FUNC_709 },
> >>>>> };
> >>>>>
> >>>>> static const std::map<ColorSpace::YcbcrEncoding, v4l2_ycbcr_encoding> ycbcrEncodingToV4l2 = {
> >>>>> + { ColorSpace::YcbcrEncoding::Default, V4L2_YCBCR_ENC_DEFAULT },
> >>>>> { ColorSpace::YcbcrEncoding::Rec601, V4L2_YCBCR_ENC_601 },
> >>>>> { ColorSpace::YcbcrEncoding::Rec709, V4L2_YCBCR_ENC_709 },
> >>>>> { ColorSpace::YcbcrEncoding::Rec2020, V4L2_YCBCR_ENC_BT2020 },
> >>>>> };
> >>>>>
> >>>>> static const std::map<ColorSpace::Range, v4l2_quantization> rangeToV4l2 = {
> >>>>> + { ColorSpace::Range::Default, V4L2_QUANTIZATION_DEFAULT },
> >>>>> { ColorSpace::Range::Full, V4L2_QUANTIZATION_FULL_RANGE },
> >>>>> { ColorSpace::Range::Limited, V4L2_QUANTIZATION_LIM_RANGE },
> >>>>> };
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list