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