[libcamera-devel] [PATCH 2/3] libcamera: colorspace: Add a default colorspace
Umang Jain
umang.jain at ideasonboard.com
Tue Jul 26 17:47:01 CEST 2022
Hi,
On 7/26/22 17:42, Umang Jain via libcamera-devel wrote:
> Hi David,
>
> On 7/26/22 15:42, David Plowman wrote:
>> Hi Umang, everyone
>>
>> Thanks for the reply. Perhaps I can add some more thoughts about this
>> question.
>
>
> Thank you for your thoughts.
>
>>
>> On Mon, 25 Jul 2022 at 12:15, Umang Jain
>> <umang.jain at ideasonboard.com> wrote:
>>> Hi David,
>>>
>>> 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.
>>>
>>> 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.
>>>
>>> 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 ?
>>>
>>>> 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).
>
>
> 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.
But libcamera doesn't map to _DEFAULT identifiers in
V4L2Device::toColorSpace() either, so we should be good here? Or still
we need additional valid()?
>
> 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" ?
>
>> What do you think?
>>
>> Thanks!
>> David
>>
>>>> Thanks!
>>>> David
>>>>
>>>> On Sun, 24 Jul 2022 at 15:44, Umang Jain via libcamera-devel
>>>> <libcamera-devel at lists.libcamera.org> 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 },
>>>>> };
>>>>> --
>>>>> 2.31.1
>>>>>
More information about the libcamera-devel
mailing list