[libcamera-devel] [PATCH v6 2/2] gstreamer: Provide framerate support for libcamerasrc
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Nov 21 22:30:47 CET 2022
Quoting Nicolas Dufresne (2022-11-21 18:30:01)
> Le vendredi 04 novembre 2022 à 11:25 +0000, Kieran Bingham via libcamera-devel a
> écrit :
> > 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.. ?
>
> You may have multiple sturctures like:
>
> video/x-raw,framerate=30/1;video/x-raw
>
> That indicates a preference for 30fps, but does not limite to other framerate.
> This is always what you'd get by using a rate adapter like videorate. In theory,
> a better approach would be to iterate all the structure, not just the very first
> one.
Is there any existing code that parses the structures that would be a
helpful reference here ?
Do we just keep trying to get a structure until we either, get a
nullpointer, or find a satisfactory/satisfiable frame rate result ?
> >
> > > > + }
> > > > +
> > > > + 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) {
> >
> >
> > > > + 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
> >
> >
> > 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 ...
> >
> >
> >
> > > > + }
> > > > +
> > > > + 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' ?
> >
> > > > 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 ?
> >
> > > > }
> > > >
> > > > 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.
> >
> > > > +
> > > > 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 ?
> >
> > 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