[libcamera-devel] [PATCH v2 2/2] gstreamer: Add multiple colorimetry support
Rishikesh Donadkar
rishikeshdonadkar at gmail.com
Thu Aug 11 12:22:56 CEST 2022
>
> 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.
>
Sure.
> 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)) {
Noted.
> Have you tried using the comparator gst_video_colorimetry_is_equal() ? I
> guess that shall be more appropriate than comparing strings?
>
> > + return true;
> > + }
>
Thanks for the pointer. Using this comparator eliminated the need for the
compare_colorspace() function.
> > + /* 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?
Right.
> > + 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>" ?
>
Yes, that would be quite useful.
> > 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
Noted.
On Wed, Aug 10, 2022 at 11:20 AM Umang Jain <umang.jain at ideasonboard.com>
wrote:
> 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)
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220811/519a9dd2/attachment.htm>
More information about the libcamera-devel
mailing list