[libcamera-devel] [PATCH v5 2/2] gstreamer: Provide framerate support for libcamerasrc.
Umang Jain
umang.jain at ideasonboard.com
Wed Nov 2 14:15:41 CET 2022
Hi Rishi,
One more comment,
On 11/2/22 5:43 PM, Umang Jain via libcamera-devel wrote:
> Hi Rishi,
>
> Thank you for the patch.
>
> On 9/15/22 5:17 PM, Rishikesh Donadkar wrote:
>> 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.
>
> I would summarise this, stating that now we preserve the fraction in a
> GstContainer and expose it similarly in the caps.
>> 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<rishikeshdonadkar at gmail.com>
>> ---
>> 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,
>
> no need for [[maybe_unused]]
>> + GstStructure *container)
>> +{
>> + GstStructure *s = gst_caps_get_structure(caps, 0);
>> + gint fps_n = 0, fps_d = 1;
>
> this is problematic. If you set fps_n = 0, it will be divide by 0 for
> frame_duration calculation in
> gst_libcamera_clamp_and_set_frameduration() as an edge case.
>
> 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,
>
> fps_n = 30, 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_structure_set(container, "framerate", GST_TYPE_FRACTION, fps_n, fps_d, nullptr);
>> + else
>> + GST_WARNING("invalid framerate in the caps.");
>> + }
>
> this can better be reworked as :
>
> 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");
> }
>
> gst_structure_set(container, "framerate", GST_TYPE_FRACTION,
> fps_n, fps_d, nullptr);
>> +}
>> +
>> +void gst_libcamera_clamp_and_set_frameduration(ControlList &controls, const ControlInfoMap &camera_controls,
>
> Would shorten names
> s/controls/ctrls
> s/camera_controls/cam_ctrls
>
> and reflow
>> + 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 }));
The Span<> cast can be dropped now, see Commit 09c1b081baa2 ("libcamera:
controls: Generate and use fixed-sized
Span types")
Ofcourse that wasn't available when you wrote this code! I'll address
this in my version.
>> +}
>> +
>> +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);
>> +
>
> unwanted
>
> These changes are already addressed on my branch. So no need to follow
> up on these.
>
> I added a few comments as well, which you can see in v6 (will be
> posted shortly)
>> 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);
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221102/ab91b92e/attachment.htm>
More information about the libcamera-devel
mailing list