[libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <> libcamera::ColorSpace mappings
Umang Jain
umang.jain at ideasonboard.com
Mon Aug 1 13:08:43 CEST 2022
Hi Laurent,
On 7/29/22 20:24, Laurent Pinchart wrote:
> Hi Umang,
>
> On Fri, Jul 29, 2022 at 03:40:52PM +0530, Umang Jain wrote:
>> On 7/29/22 13:36, Umang Jain via libcamera-devel wrote:
>>> Hi Laurent,
>>>
>>> My anaylsis on sRGB mapping ...
>>>
>>> On 7/27/22 06:47, Laurent Pinchart wrote:
>>>> Hi Umang and Rishikesh,
>>>>
>>>> Thank you for the patch.
>>>>
>>>> On Sun, Jul 24, 2022 at 08:13:55PM +0530, Umang Jain via
>>>> libcamera-devel wrote:
>>>>> From: Rishikesh Donadkar <rishikeshdonadkar at gmail.com>
>>>>>
>>>>> Provide colorimetry <=> libcamera::ColorSpace mappings via:
>>>>> - GstVideoColorimetry colorimetry_from_colorspace(colorspace);
>>>>> - ColorSpace colorspace_from_colorimetry(colorimetry);
>>>>>
>>>>> Read the colorimetry field from caps into the stream configuration.
>>>>> After stream validation, the sensor supported colorimetry will
>>>>> be retrieved and the caps will be updated accordingly.
>>>>>
>>>>> Colorimetry support for gstlibcamerasrc currently undertakes only one
>>>>> argument. Multiple colorimetry support shall be introduced in
>>>>> subsequent commits.
>>>>>
>>>>> Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar at gmail.com>
>>>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>>>>> ---
>>>>> src/gstreamer/gstlibcamera-utils.cpp | 175 +++++++++++++++++++++++++++
>>>>> 1 file changed, 175 insertions(+)
>>>>>
>>>>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp
>>>>> b/src/gstreamer/gstlibcamera-utils.cpp
>>>>> index c97c0d43..fb4f0e5c 100644
>>>>> --- a/src/gstreamer/gstlibcamera-utils.cpp
>>>>> +++ b/src/gstreamer/gstlibcamera-utils.cpp
>>>>> @@ -45,6 +45,157 @@ static struct {
>>>>> /* \todo NV42 is used in libcamera but is not mapped in GStreamer yet. */
>>>>> };
>>>>> +static GstVideoColorimetry
>>>>> +colorimetry_from_colorspace(const ColorSpace &colorSpace)
>>>>> +{
>>>>> + GstVideoColorimetry colorimetry;
>>>> I would initialize the structure with all fields set to UNKNOWN here...
>>>>
>>>>> +
>>>>> + switch (colorSpace.primaries) {
>>>>> + case ColorSpace::Primaries::Rec709:
>>>>> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_BT709;
>>>>> + break;
>>>>> + case ColorSpace::Primaries::Rec2020:
>>>>> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_BT2020;
>>>>> + break;
>>>>> + case ColorSpace::Primaries::Smpte170m:
>>>>> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_SMPTE170M;
>>>>> + break;
>>>>> + default:
>>>>> + GST_WARNING("ColorSpace primaries not mapped in
>>>>> GstLibcameraSrc");
>>>>> + colorimetry.primaries = GST_VIDEO_COLOR_PRIMARIES_UNKNOWN;
>>>> ... and drop the default case. As ColorSpace::Primaries is an enum
>>>> class, if you have cases for all the values, the compiler will warn only
>>>> if one (or more) of the enum values isn't handled in explicit cases.
>>>> This way you can avoid warnings, and have a guarantee that if the
>>>> libcamera colorspaces are later extended, the compiler will remind that
>>>> this function has to be updated. Same for the other components of the
>>>> colorspace below.
>>>>
>>>>> + }
>>>>> +
>>>>> + switch (colorSpace.transferFunction) {
>>>>> + case ColorSpace::TransferFunction::Rec709:
>>>>> + colorimetry.transfer = GST_VIDEO_TRANSFER_BT709;
>>>>> + break;
>>>>> + case ColorSpace::TransferFunction::Srgb:
>>>>> + colorimetry.transfer = GST_VIDEO_TRANSFER_SRGB;
>>>>> + break;
>>>>> + case ColorSpace::TransferFunction::Linear:
>>>>> + colorimetry.transfer = GST_VIDEO_TRANSFER_GAMMA10;
>>>>> + break;
>>>>> + default:
>>>>> + GST_WARNING("ColorSpace transfer function not mapped in GstLibcameraSrc");
>>>>> + colorimetry.transfer = GST_VIDEO_TRANSFER_UNKNOWN;
>>>>> + }
>>>>> +
>>>>> + switch (colorSpace.ycbcrEncoding) {
>>>>> + case ColorSpace::YcbcrEncoding::Rec709:
>>>>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT709;
>>>>> + break;
>>>>> + case ColorSpace::YcbcrEncoding::Rec2020:
>>>>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT2020;
>>>>> + break;
>>>>> + case ColorSpace::YcbcrEncoding::Rec601:
>>>>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;
>>>>> + break;
>>>>> + default:
>>>>> + GST_WARNING("Colorspace YcbcrEncoding not mapped in GstLibcameraSrc");
>>>>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_UNKNOWN;
>>>> Should we map YcbcrEncoding::None to GST_VIDEO_COLOR_MATRIX_RGB ?
>>> See below.
>>>
>>>> Otherwise capturing RGB or raw bayer will produce a warning. Same for
>>>> the Raw primaries, it seems that the best mapping would be
>>>> GST_VIDEO_COLOR_PRIMARIES_UNKNOWN (unless I'm missing something), but I
>>>> don't think we should warn in that case.
>>>>
>>>>> + }
>>>>> +
>>>>> + switch (colorSpace.range) {
>>>>> + case ColorSpace::Range::Full:
>>>>> + colorimetry.range = GST_VIDEO_COLOR_RANGE_0_255;
>>>>> + break;
>>>>> + case ColorSpace::Range::Limited:
>>>>> + colorimetry.range = GST_VIDEO_COLOR_RANGE_16_235;
>>>>> + break;
>>>>> + default:
>>>>> + GST_WARNING("Colorspace range not mapped in GstLibcameraSrc");
>>>>> + colorimetry.range = GST_VIDEO_COLOR_RANGE_UNKNOWN;
>>>>> + }
>>>>> +
>>>>> + return colorimetry;
>>>>> +}
>>>>> +
>>>>> +static std::optional<ColorSpace>
>>>>> +colorspace_from_colorimetry(GstVideoColorimetry *colorimetry)
>>>> I'd pass a const reference instead of a pointer, as there's no use case
>>>> for a null colorimetry.
>>>>
>>>>> +{
>>>>> + std::optional<ColorSpace> colorspace = ColorSpace::Default;
>>>> No need for std::optional here or in the return type.
>>>>
>>>>> +
>>>>> + switch (colorimetry->primaries) {
>>>>> + case GST_VIDEO_COLOR_PRIMARIES_BT709:
>>>>> + colorspace->primaries = ColorSpace::Primaries::Rec709;
>>>>> + break;
>>>>> + case GST_VIDEO_COLOR_PRIMARIES_BT2020:
>>>>> + colorspace->primaries = ColorSpace::Primaries::Rec2020;
>>>>> + break;
>>>>> + case GST_VIDEO_COLOR_PRIMARIES_SMPTE170M:
>>>>> + colorspace->primaries = ColorSpace::Primaries::Smpte170m;
>>>>> + break;
>>>>> + case GST_VIDEO_COLOR_PRIMARIES_UNKNOWN:
>>>>> + colorspace->primaries = ColorSpace::Primaries::Default;
>>>>> + break;
>>>>> + default:
>>>>> + GST_WARNING("Unknown primaries in colorimetry %d", colorimetry->primaries);
>>>>> + }
>>>>> +
>>>>> + switch (colorimetry->transfer) {
>>>>> + /* Tranfer function mappings inspired from v4l2src plugin */
>>>>> + case GST_VIDEO_TRANSFER_GAMMA18:
>>>>> + case GST_VIDEO_TRANSFER_GAMMA20:
>>>>> + case GST_VIDEO_TRANSFER_GAMMA22:
>>>>> + case GST_VIDEO_TRANSFER_GAMMA28:
>>>>> + GST_WARNING("GAMMA 18, 20, 22, 28 transfer functions not supported");
>>>>> + /* fallthrough */
>>>>> + case GST_VIDEO_TRANSFER_GAMMA10:
>>>>> + colorspace->transferFunction = ColorSpace::TransferFunction::Linear;
>>>>> + break;
>>>>> + case GST_VIDEO_TRANSFER_BT601:
>>>>> + case GST_VIDEO_TRANSFER_BT2020_12:
>>>>> + case GST_VIDEO_TRANSFER_BT2020_10:
>>>>> + case GST_VIDEO_TRANSFER_BT709:
>>>>> + colorspace->transferFunction = ColorSpace::TransferFunction::Rec709;
>>>>> + break;
>>>>> + case GST_VIDEO_TRANSFER_SRGB:
>>>>> + colorspace->transferFunction = ColorSpace::TransferFunction::Srgb;
>>>>> + break;
>>>>> + case GST_VIDEO_TRANSFER_UNKNOWN:
>>>>> + colorspace->transferFunction = ColorSpace::TransferFunction::Default;
>>>>> + break;
>>>>> + default:
>>>>> + GST_WARNING("Unknown colorimetry transfer %d", colorimetry->transfer);
>>>>> + }
>>>>> +
>>>>> + switch (colorimetry->matrix) {
>>>>> + /* FCC is about the same as BT601 with less digit */
>>>>> + case GST_VIDEO_COLOR_MATRIX_FCC:
>>>>> + case GST_VIDEO_COLOR_MATRIX_BT601:
>>>>> + colorspace->ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec601;
>>>>> + break;
>>>>> + case GST_VIDEO_COLOR_MATRIX_BT709:
>>>>> + colorspace->ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec709;
>>>>> + break;
>>>>> + case GST_VIDEO_COLOR_MATRIX_BT2020:
>>>>> + colorspace->ycbcrEncoding = ColorSpace::YcbcrEncoding::Rec2020;
>>>>> + break;
>>>>> + case GST_VIDEO_COLOR_MATRIX_RGB:
>>>> Shouldn't this map to YcbcrEncoding::None ?
>>> YcbcrEncoding for sRGB as per Kernel is V4L2_YCBCR_ENC_SYCC which is
>>> deprecated so V4L2_YCBCR_ENC_601 is used instead [1]
>>>
>>> The same has been applied *already* in libcamera codebase:
>>>
>>> const ColorSpace ColorSpace::Srgb = {
>>> Primaries::Rec709,
>>> TransferFunction::Srgb,
>>> YcbcrEncoding::Rec601,
>>> Range::Limited
>>> };
>>>
>>> So we shouldn't map GST_VIDEO_COLOR_MATRIX_RGB to YcbcrEncoding::None
>>> but to Rec601 instead.
> I'm not sure that's right. GST_VIDEO_COLOR_MATRIX_RGB is defined as
>
> GST_VIDEO_COLOR_MATRIX_RGB (1) – identity matrix. Order of coefficients
> is actually GBR, also IEC 61966-2-1 (sRGB)
>
> I understand this as meaning that the RGB data is not converted to YUV,
> which is expressed in libcamera as YcbcrEncoding::None:
>
> * \var ColorSpace::YcbcrEncoding::None
> * \brief There is no defined Y'CbCr encoding (used for non-YUV formats)
>
> The V4L2 sRGB colorspace [2] definition is confusing. sRGB is an RGB
> colorspace, so it should have no YUV encoding, and use full range. The
> sYCC colorspace is sRGB encoded in YUV using the Rec601 encoding, also
> using full range. As it's typical for devices that produce sRGB images
> to encode them to YUV using the Rec601 encoding and limited range, the
> V4L2 sRGB colorspace defines default encoding and quantization range as
> Rec601 and limited. However, when the pixels are stored as RGB, those
> two parameters don't apply.
>
> Note also how GStreamer defines the SRGB colorspace ([3]):
>
> MAKE_COLORIMETRY (SRGB, _0_255, RGB, SRGB, BT709),
>
> We could do the same and define sRGB as
>
> const ColorSpace ColorSpace::Srgb = {
> Primaries::Rec709,
> TransferFunction::Srgb,
> YcbcrEncoding::None,
> Range::Full
> };
>
> but we would then need to adapt code that produce YUV-encoded sRGB. We
> could define, for that purpose,
>
> const ColorSpace ColorSpace::Sycc = {
> Primaries::Rec709,
> TransferFunction::Srgb,
> YcbcrEncoding::Rec601,
> Range::Full
> };
>
> but that still wouldn't match the V4L2 definition. It's not necessarily
> a problem though, devices can produce { Rec709, Srgb, Rec601, Limited }
> without the need to explicitly define a ColorSpace preset in libcamera.
> I've alos just realized that the above is identical to ColorSpace::Jpeg.
>
> Another option would be to keep the ColorSpace::Srgb definition as-is
> and document that the YCbCrEncoding and Range must be ignored for
> non-YUV formats, but I don't like that much, it sounds confusing.
>
> I'm tempted to bite the bullet and define ColorSpace::Srgb with
> YcbcrEncoding::None and Range::Full, and never return ColorSpace::Srgb
> when capturing YUV data. A pipeline handler that gets ColorSpace::Srgb
> from an application would turn it into { Rec709, Srgb, Rec601, Limited }
> for YUV streams (assuming that the hardware supports that of course).
So I am getting a bit confused here for the part where YUV encoded RGB
data needs to use which range? Limited or Full? You seem to have
referred to limited, it seems - but I don't get the difference.
I guess I'm asking the difference between ColorSpace::Jpeg vs { Rec709,
Srgb, Rec601, Limited } really
The pipeline-handler specific handling on sRGB colorspace is
interesting. To re-iterate and if I am understanding things correctly
(from API point of view):
a) ColorSpace::sRGB preset will be used to present both sRGB and
YUV-encoded RGB colorspace, by the user (i.e. the user doesn't seem to
care/understand)
- libcamera PH being intelligent enough to deduce the stream
configuration and convert ColorSpace::sRGB to { Rec709, Srgb, Rec601,
Limited } and validate + return back to application
- If there is no YUV stream requested, ColorSpace::sRGB preset is
used, as-is.
b) Application intelligent enough that it's requesting a YUV stream and
ColorSpace::sRGB doesn't mean YUV-encoded RGB(which will be true after
we modify sRGB preset)
- In that case, application send in directly { Rec709, Srgb,
Rec601, Limited } as colorspace.
Is this understanding correct on my part? If yes, follow up question is:
- The pipeline-handler colorspace adjustment logic(for YUV streams)
- do you see it per-PH or something generic that all pipeline-handlers
would use ? (I guess the latter)
>
> David, how is ColorSpace::Srgb interpreted by the Raspberry Pi ISP when
> producing YUV data ?
>
> [2] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/colorspaces-details.html#colorspace-srgb-v4l2-colorspace-srgb
> [3] https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-base/gst-libs/gst/video/video-color.c#L66
>
>>> Now two question arises for reverse mapping it i.e. what should:
>>>
>>> a) YcbcrEncding::None map to
>>>
>>> b) YcbcrEncding::Rec601 map to.
>>>
>>> For b) we have two contenders:
>>>
>>> GST_VIDEO_COLOR_MATRIX_BT601
>>> GST_VIDEO_COLOR_MATRIX_RGB
>> b) is currently handled with:
>>
>> + case ColorSpace::YcbcrEncoding::Rec601:
>> + if (colorSpace == ColorSpace::Srgb)
>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_RGB;
>> + else
>> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_BT601;
>> + break;
>>
>> Let's review this in v2 (submitting shortly)
> As per the above, I think YcbcrEncding::None should map to
> GST_VIDEO_COLOR_MATRIX_RGB and YcbcrEncding::Rec601 to
> GST_VIDEO_COLOR_MATRIX_BT601, unconditionally.
>
>>> For a) I think GST_VIDEO_COLOR_MATRIX_RGB would be better ?
>>>
>>> [1] https://git.linuxtv.org/media_tree.git/tree/include/uapi/linux/videodev2.h#n344
>>>
>>>>> + case GST_VIDEO_COLOR_MATRIX_UNKNOWN:
>>>>> + colorspace->ycbcrEncoding = ColorSpace::YcbcrEncoding::Default;
>>>>> + break;
>>>>> + default:
>>>>> + GST_WARNING("Unknown colorimetry matrix %d", colorimetry->matrix);
>>>>> + }
>>>>> +
>>>>> + switch (colorimetry->range) {
>>>>> + case GST_VIDEO_COLOR_RANGE_0_255:
>>>>> + colorspace->range = ColorSpace::Range::Full;
>>>>> + break;
>>>>> + case GST_VIDEO_COLOR_RANGE_16_235:
>>>>> + colorspace->range = ColorSpace::Range::Limited;
>>>>> + break;
>>>>> + case GST_VIDEO_COLOR_RANGE_UNKNOWN:
>>>>> + colorspace->range = ColorSpace::Range::Default;
>>>>> + break;
>>>>> + default:
>>>>> + GST_WARNING("Unknown range in colorimetry %d", colorimetry->range);
>>>>> + }
>>>>> +
>>>>> + return colorspace;
>>>>> +}
>>>>> +
>>>>> static GstVideoFormat
>>>>> pixel_format_to_gst_format(const PixelFormat &format)
>>>>> {
>>>>> @@ -139,6 +290,17 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg
>>>>> "width", G_TYPE_INT, stream_cfg.size.width,
>>>>> "height", G_TYPE_INT, stream_cfg.size.height,
>>>>> nullptr);
>>>>> +
>>>>> + if (stream_cfg.colorSpace) {
>>>>> + GstVideoColorimetry colorimetry = colorimetry_from_colorspace(stream_cfg.colorSpace.value());
>>>>> + gchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);
>>>>> +
>>>>> + if (colorimetry_str != nullptr)
>>>> if (colorimetry_str)
>>>>
>>>>> + gst_structure_set(s, "colorimetry", G_TYPE_STRING, colorimetry_str, nullptr);
>>>>> + else
>>>>> + g_warning("libcamera::ColorSpace found but GstVideoColorimetry unknown");
>>>>> + }
>>>>> +
>>>>> gst_caps_append_structure(caps, s);
>>>>> return caps;
>>>>> @@ -222,6 +384,19 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
>>>>> gst_structure_get_int(s, "height", &height);
>>>>> stream_cfg.size.width = width;
>>>>> stream_cfg.size.height = height;
>>>>> +
>>>>> + /* Configure colorimetry */
>>>>> + if (gst_structure_has_field(s, "colorimetry")) {
>>>>> + const gchar *colorimetry_caps = gst_structure_get_string(s, "colorimetry");
>>>>> + GstVideoColorimetry colorimetry;
>>>>> +
>>>>> + if(gst_video_colorimetry_from_string(&colorimetry, colorimetry_caps)) {
>>>> Missing space after "if".
>>>>
>>>>> + std::optional<ColorSpace> colorSpace = colorspace_from_colorimetry(&colorimetry);
>>>>> + stream_cfg.colorSpace = colorSpace;
>>>> stream_cfg.colorSpace = colorspace_from_colorimetry(&colorimetry);
>>>>
>>>>> + } else {
>>>>> + g_print("Invalid colorimetry %s", colorimetry_caps);
>>>>> + }
>>>>> + }
>>>>> }
>>>>> #if !GST_CHECK_VERSION(1, 17, 1)
More information about the libcamera-devel
mailing list