[libcamera-devel] [PATCH 3/3] gstreamer: Provide colorimetry <> libcamera::ColorSpace mappings
Umang Jain
umang.jain at ideasonboard.com
Fri Jul 29 12:10:52 CEST 2022
Hello,
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.
>
> 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)
>
> 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