[libcamera-devel] [PATCH v1] gstreamer: Provide framerate support for libcamerasrc.

[Umang Jain]

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; ?
> +	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);