[libcamera-devel] [PATCH v3] gstreamer: Provide framerate support for libcamerasrc.
Umang Jain
umang.jain at ideasonboard.com
Thu Sep 8 10:35:12 CEST 2022
Hi Rishi,
On 9/8/22 11:36 AM, Rishikesh Donadkar wrote:
>
> > +
> > + gst_structure_set(s,
> > + "width", G_TYPE_INT, stream_cfg.size.width,
> > + "height", G_TYPE_INT,
> stream_cfg.size.height,
> > + nullptr);
> > +
> > + if (ret) {
> > + gint numerator, denominator;
> > + numerator =
> gst_value_get_fraction_numerator(framerate_container);
> > + denominator =
> gst_value_get_fraction_denominator(framerate_container);
> > + gst_structure_set(s, "framerate",
> GST_TYPE_FRACTION, numerator, denominator, nullptr);
> > + } else {
> > + gst_structure_set(s, "framerate",
> GST_TYPE_FRACTION, 0, 1, nullptr);
> > + }
> > +
> > + if (stream_cfg.colorSpace) {
>
> ?!
>
> I am not getting why you are introducing code related to colorspace
> here? Is it a copy/paste fiasco?
>
> As new caps are exposed. Shouldn't we expose everything that has been
> decided on the libcamera side that includes colorspace, resolutions,
> format and framerate?
err yes... It's not very nice though...
I wish gstreamer has/had mechanism to provide / update caps on the fly
(even partially). I have looked deep into other elements but maybe there
is a possbile way to do that, however [1] suggests there is no event to
support it.
Another option I'm actually considering is moving the new caps event for
loop (just above state->cam_->configure()) after the configure(). So, we
would go from:
streamCfg->validate();
// new caps event
camera::configure();
to
streamCfg->validate();
camera::configure();
// new caps event
Which seems logical to me i.e. announcing the caps after the camera is
actually configured. This will also help us with framerate and I don't
see any harm in announcing the caps after camera is configured (but I
maybe wrong, so requires some testing on RPi)
Does it makes sense? What do you think?
[1]
https://gstreamer.freedesktop.org/documentation/gstreamer/gstevent.html?gi-language=c#GstEventType
>
>
> On Thu, Sep 8, 2022 at 11:15 AM Umang Jain
> <umang.jain at ideasonboard.com> wrote:
>
> Hi Rishi,
>
> On 9/7/22 4:17 PM, Rishikesh Donadkar wrote:
> > 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_
> >
> > Passing initControls_ 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.
> >
> > Check the frame_duration whether it is in-bound to the max / min
> > frame-duration supported by the camera using the function
> > gst_libcamera_bind_framerate().
> >
> > If the frame_duartion is out of bounds, bind the frame_duration
> > between the max and min FrameDuration that is supported by the
> camera.
> >
> > Pass the initControls_ at the time of starting camera.
> >
> > Solve the complications in exposing the correct framerate due to
> loss in
> > precision as a result of casting the frameduration to
> int64_t(which is
> > required in libcamera to set the FrameDurationLimits control).
> >
> > Example -
> >
> > * Suppose the framerate requested is 35/1. The framerate is read
> form
> > the caps in the from of fraction that has a numerator and
> > denominator.
> >
> > * Converting to FrameDuration (in microseconds)
> > (1 * 10^6) / 35 = 28571.4286
> > int64_t frame_duration = 28571
> > (the precision here is lost.)
> > * To expose the framerate in caps, Inverting the frame_duration
> to get
> > back the framerate and converting to Seconds.
> > double framerate = 10^6 / 28571
> > and
> > 10^6/28571 which is close but not equal to 35/1 will fail
> the negotiation.
> >
> > To solve the above problem, Store the framerate requested through
> > negotiated caps in the GValue of the type GST_TYPE_FRACTION.
> >
> > Try to apply the framerate and check if the floor value of
> framerate that gets applied
> > closely matches with the floor value framerate requested in the
> negotiated caps. If
> > the value matches expose the framerate that is stored in the
> framerate_container, else expose
> > the framerate in the new caps as 0/1.
> >
> > Pass the initControls_ at camera->start().
> >
> > ---
> > Changes from v1 to v2
> > * Use GValue of the type GST_TYPE_FRACTION instead of std::pair
> to store the framerate.
> > * Don't shift the call to camera->configure(), instead bound the
> framerate
> > after the call to camera->configure().
> > * Reset the FrameDurationLimits control only if it is out of bounds.
> > * Expose the caps containing framerate information with a new
> CAPS event
> > after the call to camera->configure().
> > ---
> >
> > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>
> As I haven't contributed to any LOC of this patch, please drop of my
> s-o-b tag.
>
> Non-existent s-o-b on behalf of others shouldn't be applied ideally
> > Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar at gmail.com>
> > ---
> > src/gstreamer/gstlibcamera-utils.cpp | 116
> +++++++++++++++++++++++++++
> > src/gstreamer/gstlibcamera-utils.h | 8 ++
> > src/gstreamer/gstlibcamerasrc.cpp | 28 ++++++-
> > 3 files changed, 151 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp
> b/src/gstreamer/gstlibcamera-utils.cpp
> > index 4df5dd6c..e862f7ea 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;
> > @@ -405,6 +406,121 @@
> gst_libcamera_configure_stream_from_caps(StreamConfiguration
> &stream_cfg,
> > }
> > }
> >
> > +void
> > +gst_libcamera_get_init_controls_from_caps(ControlList
> &controls, [[maybe_unused]] GstCaps *caps,
> > + GValue *framerate_container)
> > +{
> > + GstStructure *s = gst_caps_get_structure(caps, 0);
> > + gint fps_n = -1, 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.");
> > + return;
> > + }
> > + }
> > +
> > + if (fps_n < 0 || fps_d < 0)
> > + return;
> > +
> > + gst_value_set_fraction(framerate_container, fps_n, fps_d);
> > + int64_t frame_duration = (fps_d * 1000000.0) / fps_n;
> > +
> > + controls.set(controls::FrameDurationLimits,
> > + Span<const int64_t, 2>({ frame_duration,
> frame_duration }));
>
> setting the frame_duration even before bound-checking is wrong. This
> Function should ideally get the 'framerate' and just save it for
> later
> use/bound-checking after camera::configure()
>
> Once the bound checking is successfully, only then you are allowed to
> set frame_duration on initControls_
> > +}
> > +
> > +void
> > +gst_libcamera_bind_framerate(const ControlInfoMap
> &camera_controls, ControlList &controls)
>
> maybe
> gst_libcamera_framerate_clamp()
> OR
> gst_libcamera_framerate_bounds_check()
> > +{
> > + gboolean isBound = true;
> > + auto iterFrameDuration =
> camera_controls.find(controls::FrameDurationLimits.id());
> > +
> > + if (!(iterFrameDuration != camera_controls.end() &&
> > + controls.contains(controls::FRAME_DURATION_LIMITS)))
> > + return;
> > +
> > + int64_t frame_duration =
> controls.get(controls::FrameDurationLimits).value()[0];
> > + 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 {
> > + isBound = false;
> > + }
> > +
> > + if (isBound)
> > + controls.set(controls::FrameDurationLimits,
> > + Span<const int64_t, 2>({
> frame_duration, frame_duration }));
> > +}
> > +
> > +gboolean
> > +gst_libcamera_get_framerate_from_init_controls(const
> ControlList &controls, GValue *framerate,
> > + GValue
> *framerate_container)
> > +{
> > + gint fps_caps_n, fps_caps_d, numerator, denominator;
> > +
> > + fps_caps_n =
> gst_value_get_fraction_numerator(framerate_container);
> > + fps_caps_d =
> gst_value_get_fraction_denominator(framerate_container);
> > +
> > + if (!controls.contains(controls::FRAME_DURATION_LIMITS))
> > + return false;
> > +
> > + double framerate_ret = 1000000 /
> static_cast<double>(controls.get(controls::FrameDurationLimits).value()[0]);
> > + gst_util_double_to_fraction(framerate_ret, &numerator,
> &denominator);
> > + gst_value_set_fraction(framerate, numerator, denominator);
> > +
> > + if (fps_caps_n / fps_caps_d == numerator / denominator) {
> > + return true;
> > + }
> > + return false;
> > +}
> > +
> > +GstCaps *
> > +gst_libcamera_init_controls_to_caps(const ControlList
> &controls, const StreamConfiguration &stream_cfg,
> > + GValue *framerate_container)
> > +{
> > + GstCaps *caps = gst_caps_new_empty();
> > + GstStructure *s =
> bare_structure_from_format(stream_cfg.pixelFormat);
> > + gboolean ret;
> > + GValue *framerate = g_new0(GValue, 1);
>
> Why create a new GValue here?
>
> You have everything you need through framerate_container, a
> applied and
> bound-checked framerate, you just need to query the fraction from
> it and
> set the structure?
> > + g_value_init(framerate, GST_TYPE_FRACTION);
> > +
> > + ret =
> gst_libcamera_get_framerate_from_init_controls(controls,
> framerate, framerate_container);
>
> As per the above comment, I am not sure if you really need to do
> that.
> Seems over-kill
> > +
> > + gst_structure_set(s,
> > + "width", G_TYPE_INT, stream_cfg.size.width,
> > + "height", G_TYPE_INT,
> stream_cfg.size.height,
> > + nullptr);
> > +
> > + if (ret) {
> > + gint numerator, denominator;
> > + numerator =
> gst_value_get_fraction_numerator(framerate_container);
> > + denominator =
> gst_value_get_fraction_denominator(framerate_container);
> > + gst_structure_set(s, "framerate",
> GST_TYPE_FRACTION, numerator, denominator, nullptr);
> > + } else {
> > + gst_structure_set(s, "framerate",
> GST_TYPE_FRACTION, 0, 1, nullptr);
> > + }
> > +
> > + if (stream_cfg.colorSpace) {
>
> ?!
>
> I am not getting why you are introducing code related to colorspace
> here? Is it a copy/paste fiasco?
> > + GstVideoColorimetry colorimetry =
> colorimetry_from_colorspace(stream_cfg.colorSpace.value());
> > + gchar *colorimetry_str =
> gst_video_colorimetry_to_string(&colorimetry);
> > +
> > + if (colorimetry_str)
> > + gst_structure_set(s, "colorimetry",
> G_TYPE_STRING, colorimetry_str, nullptr);
> > + else
> > + g_error("Got invalid colorimetry from
> ColorSpace: %s",
> > + ColorSpace::toString(stream_cfg.colorSpace).c_str());
> > + }
> > +
> > + gst_caps_append_structure(caps, s);
> > + g_free(framerate);
> > + return caps;
> > +}
> > +
> > #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..955a717d 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,13 @@ 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_init_controls_from_caps(libcamera::ControlList
> &controls,
> > + GstCaps *caps,
> GValue *framerate_container);
> > +void gst_libcamera_bind_framerate(const
> libcamera::ControlInfoMap &camera_controls,
> > + libcamera::ControlList &controls);
> > +GstCaps *gst_libcamera_init_controls_to_caps(const
> libcamera::ControlList &controls,
> > + const
> libcamera::StreamConfiguration &streame_cfg, GValue
> *framerate_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 16d70fea..0c981357 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();
> > @@ -461,6 +462,8 @@ gst_libcamera_src_task_enter(GstTask *task,
> [[maybe_unused]] GThread *thread,
> > GstLibcameraSrcState *state = self->state;
> > GstFlowReturn flow_ret = GST_FLOW_OK;
> > gint ret;
> > + GValue *framerate_container = g_new0(GValue, 1);
> > + framerate_container = g_value_init(framerate_container,
> GST_TYPE_FRACTION);
>
> Since it's a GValue, I would not make it framerate_* specific, so I
> would just rename and remove all "framerate_" specifics.
>
> In future, when we have to parse more parameters as a part of
> initControls_ setting, the same GValue can be used to temporarily
> hold
> all other values
> >
> > GST_DEBUG_OBJECT(self, "Streaming thread has started");
> >
> > @@ -496,6 +499,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 +508,8 @@ 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_init_controls_from_caps(state->initControls_, caps,
> > + framerate_container);
>
> passing state->initControls_ is un-necessary here as we don't need to
> set frame_duration on it, just right now (it has to go through bound
> check first which is below...)
> > }
> >
> > if (flow_ret != GST_FLOW_OK)
> > @@ -524,6 +530,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;
> > @@ -544,6 +551,23 @@ gst_libcamera_src_task_enter(GstTask *task,
> [[maybe_unused]] GThread *thread,
> > return;
> > }
> >
> > + /* bind the framerate */
>
> s/bind/Clamp OR
> /* Check framerate bounds within controls::FrameDurationLimits */
> > + gst_libcamera_bind_framerate(state->cam_->controls(),
> state->initControls_);
>
> I would just pass the controls::FrameDurationLimits and GValue
> (containing the asked framerate) and perform the bound check.
>
> You can update the GValue's framerate in that function after which
> simply, set the initControls' ::FrameDurationLimits right here.
> > +
> > + /* Expose the controls in initControls_ throught a new
> caps event. */
> > + for (gsize i = 0; i < state->srcpads_.size(); i++) {
> > + GstPad *srcpad = state->srcpads_[i];
> > + const StreamConfiguration &stream_cfg =
> state->config_->at(i);
> > +
> > + g_autoptr(GstCaps) caps =
> gst_libcamera_init_controls_to_caps(state->initControls_,
> > + stream_cfg, framerate_container);
> > +
> > + if (!gst_pad_push_event(srcpad,
> gst_event_new_caps(caps))) {
> > + flow_ret = GST_FLOW_NOT_NEGOTIATED;
> > + break;
> > + }
> > + }
> > +
> > self->allocator = gst_libcamera_allocator_new(state->cam_,
> state->config_.get());
> > if (!self->allocator) {
> > GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,
> > @@ -566,7 +590,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 +600,8 @@ gst_libcamera_src_task_enter(GstTask *task,
> [[maybe_unused]] GThread *thread,
> > }
> >
> > done:
> > + state->initControls_.clear();
> > + g_free(framerate_container);
> > switch (flow_ret) {
> > case GST_FLOW_NOT_NEGOTIATED:
> > GST_ELEMENT_FLOW_ERROR(self, flow_ret);
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220908/873fbcd0/attachment.htm>
More information about the libcamera-devel
mailing list