[libcamera-devel] [PATCH v1] gstreamer: Provide framerate support for libcamerasrc.
Umang Jain
umang.jain at ideasonboard.com
Tue Aug 30 19:56:09 CEST 2022
Hi Rishikesh,
one more comment,
On 8/29/22 12:17 PM, Umang Jain via libcamera-devel wrote:
> Hi Rishikesh,
>
> Thank you for the patch.
>
> Regarding the subject, you should have mentioend [RFC] as you
> mentioned you are still working on it and could've mention the missing
> pieces below.
>
> On 8/28/22 8:26 PM, Rishikesh Donadkar wrote:
>> This patch aims to add framerate support to libcamerasrc in the
>> direction gstreamer->libcamera.
>>
>> 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_ and pass
>> the initControls_ at the time of starting camera.
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar at gmail.com>
>>
>> ---
>> Tested the patch with 3 different framerates with the help of the
>> fpsdisplaysink element.
>>
>> https://paste.debian.net/1251951/
>>
>> https://paste.debian.net/1251952/
>>
>> https://paste.debian.net/1251953/
>
> Great results, happy to see. Also you could've mentioned the platform
> on which you tested.
> In this case RPi 4 + OV5647
>
>> ---
>> src/gstreamer/gstlibcamera-utils.cpp | 18 ++++++++++++++++++
>> src/gstreamer/gstlibcamera-utils.h | 3 +++
>> src/gstreamer/gstlibcamerasrc.cpp | 7 ++++++-
>> 3 files changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp
>> b/src/gstreamer/gstlibcamera-utils.cpp
>> index 5a21a391..7d8519da 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;
>> @@ -236,6 +237,23 @@
>> gst_libcamera_configure_stream_from_caps(StreamConfiguration
>> &stream_cfg,
>> stream_cfg.size.height = height;
>> }
>> +void
>> +gst_libcamera_configure_controls_from_caps(ControlList &controls,
>> [[maybe_unused]] GstCaps *caps)
>> +{
>> + /* read framerate from caps - convert to integer and set to
>> frame_time. */
>> + GstStructure *s = gst_caps_get_structure(caps, 0);
>> + gint fps_n = -1, fps_d = -1;
>> + if (gst_structure_has_field(s, "framerate"))
>> + gst_structure_get_fraction(s, "framerate", &fps_n, &fps_d);
>
> I think if the user input is malformed, get_fraction() will return a
> FALSE. We should check that for invalid input and log errors.
>> +
>> + if (fps_n < 0 || fps_d < 0)
>> + return;
>
> Can it happen that only the numerator is mentioned? Have you tested
> something like : "framerate=30" instead of "framerate=30/1"
>> +
>> + gdouble frame_duration = static_cast<double>(fps_d) /
>> static_cast<double>(fps_n) * 1000000.0;
>
> Since frame_duration is int64_t, and fps_d and fps_n are both
> integers, can we do:
>
> int64_t frame_duration = (fps_d * 1000000) / fps_n; ?
we also need to check the frame_duration whether it is in-bound to the
max / min frame-duration supported by the camera itself.
If it's lower than the minimum, we should set the frame_duration to minimum
If it's larger than the maximum, we should set the frame duration to
maximum.
We need to do this on the libcamerasrc side, since passing initCtrls_ 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.
>> + controls.set(controls::FrameDurationLimits,
>> + Span<const int64_t, 2>({
>> static_cast<int64_t>(frame_duration),
>> static_cast<int64_t>(frame_duration) }));
>
> you could then drop the cast from here I believe
>> +}
>> +
>> #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..1f737e84 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,8 @@ 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_configure_controls_from_caps(libcamera::ControlList
>> &controls, GstCaps *caps);
>
> nit: To be pedantic, we are only parsing controls needed at
> camera::StarT(); so maybe
> gst_libcamera_get_init_ctrls_from_caps(...);
> ?
>> +
>> #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..d1080271 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();
>> @@ -496,6 +497,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 +506,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_configure_controls_from_caps(state->initControls_,
>> caps);
>> }
>> if (flow_ret != GST_FLOW_OK)
>> @@ -524,6 +527,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;
>> @@ -566,7 +570,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 +580,7 @@ gst_libcamera_src_task_enter(GstTask *task,
>> [[maybe_unused]] GThread *thread,
>> }
>> done:
>> + state->initControls_.clear();
>> switch (flow_ret) {
>> case GST_FLOW_NOT_NEGOTIATED:
>> GST_ELEMENT_FLOW_ERROR(self, flow_ret);
>
More information about the libcamera-devel
mailing list