[libcamera-devel] [PATCH v6 1/1] gstreamer: Provide multiple colorimetry support for libcamerasrc
Rishikesh Donadkar
rishikeshdonadkar at gmail.com
Mon Sep 26 06:45:53 CEST 2022
Hello Umang and Laurent,
On Tue, Sep 20, 2022 at 12:18 AM Umang Jain <umang.jain at ideasonboard.com> wrote:
>
> Hi Laurent,
>
> Thank you for the comments
> On 9/19/22 11:22 PM, Laurent Pinchart wrote:
> > Hi Rishikesh,
> >
> > Thank you for the patch.
> >
> > On Mon, Sep 19, 2022 at 08:27:09AM +0530, Rishikesh Donadkar wrote:
> >> Iterate over each GValue in the colorimetry list,
> >> retrieve the colorimetry string, convert to colorspace,
> >> set the colorspace in the stream_cfg and validate the camera configuration.
> >> Retrieve the colorspace after validation, convert to colorimetry and
> >> check if 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>
> >> Reviewed-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> >> ---
> >> src/gstreamer/gstlibcamera-utils.cpp | 49 +++++++++++++++++++++++-----
> >> src/gstreamer/gstlibcamera-utils.h | 4 ++-
> >> src/gstreamer/gstlibcamerasrc.cpp | 2 +-
> >> 3 files changed, 44 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> >> index 244a4a79..f73b6349 100644
> >> --- a/src/gstreamer/gstlibcamera-utils.cpp
> >> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> >> @@ -316,9 +316,9 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg
> >> return caps;
> >> }
> >>
> >> -void
> >> -gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
> >> - GstCaps *caps)
> >> +void gst_libcamera_configure_stream_from_caps(std::unique_ptr<CameraConfiguration> &cam_cfg,
> >> + StreamConfiguration &stream_cfg,
> >> + GstCaps *caps)
> >> {
> >> GstVideoFormat gst_format = pixel_format_to_gst_format(stream_cfg.pixelFormat);
> >> guint i;
> >> @@ -395,15 +395,46 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
> >> stream_cfg.size.width = width;
> >> stream_cfg.size.height = height;
> >>
> >> - /* Configure colorimetry */
> >> - if (gst_structure_has_field(s, "colorimetry")) {
> >> - const gchar *colorimetry_str = gst_structure_get_string(s, "colorimetry");
> >> - GstVideoColorimetry colorimetry;
> >> + const GValue *colorimetry_ = gst_structure_get_value(s, "colorimetry");
> > Trailing underscores are reserved for class members.
> >
> >> + if (!colorimetry_)
> >> + return;
> >>
> >> - if (!gst_video_colorimetry_from_string(&colorimetry, colorimetry_str))
> >> - g_critical("Invalid colorimetry %s", colorimetry_str);
> >> + /* Configure Colorimetry */
> >> + GstVideoColorimetry colorimetry;
> >> + if (GST_VALUE_HOLDS_LIST(colorimetry_)) {
> >> + StreamConfiguration pristine_stream_cfg = stream_cfg;
> >> + for (i = 0; i < gst_value_list_get_size(colorimetry_); i++) {
> >> + const GValue *val = gst_value_list_get_value(colorimetry_, i);
> >> +
> >> + if (!gst_video_colorimetry_from_string(&colorimetry, g_value_get_string(val))) {
> >> + g_warning("Invalid colorimetry %s, trying next option", g_value_get_string(val));
> >> + continue;
> >> + }
> >> +
> >> + std::optional<ColorSpace> clrSpace = colorspace_from_colorimetry(colorimetry);
> >> + stream_cfg.colorSpace = clrSpace;
> >> +
> >> + /* Validate the configuration and check if the requested
> > Wrong style. On this topic, lines are too long. Line wraps can help, but
> > it may also indicate that some code could be split to separate functions
> > (I haven't checked).
Ack.
>
> >
> >> + * colorimetry can be applied to the sensor. Otherwise,
> >> + * restore the configuration back to pristine one.
> >> + */
> >> + if (cam_cfg->validate() != CameraConfiguration::Invalid) {
> > The RPi pipeline handler (and other pipeline handlers) requires the same
> > color space to be used for all streams, and it will pick (if I recall
> > correctly) the color space of the largest non-raw stream and apply it to
> > all other streams. For multi-stream use cases, validating the color
> > space on streams individually will probably not work as you're
> > expecting. You'll need a more complex logic.
>
> We indeed had a discussion around it I think but I think it was my idea
> to develop on top of this actually.
>
> Rishikesh, what do you think about handling this? We can have a
> exploratory session together as I think I haven't been much exposed to
> multi-stream usage via gstreamer plugin.
Sure, even I have not been much exposed to multi-stream usage, maybe
Vedant might be able to help with this.
>
> >
> >> + /* Check the colorspace returned after validation, with the colorspace before validation */
> >> + if (stream_cfg.colorSpace == clrSpace) {
> >> + g_print("Selected colorimetry %s\n", gst_video_colorimetry_to_string(&colorimetry));
> >> + break;
> >> + } else {
> >> + stream_cfg = pristine_stream_cfg;
> >> + }
> >> + }
> >> + }
> >> + } else if (G_VALUE_HOLDS_STRING(colorimetry_)) {
> >> + if (!gst_video_colorimetry_from_string(&colorimetry, g_value_get_string(colorimetry_)))
> >> + g_critical("Invalid colorimetry %s", g_value_get_string(colorimetry_));
> >>
> >> stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry);
> > Why does this not use validate() ? The asymmetry between the two makes
> > me think something is wrong.
>
> For single colorimetry, the validate happens right after the function's
> call in src/gstreamer/gstlibcamerasrc.cpp and then whatever is returned
> back, is exposed as caps. If the user-defined colorimetry as whatever is
> exposed in caps, negotiation shall pass.
>
> For multiple colorimetry, we need to "select" a supported colorspace
> candidate from the user-provided list, hence need to run through
> validate() to see what sensor agrees on any-one of them. Which is found
> to be suitable, is set in the stream configuration and re-validated
> (although futile) in
> src/gstreamer/gstlibcamerasrc.cpp::gst_libcamera_src_task_enter() and
> then exposed through caps.
>
> Hence, an extra validation is not required for the single-colorimetry
> case (which currently is the behaviour on master as well). Does that
> makes sense?
>
> >
> >> + } else {
> >> + GST_WARNING("colorimetry field type should only be string or list.");
> >> }
> >> }
> >>
> >> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h
> >> index 164189a2..7afad576 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(std::unique_ptr<libcamera::CameraConfiguration> &cam_cfg,
> > Let's pass a normal reference to the function:
> >
> > void gst_libcamera_configure_stream_from_caps(libcamera::CameraConfiguration &cam_cfg,
>
> ah good point, ack
> >
> >> + 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..25059914 100644
> >> --- a/src/gstreamer/gstlibcamerasrc.cpp
> >> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> >> @@ -503,7 +503,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(state->config_, stream_cfg, caps);
> > gst_libcamera_configure_stream_from_caps(*state->config_, stream_cfg, caps);
> >
> >> }
> >>
> >> if (flow_ret != GST_FLOW_OK)
>
Regards,
Rishikesh Donadkar
More information about the libcamera-devel
mailing list