| Message ID | 20220724144355.104978-4-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
> default: > + GST_WARNING("Colorspace YcbcrEncoding not mapped in GstLibcameraSrc"); > + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_UNKNOWN; > + } Back in v1 of colorspace and colorimetry integration nicolas pointed out that we should not use the _UNKNOWN to set the fields. https://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031941.html I am not sure whether we should set the field to _UNKNOWN in the default switch cases. On Sun, Jul 24, 2022 at 8:14 PM Umang Jain <umang.jain@ideasonboard.com> wrote: > > From: Rishikesh Donadkar <rishikeshdonadkar@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@gmail.com> > Signed-off-by: Umang Jain <umang.jain@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; > + > + 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; > + } > + > + 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; > + } > + > + 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) > +{ > + std::optional<ColorSpace> colorspace = ColorSpace::Default; > + > + 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: > + 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) > + 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)) { > + std::optional<ColorSpace> colorSpace = colorspace_from_colorimetry(&colorimetry); > + stream_cfg.colorSpace = colorSpace; > + } else { > + g_print("Invalid colorimetry %s", colorimetry_caps); > + } > + } > } > > #if !GST_CHECK_VERSION(1, 17, 1) > -- > 2.31.1 >
Hi Rishi, On 7/24/22 22:03, Rishikesh Donadkar wrote: >> default: >> + GST_WARNING("Colorspace YcbcrEncoding not mapped in GstLibcameraSrc"); >> + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_UNKNOWN; >> + } > Back in v1 of colorspace and colorimetry integration nicolas pointed > out that we should not use the > _UNKNOWN to set the fields. > https://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031941.html > I am not sure whether we should set the field to _UNKNOWN in the > default switch cases. That's what is in happening here. It's a parsing error hence we use _UNKNOWN along with a warning. This means a identifier exists in the libcamera(probably a new identifier is added to libcamera) but not mapped in gstlibcamerasrc, hence, it shall pop the warning and the mapping for the new identifier needs to be added in gstlibcamerasrc-utils :-) As discussed on the IRC, we are working on case-by-case basis here, so as we get reports of missing identifiers, we shall extend the mappings. Hence the entire mapping/warnings has been set up that way. > > > On Sun, Jul 24, 2022 at 8:14 PM Umang Jain <umang.jain@ideasonboard.com> wrote: >> From: Rishikesh Donadkar <rishikeshdonadkar@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@gmail.com> >> Signed-off-by: Umang Jain <umang.jain@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; >> + >> + 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; >> + } >> + >> + 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; >> + } >> + >> + 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) >> +{ >> + std::optional<ColorSpace> colorspace = ColorSpace::Default; >> + >> + 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: >> + 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) >> + 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)) { >> + std::optional<ColorSpace> colorSpace = colorspace_from_colorimetry(&colorimetry); >> + stream_cfg.colorSpace = colorSpace; >> + } else { >> + g_print("Invalid colorimetry %s", colorimetry_caps); >> + } >> + } >> } >> >> #if !GST_CHECK_VERSION(1, 17, 1) >> -- >> 2.31.1 >>
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@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@gmail.com> > Signed-off-by: Umang Jain <umang.jain@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 ? 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 ? > + 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)
Le dimanche 24 juillet 2022 à 22:03 +0530, Rishikesh Donadkar via libcamera- devel a écrit : > > default: > > + GST_WARNING("Colorspace YcbcrEncoding not mapped in > > GstLibcameraSrc"); > > + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_UNKNOWN; > > + } > > Back in v1 of colorspace and colorimetry integration nicolas pointed > out that we should not use the > _UNKNOWN to set the fields. > https://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031941.html > I am not sure whether we should set the field to _UNKNOWN in the > default switch cases. Its perverse, since the use of a WARNING would mean you want to mitigate the missing mapping, but in some case it will fail to negotiate. I suggest to post an error (perhaps GST_STREAM_ERROR_NOT_IMPLEMENTED ?) Or properly mitigate it. If this should just never be reached, then there is g_assert_not_reached() or g_error() that will abort the program. > > > On Sun, Jul 24, 2022 at 8:14 PM Umang Jain <umang.jain@ideasonboard.com> > wrote: > > > > From: Rishikesh Donadkar <rishikeshdonadkar@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@gmail.com> > > Signed-off-by: Umang Jain <umang.jain@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; > > + > > + 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; > > + } > > + > > + 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; > > + } > > + > > + 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) > > +{ > > + std::optional<ColorSpace> colorspace = ColorSpace::Default; > > + > > + 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: > > + 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) > > + 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)) { > > + std::optional<ColorSpace> colorSpace = > > colorspace_from_colorimetry(&colorimetry); > > + stream_cfg.colorSpace = colorSpace; > > + } else { > > + g_print("Invalid colorimetry %s", colorimetry_caps); > > + } > > + } > > } > > > > #if !GST_CHECK_VERSION(1, 17, 1) > > -- > > 2.31.1 > >
Le dimanche 24 juillet 2022 à 23:48 +0530, Umang Jain via libcamera-devel a écrit : > Hi Rishi, > > On 7/24/22 22:03, Rishikesh Donadkar wrote: > > > default: > > > + GST_WARNING("Colorspace YcbcrEncoding not mapped in GstLibcameraSrc"); > > > + colorimetry.matrix = GST_VIDEO_COLOR_MATRIX_UNKNOWN; > > > + } > > Back in v1 of colorspace and colorimetry integration nicolas pointed > > out that we should not use the > > _UNKNOWN to set the fields. > > https://lists.libcamera.org/pipermail/libcamera-devel/2022-July/031941.html > > I am not sure whether we should set the field to _UNKNOWN in the > > default switch cases. > > > That's what is in happening here. It's a parsing error hence we use > _UNKNOWN along with a warning. > > This means a identifier exists in the libcamera(probably a new > identifier is added to libcamera) but not mapped in gstlibcamerasrc, > hence, it shall pop the warning and the mapping for the new identifier > needs to be added in gstlibcamerasrc-utils :-) > > As discussed on the IRC, we are working on case-by-case basis here, so > as we get reports of missing identifiers, we shall extend the mappings. > Hence the entire mapping/warnings has been set up that way. Perhaps you should add some more info to the trace in you want to "get reports of missing identifiers" ? As said in previous answers, WARNING should come with proper mitigation, UNKNOWN may lead to a pipeline error later, as its an invalid value. > > > > > > On Sun, Jul 24, 2022 at 8:14 PM Umang Jain <umang.jain@ideasonboard.com> wrote: > > > From: Rishikesh Donadkar <rishikeshdonadkar@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@gmail.com> > > > Signed-off-by: Umang Jain <umang.jain@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; > > > + > > > + 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; > > > + } > > > + > > > + 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; > > > + } > > > + > > > + 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) > > > +{ > > > + std::optional<ColorSpace> colorspace = ColorSpace::Default; > > > + > > > + 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: > > > + 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) > > > + 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)) { > > > + std::optional<ColorSpace> colorSpace = colorspace_from_colorimetry(&colorimetry); > > > + stream_cfg.colorSpace = colorSpace; > > > + } else { > > > + g_print("Invalid colorimetry %s", colorimetry_caps); > > > + } > > > + } > > > } > > > > > > #if !GST_CHECK_VERSION(1, 17, 1) > > > -- > > > 2.31.1 > > >
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@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@gmail.com> >> Signed-off-by: Umang Jain <umang.jain@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 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)
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@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@gmail.com> >>> Signed-off-by: Umang Jain <umang.jain@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)
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@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@gmail.com> > >>> Signed-off-by: Umang Jain <umang.jain@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). 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)
Le vendredi 29 juillet 2022 à 17:54 +0300, Laurent Pinchart a écrit : > 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@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@gmail.com> > > > > > Signed-off-by: Umang Jain <umang.jain@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) To add to this, sRGB and GST_VIDEO_COLOR_MATRIX_RGB have absolutely no relationship. Please try not to confuse things. GST_VIDEO_COLOR_MATRIX_RGB is strictly used for the case the matrix is not applicable. Notably for all the RGB type of data. This is clearly a 1 to 1 match with YcbcrEncoding::None. > > 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). > > 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) >
Hi Umang, Have you considered creating your baseline based on v4lsrc existing mapping ? You can check you V4L2 <-> libcamera <-> gstreamer mapping against the v4l2src v4l2 <-> gst mapping ? I don't pretend this will have no issues at all, but at leas the mapping exercise base on V4L2 text have been done there once. https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-good/sys/v4l2/gstv4l2object.c#L2055 regards, Nicolas
Hi Nicolas, (CC'ing David) On Fri, Jul 29, 2022 at 11:29:30AM -0400, Nicolas Dufresne wrote: > Hi Umang, > > Have you considered creating your baseline based on v4lsrc existing mapping ? > You can check you V4L2 <-> libcamera <-> gstreamer mapping against the v4l2src > v4l2 <-> gst mapping ? I don't pretend this will have no issues at all, but at > leas the mapping exercise base on V4L2 text have been done there once. > > https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-good/sys/v4l2/gstv4l2object.c#L2055 Thanks for the pointer. That logic seems sensible, and likely something we'll want to do in V4L2Device::fromColorSpace() and V4L2Device::toColorSpace().
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@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@gmail.com> >>>>> Signed-off-by: Umang Jain <umang.jain@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)
Hi Umang, On Mon, Aug 01, 2022 at 04:38:43PM +0530, Umang Jain wrote: > On 7/29/22 20:24, Laurent Pinchart wrote: > > 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: > >>> On 7/27/22 06:47, Laurent Pinchart wrote: > >>>> On Sun, Jul 24, 2022 at 08:13:55PM +0530, Umang Jain via > >>>> libcamera-devel wrote: > >>>>> From: Rishikesh Donadkar <rishikeshdonadkar@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@gmail.com> > >>>>> Signed-off-by: Umang Jain <umang.jain@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 difference is that, after conversion from RGB to YUV using the Rec601 matrix, the Y and U/V values will be limited to [16, 235] and [16, 240] respectively (by shifting and scaling them to that range). sYCC mandates usage of limited range, but many devices that use the sRGB color space encoded in YUV produce full range instead, which led to the naming of the informal JPEG color space. > 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) To be clear, assuming we will redefine sRGB the same way as GStreamer, const ColorSpace ColorSpace::Srgb = { Primaries::Rec709, TransferFunction::Srgb, YcbcrEncoding::None, Range::Full }; then it can represent RGB data only. If an application picks it to capture RGB data, all is fine. If an application picks it to capture YUV data, that's technically wrong, but libcamera should adjust it as our color space API requires pipeline handlers to adjust color spaces requested by applications and not supported by the device. > - 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 Nearly. I think the logic should check the transfer function, and if it's Srgb and the encoding is None for a YUV format, it should pick Rec601 as the encoding. Actually, looking at the presets we have, something like the following could work: if (format == YUV && encoding == None) { if (transferFunction == Rec709) { switch (primaries) { case Raw: case Smpte170m: encoding = Rec601; break; case Rec709: encoding = Rec709; break; case Rec2020: encoding = Rec2020; break; } } else { encoding = Rec601; } } (the Raw case should never happen, I've picked a default encoding in that case.) I'm sure there are other ways to express the logic, looking at the GStreamer v4l2src element could help. It would also be nice to centralize this kind of adjustement logic somewhere (possibly in the ColorSpace class). I said "nearly" above as I'm not sure about the default range we should select (limited vs. full). sYCC is limited, JPEG is full. Should we assume that an application that incorrectly asks for ColorSpace::Srgb for a YUV stream will actually want sYCC or JPEG ? > - If there is no YUV stream requested, ColorSpace::sRGB preset is > used, as-is. Right. > 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. That, or the same with Full range, depending on what the application wants. > Is this understanding correct on my part? If yes, follow up question is: I think so. > - 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) I think I've answered that above :-) I'd like to centralize as much logic as possible, and at least the logic that picks a default YCbCr encoding for YUV formats. For the rest, the adjustment also depends on what the hardware supports, so fully generic helpers may not be possible. Even if we can't implement the adjustment code in shared code that would work with all pipeline handlers, I would like to document the adjustment heuristics to have consistent behaviour across pipeline handlers. > > 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)
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; + + 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; + } + + 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; + } + + 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) +{ + std::optional<ColorSpace> colorspace = ColorSpace::Default; + + 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: + 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) + 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)) { + std::optional<ColorSpace> colorSpace = colorspace_from_colorimetry(&colorimetry); + stream_cfg.colorSpace = colorSpace; + } else { + g_print("Invalid colorimetry %s", colorimetry_caps); + } + } } #if !GST_CHECK_VERSION(1, 17, 1)