[libcamera-devel] [PATCH v6 2/2] gstreamer: Provide framerate support for libcamerasrc
Umang Jain
umang.jain at ideasonboard.com
Tue Nov 8 09:23:21 CET 2022
Hi Kieran,
Thank you for the reviews.
On 11/4/22 4:55 PM, Kieran Bingham wrote:
> Quoting Umang Jain via libcamera-devel (2022-11-02 14:13:13)
>> Hello,
>>
>> On 11/2/22 7:26 PM, Umang Jain wrote:
>>> From: Rishikesh Donadkar <rishikeshdonadkar at gmail.com>
>>>
>>> Control the framerate by passing the controls::FrameDurationLimits
>>> during Camera::start(). Framerate in gstreamer is expressed as
>>> GST_TYPE_FRACTION so we maximise on maintaining it as a fraction
>>> throughout and only do arithematic computations as and when required
>>> (to compute frame-duration and vice-versa).
>>>
>>> To weed out abritrary framerate as input, place the clamping via the
>>> controls::FrameDurationLimits provided after camera::configure() phase.
>>> This is handled by a helper function
>>> gst_libcamera_clamp_and_set_frameduration().
>>>
>>> Set the bound checked framerate (done in the above mentioned helper)
>>> into the caps and pass the ControlList containing the frame-duration
>>> to Camera::start(ctrls).
>>>
>>> Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar at gmail.com>
>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>>> Tested-by: Umang Jain <umang.jain at ideasonboard.com>
>>> ---
>>> src/gstreamer/gstlibcamera-utils.cpp | 78 ++++++++++++++++++++++++++++
>>> src/gstreamer/gstlibcamera-utils.h | 6 +++
>>> src/gstreamer/gstlibcamerasrc.cpp | 15 +++++-
>>> 3 files changed, 98 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
>>> index 244a4a79..c14f72ec 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,83 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
>>> }
>>> }
>>>
>>> +void gst_libcamera_get_framerate_from_caps(GstCaps *caps,
>>> + GstStructure *container)
>>> +{
>>> + GstStructure *s = gst_caps_get_structure(caps, 0);
>>> + /*
>>> + * Default to 30 fps. If the "framerate" fraction is invalid below,
>>> + * libcamerasrc will set 30fps as the framerate.
>>> + */
>>> + gint 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_WARNING("Invalid framerate in the caps");
> I wonder if this should fail negotiation - but I don't actually mind if
> it continues. It's a bit like the negotiation failure below though.. ?
If there's a framerate input syntax error on the user's side, the frame
will default to 30/1 as per the comment above.
We flag the input syntax error as a warning and continue with 30fps (as
default).
>
>>> + }
>>> +
>>> + gst_structure_set(container, "framerate", GST_TYPE_FRACTION,
>>> + fps_n, fps_d, nullptr);
>>> +}
>>> +
>>> +void gst_libcamera_clamp_and_set_frameduration(ControlList &initCtrls,
>>> + const ControlInfoMap &cam_ctrls,
>>> + GstStructure *container)
>>> +{
>>> + gboolean in_bounds = false;
>>> + gint fps_caps_n, fps_caps_d;
>>> +
>>> + if (!gst_structure_has_field_typed(container, "framerate", GST_TYPE_FRACTION))
>>> + return;
>>> +
>>> + auto iterFrameDuration = cam_ctrls.find(controls::FrameDurationLimits.id());
>>> + if (iterFrameDuration == cam_ctrls.end()) {
>>> + GST_WARNING("FrameDurationLimits not found in camera controls.");
>>> + 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) {
> I don't think we need to open code clamp() here:
>
> int64_t target_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>();
>
> int64_t frame_duration = std::clamp(target_duration,
> min_frame_duration,
> max_frame_duration);
>
> if (frame_duration != target_duration) {
ack.
>
>
>>> + gint framerate_clamped = 1000000 / frame_duration;
>>> +
>>> + /* Update the framerate which then will be exposed in caps. */
>>> + gst_structure_set(container, "framerate", GST_TYPE_FRACTION,
>>> + framerate_clamped, 1, nullptr);
>> So the logic here is that, if the input is too high/low fps - it will be
>> bound to min/max FrameDuration and exposed it caps.
>>
>> It doesn't mean, the camera will go on to stream ... it means that
>> bound-limit will be exposed in caps but the negotiation will probably
>> fail (representing that the fps requested was too high/low and not
>> supported by the camera)
>>
>> For e.g.
>>
>> Input: framerate=100/1
>> [[capped to framerate=43/1 by this patch on OV5647 ]]
>> Exposed in caps : framerate=43/1
>> [[negotation failed]]
>> Expected framerate=100/1 but got framerate=43/1
>>
>> It seems to me this is expected. But I am not 100% sure here.
>>
>> Can the expectation be to stream the camera somehow (not sure how to
>> surpass negotitaiton in caps) with the bound-limit set? Probably worth
>> exploring other gstreamer plugins specific to framerate handling...
> Indeed, this seems to be how the situation is reported by a user on
> github:
> - https://github.com/raspberrypi/libcamera/issues/30#issuecomment-1301117923
Indeed.
>
> I believe it's the correct behaviour. If a pipeline explicitly requests 1000 FPS, and
> the camera can not deliver, it is a failure to negotiate. But it's worth
> checking on #gstreamer or hearing from Nicolas ?
>
> - I've asked on #gstreamer ...
The discussion on #gstreamer is on the lines that, continue with best
efforts mode.
Which means we (unconditionally) need to report the caps back to
whatever is requested (maybe 1000/1 fps is requested) and continue
silently with max fps achieveable inside libcamera.
I don't like this (as it violates WYSIWYG) and we allow users to set
very absurd framerate values. Any thoughts?
>
>
>
>>> + }
>>> +
>>> + initCtrls.set(controls::FrameDurationLimits,
>>> + { frame_duration, frame_duration });
>>> +}
>>> +
>>> +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..955d6eac 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");
>>> +
> Container seems to be a really non-descript term here ...
>
> Is this only used for framerate or is it expected to be used to contain
> 'anything' ?
It can contain additional fields in the future. For now, it's only
framerate.
>
>>> GST_DEBUG_OBJECT(self, "Streaming thread has started");
>>>
>>> gint stream_id_num = 0;
>>> @@ -504,6 +507,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);
> maybe container could be called 'framerate' then ? It's going to store
> the framerate right ?
I essentionally introduced a container so that incoming caps data can be
bundled together and filtered accordingly. For e.g.
+ gst_libcamera_get_<new_caps_field>_from_caps(caps, container);
As we introduce more caps support - we can just set them in the
container, write a helper function to check/sanitise the values and use
it to expose in supported caps for negotiation. Hence, the 'generic'
move-around container from the start.
(I agree 'container' is not a good naming shceme, I'll ponder over that)
>
>>> }
>>>
>>> if (flow_ret != GST_FLOW_OK)
>>> @@ -525,6 +529,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 +540,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_libcamera_framerate_to_caps took a GstStructure *, it can do the
> conversion in that function, and you can use the name 'framerate' for
> the structure variable in this scope.
Not sure if I understand this fully, but yes,
gst_libcamera_framerate_to_caps() can take a GstStructure and set the
framerate into caps. It will complement the
gst_libcamera_get_framerate_from_caps() signature.
>
>>> +
>>> if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) {
>>> flow_ret = GST_FLOW_NOT_NEGOTIATED;
>>> break;
>>> @@ -567,7 +578,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 +588,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
>>> }
>>>
>>> done:
>>> + state->initControls_.clear();
>>> + g_free(container);
> Does the gstreamer framework provide any smart unique pointer to hold
> the container that would free automatically when this function goes out
> of scope ?
Will check. g_autoptr() comes to find or maybe something similar.
>
> Aside from all that, which really are solveable /minors /bikeshedding:
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
>
>>> switch (flow_ret) {
>>> case GST_FLOW_NOT_NEGOTIATED:
>>> GST_ELEMENT_FLOW_ERROR(self, flow_ret);
More information about the libcamera-devel
mailing list