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