[libcamera-devel] [PATCH v2 2/2] gstreamer: Add multiple colorimetry support
Umang Jain
umang.jain at ideasonboard.com
Wed Aug 10 07:50:08 CEST 2022
Hi Rishikesh,
On 8/9/22 20:05, Rishikesh Donadkar wrote:
> This patch provides the following:
> - Add support to handle colorimetry in the GStreamer->libcamera
> direction.
> - Add support for multiple colorimetry.
>
> The function colorspace_from_colorimetry() takes in a
> GstVideoColorimetry and returns libcamera::ColorSpace
>
> Append copy of the structure with best suitable resolution into a
> fresh new caps and normalize the caps using gst_caps_normalize().
> This will return caps where the colorimetry list is expanded.
> The ncaps will contain as many structures as the number of
> colorimetry specified in the GStreamer pipeline.
>
> Iterate over each structure in the ncaps, retrieve the colorimetry string,
> convert to colorspace using the function colorspace_from_colorimetry()
> and validate the camera configuration.Retrieve the colorspace after
> validation, convert to colorimetry and check if the it is same as
> the colorimetry requested.
>
> If none of the colorimetry requested is supported by the camera (i.e. not
> the same after validation) then set the stream_cfg to the previous
> configuration that was present before trying new colorimetry.
>
> Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar at gmail.com>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
> src/gstreamer/gstlibcamera-utils.cpp | 152 ++++++++++++++++++++++++++-
> src/gstreamer/gstlibcamera-utils.h | 4 +-
> src/gstreamer/gstlibcamerasrc.cpp | 3 +-
> 3 files changed, 153 insertions(+), 6 deletions(-)
>
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index dbb47c5a..078b9343 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -104,6 +104,93 @@ colorimetry_from_colorspace(const ColorSpace &colorSpace)
> return colorimetry;
> }
>
> +static ColorSpace
> +colorspace_from_colorimetry(const GstVideoColorimetry &colorimetry)
As mentioned in my earlier reply, this is a part of colorimetry <>
libcamera::Colorspace mappings which are currently handled in
[v3 PATCH 4/4] gstreamer: Provide colorimetry <> ColorSpace mappings
Can you please rebase over that series and provide a true multiple
colorimetry support patch ? Which which then be easier to review
(because it will contain multiple colorimetry bits only, replacing the
single colorimetry handling) hence I believe there will some code sharing.
Meanwhile I have left a comments below which you can incorporate in next
version.
> +{
> + ColorSpace colorspace = ColorSpace::Raw;
> +
> + switch (colorimetry.primaries) {
> + case GST_VIDEO_COLOR_PRIMARIES_UNKNOWN:
> + /* Unknown primaries map to raw colorspace in GStreamer */
> + return ColorSpace::Raw;
> + case GST_VIDEO_COLOR_PRIMARIES_SMPTE170M:
> + colorspace.primaries = ColorSpace::Primaries::Smpte170m;
> + break;
> + case GST_VIDEO_COLOR_PRIMARIES_BT709:
> + colorspace.primaries = ColorSpace::Primaries::Rec709;
> + break;
> + case GST_VIDEO_COLOR_PRIMARIES_BT2020:
> + colorspace.primaries = ColorSpace::Primaries::Rec2020;
> + break;
> + default:
> + GST_WARNING("Colorimetry primaries %d not mapped in gstlibcamera",
> + colorimetry.primaries);
> + return ColorSpace::Raw;
> + }
> +
> + switch (colorimetry.transfer) {
> + /* Transfer 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_SRGB:
> + colorspace.transferFunction = ColorSpace::TransferFunction::Srgb;
> + 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;
> + default:
> + GST_WARNING("Colorimetry transfer function %d not mapped in gstlibcamera",
> + colorimetry.transfer);
> + return ColorSpace::Raw;
> + }
> +
> + switch (colorimetry.matrix) {
> + case GST_VIDEO_COLOR_MATRIX_RGB:
> + colorspace.ycbcrEncoding = ColorSpace::YcbcrEncoding::None;
> + break;
> + /* FCC is about the same as BT601 with less digits */
> + 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;
> + default:
> + GST_WARNING("Colorimetry matrix %d not mapped in gstlibcamera",
> + colorimetry.matrix);
> + return ColorSpace::Raw;
> + }
> +
> + 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;
> + default:
> + GST_WARNING("Colorimetry range %d not mapped in gstlibcamera",
> + colorimetry.range);
> + return ColorSpace::Raw;
> + }
> +
> + return colorspace;
> +}
> +
> static GstVideoFormat
> pixel_format_to_gst_format(const PixelFormat &format)
> {
> @@ -215,13 +302,47 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg
> return caps;
> }
>
> +static void
> +configure_colorspace_from_caps(StreamConfiguration &stream_cfg,
> + GstStructure *s)
> +{
> + if (gst_structure_has_field(s, "colorimetry")) {
> + const gchar *colorimetry_str = gst_structure_get_string(s, "colorimetry");
> + GstVideoColorimetry colorimetry;
> +
> + if (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str))
> + g_critical("Invalid colorimetry %s", colorimetry_str);
> +
> + stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry);
> + /* Check if colorimetry had any identifiers which did not map */
> + if (colorimetry.primaries != GST_VIDEO_COLOR_PRIMARIES_UNKNOWN &&
> + stream_cfg.colorSpace == ColorSpace::Raw) {
> + GST_ERROR("One or more identifiers could not be mapped for %s colorimetry",
> + colorimetry_str);
> + stream_cfg.colorSpace = std::nullopt;
> + }
The rebase will reflect that this code block as been 'moved', which is
not quite apparent right now.
> + }
> +}
> +
> +static gboolean
> +check_colorspace(const ColorSpace colorSpace, const gchar *colorimetry_old)
More suitable : 'compare_colorspace(old, new)'
> +{
> + GstVideoColorimetry colorimetry = colorimetry_from_colorspace(colorSpace);
> + g_autofree gchar *colorimetry_new = gst_video_colorimetry_to_string(&colorimetry);
> + if (!g_strcmp0(colorimetry_old, colorimetry_new)) {
Have you tried using the comparator gst_video_colorimetry_is_equal() ? I
guess that shall be more appropriate than comparing strings?
> + return true;
> + }
No need for {} as only single line after "if"
> + return false;
> +}
> +
> void
> -gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
> +gst_libcamera_configure_stream_from_caps(CameraConfiguration &cam_cfg,
> + StreamConfiguration &stream_cfg,
> GstCaps *caps)
> {
> GstVideoFormat gst_format = pixel_format_to_gst_format(stream_cfg.pixelFormat);
> guint i;
> - gint best_fixed = -1, best_in_range = -1;
> + gint best_fixed = -1, best_in_range = -1, colorimetry_index = -1;
> GstStructure *s;
>
> /*
> @@ -267,10 +388,13 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
> }
>
> /* Prefer reliable fixed value over ranges */
> - if (best_fixed >= 0)
> + if (best_fixed >= 0) {
> s = gst_caps_get_structure(caps, best_fixed);
> - else
> + colorimetry_index = best_fixed;
> + } else {
> s = gst_caps_get_structure(caps, best_in_range);
> + colorimetry_index = best_in_range;
> + }
>
> if (gst_structure_has_name(s, "video/x-raw")) {
> const gchar *format = gst_video_format_to_string(gst_format);
> @@ -293,6 +417,26 @@ 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;
> +
> + /* Create new caps, copy the structure with best resolutions
> + * and normalize the caps.
> + */
> + GstCaps *ncaps = gst_caps_copy_nth(caps, colorimetry_index);
> + ncaps = gst_caps_normalize(ncaps);
> +
> + /* Configure Colorimetry */
> + StreamConfiguration dup_stream_cfg = stream_cfg;
This is used when no colorimetry candidate is found, so probably rename
as pristine_stream_cfg?
> + for (i = 0; i < gst_caps_get_size(ncaps); i++) {
> + GstStructure *ns = gst_caps_get_structure(ncaps, i);
> + configure_colorspace_from_caps(stream_cfg, ns);
> + g_autofree const gchar *colorimetry_old = gst_structure_get_string(ns, "colorimetry");
s/colorimetry_old/colorimetry
> + if (cam_cfg.validate() != CameraConfiguration::Invalid) {
> + if (check_colorspace(stream_cfg.colorSpace.value(), colorimetry_old))
> + break;
Should we log a INFO log here: "Selected colorimetry: <candidate>" ?
> + else
> + stream_cfg = dup_stream_cfg;
> + }
> + }
> }
>
> #if !GST_CHECK_VERSION(1, 17, 1)
> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h
> index 164189a2..90be6abe 100644
> --- a/src/gstreamer/gstlibcamera-utils.h
> +++ b/src/gstreamer/gstlibcamera-utils.h
> @@ -8,6 +8,7 @@
>
> #pragma once
>
> +#include <libcamera/camera.h>
> #include <libcamera/camera_manager.h>
> #include <libcamera/stream.h>
>
> @@ -16,7 +17,8 @@
>
> GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats);
> GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg);
> -void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg,
> +void gst_libcamera_configure_stream_from_caps(libcamera::CameraConfiguration &cam_cfg,
> + libcamera::StreamConfiguration &stream_cfg,
> GstCaps *caps);
> #if !GST_CHECK_VERSION(1, 17, 1)
> gboolean gst_task_resume(GstTask *task);
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 16d70fea..3617170e 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -492,6 +492,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
> for (gsize i = 0; i < state->srcpads_.size(); i++) {
> GstPad *srcpad = state->srcpads_[i];
> StreamConfiguration &stream_cfg = state->config_->at(i);
> + CameraConfiguration &cam_cfg = *(state->config_);
>
> /* Retrieve the supported caps. */
> g_autoptr(GstCaps) filter = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());
> @@ -503,7 +504,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
>
> /* Fixate caps and configure the stream. */
> caps = gst_caps_make_writable(caps);
> - gst_libcamera_configure_stream_from_caps(stream_cfg, caps);
> + gst_libcamera_configure_stream_from_caps(cam_cfg, stream_cfg, caps);
You can directly pass state->config_ here instead of taking it in a
holder variable reference cam_cfg
> }
>
> if (flow_ret != GST_FLOW_OK)
More information about the libcamera-devel
mailing list