<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">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.<br></blockquote><div>I don't think gstreamer has/had such a mechanism. GStreamer works with caps as a whole.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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:<br><br> streamCfg->validate();<br> // new caps event<br> camera::configure();<br><br>to<br><br> streamCfg->validate();<br> camera::configure();<br> // new caps event<br><br>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)<br></blockquote><div>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.</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Sep 8, 2022 at 2:05 PM 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">
<div>
Hi Rishi,<br>
<br>
<div>On 9/8/22 11:36 AM, Rishikesh Donadkar
wrote:<br>
</div>
<blockquote type="cite">
<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 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>
</blockquote>
<br>
err yes... It's not very nice though... <br>
<br>
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.<br>
<br>
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:<br>
<br>
streamCfg->validate();<br>
// new caps event<br>
camera::configure();<br>
<br>
to<br>
<br>
streamCfg->validate();<br>
camera::configure();<br>
// new caps event<br>
<br>
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) <br>
<br>
Does it makes sense? What do you think?<br>
<br>
[1]
<a href="https://gstreamer.freedesktop.org/documentation/gstreamer/gstevent.html?gi-language=c#GstEventType" target="_blank">https://gstreamer.freedesktop.org/documentation/gstreamer/gstevent.html?gi-language=c#GstEventType</a><br>
<br>
<blockquote type="cite">
<div dir="ltr">
<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" target="_blank">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>
</blockquote>
<br>
</div>
</blockquote></div>