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

[Umang Jain]

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