<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<br>
Hi Rishi,<br>
<br>
One more comment,<br>
<br>
<div class="moz-cite-prefix">On 11/2/22 5:43 PM, Umang Jain via
libcamera-devel wrote:<br>
</div>
<blockquote type="cite"
cite="mid:768aef5f-3243-1242-2f81-2728d3d6bda5@ideasonboard.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
Hi Rishi,<br>
<br>
Thank you for the patch. <br>
<br>
<div class="moz-cite-prefix">On 9/15/22 5:17 PM, Rishikesh
Donadkar wrote:<br>
</div>
<blockquote type="cite"
cite="mid:20220915114734.115572-3-rishikeshdonadkar@gmail.com">
<pre class="moz-quote-pre" wrap="">Control the framerate by setting the controls::FrameDurationLimits
in an ControlList and passing the control list at camera::start(). This
requires converting the framerate which of the type
GST_TYPE_FRACTION into FrameDuration which of the type int64_t. This
conversion causes loss of precision and the precise framerate cannot be
obtained by Inverting the control::FrameDurationLimits value.
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.</pre>
</blockquote>
<br>
I would summarise this, stating that now we preserve the fraction
in a GstContainer and expose it similarly in the caps.<br>
<blockquote type="cite"
cite="mid:20220915114734.115572-3-rishikeshdonadkar@gmail.com">
<pre class="moz-quote-pre" wrap="">Get the framerate from the caps and convert it to FrameDuration.
Clamp the frame_duration between the min/max FrameDuration supported
by the camera. Set the FrameDurationLimits control in the initControls_.
Store the clamped/unclamped framerate in the container to be retrieved later.
Set the bound checked framerate from the container into the caps.
Pass the ControlList initControls_ at camera->start().
Signed-off-by: Rishikesh Donadkar <a class="moz-txt-link-rfc2396E" href="mailto:rishikeshdonadkar@gmail.com" moz-do-not-send="true"><rishikeshdonadkar@gmail.com></a>
---
src/gstreamer/gstlibcamera-utils.cpp | 70 ++++++++++++++++++++++++++++
src/gstreamer/gstlibcamera-utils.h | 6 +++
src/gstreamer/gstlibcamerasrc.cpp | 16 ++++++-
3 files changed, 91 insertions(+), 1 deletion(-)
diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
index 244a4a79..89f116ba 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;
@@ -407,6 +408,75 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
}
}
+void gst_libcamera_get_framerate_from_caps([[maybe_unused]] GstCaps *caps,</pre>
</blockquote>
<br>
no need for [[maybe_unused]]<br>
<blockquote type="cite"
cite="mid:20220915114734.115572-3-rishikeshdonadkar@gmail.com">
<pre class="moz-quote-pre" wrap="">+ GstStructure *container)
+{
+ GstStructure *s = gst_caps_get_structure(caps, 0);
+ gint fps_n = 0, fps_d = 1;</pre>
</blockquote>
<br>
this is problematic. If you set fps_n = 0, it will be divide by 0
for frame_duration calculation in <br>
gst_libcamera_clamp_and_set_frameduration() as an edge case.<br>
<br>
Instead, I would treat this as a default value (set in case there
is a error below and set the framerate as 30/1 hence,<br>
<br>
fps_n = 30, fps_d = 1<br>
<blockquote type="cite"
cite="mid:20220915114734.115572-3-rishikeshdonadkar@gmail.com">
<pre class="moz-quote-pre" wrap="">+
+ if (gst_structure_has_field_typed(s, "framerate", GST_TYPE_FRACTION)) {
+ if (gst_structure_get_fraction(s, "framerate", &fps_n, &fps_d))
+ gst_structure_set(container, "framerate", GST_TYPE_FRACTION, fps_n, fps_d, nullptr);
+ else
+ GST_WARNING("invalid framerate in the caps.");
+ }</pre>
</blockquote>
<br>
this can better be reworked as :<br>
<br>
<font face="monospace"> if (gst_structure_has_field_typed(s,
"framerate", GST_TYPE_FRACTION)) {<br>
if (!gst_structure_get_fraction(s, "framerate",
&fps_n, &fps_d))</font><br>
<font face="monospace"> GST_WARNING("Invalid framerate
in the caps"); </font>
<pre> }
gst_structure_set(container, "framerate", GST_TYPE_FRACTION,
fps_n, fps_d, nullptr);</pre>
<blockquote type="cite"
cite="mid:20220915114734.115572-3-rishikeshdonadkar@gmail.com">
<pre class="moz-quote-pre" wrap="">+}
+
+void gst_libcamera_clamp_and_set_frameduration(ControlList &controls, const ControlInfoMap &camera_controls,</pre>
</blockquote>
<br>
Would shorten names <br>
s/controls/ctrls<br>
s/camera_controls/cam_ctrls <br>
<br>
and reflow<br>
<blockquote type="cite"
cite="mid:20220915114734.115572-3-rishikeshdonadkar@gmail.com">
<pre class="moz-quote-pre" wrap="">+ GstStructure *container)
+{
+ gboolean in_bounds = false;
+ gint fps_caps_n, fps_caps_d, fps_n, fps_d;
+
+ if (!gst_structure_has_field_typed(container, "framerate", GST_TYPE_FRACTION))
+ return;
+
+ auto iterFrameDuration = camera_controls.find(controls::FrameDurationLimits.id());
+ if (iterFrameDuration == camera_controls.end()) {
+ GST_WARNING("Valid bounds for FrameDuration not found");
+ return;
+ }
+
+ const GValue *framerate = gst_structure_get_value(container, "framerate");
+
+ fps_caps_n = gst_value_get_fraction_numerator(framerate);
+ fps_caps_d = gst_value_get_fraction_denominator(framerate);
+
+ int64_t frame_duration = (fps_caps_d * 1000000.0) / fps_caps_n;
+ 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 {
+ in_bounds = true;
+ }
+
+ if (!in_bounds) {
+ double framerate_clamped = 1000000.0 / frame_duration;
+ gst_util_double_to_fraction(framerate_clamped, &fps_n, &fps_d);
+ gst_structure_set(container, "framerate", GST_TYPE_FRACTION, fps_n, fps_d, nullptr);
+ }
+
+ controls.set(controls::FrameDurationLimits,
+ Span<const int64_t, 2>({ frame_duration, frame_duration }));</pre>
</blockquote>
</blockquote>
<br>
The Span<> cast can be dropped now, see Commit 09c1b081baa2
("libcamera: controls: Generate and use fixed-sized<br>
Span types")<br>
<br>
Ofcourse that wasn't available when you wrote this code! I'll
address this in my version.<br>
<blockquote type="cite"
cite="mid:768aef5f-3243-1242-2f81-2728d3d6bda5@ideasonboard.com">
<blockquote type="cite"
cite="mid:20220915114734.115572-3-rishikeshdonadkar@gmail.com">
<pre class="moz-quote-pre" wrap="">
+}
+
+void gst_libcamera_framerate_to_caps(GstCaps *caps, const GValue *container)
+{
+ if (!GST_VALUE_HOLDS_FRACTION(container))
+ return;
+
+ GstStructure *s = gst_caps_get_structure(caps, 0);
+ gint fps_caps_n, fps_caps_d;
+
+ fps_caps_n = gst_value_get_fraction_numerator(container);
+ fps_caps_d = gst_value_get_fraction_denominator(container);
+ gst_structure_set(s, "framerate", GST_TYPE_FRACTION, fps_caps_n, fps_caps_d, nullptr);
+}
+
#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..3d217fcf 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,11 @@ 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_framerate_from_caps(GstCaps *caps, GstStructure *container);
+void gst_libcamera_clamp_and_set_frameduration(libcamera::ControlList &controls,
+ const libcamera::ControlInfoMap &camera_controls, GstStructure *container);
+void gst_libcamera_framerate_to_caps(GstCaps *caps, const GValue *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 60032236..37f07676 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();
@@ -462,6 +463,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
GstFlowReturn flow_ret = GST_FLOW_OK;
gint ret;
+ GstStructure *container = gst_structure_new_empty("container");
+
GST_DEBUG_OBJECT(self, "Streaming thread has started");
gint stream_id_num = 0;
@@ -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);
+</pre>
</blockquote>
<br>
unwanted<br>
<br>
These changes are already addressed on my branch. So no need to
follow up on these. <br>
<br>
I added a few comments as well, which you can see in v6 (will be
posted shortly)<br>
<blockquote type="cite"
cite="mid:20220915114734.115572-3-rishikeshdonadkar@gmail.com">
<pre class="moz-quote-pre" wrap=""> if (gst_caps_is_empty(caps)) {
flow_ret = GST_FLOW_NOT_NEGOTIATED;
break;
@@ -504,6 +508,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
/* Fixate caps and configure the stream. */
caps = gst_caps_make_writable(caps);
gst_libcamera_configure_stream_from_caps(stream_cfg, caps);
+ gst_libcamera_get_framerate_from_caps(caps, container);
}
if (flow_ret != GST_FLOW_OK)
@@ -525,6 +530,10 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
goto done;
}
+ /* Check frame duration bounds within controls::FrameDurationLimits */
+ gst_libcamera_clamp_and_set_frameduration(state->initControls_,
+ state->cam_->controls(), container);
+
/*
* Regardless if it has been modified, create clean caps and push the
* caps event. Downstream will decide if the caps are acceptable.
@@ -532,8 +541,11 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
for (gsize i = 0; i < state->srcpads_.size(); i++) {
GstPad *srcpad = state->srcpads_[i];
const StreamConfiguration &stream_cfg = state->config_->at(i);
+ const GValue *framerate = gst_structure_get_value(container, "framerate");
g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg);
+ gst_libcamera_framerate_to_caps(caps, framerate);
+
if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) {
flow_ret = GST_FLOW_NOT_NEGOTIATED;
break;
@@ -567,7 +579,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)),
@@ -577,6 +589,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
}
done:
+ state->initControls_.clear();
+ g_free(container);
switch (flow_ret) {
case GST_FLOW_NOT_NEGOTIATED:
GST_ELEMENT_FLOW_ERROR(self, flow_ret);
</pre>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>