| Message ID | 20220907104711.87365-1-rishikeshdonadkar@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Rishi, On 9/7/22 4:17 PM, Rishikesh Donadkar wrote: > Add a field of the type ControlList to GstLibcameraSrcState. > > Get the framerate from the caps and convert it to FrameDuration. > Set the FrameDurationLimits control in the initControls_ > > Passing initControls_ at camera::start() doesn't tell us if the controls > have been applied to the camera successfully or not (or they have adjusted in > some fashion). Until that is introduced in camera::start() API, > we need to carry out such handling on the application side. > > Check the frame_duration whether it is in-bound to the max / min > frame-duration supported by the camera using the function > gst_libcamera_bind_framerate(). > > If the frame_duartion is out of bounds, bind the frame_duration > between the max and min FrameDuration that is supported by the camera. > > Pass the initControls_ at the time of starting camera. > > Solve the complications in exposing the correct framerate due to loss in > precision as a result of casting the frameduration to int64_t(which is > required in libcamera to set the FrameDurationLimits control). > > Example - > > * Suppose the framerate requested is 35/1. The framerate is read form > the caps in the from of fraction that has a numerator and > denominator. > > * Converting to FrameDuration (in microseconds) > (1 * 10^6) / 35 = 28571.4286 > int64_t frame_duration = 28571 > (the precision here is lost.) > * To expose the framerate in caps, Inverting the frame_duration to get > back the framerate and converting to Seconds. > double framerate = 10^6 / 28571 > and > 10^6/28571 which is close but not equal to 35/1 will fail the negotiation. > > To solve the above problem, Store the framerate requested through > negotiated caps in the GValue of the type GST_TYPE_FRACTION. > > Try to apply the framerate and check if the floor value of framerate that gets applied > closely matches with the floor value framerate requested in the negotiated caps. If > the value matches expose the framerate that is stored in the framerate_container, else expose > the framerate in the new caps as 0/1. > > Pass the initControls_ at camera->start(). > > --- > Changes from v1 to v2 > * Use GValue of the type GST_TYPE_FRACTION instead of std::pair to store the framerate. > * Don't shift the call to camera->configure(), instead bound the framerate > after the call to camera->configure(). > * Reset the FrameDurationLimits control only if it is out of bounds. > * Expose the caps containing framerate information with a new CAPS event > after the call to camera->configure(). > --- > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> As I haven't contributed to any LOC of this patch, please drop of my s-o-b tag. Non-existent s-o-b on behalf of others shouldn't be applied ideally > Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com> > --- > src/gstreamer/gstlibcamera-utils.cpp | 116 +++++++++++++++++++++++++++ > src/gstreamer/gstlibcamera-utils.h | 8 ++ > src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++- > 3 files changed, 151 insertions(+), 1 deletion(-) > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > index 4df5dd6c..e862f7ea 100644 > --- a/src/gstreamer/gstlibcamera-utils.cpp > +++ b/src/gstreamer/gstlibcamera-utils.cpp > @@ -8,6 +8,7 @@ > > #include "gstlibcamera-utils.h" > > +#include <libcamera/control_ids.h> > #include <libcamera/formats.h> > > using namespace libcamera; > @@ -405,6 +406,121 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > } > } > > +void > +gst_libcamera_get_init_controls_from_caps(ControlList &controls, [[maybe_unused]] GstCaps *caps, > + GValue *framerate_container) > +{ > + GstStructure *s = gst_caps_get_structure(caps, 0); > + gint fps_n = -1, fps_d = -1; > + > + if (gst_structure_has_field_typed(s, "framerate", GST_TYPE_FRACTION)) { > + if (!gst_structure_get_fraction(s, "framerate", &fps_n, &fps_d)) { > + GST_WARNING("invalid framerate in the caps."); > + return; > + } > + } > + > + if (fps_n < 0 || fps_d < 0) > + return; > + > + gst_value_set_fraction(framerate_container, fps_n, fps_d); > + int64_t frame_duration = (fps_d * 1000000.0) / fps_n; > + > + controls.set(controls::FrameDurationLimits, > + Span<const int64_t, 2>({ frame_duration, frame_duration })); setting the frame_duration even before bound-checking is wrong. This Function should ideally get the 'framerate' and just save it for later use/bound-checking after camera::configure() Once the bound checking is successfully, only then you are allowed to set frame_duration on initControls_ > +} > + > +void > +gst_libcamera_bind_framerate(const ControlInfoMap &camera_controls, ControlList &controls) maybe gst_libcamera_framerate_clamp() OR gst_libcamera_framerate_bounds_check() > +{ > + gboolean isBound = true; > + auto iterFrameDuration = camera_controls.find(controls::FrameDurationLimits.id()); > + > + if (!(iterFrameDuration != camera_controls.end() && > + controls.contains(controls::FRAME_DURATION_LIMITS))) > + return; > + > + int64_t frame_duration = controls.get(controls::FrameDurationLimits).value()[0]; > + int64_t min_frame_duration = iterFrameDuration->second.min().get<int64_t>(); > + int64_t max_frame_duration = iterFrameDuration->second.max().get<int64_t>(); > + > + if (frame_duration < min_frame_duration) { > + frame_duration = min_frame_duration; > + } else if (frame_duration > max_frame_duration) { > + frame_duration = max_frame_duration; > + } else { > + isBound = false; > + } > + > + if (isBound) > + controls.set(controls::FrameDurationLimits, > + Span<const int64_t, 2>({ frame_duration, frame_duration })); > +} > + > +gboolean > +gst_libcamera_get_framerate_from_init_controls(const ControlList &controls, GValue *framerate, > + GValue *framerate_container) > +{ > + gint fps_caps_n, fps_caps_d, numerator, denominator; > + > + fps_caps_n = gst_value_get_fraction_numerator(framerate_container); > + fps_caps_d = gst_value_get_fraction_denominator(framerate_container); > + > + if (!controls.contains(controls::FRAME_DURATION_LIMITS)) > + return false; > + > + double framerate_ret = 1000000 / static_cast<double>(controls.get(controls::FrameDurationLimits).value()[0]); > + gst_util_double_to_fraction(framerate_ret, &numerator, &denominator); > + gst_value_set_fraction(framerate, numerator, denominator); > + > + if (fps_caps_n / fps_caps_d == numerator / denominator) { > + return true; > + } > + return false; > +} > + > +GstCaps * > +gst_libcamera_init_controls_to_caps(const ControlList &controls, const StreamConfiguration &stream_cfg, > + GValue *framerate_container) > +{ > + GstCaps *caps = gst_caps_new_empty(); > + GstStructure *s = bare_structure_from_format(stream_cfg.pixelFormat); > + gboolean ret; > + GValue *framerate = g_new0(GValue, 1); Why create a new GValue here? You have everything you need through framerate_container, a applied and bound-checked framerate, you just need to query the fraction from it and set the structure? > + g_value_init(framerate, GST_TYPE_FRACTION); > + > + ret = gst_libcamera_get_framerate_from_init_controls(controls, framerate, framerate_container); As per the above comment, I am not sure if you really need to do that. Seems over-kill > + > + gst_structure_set(s, > + "width", G_TYPE_INT, stream_cfg.size.width, > + "height", G_TYPE_INT, stream_cfg.size.height, > + nullptr); > + > + if (ret) { > + gint numerator, denominator; > + numerator = gst_value_get_fraction_numerator(framerate_container); > + denominator = gst_value_get_fraction_denominator(framerate_container); > + gst_structure_set(s, "framerate", GST_TYPE_FRACTION, numerator, denominator, nullptr); > + } else { > + gst_structure_set(s, "framerate", GST_TYPE_FRACTION, 0, 1, nullptr); > + } > + > + if (stream_cfg.colorSpace) { ?! I am not getting why you are introducing code related to colorspace here? Is it a copy/paste fiasco? > + GstVideoColorimetry colorimetry = colorimetry_from_colorspace(stream_cfg.colorSpace.value()); > + gchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry); > + > + if (colorimetry_str) > + gst_structure_set(s, "colorimetry", G_TYPE_STRING, colorimetry_str, nullptr); > + else > + g_error("Got invalid colorimetry from ColorSpace: %s", > + ColorSpace::toString(stream_cfg.colorSpace).c_str()); > + } > + > + gst_caps_append_structure(caps, s); > + g_free(framerate); > + return caps; > +} > + > #if !GST_CHECK_VERSION(1, 17, 1) > gboolean > gst_task_resume(GstTask *task) > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h > index 164189a2..955a717d 100644 > --- a/src/gstreamer/gstlibcamera-utils.h > +++ b/src/gstreamer/gstlibcamera-utils.h > @@ -9,6 +9,7 @@ > #pragma once > > #include <libcamera/camera_manager.h> > +#include <libcamera/controls.h> > #include <libcamera/stream.h> > > #include <gst/gst.h> > @@ -18,6 +19,13 @@ GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &fo > GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg); > void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg, > GstCaps *caps); > +void gst_libcamera_get_init_controls_from_caps(libcamera::ControlList &controls, > + GstCaps *caps, GValue *framerate_container); > +void gst_libcamera_bind_framerate(const libcamera::ControlInfoMap &camera_controls, > + libcamera::ControlList &controls); > +GstCaps *gst_libcamera_init_controls_to_caps(const libcamera::ControlList &controls, > + const libcamera::StreamConfiguration &streame_cfg, GValue *framerate_container); > + > #if !GST_CHECK_VERSION(1, 17, 1) > gboolean gst_task_resume(GstTask *task); > #endif > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 16d70fea..0c981357 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -131,6 +131,7 @@ struct GstLibcameraSrcState { > std::queue<std::unique_ptr<RequestWrap>> completedRequests_ > LIBCAMERA_TSA_GUARDED_BY(lock_); > > + ControlList initControls_; > guint group_id_; > > int queueRequest(); > @@ -461,6 +462,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > GstLibcameraSrcState *state = self->state; > GstFlowReturn flow_ret = GST_FLOW_OK; > gint ret; > + GValue *framerate_container = g_new0(GValue, 1); > + framerate_container = g_value_init(framerate_container, GST_TYPE_FRACTION); Since it's a GValue, I would not make it framerate_* specific, so I would just rename and remove all "framerate_" specifics. In future, when we have to parse more parameters as a part of initControls_ setting, the same GValue can be used to temporarily hold all other values > > GST_DEBUG_OBJECT(self, "Streaming thread has started"); > > @@ -496,6 +499,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > /* Retrieve the supported caps. */ > g_autoptr(GstCaps) filter = gst_libcamera_stream_formats_to_caps(stream_cfg.formats()); > g_autoptr(GstCaps) caps = gst_pad_peer_query_caps(srcpad, filter); > + > if (gst_caps_is_empty(caps)) { > flow_ret = GST_FLOW_NOT_NEGOTIATED; > break; > @@ -504,6 +508,8 @@ 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_get_init_controls_from_caps(state->initControls_, caps, > + framerate_container); passing state->initControls_ is un-necessary here as we don't need to set frame_duration on it, just right now (it has to go through bound check first which is below...) > } > > if (flow_ret != GST_FLOW_OK) > @@ -524,6 +530,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > const StreamConfiguration &stream_cfg = state->config_->at(i); > > g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg); > + > if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) { > flow_ret = GST_FLOW_NOT_NEGOTIATED; > break; > @@ -544,6 +551,23 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > return; > } > > + /* bind the framerate */ s/bind/Clamp OR /* Check framerate bounds within controls::FrameDurationLimits */ > + gst_libcamera_bind_framerate(state->cam_->controls(), state->initControls_); I would just pass the controls::FrameDurationLimits and GValue (containing the asked framerate) and perform the bound check. You can update the GValue's framerate in that function after which simply, set the initControls' ::FrameDurationLimits right here. > + > + /* Expose the controls in initControls_ throught a new caps event. */ > + for (gsize i = 0; i < state->srcpads_.size(); i++) { > + GstPad *srcpad = state->srcpads_[i]; > + const StreamConfiguration &stream_cfg = state->config_->at(i); > + > + g_autoptr(GstCaps) caps = gst_libcamera_init_controls_to_caps(state->initControls_, > + stream_cfg, framerate_container); > + > + if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) { > + flow_ret = GST_FLOW_NOT_NEGOTIATED; > + break; > + } > + } > + > self->allocator = gst_libcamera_allocator_new(state->cam_, state->config_.get()); > if (!self->allocator) { > GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT, > @@ -566,7 +590,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > gst_flow_combiner_add_pad(self->flow_combiner, srcpad); > } > > - ret = state->cam_->start(); > + ret = state->cam_->start(&state->initControls_); > if (ret) { > GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, > ("Failed to start the camera: %s", g_strerror(-ret)), > @@ -576,6 +600,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, > } > > done: > + state->initControls_.clear(); > + g_free(framerate_container); > switch (flow_ret) { > case GST_FLOW_NOT_NEGOTIATED: > GST_ELEMENT_FLOW_ERROR(self, flow_ret);
> > > + > > + gst_structure_set(s, > > + "width", G_TYPE_INT, stream_cfg.size.width, > > + "height", G_TYPE_INT, stream_cfg.size.height, > > + nullptr); > > + > > + if (ret) { > > + gint numerator, denominator; > > + numerator = > gst_value_get_fraction_numerator(framerate_container); > > + denominator = > gst_value_get_fraction_denominator(framerate_container); > > + gst_structure_set(s, "framerate", GST_TYPE_FRACTION, > numerator, denominator, nullptr); > > + } else { > > + gst_structure_set(s, "framerate", GST_TYPE_FRACTION, 0, 1, > nullptr); > > + } > > + > > + if (stream_cfg.colorSpace) { > > ?! > > I am not getting why you are introducing code related to colorspace > here? Is it a copy/paste fiasco? > As new caps are exposed. Shouldn't we expose everything that has been decided on the libcamera side that includes colorspace, resolutions, format and framerate? On Thu, Sep 8, 2022 at 11:15 AM Umang Jain <umang.jain@ideasonboard.com> wrote: > Hi Rishi, > > On 9/7/22 4:17 PM, Rishikesh Donadkar wrote: > > Add a field of the type ControlList to GstLibcameraSrcState. > > > > Get the framerate from the caps and convert it to FrameDuration. > > Set the FrameDurationLimits control in the initControls_ > > > > Passing initControls_ at camera::start() doesn't tell us if the controls > > have been applied to the camera successfully or not (or they have > adjusted in > > some fashion). Until that is introduced in camera::start() API, > > we need to carry out such handling on the application side. > > > > Check the frame_duration whether it is in-bound to the max / min > > frame-duration supported by the camera using the function > > gst_libcamera_bind_framerate(). > > > > If the frame_duartion is out of bounds, bind the frame_duration > > between the max and min FrameDuration that is supported by the camera. > > > > Pass the initControls_ at the time of starting camera. > > > > Solve the complications in exposing the correct framerate due to loss in > > precision as a result of casting the frameduration to int64_t(which is > > required in libcamera to set the FrameDurationLimits control). > > > > Example - > > > > * Suppose the framerate requested is 35/1. The framerate is read form > > the caps in the from of fraction that has a numerator and > > denominator. > > > > * Converting to FrameDuration (in microseconds) > > (1 * 10^6) / 35 = 28571.4286 > > int64_t frame_duration = 28571 > > (the precision here is lost.) > > * To expose the framerate in caps, Inverting the frame_duration to get > > back the framerate and converting to Seconds. > > double framerate = 10^6 / 28571 > > and > > 10^6/28571 which is close but not equal to 35/1 will fail the > negotiation. > > > > To solve the above problem, Store the framerate requested through > > negotiated caps in the GValue of the type GST_TYPE_FRACTION. > > > > Try to apply the framerate and check if the floor value of framerate > that gets applied > > closely matches with the floor value framerate requested in the > negotiated caps. If > > the value matches expose the framerate that is stored in the > framerate_container, else expose > > the framerate in the new caps as 0/1. > > > > Pass the initControls_ at camera->start(). > > > > --- > > Changes from v1 to v2 > > * Use GValue of the type GST_TYPE_FRACTION instead of std::pair to store > the framerate. > > * Don't shift the call to camera->configure(), instead bound the > framerate > > after the call to camera->configure(). > > * Reset the FrameDurationLimits control only if it is out of bounds. > > * Expose the caps containing framerate information with a new CAPS event > > after the call to camera->configure(). > > --- > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > As I haven't contributed to any LOC of this patch, please drop of my > s-o-b tag. > > Non-existent s-o-b on behalf of others shouldn't be applied ideally > > Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com> > > --- > > src/gstreamer/gstlibcamera-utils.cpp | 116 +++++++++++++++++++++++++++ > > src/gstreamer/gstlibcamera-utils.h | 8 ++ > > src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++- > > 3 files changed, 151 insertions(+), 1 deletion(-) > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp > b/src/gstreamer/gstlibcamera-utils.cpp > > index 4df5dd6c..e862f7ea 100644 > > --- a/src/gstreamer/gstlibcamera-utils.cpp > > +++ b/src/gstreamer/gstlibcamera-utils.cpp > > @@ -8,6 +8,7 @@ > > > > #include "gstlibcamera-utils.h" > > > > +#include <libcamera/control_ids.h> > > #include <libcamera/formats.h> > > > > using namespace libcamera; > > @@ -405,6 +406,121 @@ > gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > > } > > } > > > > +void > > +gst_libcamera_get_init_controls_from_caps(ControlList &controls, > [[maybe_unused]] GstCaps *caps, > > + GValue *framerate_container) > > +{ > > + GstStructure *s = gst_caps_get_structure(caps, 0); > > + gint fps_n = -1, fps_d = -1; > > + > > + if (gst_structure_has_field_typed(s, "framerate", > GST_TYPE_FRACTION)) { > > + if (!gst_structure_get_fraction(s, "framerate", &fps_n, > &fps_d)) { > > + GST_WARNING("invalid framerate in the caps."); > > + return; > > + } > > + } > > + > > + if (fps_n < 0 || fps_d < 0) > > + return; > > + > > + gst_value_set_fraction(framerate_container, fps_n, fps_d); > > + int64_t frame_duration = (fps_d * 1000000.0) / fps_n; > > + > > + controls.set(controls::FrameDurationLimits, > > + Span<const int64_t, 2>({ frame_duration, > frame_duration })); > > setting the frame_duration even before bound-checking is wrong. This > Function should ideally get the 'framerate' and just save it for later > use/bound-checking after camera::configure() > > Once the bound checking is successfully, only then you are allowed to > set frame_duration on initControls_ > > +} > > + > > +void > > +gst_libcamera_bind_framerate(const ControlInfoMap &camera_controls, > ControlList &controls) > > maybe > gst_libcamera_framerate_clamp() > OR > gst_libcamera_framerate_bounds_check() > > +{ > > + gboolean isBound = true; > > + auto iterFrameDuration = > camera_controls.find(controls::FrameDurationLimits.id()); > > + > > + if (!(iterFrameDuration != camera_controls.end() && > > + controls.contains(controls::FRAME_DURATION_LIMITS))) > > + return; > > + > > + int64_t frame_duration = > controls.get(controls::FrameDurationLimits).value()[0]; > > + int64_t min_frame_duration = > iterFrameDuration->second.min().get<int64_t>(); > > + int64_t max_frame_duration = > iterFrameDuration->second.max().get<int64_t>(); > > + > > + if (frame_duration < min_frame_duration) { > > + frame_duration = min_frame_duration; > > + } else if (frame_duration > max_frame_duration) { > > + frame_duration = max_frame_duration; > > + } else { > > + isBound = false; > > + } > > + > > + if (isBound) > > + controls.set(controls::FrameDurationLimits, > > + Span<const int64_t, 2>({ frame_duration, > frame_duration })); > > +} > > + > > +gboolean > > +gst_libcamera_get_framerate_from_init_controls(const ControlList > &controls, GValue *framerate, > > + GValue *framerate_container) > > +{ > > + gint fps_caps_n, fps_caps_d, numerator, denominator; > > + > > + fps_caps_n = gst_value_get_fraction_numerator(framerate_container); > > + fps_caps_d = > gst_value_get_fraction_denominator(framerate_container); > > + > > + if (!controls.contains(controls::FRAME_DURATION_LIMITS)) > > + return false; > > + > > + double framerate_ret = 1000000 / > static_cast<double>(controls.get(controls::FrameDurationLimits).value()[0]); > > + gst_util_double_to_fraction(framerate_ret, &numerator, > &denominator); > > + gst_value_set_fraction(framerate, numerator, denominator); > > + > > + if (fps_caps_n / fps_caps_d == numerator / denominator) { > > + return true; > > + } > > + return false; > > +} > > + > > +GstCaps * > > +gst_libcamera_init_controls_to_caps(const ControlList &controls, const > StreamConfiguration &stream_cfg, > > + GValue *framerate_container) > > +{ > > + GstCaps *caps = gst_caps_new_empty(); > > + GstStructure *s = > bare_structure_from_format(stream_cfg.pixelFormat); > > + gboolean ret; > > + GValue *framerate = g_new0(GValue, 1); > > Why create a new GValue here? > > You have everything you need through framerate_container, a applied and > bound-checked framerate, you just need to query the fraction from it and > set the structure? > > + g_value_init(framerate, GST_TYPE_FRACTION); > > + > > + ret = gst_libcamera_get_framerate_from_init_controls(controls, > framerate, framerate_container); > > As per the above comment, I am not sure if you really need to do that. > Seems over-kill > > + > > + gst_structure_set(s, > > + "width", G_TYPE_INT, stream_cfg.size.width, > > + "height", G_TYPE_INT, stream_cfg.size.height, > > + nullptr); > > + > > + if (ret) { > > + gint numerator, denominator; > > + numerator = > gst_value_get_fraction_numerator(framerate_container); > > + denominator = > gst_value_get_fraction_denominator(framerate_container); > > + gst_structure_set(s, "framerate", GST_TYPE_FRACTION, > numerator, denominator, nullptr); > > + } else { > > + gst_structure_set(s, "framerate", GST_TYPE_FRACTION, 0, 1, > nullptr); > > + } > > + > > + if (stream_cfg.colorSpace) { > > ?! > > I am not getting why you are introducing code related to colorspace > here? Is it a copy/paste fiasco? > > + GstVideoColorimetry colorimetry = > colorimetry_from_colorspace(stream_cfg.colorSpace.value()); > > + gchar *colorimetry_str = > gst_video_colorimetry_to_string(&colorimetry); > > + > > + if (colorimetry_str) > > + gst_structure_set(s, "colorimetry", G_TYPE_STRING, > colorimetry_str, nullptr); > > + else > > + g_error("Got invalid colorimetry from ColorSpace: > %s", > > + > ColorSpace::toString(stream_cfg.colorSpace).c_str()); > > + } > > + > > + gst_caps_append_structure(caps, s); > > + g_free(framerate); > > + return caps; > > +} > > + > > #if !GST_CHECK_VERSION(1, 17, 1) > > gboolean > > gst_task_resume(GstTask *task) > > diff --git a/src/gstreamer/gstlibcamera-utils.h > b/src/gstreamer/gstlibcamera-utils.h > > index 164189a2..955a717d 100644 > > --- a/src/gstreamer/gstlibcamera-utils.h > > +++ b/src/gstreamer/gstlibcamera-utils.h > > @@ -9,6 +9,7 @@ > > #pragma once > > > > #include <libcamera/camera_manager.h> > > +#include <libcamera/controls.h> > > #include <libcamera/stream.h> > > > > #include <gst/gst.h> > > @@ -18,6 +19,13 @@ GstCaps *gst_libcamera_stream_formats_to_caps(const > libcamera::StreamFormats &fo > > GstCaps *gst_libcamera_stream_configuration_to_caps(const > libcamera::StreamConfiguration &stream_cfg); > > void > gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration > &stream_cfg, > > GstCaps *caps); > > +void gst_libcamera_get_init_controls_from_caps(libcamera::ControlList > &controls, > > + GstCaps *caps, GValue > *framerate_container); > > +void gst_libcamera_bind_framerate(const libcamera::ControlInfoMap > &camera_controls, > > + libcamera::ControlList &controls); > > +GstCaps *gst_libcamera_init_controls_to_caps(const > libcamera::ControlList &controls, > > + const > libcamera::StreamConfiguration &streame_cfg, GValue *framerate_container); > > + > > #if !GST_CHECK_VERSION(1, 17, 1) > > gboolean gst_task_resume(GstTask *task); > > #endif > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp > b/src/gstreamer/gstlibcamerasrc.cpp > > index 16d70fea..0c981357 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -131,6 +131,7 @@ struct GstLibcameraSrcState { > > std::queue<std::unique_ptr<RequestWrap>> completedRequests_ > > LIBCAMERA_TSA_GUARDED_BY(lock_); > > > > + ControlList initControls_; > > guint group_id_; > > > > int queueRequest(); > > @@ -461,6 +462,8 @@ gst_libcamera_src_task_enter(GstTask *task, > [[maybe_unused]] GThread *thread, > > GstLibcameraSrcState *state = self->state; > > GstFlowReturn flow_ret = GST_FLOW_OK; > > gint ret; > > + GValue *framerate_container = g_new0(GValue, 1); > > + framerate_container = g_value_init(framerate_container, > GST_TYPE_FRACTION); > > Since it's a GValue, I would not make it framerate_* specific, so I > would just rename and remove all "framerate_" specifics. > > In future, when we have to parse more parameters as a part of > initControls_ setting, the same GValue can be used to temporarily hold > all other values > > > > GST_DEBUG_OBJECT(self, "Streaming thread has started"); > > > > @@ -496,6 +499,7 @@ gst_libcamera_src_task_enter(GstTask *task, > [[maybe_unused]] GThread *thread, > > /* Retrieve the supported caps. */ > > g_autoptr(GstCaps) filter = > gst_libcamera_stream_formats_to_caps(stream_cfg.formats()); > > g_autoptr(GstCaps) caps = gst_pad_peer_query_caps(srcpad, > filter); > > + > > if (gst_caps_is_empty(caps)) { > > flow_ret = GST_FLOW_NOT_NEGOTIATED; > > break; > > @@ -504,6 +508,8 @@ 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_get_init_controls_from_caps(state->initControls_, caps, > > + > framerate_container); > > passing state->initControls_ is un-necessary here as we don't need to > set frame_duration on it, just right now (it has to go through bound > check first which is below...) > > } > > > > if (flow_ret != GST_FLOW_OK) > > @@ -524,6 +530,7 @@ gst_libcamera_src_task_enter(GstTask *task, > [[maybe_unused]] GThread *thread, > > const StreamConfiguration &stream_cfg = > state->config_->at(i); > > > > g_autoptr(GstCaps) caps = > gst_libcamera_stream_configuration_to_caps(stream_cfg); > > + > > if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) > { > > flow_ret = GST_FLOW_NOT_NEGOTIATED; > > break; > > @@ -544,6 +551,23 @@ gst_libcamera_src_task_enter(GstTask *task, > [[maybe_unused]] GThread *thread, > > return; > > } > > > > + /* bind the framerate */ > > s/bind/Clamp OR > /* Check framerate bounds within controls::FrameDurationLimits */ > > + gst_libcamera_bind_framerate(state->cam_->controls(), > state->initControls_); > > I would just pass the controls::FrameDurationLimits and GValue > (containing the asked framerate) and perform the bound check. > > You can update the GValue's framerate in that function after which > simply, set the initControls' ::FrameDurationLimits right here. > > + > > + /* Expose the controls in initControls_ throught a new caps event. > */ > > + for (gsize i = 0; i < state->srcpads_.size(); i++) { > > + GstPad *srcpad = state->srcpads_[i]; > > + const StreamConfiguration &stream_cfg = > state->config_->at(i); > > + > > + g_autoptr(GstCaps) caps = > gst_libcamera_init_controls_to_caps(state->initControls_, > > + > stream_cfg, framerate_container); > > + > > + if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) > { > > + flow_ret = GST_FLOW_NOT_NEGOTIATED; > > + break; > > + } > > + } > > + > > self->allocator = gst_libcamera_allocator_new(state->cam_, > state->config_.get()); > > if (!self->allocator) { > > GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT, > > @@ -566,7 +590,7 @@ gst_libcamera_src_task_enter(GstTask *task, > [[maybe_unused]] GThread *thread, > > gst_flow_combiner_add_pad(self->flow_combiner, srcpad); > > } > > > > - ret = state->cam_->start(); > > + ret = state->cam_->start(&state->initControls_); > > if (ret) { > > GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, > > ("Failed to start the camera: %s", > g_strerror(-ret)), > > @@ -576,6 +600,8 @@ gst_libcamera_src_task_enter(GstTask *task, > [[maybe_unused]] GThread *thread, > > } > > > > done: > > + state->initControls_.clear(); > > + g_free(framerate_container); > > switch (flow_ret) { > > case GST_FLOW_NOT_NEGOTIATED: > > GST_ELEMENT_FLOW_ERROR(self, flow_ret); > >
Hi Rishi, On 9/8/22 11:36 AM, Rishikesh Donadkar wrote: > > > + > > + gst_structure_set(s, > > + "width", G_TYPE_INT, stream_cfg.size.width, > > + "height", G_TYPE_INT, > stream_cfg.size.height, > > + nullptr); > > + > > + if (ret) { > > + gint numerator, denominator; > > + numerator = > gst_value_get_fraction_numerator(framerate_container); > > + denominator = > gst_value_get_fraction_denominator(framerate_container); > > + gst_structure_set(s, "framerate", > GST_TYPE_FRACTION, numerator, denominator, nullptr); > > + } else { > > + gst_structure_set(s, "framerate", > GST_TYPE_FRACTION, 0, 1, nullptr); > > + } > > + > > + if (stream_cfg.colorSpace) { > > ?! > > I am not getting why you are introducing code related to colorspace > here? Is it a copy/paste fiasco? > > As new caps are exposed. Shouldn't we expose everything that has been > decided on the libcamera side that includes colorspace, resolutions, > format and framerate? err yes... It's not very nice though... I wish gstreamer has/had mechanism to provide / update caps on the fly (even partially). I have looked deep into other elements but maybe there is a possbile way to do that, however [1] suggests there is no event to support it. Another option I'm actually considering is moving the new caps event for loop (just above state->cam_->configure()) after the configure(). So, we would go from: streamCfg->validate(); // new caps event camera::configure(); to streamCfg->validate(); camera::configure(); // new caps event Which seems logical to me i.e. announcing the caps after the camera is actually configured. This will also help us with framerate and I don't see any harm in announcing the caps after camera is configured (but I maybe wrong, so requires some testing on RPi) Does it makes sense? What do you think? [1] https://gstreamer.freedesktop.org/documentation/gstreamer/gstevent.html?gi-language=c#GstEventType > > > On Thu, Sep 8, 2022 at 11:15 AM Umang Jain > <umang.jain@ideasonboard.com> wrote: > > Hi Rishi, > > On 9/7/22 4:17 PM, Rishikesh Donadkar wrote: > > Add a field of the type ControlList to GstLibcameraSrcState. > > > > Get the framerate from the caps and convert it to FrameDuration. > > Set the FrameDurationLimits control in the initControls_ > > > > Passing initControls_ at camera::start() doesn't tell us if the > controls > > have been applied to the camera successfully or not (or they > have adjusted in > > some fashion). Until that is introduced in camera::start() API, > > we need to carry out such handling on the application side. > > > > Check the frame_duration whether it is in-bound to the max / min > > frame-duration supported by the camera using the function > > gst_libcamera_bind_framerate(). > > > > If the frame_duartion is out of bounds, bind the frame_duration > > between the max and min FrameDuration that is supported by the > camera. > > > > Pass the initControls_ at the time of starting camera. > > > > Solve the complications in exposing the correct framerate due to > loss in > > precision as a result of casting the frameduration to > int64_t(which is > > required in libcamera to set the FrameDurationLimits control). > > > > Example - > > > > * Suppose the framerate requested is 35/1. The framerate is read > form > > the caps in the from of fraction that has a numerator and > > denominator. > > > > * Converting to FrameDuration (in microseconds) > > (1 * 10^6) / 35 = 28571.4286 > > int64_t frame_duration = 28571 > > (the precision here is lost.) > > * To expose the framerate in caps, Inverting the frame_duration > to get > > back the framerate and converting to Seconds. > > double framerate = 10^6 / 28571 > > and > > 10^6/28571 which is close but not equal to 35/1 will fail > the negotiation. > > > > To solve the above problem, Store the framerate requested through > > negotiated caps in the GValue of the type GST_TYPE_FRACTION. > > > > Try to apply the framerate and check if the floor value of > framerate that gets applied > > closely matches with the floor value framerate requested in the > negotiated caps. If > > the value matches expose the framerate that is stored in the > framerate_container, else expose > > the framerate in the new caps as 0/1. > > > > Pass the initControls_ at camera->start(). > > > > --- > > Changes from v1 to v2 > > * Use GValue of the type GST_TYPE_FRACTION instead of std::pair > to store the framerate. > > * Don't shift the call to camera->configure(), instead bound the > framerate > > after the call to camera->configure(). > > * Reset the FrameDurationLimits control only if it is out of bounds. > > * Expose the caps containing framerate information with a new > CAPS event > > after the call to camera->configure(). > > --- > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > As I haven't contributed to any LOC of this patch, please drop of my > s-o-b tag. > > Non-existent s-o-b on behalf of others shouldn't be applied ideally > > Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com> > > --- > > src/gstreamer/gstlibcamera-utils.cpp | 116 > +++++++++++++++++++++++++++ > > src/gstreamer/gstlibcamera-utils.h | 8 ++ > > src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++- > > 3 files changed, 151 insertions(+), 1 deletion(-) > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp > b/src/gstreamer/gstlibcamera-utils.cpp > > index 4df5dd6c..e862f7ea 100644 > > --- a/src/gstreamer/gstlibcamera-utils.cpp > > +++ b/src/gstreamer/gstlibcamera-utils.cpp > > @@ -8,6 +8,7 @@ > > > > #include "gstlibcamera-utils.h" > > > > +#include <libcamera/control_ids.h> > > #include <libcamera/formats.h> > > > > using namespace libcamera; > > @@ -405,6 +406,121 @@ > gst_libcamera_configure_stream_from_caps(StreamConfiguration > &stream_cfg, > > } > > } > > > > +void > > +gst_libcamera_get_init_controls_from_caps(ControlList > &controls, [[maybe_unused]] GstCaps *caps, > > + GValue *framerate_container) > > +{ > > + GstStructure *s = gst_caps_get_structure(caps, 0); > > + gint fps_n = -1, fps_d = -1; > > + > > + if (gst_structure_has_field_typed(s, "framerate", > GST_TYPE_FRACTION)) { > > + if (!gst_structure_get_fraction(s, "framerate", > &fps_n, &fps_d)) { > > + GST_WARNING("invalid framerate in the caps."); > > + return; > > + } > > + } > > + > > + if (fps_n < 0 || fps_d < 0) > > + return; > > + > > + gst_value_set_fraction(framerate_container, fps_n, fps_d); > > + int64_t frame_duration = (fps_d * 1000000.0) / fps_n; > > + > > + controls.set(controls::FrameDurationLimits, > > + Span<const int64_t, 2>({ frame_duration, > frame_duration })); > > setting the frame_duration even before bound-checking is wrong. This > Function should ideally get the 'framerate' and just save it for > later > use/bound-checking after camera::configure() > > Once the bound checking is successfully, only then you are allowed to > set frame_duration on initControls_ > > +} > > + > > +void > > +gst_libcamera_bind_framerate(const ControlInfoMap > &camera_controls, ControlList &controls) > > maybe > gst_libcamera_framerate_clamp() > OR > gst_libcamera_framerate_bounds_check() > > +{ > > + gboolean isBound = true; > > + auto iterFrameDuration = > camera_controls.find(controls::FrameDurationLimits.id()); > > + > > + if (!(iterFrameDuration != camera_controls.end() && > > + controls.contains(controls::FRAME_DURATION_LIMITS))) > > + return; > > + > > + int64_t frame_duration = > controls.get(controls::FrameDurationLimits).value()[0]; > > + int64_t min_frame_duration = > iterFrameDuration->second.min().get<int64_t>(); > > + int64_t max_frame_duration = > iterFrameDuration->second.max().get<int64_t>(); > > + > > + if (frame_duration < min_frame_duration) { > > + frame_duration = min_frame_duration; > > + } else if (frame_duration > max_frame_duration) { > > + frame_duration = max_frame_duration; > > + } else { > > + isBound = false; > > + } > > + > > + if (isBound) > > + controls.set(controls::FrameDurationLimits, > > + Span<const int64_t, 2>({ > frame_duration, frame_duration })); > > +} > > + > > +gboolean > > +gst_libcamera_get_framerate_from_init_controls(const > ControlList &controls, GValue *framerate, > > + GValue > *framerate_container) > > +{ > > + gint fps_caps_n, fps_caps_d, numerator, denominator; > > + > > + fps_caps_n = > gst_value_get_fraction_numerator(framerate_container); > > + fps_caps_d = > gst_value_get_fraction_denominator(framerate_container); > > + > > + if (!controls.contains(controls::FRAME_DURATION_LIMITS)) > > + return false; > > + > > + double framerate_ret = 1000000 / > static_cast<double>(controls.get(controls::FrameDurationLimits).value()[0]); > > + gst_util_double_to_fraction(framerate_ret, &numerator, > &denominator); > > + gst_value_set_fraction(framerate, numerator, denominator); > > + > > + if (fps_caps_n / fps_caps_d == numerator / denominator) { > > + return true; > > + } > > + return false; > > +} > > + > > +GstCaps * > > +gst_libcamera_init_controls_to_caps(const ControlList > &controls, const StreamConfiguration &stream_cfg, > > + GValue *framerate_container) > > +{ > > + GstCaps *caps = gst_caps_new_empty(); > > + GstStructure *s = > bare_structure_from_format(stream_cfg.pixelFormat); > > + gboolean ret; > > + GValue *framerate = g_new0(GValue, 1); > > Why create a new GValue here? > > You have everything you need through framerate_container, a > applied and > bound-checked framerate, you just need to query the fraction from > it and > set the structure? > > + g_value_init(framerate, GST_TYPE_FRACTION); > > + > > + ret = > gst_libcamera_get_framerate_from_init_controls(controls, > framerate, framerate_container); > > As per the above comment, I am not sure if you really need to do > that. > Seems over-kill > > + > > + gst_structure_set(s, > > + "width", G_TYPE_INT, stream_cfg.size.width, > > + "height", G_TYPE_INT, > stream_cfg.size.height, > > + nullptr); > > + > > + if (ret) { > > + gint numerator, denominator; > > + numerator = > gst_value_get_fraction_numerator(framerate_container); > > + denominator = > gst_value_get_fraction_denominator(framerate_container); > > + gst_structure_set(s, "framerate", > GST_TYPE_FRACTION, numerator, denominator, nullptr); > > + } else { > > + gst_structure_set(s, "framerate", > GST_TYPE_FRACTION, 0, 1, nullptr); > > + } > > + > > + if (stream_cfg.colorSpace) { > > ?! > > I am not getting why you are introducing code related to colorspace > here? Is it a copy/paste fiasco? > > + GstVideoColorimetry colorimetry = > colorimetry_from_colorspace(stream_cfg.colorSpace.value()); > > + gchar *colorimetry_str = > gst_video_colorimetry_to_string(&colorimetry); > > + > > + if (colorimetry_str) > > + gst_structure_set(s, "colorimetry", > G_TYPE_STRING, colorimetry_str, nullptr); > > + else > > + g_error("Got invalid colorimetry from > ColorSpace: %s", > > + ColorSpace::toString(stream_cfg.colorSpace).c_str()); > > + } > > + > > + gst_caps_append_structure(caps, s); > > + g_free(framerate); > > + return caps; > > +} > > + > > #if !GST_CHECK_VERSION(1, 17, 1) > > gboolean > > gst_task_resume(GstTask *task) > > diff --git a/src/gstreamer/gstlibcamera-utils.h > b/src/gstreamer/gstlibcamera-utils.h > > index 164189a2..955a717d 100644 > > --- a/src/gstreamer/gstlibcamera-utils.h > > +++ b/src/gstreamer/gstlibcamera-utils.h > > @@ -9,6 +9,7 @@ > > #pragma once > > > > #include <libcamera/camera_manager.h> > > +#include <libcamera/controls.h> > > #include <libcamera/stream.h> > > > > #include <gst/gst.h> > > @@ -18,6 +19,13 @@ GstCaps > *gst_libcamera_stream_formats_to_caps(const > libcamera::StreamFormats &fo > > GstCaps *gst_libcamera_stream_configuration_to_caps(const > libcamera::StreamConfiguration &stream_cfg); > > void > gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration > &stream_cfg, > > GstCaps *caps); > > +void > gst_libcamera_get_init_controls_from_caps(libcamera::ControlList > &controls, > > + GstCaps *caps, > GValue *framerate_container); > > +void gst_libcamera_bind_framerate(const > libcamera::ControlInfoMap &camera_controls, > > + libcamera::ControlList &controls); > > +GstCaps *gst_libcamera_init_controls_to_caps(const > libcamera::ControlList &controls, > > + const > libcamera::StreamConfiguration &streame_cfg, GValue > *framerate_container); > > + > > #if !GST_CHECK_VERSION(1, 17, 1) > > gboolean gst_task_resume(GstTask *task); > > #endif > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp > b/src/gstreamer/gstlibcamerasrc.cpp > > index 16d70fea..0c981357 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -131,6 +131,7 @@ struct GstLibcameraSrcState { > > std::queue<std::unique_ptr<RequestWrap>> completedRequests_ > > LIBCAMERA_TSA_GUARDED_BY(lock_); > > > > + ControlList initControls_; > > guint group_id_; > > > > int queueRequest(); > > @@ -461,6 +462,8 @@ gst_libcamera_src_task_enter(GstTask *task, > [[maybe_unused]] GThread *thread, > > GstLibcameraSrcState *state = self->state; > > GstFlowReturn flow_ret = GST_FLOW_OK; > > gint ret; > > + GValue *framerate_container = g_new0(GValue, 1); > > + framerate_container = g_value_init(framerate_container, > GST_TYPE_FRACTION); > > Since it's a GValue, I would not make it framerate_* specific, so I > would just rename and remove all "framerate_" specifics. > > In future, when we have to parse more parameters as a part of > initControls_ setting, the same GValue can be used to temporarily > hold > all other values > > > > GST_DEBUG_OBJECT(self, "Streaming thread has started"); > > > > @@ -496,6 +499,7 @@ gst_libcamera_src_task_enter(GstTask *task, > [[maybe_unused]] GThread *thread, > > /* Retrieve the supported caps. */ > > g_autoptr(GstCaps) filter = > gst_libcamera_stream_formats_to_caps(stream_cfg.formats()); > > g_autoptr(GstCaps) caps = > gst_pad_peer_query_caps(srcpad, filter); > > + > > if (gst_caps_is_empty(caps)) { > > flow_ret = GST_FLOW_NOT_NEGOTIATED; > > break; > > @@ -504,6 +508,8 @@ 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_get_init_controls_from_caps(state->initControls_, caps, > > + framerate_container); > > passing state->initControls_ is un-necessary here as we don't need to > set frame_duration on it, just right now (it has to go through bound > check first which is below...) > > } > > > > if (flow_ret != GST_FLOW_OK) > > @@ -524,6 +530,7 @@ gst_libcamera_src_task_enter(GstTask *task, > [[maybe_unused]] GThread *thread, > > const StreamConfiguration &stream_cfg = > state->config_->at(i); > > > > g_autoptr(GstCaps) caps = > gst_libcamera_stream_configuration_to_caps(stream_cfg); > > + > > if (!gst_pad_push_event(srcpad, > gst_event_new_caps(caps))) { > > flow_ret = GST_FLOW_NOT_NEGOTIATED; > > break; > > @@ -544,6 +551,23 @@ gst_libcamera_src_task_enter(GstTask *task, > [[maybe_unused]] GThread *thread, > > return; > > } > > > > + /* bind the framerate */ > > s/bind/Clamp OR > /* Check framerate bounds within controls::FrameDurationLimits */ > > + gst_libcamera_bind_framerate(state->cam_->controls(), > state->initControls_); > > I would just pass the controls::FrameDurationLimits and GValue > (containing the asked framerate) and perform the bound check. > > You can update the GValue's framerate in that function after which > simply, set the initControls' ::FrameDurationLimits right here. > > + > > + /* Expose the controls in initControls_ throught a new > caps event. */ > > + for (gsize i = 0; i < state->srcpads_.size(); i++) { > > + GstPad *srcpad = state->srcpads_[i]; > > + const StreamConfiguration &stream_cfg = > state->config_->at(i); > > + > > + g_autoptr(GstCaps) caps = > gst_libcamera_init_controls_to_caps(state->initControls_, > > + stream_cfg, framerate_container); > > + > > + if (!gst_pad_push_event(srcpad, > gst_event_new_caps(caps))) { > > + flow_ret = GST_FLOW_NOT_NEGOTIATED; > > + break; > > + } > > + } > > + > > self->allocator = gst_libcamera_allocator_new(state->cam_, > state->config_.get()); > > if (!self->allocator) { > > GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT, > > @@ -566,7 +590,7 @@ gst_libcamera_src_task_enter(GstTask *task, > [[maybe_unused]] GThread *thread, > > gst_flow_combiner_add_pad(self->flow_combiner, srcpad); > > } > > > > - ret = state->cam_->start(); > > + ret = state->cam_->start(&state->initControls_); > > if (ret) { > > GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, > > ("Failed to start the camera: > %s", g_strerror(-ret)), > > @@ -576,6 +600,8 @@ gst_libcamera_src_task_enter(GstTask *task, > [[maybe_unused]] GThread *thread, > > } > > > > done: > > + state->initControls_.clear(); > > + g_free(framerate_container); > > switch (flow_ret) { > > case GST_FLOW_NOT_NEGOTIATED: > > GST_ELEMENT_FLOW_ERROR(self, flow_ret); >
> > I wish gstreamer has/had mechanism to provide / update caps on the fly > (even partially). I have looked deep into other elements but maybe there is > a possbile way to do that, however [1] suggests there is no event to > support it. > I don't think gstreamer has/had such a mechanism. GStreamer works with caps as a whole. > Another option I'm actually considering is moving the new caps event for > loop (just above state->cam_->configure()) after the configure(). So, we > would go from: > > streamCfg->validate(); > // new caps event > camera::configure(); > > to > > streamCfg->validate(); > camera::configure(); > // new caps event > > Which seems logical to me i.e. announcing the caps after the camera is > actually configured. This will also help us with framerate and I don't see > any harm in announcing the caps after camera is configured (but I maybe > wrong, so requires some testing on RPi) > Yes, it makes sense. It will help quite a bit with framerate and bound checking. And the framerate can be exposed to gstreamer on the same line as colorimetry is exposed without another new CAPS event. On Thu, Sep 8, 2022 at 2:05 PM Umang Jain <umang.jain@ideasonboard.com> wrote: > Hi Rishi, > > On 9/8/22 11:36 AM, Rishikesh Donadkar wrote: > > > + >> > + gst_structure_set(s, >> > + "width", G_TYPE_INT, stream_cfg.size.width, >> > + "height", G_TYPE_INT, stream_cfg.size.height, >> > + nullptr); >> > + >> > + if (ret) { >> > + gint numerator, denominator; >> > + numerator = >> gst_value_get_fraction_numerator(framerate_container); >> > + denominator = >> gst_value_get_fraction_denominator(framerate_container); >> > + gst_structure_set(s, "framerate", GST_TYPE_FRACTION, >> numerator, denominator, nullptr); >> > + } else { >> > + gst_structure_set(s, "framerate", GST_TYPE_FRACTION, 0, >> 1, nullptr); >> > + } >> > + >> > + if (stream_cfg.colorSpace) { >> >> ?! >> >> I am not getting why you are introducing code related to colorspace >> here? Is it a copy/paste fiasco? >> > As new caps are exposed. Shouldn't we expose everything that has been > decided on the libcamera side that includes colorspace, resolutions, > format and framerate? > > > err yes... It's not very nice though... > > I wish gstreamer has/had mechanism to provide / update caps on the fly > (even partially). I have looked deep into other elements but maybe there is > a possbile way to do that, however [1] suggests there is no event to > support it. > > Another option I'm actually considering is moving the new caps event for > loop (just above state->cam_->configure()) after the configure(). So, we > would go from: > > streamCfg->validate(); > // new caps event > camera::configure(); > > to > > streamCfg->validate(); > camera::configure(); > // new caps event > > Which seems logical to me i.e. announcing the caps after the camera is > actually configured. This will also help us with framerate and I don't see > any harm in announcing the caps after camera is configured (but I maybe > wrong, so requires some testing on RPi) > > Does it makes sense? What do you think? > > [1] > https://gstreamer.freedesktop.org/documentation/gstreamer/gstevent.html?gi-language=c#GstEventType > > > > On Thu, Sep 8, 2022 at 11:15 AM Umang Jain <umang.jain@ideasonboard.com> > wrote: > >> Hi Rishi, >> >> On 9/7/22 4:17 PM, Rishikesh Donadkar wrote: >> > Add a field of the type ControlList to GstLibcameraSrcState. >> > >> > Get the framerate from the caps and convert it to FrameDuration. >> > Set the FrameDurationLimits control in the initControls_ >> > >> > Passing initControls_ at camera::start() doesn't tell us if the controls >> > have been applied to the camera successfully or not (or they have >> adjusted in >> > some fashion). Until that is introduced in camera::start() API, >> > we need to carry out such handling on the application side. >> > >> > Check the frame_duration whether it is in-bound to the max / min >> > frame-duration supported by the camera using the function >> > gst_libcamera_bind_framerate(). >> > >> > If the frame_duartion is out of bounds, bind the frame_duration >> > between the max and min FrameDuration that is supported by the camera. >> > >> > Pass the initControls_ at the time of starting camera. >> > >> > Solve the complications in exposing the correct framerate due to loss in >> > precision as a result of casting the frameduration to int64_t(which is >> > required in libcamera to set the FrameDurationLimits control). >> > >> > Example - >> > >> > * Suppose the framerate requested is 35/1. The framerate is read form >> > the caps in the from of fraction that has a numerator and >> > denominator. >> > >> > * Converting to FrameDuration (in microseconds) >> > (1 * 10^6) / 35 = 28571.4286 >> > int64_t frame_duration = 28571 >> > (the precision here is lost.) >> > * To expose the framerate in caps, Inverting the frame_duration to get >> > back the framerate and converting to Seconds. >> > double framerate = 10^6 / 28571 >> > and >> > 10^6/28571 which is close but not equal to 35/1 will fail the >> negotiation. >> > >> > To solve the above problem, Store the framerate requested through >> > negotiated caps in the GValue of the type GST_TYPE_FRACTION. >> > >> > Try to apply the framerate and check if the floor value of framerate >> that gets applied >> > closely matches with the floor value framerate requested in the >> negotiated caps. If >> > the value matches expose the framerate that is stored in the >> framerate_container, else expose >> > the framerate in the new caps as 0/1. >> > >> > Pass the initControls_ at camera->start(). >> > >> > --- >> > Changes from v1 to v2 >> > * Use GValue of the type GST_TYPE_FRACTION instead of std::pair to >> store the framerate. >> > * Don't shift the call to camera->configure(), instead bound the >> framerate >> > after the call to camera->configure(). >> > * Reset the FrameDurationLimits control only if it is out of bounds. >> > * Expose the caps containing framerate information with a new CAPS event >> > after the call to camera->configure(). >> > --- >> > >> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> >> As I haven't contributed to any LOC of this patch, please drop of my >> s-o-b tag. >> >> Non-existent s-o-b on behalf of others shouldn't be applied ideally >> > Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com> >> > --- >> > src/gstreamer/gstlibcamera-utils.cpp | 116 +++++++++++++++++++++++++++ >> > src/gstreamer/gstlibcamera-utils.h | 8 ++ >> > src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++- >> > 3 files changed, 151 insertions(+), 1 deletion(-) >> > >> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp >> b/src/gstreamer/gstlibcamera-utils.cpp >> > index 4df5dd6c..e862f7ea 100644 >> > --- a/src/gstreamer/gstlibcamera-utils.cpp >> > +++ b/src/gstreamer/gstlibcamera-utils.cpp >> > @@ -8,6 +8,7 @@ >> > >> > #include "gstlibcamera-utils.h" >> > >> > +#include <libcamera/control_ids.h> >> > #include <libcamera/formats.h> >> > >> > using namespace libcamera; >> > @@ -405,6 +406,121 @@ >> gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, >> > } >> > } >> > >> > +void >> > +gst_libcamera_get_init_controls_from_caps(ControlList &controls, >> [[maybe_unused]] GstCaps *caps, >> > + GValue *framerate_container) >> > +{ >> > + GstStructure *s = gst_caps_get_structure(caps, 0); >> > + gint fps_n = -1, fps_d = -1; >> > + >> > + if (gst_structure_has_field_typed(s, "framerate", >> GST_TYPE_FRACTION)) { >> > + if (!gst_structure_get_fraction(s, "framerate", &fps_n, >> &fps_d)) { >> > + GST_WARNING("invalid framerate in the caps."); >> > + return; >> > + } >> > + } >> > + >> > + if (fps_n < 0 || fps_d < 0) >> > + return; >> > + >> > + gst_value_set_fraction(framerate_container, fps_n, fps_d); >> > + int64_t frame_duration = (fps_d * 1000000.0) / fps_n; >> > + >> > + controls.set(controls::FrameDurationLimits, >> > + Span<const int64_t, 2>({ frame_duration, >> frame_duration })); >> >> setting the frame_duration even before bound-checking is wrong. This >> Function should ideally get the 'framerate' and just save it for later >> use/bound-checking after camera::configure() >> >> Once the bound checking is successfully, only then you are allowed to >> set frame_duration on initControls_ >> > +} >> > + >> > +void >> > +gst_libcamera_bind_framerate(const ControlInfoMap &camera_controls, >> ControlList &controls) >> >> maybe >> gst_libcamera_framerate_clamp() >> OR >> gst_libcamera_framerate_bounds_check() >> > +{ >> > + gboolean isBound = true; >> > + auto iterFrameDuration = >> camera_controls.find(controls::FrameDurationLimits.id()); >> > + >> > + if (!(iterFrameDuration != camera_controls.end() && >> > + controls.contains(controls::FRAME_DURATION_LIMITS))) >> > + return; >> > + >> > + int64_t frame_duration = >> controls.get(controls::FrameDurationLimits).value()[0]; >> > + int64_t min_frame_duration = >> iterFrameDuration->second.min().get<int64_t>(); >> > + int64_t max_frame_duration = >> iterFrameDuration->second.max().get<int64_t>(); >> > + >> > + if (frame_duration < min_frame_duration) { >> > + frame_duration = min_frame_duration; >> > + } else if (frame_duration > max_frame_duration) { >> > + frame_duration = max_frame_duration; >> > + } else { >> > + isBound = false; >> > + } >> > + >> > + if (isBound) >> > + controls.set(controls::FrameDurationLimits, >> > + Span<const int64_t, 2>({ frame_duration, >> frame_duration })); >> > +} >> > + >> > +gboolean >> > +gst_libcamera_get_framerate_from_init_controls(const ControlList >> &controls, GValue *framerate, >> > + GValue >> *framerate_container) >> > +{ >> > + gint fps_caps_n, fps_caps_d, numerator, denominator; >> > + >> > + fps_caps_n = >> gst_value_get_fraction_numerator(framerate_container); >> > + fps_caps_d = >> gst_value_get_fraction_denominator(framerate_container); >> > + >> > + if (!controls.contains(controls::FRAME_DURATION_LIMITS)) >> > + return false; >> > + >> > + double framerate_ret = 1000000 / >> static_cast<double>(controls.get(controls::FrameDurationLimits).value()[0]); >> > + gst_util_double_to_fraction(framerate_ret, &numerator, >> &denominator); >> > + gst_value_set_fraction(framerate, numerator, denominator); >> > + >> > + if (fps_caps_n / fps_caps_d == numerator / denominator) { >> > + return true; >> > + } >> > + return false; >> > +} >> > + >> > +GstCaps * >> > +gst_libcamera_init_controls_to_caps(const ControlList &controls, const >> StreamConfiguration &stream_cfg, >> > + GValue *framerate_container) >> > +{ >> > + GstCaps *caps = gst_caps_new_empty(); >> > + GstStructure *s = >> bare_structure_from_format(stream_cfg.pixelFormat); >> > + gboolean ret; >> > + GValue *framerate = g_new0(GValue, 1); >> >> Why create a new GValue here? >> >> You have everything you need through framerate_container, a applied and >> bound-checked framerate, you just need to query the fraction from it and >> set the structure? >> > + g_value_init(framerate, GST_TYPE_FRACTION); >> > + >> > + ret = gst_libcamera_get_framerate_from_init_controls(controls, >> framerate, framerate_container); >> >> As per the above comment, I am not sure if you really need to do that. >> Seems over-kill >> > + >> > + gst_structure_set(s, >> > + "width", G_TYPE_INT, stream_cfg.size.width, >> > + "height", G_TYPE_INT, stream_cfg.size.height, >> > + nullptr); >> > + >> > + if (ret) { >> > + gint numerator, denominator; >> > + numerator = >> gst_value_get_fraction_numerator(framerate_container); >> > + denominator = >> gst_value_get_fraction_denominator(framerate_container); >> > + gst_structure_set(s, "framerate", GST_TYPE_FRACTION, >> numerator, denominator, nullptr); >> > + } else { >> > + gst_structure_set(s, "framerate", GST_TYPE_FRACTION, 0, >> 1, nullptr); >> > + } >> > + >> > + if (stream_cfg.colorSpace) { >> >> ?! >> >> I am not getting why you are introducing code related to colorspace >> here? Is it a copy/paste fiasco? >> > + GstVideoColorimetry colorimetry = >> colorimetry_from_colorspace(stream_cfg.colorSpace.value()); >> > + gchar *colorimetry_str = >> gst_video_colorimetry_to_string(&colorimetry); >> > + >> > + if (colorimetry_str) >> > + gst_structure_set(s, "colorimetry", >> G_TYPE_STRING, colorimetry_str, nullptr); >> > + else >> > + g_error("Got invalid colorimetry from ColorSpace: >> %s", >> > + >> ColorSpace::toString(stream_cfg.colorSpace).c_str()); >> > + } >> > + >> > + gst_caps_append_structure(caps, s); >> > + g_free(framerate); >> > + return caps; >> > +} >> > + >> > #if !GST_CHECK_VERSION(1, 17, 1) >> > gboolean >> > gst_task_resume(GstTask *task) >> > diff --git a/src/gstreamer/gstlibcamera-utils.h >> b/src/gstreamer/gstlibcamera-utils.h >> > index 164189a2..955a717d 100644 >> > --- a/src/gstreamer/gstlibcamera-utils.h >> > +++ b/src/gstreamer/gstlibcamera-utils.h >> > @@ -9,6 +9,7 @@ >> > #pragma once >> > >> > #include <libcamera/camera_manager.h> >> > +#include <libcamera/controls.h> >> > #include <libcamera/stream.h> >> > >> > #include <gst/gst.h> >> > @@ -18,6 +19,13 @@ GstCaps *gst_libcamera_stream_formats_to_caps(const >> libcamera::StreamFormats &fo >> > GstCaps *gst_libcamera_stream_configuration_to_caps(const >> libcamera::StreamConfiguration &stream_cfg); >> > void >> gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration >> &stream_cfg, >> > GstCaps *caps); >> > +void gst_libcamera_get_init_controls_from_caps(libcamera::ControlList >> &controls, >> > + GstCaps *caps, GValue >> *framerate_container); >> > +void gst_libcamera_bind_framerate(const libcamera::ControlInfoMap >> &camera_controls, >> > + libcamera::ControlList &controls); >> > +GstCaps *gst_libcamera_init_controls_to_caps(const >> libcamera::ControlList &controls, >> > + const >> libcamera::StreamConfiguration &streame_cfg, GValue *framerate_container); >> > + >> > #if !GST_CHECK_VERSION(1, 17, 1) >> > gboolean gst_task_resume(GstTask *task); >> > #endif >> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp >> b/src/gstreamer/gstlibcamerasrc.cpp >> > index 16d70fea..0c981357 100644 >> > --- a/src/gstreamer/gstlibcamerasrc.cpp >> > +++ b/src/gstreamer/gstlibcamerasrc.cpp >> > @@ -131,6 +131,7 @@ struct GstLibcameraSrcState { >> > std::queue<std::unique_ptr<RequestWrap>> completedRequests_ >> > LIBCAMERA_TSA_GUARDED_BY(lock_); >> > >> > + ControlList initControls_; >> > guint group_id_; >> > >> > int queueRequest(); >> > @@ -461,6 +462,8 @@ gst_libcamera_src_task_enter(GstTask *task, >> [[maybe_unused]] GThread *thread, >> > GstLibcameraSrcState *state = self->state; >> > GstFlowReturn flow_ret = GST_FLOW_OK; >> > gint ret; >> > + GValue *framerate_container = g_new0(GValue, 1); >> > + framerate_container = g_value_init(framerate_container, >> GST_TYPE_FRACTION); >> >> Since it's a GValue, I would not make it framerate_* specific, so I >> would just rename and remove all "framerate_" specifics. >> >> In future, when we have to parse more parameters as a part of >> initControls_ setting, the same GValue can be used to temporarily hold >> all other values >> > >> > GST_DEBUG_OBJECT(self, "Streaming thread has started"); >> > >> > @@ -496,6 +499,7 @@ gst_libcamera_src_task_enter(GstTask *task, >> [[maybe_unused]] GThread *thread, >> > /* Retrieve the supported caps. */ >> > g_autoptr(GstCaps) filter = >> gst_libcamera_stream_formats_to_caps(stream_cfg.formats()); >> > g_autoptr(GstCaps) caps = gst_pad_peer_query_caps(srcpad, >> filter); >> > + >> > if (gst_caps_is_empty(caps)) { >> > flow_ret = GST_FLOW_NOT_NEGOTIATED; >> > break; >> > @@ -504,6 +508,8 @@ 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_get_init_controls_from_caps(state->initControls_, caps, >> > + >> framerate_container); >> >> passing state->initControls_ is un-necessary here as we don't need to >> set frame_duration on it, just right now (it has to go through bound >> check first which is below...) >> > } >> > >> > if (flow_ret != GST_FLOW_OK) >> > @@ -524,6 +530,7 @@ gst_libcamera_src_task_enter(GstTask *task, >> [[maybe_unused]] GThread *thread, >> > const StreamConfiguration &stream_cfg = >> state->config_->at(i); >> > >> > g_autoptr(GstCaps) caps = >> gst_libcamera_stream_configuration_to_caps(stream_cfg); >> > + >> > if (!gst_pad_push_event(srcpad, >> gst_event_new_caps(caps))) { >> > flow_ret = GST_FLOW_NOT_NEGOTIATED; >> > break; >> > @@ -544,6 +551,23 @@ gst_libcamera_src_task_enter(GstTask *task, >> [[maybe_unused]] GThread *thread, >> > return; >> > } >> > >> > + /* bind the framerate */ >> >> s/bind/Clamp OR >> /* Check framerate bounds within controls::FrameDurationLimits */ >> > + gst_libcamera_bind_framerate(state->cam_->controls(), >> state->initControls_); >> >> I would just pass the controls::FrameDurationLimits and GValue >> (containing the asked framerate) and perform the bound check. >> >> You can update the GValue's framerate in that function after which >> simply, set the initControls' ::FrameDurationLimits right here. >> > + >> > + /* Expose the controls in initControls_ throught a new caps >> event. */ >> > + for (gsize i = 0; i < state->srcpads_.size(); i++) { >> > + GstPad *srcpad = state->srcpads_[i]; >> > + const StreamConfiguration &stream_cfg = >> state->config_->at(i); >> > + >> > + g_autoptr(GstCaps) caps = >> gst_libcamera_init_controls_to_caps(state->initControls_, >> > + >> stream_cfg, framerate_container); >> > + >> > + if (!gst_pad_push_event(srcpad, >> gst_event_new_caps(caps))) { >> > + flow_ret = GST_FLOW_NOT_NEGOTIATED; >> > + break; >> > + } >> > + } >> > + >> > self->allocator = gst_libcamera_allocator_new(state->cam_, >> state->config_.get()); >> > if (!self->allocator) { >> > GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT, >> > @@ -566,7 +590,7 @@ gst_libcamera_src_task_enter(GstTask *task, >> [[maybe_unused]] GThread *thread, >> > gst_flow_combiner_add_pad(self->flow_combiner, srcpad); >> > } >> > >> > - ret = state->cam_->start(); >> > + ret = state->cam_->start(&state->initControls_); >> > if (ret) { >> > GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, >> > ("Failed to start the camera: %s", >> g_strerror(-ret)), >> > @@ -576,6 +600,8 @@ gst_libcamera_src_task_enter(GstTask *task, >> [[maybe_unused]] GThread *thread, >> > } >> > >> > done: >> > + state->initControls_.clear(); >> > + g_free(framerate_container); >> > switch (flow_ret) { >> > case GST_FLOW_NOT_NEGOTIATED: >> > GST_ELEMENT_FLOW_ERROR(self, flow_ret); >> >> >
diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp index 4df5dd6c..e862f7ea 100644 --- a/src/gstreamer/gstlibcamera-utils.cpp +++ b/src/gstreamer/gstlibcamera-utils.cpp @@ -8,6 +8,7 @@ #include "gstlibcamera-utils.h" +#include <libcamera/control_ids.h> #include <libcamera/formats.h> using namespace libcamera; @@ -405,6 +406,121 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, } } +void +gst_libcamera_get_init_controls_from_caps(ControlList &controls, [[maybe_unused]] GstCaps *caps, + GValue *framerate_container) +{ + GstStructure *s = gst_caps_get_structure(caps, 0); + gint fps_n = -1, fps_d = -1; + + if (gst_structure_has_field_typed(s, "framerate", GST_TYPE_FRACTION)) { + if (!gst_structure_get_fraction(s, "framerate", &fps_n, &fps_d)) { + GST_WARNING("invalid framerate in the caps."); + return; + } + } + + if (fps_n < 0 || fps_d < 0) + return; + + gst_value_set_fraction(framerate_container, fps_n, fps_d); + int64_t frame_duration = (fps_d * 1000000.0) / fps_n; + + controls.set(controls::FrameDurationLimits, + Span<const int64_t, 2>({ frame_duration, frame_duration })); +} + +void +gst_libcamera_bind_framerate(const ControlInfoMap &camera_controls, ControlList &controls) +{ + gboolean isBound = true; + auto iterFrameDuration = camera_controls.find(controls::FrameDurationLimits.id()); + + if (!(iterFrameDuration != camera_controls.end() && + controls.contains(controls::FRAME_DURATION_LIMITS))) + return; + + int64_t frame_duration = controls.get(controls::FrameDurationLimits).value()[0]; + int64_t min_frame_duration = iterFrameDuration->second.min().get<int64_t>(); + int64_t max_frame_duration = iterFrameDuration->second.max().get<int64_t>(); + + if (frame_duration < min_frame_duration) { + frame_duration = min_frame_duration; + } else if (frame_duration > max_frame_duration) { + frame_duration = max_frame_duration; + } else { + isBound = false; + } + + if (isBound) + controls.set(controls::FrameDurationLimits, + Span<const int64_t, 2>({ frame_duration, frame_duration })); +} + +gboolean +gst_libcamera_get_framerate_from_init_controls(const ControlList &controls, GValue *framerate, + GValue *framerate_container) +{ + gint fps_caps_n, fps_caps_d, numerator, denominator; + + fps_caps_n = gst_value_get_fraction_numerator(framerate_container); + fps_caps_d = gst_value_get_fraction_denominator(framerate_container); + + if (!controls.contains(controls::FRAME_DURATION_LIMITS)) + return false; + + double framerate_ret = 1000000 / static_cast<double>(controls.get(controls::FrameDurationLimits).value()[0]); + gst_util_double_to_fraction(framerate_ret, &numerator, &denominator); + gst_value_set_fraction(framerate, numerator, denominator); + + if (fps_caps_n / fps_caps_d == numerator / denominator) { + return true; + } + return false; +} + +GstCaps * +gst_libcamera_init_controls_to_caps(const ControlList &controls, const StreamConfiguration &stream_cfg, + GValue *framerate_container) +{ + GstCaps *caps = gst_caps_new_empty(); + GstStructure *s = bare_structure_from_format(stream_cfg.pixelFormat); + gboolean ret; + GValue *framerate = g_new0(GValue, 1); + g_value_init(framerate, GST_TYPE_FRACTION); + + ret = gst_libcamera_get_framerate_from_init_controls(controls, framerate, framerate_container); + + gst_structure_set(s, + "width", G_TYPE_INT, stream_cfg.size.width, + "height", G_TYPE_INT, stream_cfg.size.height, + nullptr); + + if (ret) { + gint numerator, denominator; + numerator = gst_value_get_fraction_numerator(framerate_container); + denominator = gst_value_get_fraction_denominator(framerate_container); + gst_structure_set(s, "framerate", GST_TYPE_FRACTION, numerator, denominator, nullptr); + } else { + gst_structure_set(s, "framerate", GST_TYPE_FRACTION, 0, 1, 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) + gst_structure_set(s, "colorimetry", G_TYPE_STRING, colorimetry_str, nullptr); + else + g_error("Got invalid colorimetry from ColorSpace: %s", + ColorSpace::toString(stream_cfg.colorSpace).c_str()); + } + + gst_caps_append_structure(caps, s); + g_free(framerate); + return caps; +} + #if !GST_CHECK_VERSION(1, 17, 1) gboolean gst_task_resume(GstTask *task) diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h index 164189a2..955a717d 100644 --- a/src/gstreamer/gstlibcamera-utils.h +++ b/src/gstreamer/gstlibcamera-utils.h @@ -9,6 +9,7 @@ #pragma once #include <libcamera/camera_manager.h> +#include <libcamera/controls.h> #include <libcamera/stream.h> #include <gst/gst.h> @@ -18,6 +19,13 @@ GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &fo GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg); void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg, GstCaps *caps); +void gst_libcamera_get_init_controls_from_caps(libcamera::ControlList &controls, + GstCaps *caps, GValue *framerate_container); +void gst_libcamera_bind_framerate(const libcamera::ControlInfoMap &camera_controls, + libcamera::ControlList &controls); +GstCaps *gst_libcamera_init_controls_to_caps(const libcamera::ControlList &controls, + const libcamera::StreamConfiguration &streame_cfg, GValue *framerate_container); + #if !GST_CHECK_VERSION(1, 17, 1) gboolean gst_task_resume(GstTask *task); #endif diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index 16d70fea..0c981357 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -131,6 +131,7 @@ struct GstLibcameraSrcState { std::queue<std::unique_ptr<RequestWrap>> completedRequests_ LIBCAMERA_TSA_GUARDED_BY(lock_); + ControlList initControls_; guint group_id_; int queueRequest(); @@ -461,6 +462,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, GstLibcameraSrcState *state = self->state; GstFlowReturn flow_ret = GST_FLOW_OK; gint ret; + GValue *framerate_container = g_new0(GValue, 1); + framerate_container = g_value_init(framerate_container, GST_TYPE_FRACTION); GST_DEBUG_OBJECT(self, "Streaming thread has started"); @@ -496,6 +499,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, /* Retrieve the supported caps. */ g_autoptr(GstCaps) filter = gst_libcamera_stream_formats_to_caps(stream_cfg.formats()); g_autoptr(GstCaps) caps = gst_pad_peer_query_caps(srcpad, filter); + if (gst_caps_is_empty(caps)) { flow_ret = GST_FLOW_NOT_NEGOTIATED; break; @@ -504,6 +508,8 @@ 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_get_init_controls_from_caps(state->initControls_, caps, + framerate_container); } if (flow_ret != GST_FLOW_OK) @@ -524,6 +530,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, const StreamConfiguration &stream_cfg = state->config_->at(i); g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg); + if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) { flow_ret = GST_FLOW_NOT_NEGOTIATED; break; @@ -544,6 +551,23 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, return; } + /* bind the framerate */ + gst_libcamera_bind_framerate(state->cam_->controls(), state->initControls_); + + /* Expose the controls in initControls_ throught a new caps event. */ + for (gsize i = 0; i < state->srcpads_.size(); i++) { + GstPad *srcpad = state->srcpads_[i]; + const StreamConfiguration &stream_cfg = state->config_->at(i); + + g_autoptr(GstCaps) caps = gst_libcamera_init_controls_to_caps(state->initControls_, + stream_cfg, framerate_container); + + if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) { + flow_ret = GST_FLOW_NOT_NEGOTIATED; + break; + } + } + self->allocator = gst_libcamera_allocator_new(state->cam_, state->config_.get()); if (!self->allocator) { GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT, @@ -566,7 +590,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, gst_flow_combiner_add_pad(self->flow_combiner, srcpad); } - ret = state->cam_->start(); + ret = state->cam_->start(&state->initControls_); if (ret) { GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS, ("Failed to start the camera: %s", g_strerror(-ret)), @@ -576,6 +600,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread, } done: + state->initControls_.clear(); + g_free(framerate_container); switch (flow_ret) { case GST_FLOW_NOT_NEGOTIATED: GST_ELEMENT_FLOW_ERROR(self, flow_ret);
Add a field of the type ControlList to GstLibcameraSrcState. Get the framerate from the caps and convert it to FrameDuration. Set the FrameDurationLimits control in the initControls_ Passing initControls_ at camera::start() doesn't tell us if the controls have been applied to the camera successfully or not (or they have adjusted in some fashion). Until that is introduced in camera::start() API, we need to carry out such handling on the application side. Check the frame_duration whether it is in-bound to the max / min frame-duration supported by the camera using the function gst_libcamera_bind_framerate(). If the frame_duartion is out of bounds, bind the frame_duration between the max and min FrameDuration that is supported by the camera. Pass the initControls_ at the time of starting camera. Solve the complications in exposing the correct framerate due to loss in precision as a result of casting the frameduration to int64_t(which is required in libcamera to set the FrameDurationLimits control). Example - * Suppose the framerate requested is 35/1. The framerate is read form the caps in the from of fraction that has a numerator and denominator. * Converting to FrameDuration (in microseconds) (1 * 10^6) / 35 = 28571.4286 int64_t frame_duration = 28571 (the precision here is lost.) * To expose the framerate in caps, Inverting the frame_duration to get back the framerate and converting to Seconds. double framerate = 10^6 / 28571 and 10^6/28571 which is close but not equal to 35/1 will fail the negotiation. To solve the above problem, Store the framerate requested through negotiated caps in the GValue of the type GST_TYPE_FRACTION. Try to apply the framerate and check if the floor value of framerate that gets applied closely matches with the floor value framerate requested in the negotiated caps. If the value matches expose the framerate that is stored in the framerate_container, else expose the framerate in the new caps as 0/1. Pass the initControls_ at camera->start(). --- Changes from v1 to v2 * Use GValue of the type GST_TYPE_FRACTION instead of std::pair to store the framerate. * Don't shift the call to camera->configure(), instead bound the framerate after the call to camera->configure(). * Reset the FrameDurationLimits control only if it is out of bounds. * Expose the caps containing framerate information with a new CAPS event after the call to camera->configure(). --- Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com> --- src/gstreamer/gstlibcamera-utils.cpp | 116 +++++++++++++++++++++++++++ src/gstreamer/gstlibcamera-utils.h | 8 ++ src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++- 3 files changed, 151 insertions(+), 1 deletion(-)