[{"id":24956,"web_url":"https://patchwork.libcamera.org/comment/24956/","msgid":"<96c8f5d4-a742-f5a9-a9db-8bb1bb30f67d@ideasonboard.com>","date":"2022-09-08T05:45:06","subject":"Re: [libcamera-devel] [PATCH v3] gstreamer: Provide framerate\n\tsupport for libcamerasrc.","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Rishi,\n\nOn 9/7/22 4:17 PM, Rishikesh Donadkar wrote:\n> Add a field of the type ControlList to GstLibcameraSrcState.\n>\n> Get the framerate from the caps and convert it to FrameDuration.\n> Set the FrameDurationLimits control in the initControls_\n>\n> Passing initControls_ at camera::start() doesn't tell us if the controls\n> have been applied to the camera successfully or not (or they have adjusted in\n> some fashion). Until that is introduced in camera::start() API,\n> we need to carry out such handling on the application side.\n>\n> Check the frame_duration whether it is in-bound to the max / min\n> frame-duration supported by the camera using the function\n> gst_libcamera_bind_framerate().\n>\n> If the frame_duartion is out of bounds, bind the frame_duration\n> between the max and min FrameDuration that is supported by the camera.\n>\n> Pass the initControls_ at the time of starting camera.\n>\n> Solve the complications in exposing the correct framerate due to loss in\n> precision as a result of casting the frameduration to int64_t(which is\n> required in libcamera to set the FrameDurationLimits control).\n>\n> Example -\n>\n> * Suppose the framerate requested is 35/1. The framerate is read form\n>    the caps in the from of fraction that has a numerator and\n>    denominator.\n>\n> * Converting to FrameDuration (in microseconds)\n>      (1 * 10^6) / 35 = 28571.4286\n>      int64_t frame_duration = 28571\n>      (the precision here is lost.)\n> * To expose the framerate in caps, Inverting the frame_duration to get\n>    back the framerate and converting to Seconds.\n>      double framerate = 10^6 / 28571\n>      and\n>      10^6/28571 which is close but not equal to 35/1 will fail the negotiation.\n>\n> To solve the above problem, Store the framerate requested through\n> negotiated caps in the GValue of the type GST_TYPE_FRACTION.\n>\n> Try to apply the framerate and check if the floor value of framerate that gets applied\n> closely matches with the floor value framerate requested in the negotiated caps. If\n> the value matches expose the framerate that is stored in the framerate_container, else expose\n> the framerate in the new caps as 0/1.\n>\n> Pass the initControls_ at camera->start().\n>\n> ---\n> Changes from v1 to v2\n> * Use GValue of the type GST_TYPE_FRACTION instead of std::pair to store the framerate.\n> * Don't shift the call to camera->configure(), instead bound the framerate\n>    after the call to camera->configure().\n> * Reset the FrameDurationLimits control only if it is out of bounds.\n> * Expose the caps containing framerate information with a new CAPS event\n>    after the call to camera->configure().\n> ---\n>\n> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n\nAs I haven't contributed to any LOC of this patch, please drop of my \ns-o-b tag.\n\nNon-existent s-o-b on behalf of others shouldn't be applied ideally\n> Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com>\n> ---\n>   src/gstreamer/gstlibcamera-utils.cpp | 116 +++++++++++++++++++++++++++\n>   src/gstreamer/gstlibcamera-utils.h   |   8 ++\n>   src/gstreamer/gstlibcamerasrc.cpp    |  28 ++++++-\n>   3 files changed, 151 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> index 4df5dd6c..e862f7ea 100644\n> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> @@ -8,6 +8,7 @@\n>   \n>   #include \"gstlibcamera-utils.h\"\n>   \n> +#include <libcamera/control_ids.h>\n>   #include <libcamera/formats.h>\n>   \n>   using namespace libcamera;\n> @@ -405,6 +406,121 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n>   \t}\n>   }\n>   \n> +void\n> +gst_libcamera_get_init_controls_from_caps(ControlList &controls, [[maybe_unused]] GstCaps *caps,\n> +\t\t\t\t\t  GValue *framerate_container)\n> +{\n> +\tGstStructure *s = gst_caps_get_structure(caps, 0);\n> +\tgint fps_n = -1, fps_d = -1;\n> +\n> +\tif (gst_structure_has_field_typed(s, \"framerate\", GST_TYPE_FRACTION)) {\n> +\t\tif (!gst_structure_get_fraction(s, \"framerate\", &fps_n, &fps_d)) {\n> +\t\t\tGST_WARNING(\"invalid framerate in the caps.\");\n> +\t\t\treturn;\n> +\t\t}\n> +\t}\n> +\n> +\tif (fps_n < 0 || fps_d < 0)\n> +\t\treturn;\n> +\n> +\tgst_value_set_fraction(framerate_container, fps_n, fps_d);\n> +\tint64_t frame_duration = (fps_d * 1000000.0) / fps_n;\n> +\n> +\tcontrols.set(controls::FrameDurationLimits,\n> +\t\t     Span<const int64_t, 2>({ frame_duration, frame_duration }));\n\nsetting the frame_duration even before bound-checking is wrong. This \nFunction should ideally get the 'framerate' and just save it for later \nuse/bound-checking after camera::configure()\n\nOnce the bound checking is successfully, only then you are allowed to \nset frame_duration on initControls_\n> +}\n> +\n> +void\n> +gst_libcamera_bind_framerate(const ControlInfoMap &camera_controls, ControlList &controls)\n\nmaybe\ngst_libcamera_framerate_clamp()\nOR\ngst_libcamera_framerate_bounds_check()\n> +{\n> +\tgboolean isBound = true;\n> +\tauto iterFrameDuration = camera_controls.find(controls::FrameDurationLimits.id());\n> +\n> +\tif (!(iterFrameDuration != camera_controls.end() &&\n> +\t      controls.contains(controls::FRAME_DURATION_LIMITS)))\n> +\t\treturn;\n> +\n> +\tint64_t frame_duration = controls.get(controls::FrameDurationLimits).value()[0];\n> +\tint64_t min_frame_duration = iterFrameDuration->second.min().get<int64_t>();\n> +\tint64_t max_frame_duration = iterFrameDuration->second.max().get<int64_t>();\n> +\n> +\tif (frame_duration < min_frame_duration) {\n> +\t\tframe_duration = min_frame_duration;\n> +\t} else if (frame_duration > max_frame_duration) {\n> +\t\tframe_duration = max_frame_duration;\n> +\t} else {\n> +\t\tisBound = false;\n> +\t}\n> +\n> +\tif (isBound)\n> +\t\tcontrols.set(controls::FrameDurationLimits,\n> +\t\t\t     Span<const int64_t, 2>({ frame_duration, frame_duration }));\n> +}\n> +\n> +gboolean\n> +gst_libcamera_get_framerate_from_init_controls(const ControlList &controls, GValue *framerate,\n> +\t\t\t\t\t       GValue *framerate_container)\n> +{\n> +\tgint fps_caps_n, fps_caps_d, numerator, denominator;\n> +\n> +\tfps_caps_n = gst_value_get_fraction_numerator(framerate_container);\n> +\tfps_caps_d = gst_value_get_fraction_denominator(framerate_container);\n> +\n> +\tif (!controls.contains(controls::FRAME_DURATION_LIMITS))\n> +\t\treturn false;\n> +\n> +\tdouble framerate_ret = 1000000 / static_cast<double>(controls.get(controls::FrameDurationLimits).value()[0]);\n> +\tgst_util_double_to_fraction(framerate_ret, &numerator, &denominator);\n> +\tgst_value_set_fraction(framerate, numerator, denominator);\n> +\n> +\tif (fps_caps_n / fps_caps_d == numerator / denominator) {\n> +\t\treturn true;\n> +\t}\n> +\treturn false;\n> +}\n> +\n> +GstCaps *\n> +gst_libcamera_init_controls_to_caps(const ControlList &controls, const StreamConfiguration &stream_cfg,\n> +\t\t\t\t    GValue *framerate_container)\n> +{\n> +\tGstCaps *caps = gst_caps_new_empty();\n> +\tGstStructure *s = bare_structure_from_format(stream_cfg.pixelFormat);\n> +\tgboolean ret;\n> +\tGValue *framerate = g_new0(GValue, 1);\n\nWhy create a new GValue here?\n\nYou have everything you need through framerate_container, a applied and \nbound-checked framerate, you just need to query the fraction from it and \nset the structure?\n> +\tg_value_init(framerate, GST_TYPE_FRACTION);\n> +\n> +\tret = gst_libcamera_get_framerate_from_init_controls(controls, framerate, framerate_container);\n\nAs per the above comment, I am not sure if you really need to do that. \nSeems over-kill\n> +\n> +\tgst_structure_set(s,\n> +\t\t\t  \"width\", G_TYPE_INT, stream_cfg.size.width,\n> +\t\t\t  \"height\", G_TYPE_INT, stream_cfg.size.height,\n> +\t\t\t  nullptr);\n> +\n> +\tif (ret) {\n> +\t\tgint numerator, denominator;\n> +\t\tnumerator = gst_value_get_fraction_numerator(framerate_container);\n> +\t\tdenominator = gst_value_get_fraction_denominator(framerate_container);\n> +\t\tgst_structure_set(s, \"framerate\", GST_TYPE_FRACTION, numerator, denominator, nullptr);\n> +\t} else {\n> +\t\tgst_structure_set(s, \"framerate\", GST_TYPE_FRACTION, 0, 1, nullptr);\n> +\t}\n> +\n> +\tif (stream_cfg.colorSpace) {\n\n?!\n\nI am not getting why you are introducing code related to colorspace \nhere? Is it a copy/paste fiasco?\n> +\t\tGstVideoColorimetry colorimetry = colorimetry_from_colorspace(stream_cfg.colorSpace.value());\n> +\t\tgchar *colorimetry_str = gst_video_colorimetry_to_string(&colorimetry);\n> +\n> +\t\tif (colorimetry_str)\n> +\t\t\tgst_structure_set(s, \"colorimetry\", G_TYPE_STRING, colorimetry_str, nullptr);\n> +\t\telse\n> +\t\t\tg_error(\"Got invalid colorimetry from ColorSpace: %s\",\n> +\t\t\t\tColorSpace::toString(stream_cfg.colorSpace).c_str());\n> +\t}\n> +\n> +\tgst_caps_append_structure(caps, s);\n> +\tg_free(framerate);\n> +\treturn caps;\n> +}\n> +\n>   #if !GST_CHECK_VERSION(1, 17, 1)\n>   gboolean\n>   gst_task_resume(GstTask *task)\n> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h\n> index 164189a2..955a717d 100644\n> --- a/src/gstreamer/gstlibcamera-utils.h\n> +++ b/src/gstreamer/gstlibcamera-utils.h\n> @@ -9,6 +9,7 @@\n>   #pragma once\n>   \n>   #include <libcamera/camera_manager.h>\n> +#include <libcamera/controls.h>\n>   #include <libcamera/stream.h>\n>   \n>   #include <gst/gst.h>\n> @@ -18,6 +19,13 @@ GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &fo\n>   GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg);\n>   void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg,\n>   \t\t\t\t\t      GstCaps *caps);\n> +void gst_libcamera_get_init_controls_from_caps(libcamera::ControlList &controls,\n> +\t\t\t\t\t       GstCaps *caps, GValue *framerate_container);\n> +void gst_libcamera_bind_framerate(const libcamera::ControlInfoMap &camera_controls,\n> +\t\t\t\t  libcamera::ControlList &controls);\n> +GstCaps *gst_libcamera_init_controls_to_caps(const libcamera::ControlList &controls,\n> +\t\t\t\t\t     const libcamera::StreamConfiguration &streame_cfg, GValue *framerate_container);\n> +\n>   #if !GST_CHECK_VERSION(1, 17, 1)\n>   gboolean gst_task_resume(GstTask *task);\n>   #endif\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 16d70fea..0c981357 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -131,6 +131,7 @@ struct GstLibcameraSrcState {\n>   \tstd::queue<std::unique_ptr<RequestWrap>> completedRequests_\n>   \t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n>   \n> +\tControlList initControls_;\n>   \tguint group_id_;\n>   \n>   \tint queueRequest();\n> @@ -461,6 +462,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n>   \tGstLibcameraSrcState *state = self->state;\n>   \tGstFlowReturn flow_ret = GST_FLOW_OK;\n>   \tgint ret;\n> +\tGValue *framerate_container = g_new0(GValue, 1);\n> +\tframerate_container = g_value_init(framerate_container, GST_TYPE_FRACTION);\n\nSince it's a GValue, I would not make it framerate_* specific, so I \nwould just rename and remove all \"framerate_\" specifics.\n\nIn future, when we have to parse more parameters as a part of \ninitControls_ setting, the same GValue can be used to temporarily hold \nall other values\n>   \n>   \tGST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n>   \n> @@ -496,6 +499,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n>   \t\t/* Retrieve the supported caps. */\n>   \t\tg_autoptr(GstCaps) filter = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());\n>   \t\tg_autoptr(GstCaps) caps = gst_pad_peer_query_caps(srcpad, filter);\n> +\n>   \t\tif (gst_caps_is_empty(caps)) {\n>   \t\t\tflow_ret = GST_FLOW_NOT_NEGOTIATED;\n>   \t\t\tbreak;\n> @@ -504,6 +508,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n>   \t\t/* Fixate caps and configure the stream. */\n>   \t\tcaps = gst_caps_make_writable(caps);\n>   \t\tgst_libcamera_configure_stream_from_caps(stream_cfg, caps);\n> +\t\tgst_libcamera_get_init_controls_from_caps(state->initControls_, caps,\n> +\t\t\t\t\t\t\t  framerate_container);\n\npassing state->initControls_ is un-necessary here as we don't need to \nset frame_duration on it, just right now (it has to go through bound \ncheck first which is below...)\n>   \t}\n>   \n>   \tif (flow_ret != GST_FLOW_OK)\n> @@ -524,6 +530,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n>   \t\tconst StreamConfiguration &stream_cfg = state->config_->at(i);\n>   \n>   \t\tg_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg);\n> +\n>   \t\tif (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) {\n>   \t\t\tflow_ret = GST_FLOW_NOT_NEGOTIATED;\n>   \t\t\tbreak;\n> @@ -544,6 +551,23 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n>   \t\treturn;\n>   \t}\n>   \n> +\t/* bind the framerate */\n\ns/bind/Clamp OR\n/* Check framerate bounds within  controls::FrameDurationLimits */\n> +\tgst_libcamera_bind_framerate(state->cam_->controls(), state->initControls_);\n\nI would just pass the controls::FrameDurationLimits and GValue \n(containing the asked framerate) and perform the bound check.\n\nYou can update the GValue's framerate in that function after which \nsimply, set the initControls' ::FrameDurationLimits right here.\n> +\n> +\t/* Expose the controls in initControls_ throught a new caps event. */\n> +\tfor (gsize i = 0; i < state->srcpads_.size(); i++) {\n> +\t\tGstPad *srcpad = state->srcpads_[i];\n> +\t\tconst StreamConfiguration &stream_cfg = state->config_->at(i);\n> +\n> +\t\tg_autoptr(GstCaps) caps = gst_libcamera_init_controls_to_caps(state->initControls_,\n> +\t\t\t\t\t\t\t\t\t      stream_cfg, framerate_container);\n> +\n> +\t\tif (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) {\n> +\t\t\tflow_ret = GST_FLOW_NOT_NEGOTIATED;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n>   \tself->allocator = gst_libcamera_allocator_new(state->cam_, state->config_.get());\n>   \tif (!self->allocator) {\n>   \t\tGST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,\n> @@ -566,7 +590,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n>   \t\tgst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n>   \t}\n>   \n> -\tret = state->cam_->start();\n> +\tret = state->cam_->start(&state->initControls_);\n>   \tif (ret) {\n>   \t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n>   \t\t\t\t  (\"Failed to start the camera: %s\", g_strerror(-ret)),\n> @@ -576,6 +600,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n>   \t}\n>   \n>   done:\n> +\tstate->initControls_.clear();\n> +\tg_free(framerate_container);\n>   \tswitch (flow_ret) {\n>   \tcase GST_FLOW_NOT_NEGOTIATED:\n>   \t\tGST_ELEMENT_FLOW_ERROR(self, flow_ret);","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4E064C3272\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  8 Sep 2022 05:45:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B824962097;\n\tThu,  8 Sep 2022 07:45:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A00036208F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  8 Sep 2022 07:45:12 +0200 (CEST)","from [IPV6:2401:4900:1f3e:50f3:a89c:93ec:bcd7:8b90] (unknown\n\t[IPv6:2401:4900:1f3e:50f3:a89c:93ec:bcd7:8b90])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 386536CC;\n\tThu,  8 Sep 2022 07:45:11 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1662615913;\n\tbh=CXy4bjdwPHEguXSHQp48+lKdVicAJubxj1D3QBbkhNY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Lxs24gxtnOonKnN1XvxIH9a3BgZt2ZT9sbrzbA6bjpkQNmm5BDpJawTG8PW7qcwgZ\n\t5J33gNt4GZLAi6HhDG3vBvyNSzLFU5twN/9DPJ3j2M19o2GpUOW3w1akDkXi+45nFu\n\tvy5lKvrv8GhGaLZnajPjmnu5fCmgp/si91XYvk9It59x8+Wmt+NgANPNuevN2xj6ax\n\t5zhOXYJKwpaP0n9s29Vqi91HxOU+n8yNdWek5J0OL0c3wWrxySK9oKsBMZ7aBojBE4\n\t3tdSqwPItFEpOQUyTLRFaJKrjV35x5FBmpyAujEdZmg9z+ZfpN6h8btDcky1MDMue1\n\tWdhpkwFswFvqw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1662615912;\n\tbh=CXy4bjdwPHEguXSHQp48+lKdVicAJubxj1D3QBbkhNY=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Zi88Nly697s+vRYTS8Ema7Xz/K9pYiufkikfBpXpXn9Wi8iy9skVdWSq2WyUqnwK/\n\tL8s1a5edtpZCP2KjwJm8uyy2AHOp3Q2qnztqMypQYTGHpONQPj/II7a6+GDwUM46EU\n\tVkOmIwSoKlMYEHroGFqj071PSFDfm4oRCBGiS8Yc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Zi88Nly6\"; dkim-atps=neutral","Message-ID":"<96c8f5d4-a742-f5a9-a9db-8bb1bb30f67d@ideasonboard.com>","Date":"Thu, 8 Sep 2022 11:15:06 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.12.0","Content-Language":"en-US","To":"Rishikesh Donadkar <rishikeshdonadkar@gmail.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220907104711.87365-1-rishikeshdonadkar@gmail.com>","In-Reply-To":"<20220907104711.87365-1-rishikeshdonadkar@gmail.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3] gstreamer: Provide framerate\n\tsupport for libcamerasrc.","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Umang Jain via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Umang Jain <umang.jain@ideasonboard.com>","Cc":"vedantparanjape160201@gmail.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":24957,"web_url":"https://patchwork.libcamera.org/comment/24957/","msgid":"<CAEQmg0mq8GSkDYLGF4TDXoS4wWTHyrNhFgoismhs3x-5TxB9gQ@mail.gmail.com>","date":"2022-09-08T06:06:24","subject":"Re: [libcamera-devel] [PATCH v3] gstreamer: Provide framerate\n\tsupport for libcamerasrc.","submitter":{"id":118,"url":"https://patchwork.libcamera.org/api/people/118/","name":"Rishikesh Donadkar","email":"rishikeshdonadkar@gmail.com"},"content":">\n> > +\n> > +     gst_structure_set(s,\n> > +                       \"width\", G_TYPE_INT, stream_cfg.size.width,\n> > +                       \"height\", G_TYPE_INT, stream_cfg.size.height,\n> > +                       nullptr);\n> > +\n> > +     if (ret) {\n> > +             gint numerator, denominator;\n> > +             numerator =\n> gst_value_get_fraction_numerator(framerate_container);\n> > +             denominator =\n> gst_value_get_fraction_denominator(framerate_container);\n> > +             gst_structure_set(s, \"framerate\", GST_TYPE_FRACTION,\n> numerator, denominator, nullptr);\n> > +     } else {\n> > +             gst_structure_set(s, \"framerate\", GST_TYPE_FRACTION, 0, 1,\n> nullptr);\n> > +     }\n> > +\n> > +     if (stream_cfg.colorSpace) {\n>\n> ?!\n>\n> I am not getting why you are introducing code related to colorspace\n> here? Is it a copy/paste fiasco?\n>\nAs new caps are exposed. Shouldn't we expose everything that has been\ndecided on the libcamera side that includes colorspace, resolutions,\nformat and framerate?\n\n\nOn Thu, Sep 8, 2022 at 11:15 AM Umang Jain <umang.jain@ideasonboard.com>\nwrote:\n\n> Hi Rishi,\n>\n> On 9/7/22 4:17 PM, Rishikesh Donadkar wrote:\n> > Add a field of the type ControlList to GstLibcameraSrcState.\n> >\n> > Get the framerate from the caps and convert it to FrameDuration.\n> > Set the FrameDurationLimits control in the initControls_\n> >\n> > Passing initControls_ at camera::start() doesn't tell us if the controls\n> > have been applied to the camera successfully or not (or they have\n> adjusted in\n> > some fashion). Until that is introduced in camera::start() API,\n> > we need to carry out such handling on the application side.\n> >\n> > Check the frame_duration whether it is in-bound to the max / min\n> > frame-duration supported by the camera using the function\n> > gst_libcamera_bind_framerate().\n> >\n> > If the frame_duartion is out of bounds, bind the frame_duration\n> > between the max and min FrameDuration that is supported by the camera.\n> >\n> > Pass the initControls_ at the time of starting camera.\n> >\n> > Solve the complications in exposing the correct framerate due to loss in\n> > precision as a result of casting the frameduration to int64_t(which is\n> > required in libcamera to set the FrameDurationLimits control).\n> >\n> > Example -\n> >\n> > * Suppose the framerate requested is 35/1. The framerate is read form\n> >    the caps in the from of fraction that has a numerator and\n> >    denominator.\n> >\n> > * Converting to FrameDuration (in microseconds)\n> >      (1 * 10^6) / 35 = 28571.4286\n> >      int64_t frame_duration = 28571\n> >      (the precision here is lost.)\n> > * To expose the framerate in caps, Inverting the frame_duration to get\n> >    back the framerate and converting to Seconds.\n> >      double framerate = 10^6 / 28571\n> >      and\n> >      10^6/28571 which is close but not equal to 35/1 will fail the\n> negotiation.\n> >\n> > To solve the above problem, Store the framerate requested through\n> > negotiated caps in the GValue of the type GST_TYPE_FRACTION.\n> >\n> > Try to apply the framerate and check if the floor value of framerate\n> that gets applied\n> > closely matches with the floor value framerate requested in the\n> negotiated caps. If\n> > the value matches expose the framerate that is stored in the\n> framerate_container, else expose\n> > the framerate in the new caps as 0/1.\n> >\n> > Pass the initControls_ at camera->start().\n> >\n> > ---\n> > Changes from v1 to v2\n> > * Use GValue of the type GST_TYPE_FRACTION instead of std::pair to store\n> the framerate.\n> > * Don't shift the call to camera->configure(), instead bound the\n> framerate\n> >    after the call to camera->configure().\n> > * Reset the FrameDurationLimits control only if it is out of bounds.\n> > * Expose the caps containing framerate information with a new CAPS event\n> >    after the call to camera->configure().\n> > ---\n> >\n> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n>\n> As I haven't contributed to any LOC of this patch, please drop of my\n> s-o-b tag.\n>\n> Non-existent s-o-b on behalf of others shouldn't be applied ideally\n> > Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com>\n> > ---\n> >   src/gstreamer/gstlibcamera-utils.cpp | 116 +++++++++++++++++++++++++++\n> >   src/gstreamer/gstlibcamera-utils.h   |   8 ++\n> >   src/gstreamer/gstlibcamerasrc.cpp    |  28 ++++++-\n> >   3 files changed, 151 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp\n> b/src/gstreamer/gstlibcamera-utils.cpp\n> > index 4df5dd6c..e862f7ea 100644\n> > --- a/src/gstreamer/gstlibcamera-utils.cpp\n> > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > @@ -8,6 +8,7 @@\n> >\n> >   #include \"gstlibcamera-utils.h\"\n> >\n> > +#include <libcamera/control_ids.h>\n> >   #include <libcamera/formats.h>\n> >\n> >   using namespace libcamera;\n> > @@ -405,6 +406,121 @@\n> gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> >       }\n> >   }\n> >\n> > +void\n> > +gst_libcamera_get_init_controls_from_caps(ControlList &controls,\n> [[maybe_unused]] GstCaps *caps,\n> > +                                       GValue *framerate_container)\n> > +{\n> > +     GstStructure *s = gst_caps_get_structure(caps, 0);\n> > +     gint fps_n = -1, fps_d = -1;\n> > +\n> > +     if (gst_structure_has_field_typed(s, \"framerate\",\n> GST_TYPE_FRACTION)) {\n> > +             if (!gst_structure_get_fraction(s, \"framerate\", &fps_n,\n> &fps_d)) {\n> > +                     GST_WARNING(\"invalid framerate in the caps.\");\n> > +                     return;\n> > +             }\n> > +     }\n> > +\n> > +     if (fps_n < 0 || fps_d < 0)\n> > +             return;\n> > +\n> > +     gst_value_set_fraction(framerate_container, fps_n, fps_d);\n> > +     int64_t frame_duration = (fps_d * 1000000.0) / fps_n;\n> > +\n> > +     controls.set(controls::FrameDurationLimits,\n> > +                  Span<const int64_t, 2>({ frame_duration,\n> frame_duration }));\n>\n> setting the frame_duration even before bound-checking is wrong. This\n> Function should ideally get the 'framerate' and just save it for later\n> use/bound-checking after camera::configure()\n>\n> Once the bound checking is successfully, only then you are allowed to\n> set frame_duration on initControls_\n> > +}\n> > +\n> > +void\n> > +gst_libcamera_bind_framerate(const ControlInfoMap &camera_controls,\n> ControlList &controls)\n>\n> maybe\n> gst_libcamera_framerate_clamp()\n> OR\n> gst_libcamera_framerate_bounds_check()\n> > +{\n> > +     gboolean isBound = true;\n> > +     auto iterFrameDuration =\n> camera_controls.find(controls::FrameDurationLimits.id());\n> > +\n> > +     if (!(iterFrameDuration != camera_controls.end() &&\n> > +           controls.contains(controls::FRAME_DURATION_LIMITS)))\n> > +             return;\n> > +\n> > +     int64_t frame_duration =\n> controls.get(controls::FrameDurationLimits).value()[0];\n> > +     int64_t min_frame_duration =\n> iterFrameDuration->second.min().get<int64_t>();\n> > +     int64_t max_frame_duration =\n> iterFrameDuration->second.max().get<int64_t>();\n> > +\n> > +     if (frame_duration < min_frame_duration) {\n> > +             frame_duration = min_frame_duration;\n> > +     } else if (frame_duration > max_frame_duration) {\n> > +             frame_duration = max_frame_duration;\n> > +     } else {\n> > +             isBound = false;\n> > +     }\n> > +\n> > +     if (isBound)\n> > +             controls.set(controls::FrameDurationLimits,\n> > +                          Span<const int64_t, 2>({ frame_duration,\n> frame_duration }));\n> > +}\n> > +\n> > +gboolean\n> > +gst_libcamera_get_framerate_from_init_controls(const ControlList\n> &controls, GValue *framerate,\n> > +                                            GValue *framerate_container)\n> > +{\n> > +     gint fps_caps_n, fps_caps_d, numerator, denominator;\n> > +\n> > +     fps_caps_n = gst_value_get_fraction_numerator(framerate_container);\n> > +     fps_caps_d =\n> gst_value_get_fraction_denominator(framerate_container);\n> > +\n> > +     if (!controls.contains(controls::FRAME_DURATION_LIMITS))\n> > +             return false;\n> > +\n> > +     double framerate_ret = 1000000 /\n> static_cast<double>(controls.get(controls::FrameDurationLimits).value()[0]);\n> > +     gst_util_double_to_fraction(framerate_ret, &numerator,\n> &denominator);\n> > +     gst_value_set_fraction(framerate, numerator, denominator);\n> > +\n> > +     if (fps_caps_n / fps_caps_d == numerator / denominator) {\n> > +             return true;\n> > +     }\n> > +     return false;\n> > +}\n> > +\n> > +GstCaps *\n> > +gst_libcamera_init_controls_to_caps(const ControlList &controls, const\n> StreamConfiguration &stream_cfg,\n> > +                                 GValue *framerate_container)\n> > +{\n> > +     GstCaps *caps = gst_caps_new_empty();\n> > +     GstStructure *s =\n> bare_structure_from_format(stream_cfg.pixelFormat);\n> > +     gboolean ret;\n> > +     GValue *framerate = g_new0(GValue, 1);\n>\n> Why create a new GValue here?\n>\n> You have everything you need through framerate_container, a applied and\n> bound-checked framerate, you just need to query the fraction from it and\n> set the structure?\n> > +     g_value_init(framerate, GST_TYPE_FRACTION);\n> > +\n> > +     ret = gst_libcamera_get_framerate_from_init_controls(controls,\n> framerate, framerate_container);\n>\n> As per the above comment, I am not sure if you really need to do that.\n> Seems over-kill\n> > +\n> > +     gst_structure_set(s,\n> > +                       \"width\", G_TYPE_INT, stream_cfg.size.width,\n> > +                       \"height\", G_TYPE_INT, stream_cfg.size.height,\n> > +                       nullptr);\n> > +\n> > +     if (ret) {\n> > +             gint numerator, denominator;\n> > +             numerator =\n> gst_value_get_fraction_numerator(framerate_container);\n> > +             denominator =\n> gst_value_get_fraction_denominator(framerate_container);\n> > +             gst_structure_set(s, \"framerate\", GST_TYPE_FRACTION,\n> numerator, denominator, nullptr);\n> > +     } else {\n> > +             gst_structure_set(s, \"framerate\", GST_TYPE_FRACTION, 0, 1,\n> nullptr);\n> > +     }\n> > +\n> > +     if (stream_cfg.colorSpace) {\n>\n> ?!\n>\n> I am not getting why you are introducing code related to colorspace\n> here? Is it a copy/paste fiasco?\n> > +             GstVideoColorimetry colorimetry =\n> colorimetry_from_colorspace(stream_cfg.colorSpace.value());\n> > +             gchar *colorimetry_str =\n> gst_video_colorimetry_to_string(&colorimetry);\n> > +\n> > +             if (colorimetry_str)\n> > +                     gst_structure_set(s, \"colorimetry\", G_TYPE_STRING,\n> colorimetry_str, nullptr);\n> > +             else\n> > +                     g_error(\"Got invalid colorimetry from ColorSpace:\n> %s\",\n> > +\n>  ColorSpace::toString(stream_cfg.colorSpace).c_str());\n> > +     }\n> > +\n> > +     gst_caps_append_structure(caps, s);\n> > +     g_free(framerate);\n> > +     return caps;\n> > +}\n> > +\n> >   #if !GST_CHECK_VERSION(1, 17, 1)\n> >   gboolean\n> >   gst_task_resume(GstTask *task)\n> > diff --git a/src/gstreamer/gstlibcamera-utils.h\n> b/src/gstreamer/gstlibcamera-utils.h\n> > index 164189a2..955a717d 100644\n> > --- a/src/gstreamer/gstlibcamera-utils.h\n> > +++ b/src/gstreamer/gstlibcamera-utils.h\n> > @@ -9,6 +9,7 @@\n> >   #pragma once\n> >\n> >   #include <libcamera/camera_manager.h>\n> > +#include <libcamera/controls.h>\n> >   #include <libcamera/stream.h>\n> >\n> >   #include <gst/gst.h>\n> > @@ -18,6 +19,13 @@ GstCaps *gst_libcamera_stream_formats_to_caps(const\n> libcamera::StreamFormats &fo\n> >   GstCaps *gst_libcamera_stream_configuration_to_caps(const\n> libcamera::StreamConfiguration &stream_cfg);\n> >   void\n> gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration\n> &stream_cfg,\n> >                                             GstCaps *caps);\n> > +void gst_libcamera_get_init_controls_from_caps(libcamera::ControlList\n> &controls,\n> > +                                            GstCaps *caps, GValue\n> *framerate_container);\n> > +void gst_libcamera_bind_framerate(const libcamera::ControlInfoMap\n> &camera_controls,\n> > +                               libcamera::ControlList &controls);\n> > +GstCaps *gst_libcamera_init_controls_to_caps(const\n> libcamera::ControlList &controls,\n> > +                                          const\n> libcamera::StreamConfiguration &streame_cfg, GValue *framerate_container);\n> > +\n> >   #if !GST_CHECK_VERSION(1, 17, 1)\n> >   gboolean gst_task_resume(GstTask *task);\n> >   #endif\n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> b/src/gstreamer/gstlibcamerasrc.cpp\n> > index 16d70fea..0c981357 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -131,6 +131,7 @@ struct GstLibcameraSrcState {\n> >       std::queue<std::unique_ptr<RequestWrap>> completedRequests_\n> >               LIBCAMERA_TSA_GUARDED_BY(lock_);\n> >\n> > +     ControlList initControls_;\n> >       guint group_id_;\n> >\n> >       int queueRequest();\n> > @@ -461,6 +462,8 @@ gst_libcamera_src_task_enter(GstTask *task,\n> [[maybe_unused]] GThread *thread,\n> >       GstLibcameraSrcState *state = self->state;\n> >       GstFlowReturn flow_ret = GST_FLOW_OK;\n> >       gint ret;\n> > +     GValue *framerate_container = g_new0(GValue, 1);\n> > +     framerate_container = g_value_init(framerate_container,\n> GST_TYPE_FRACTION);\n>\n> Since it's a GValue, I would not make it framerate_* specific, so I\n> would just rename and remove all \"framerate_\" specifics.\n>\n> In future, when we have to parse more parameters as a part of\n> initControls_ setting, the same GValue can be used to temporarily hold\n> all other values\n> >\n> >       GST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n> >\n> > @@ -496,6 +499,7 @@ gst_libcamera_src_task_enter(GstTask *task,\n> [[maybe_unused]] GThread *thread,\n> >               /* Retrieve the supported caps. */\n> >               g_autoptr(GstCaps) filter =\n> gst_libcamera_stream_formats_to_caps(stream_cfg.formats());\n> >               g_autoptr(GstCaps) caps = gst_pad_peer_query_caps(srcpad,\n> filter);\n> > +\n> >               if (gst_caps_is_empty(caps)) {\n> >                       flow_ret = GST_FLOW_NOT_NEGOTIATED;\n> >                       break;\n> > @@ -504,6 +508,8 @@ gst_libcamera_src_task_enter(GstTask *task,\n> [[maybe_unused]] GThread *thread,\n> >               /* Fixate caps and configure the stream. */\n> >               caps = gst_caps_make_writable(caps);\n> >               gst_libcamera_configure_stream_from_caps(stream_cfg, caps);\n> > +\n>  gst_libcamera_get_init_controls_from_caps(state->initControls_, caps,\n> > +\n>  framerate_container);\n>\n> passing state->initControls_ is un-necessary here as we don't need to\n> set frame_duration on it, just right now (it has to go through bound\n> check first which is below...)\n> >       }\n> >\n> >       if (flow_ret != GST_FLOW_OK)\n> > @@ -524,6 +530,7 @@ gst_libcamera_src_task_enter(GstTask *task,\n> [[maybe_unused]] GThread *thread,\n> >               const StreamConfiguration &stream_cfg =\n> state->config_->at(i);\n> >\n> >               g_autoptr(GstCaps) caps =\n> gst_libcamera_stream_configuration_to_caps(stream_cfg);\n> > +\n> >               if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps)))\n> {\n> >                       flow_ret = GST_FLOW_NOT_NEGOTIATED;\n> >                       break;\n> > @@ -544,6 +551,23 @@ gst_libcamera_src_task_enter(GstTask *task,\n> [[maybe_unused]] GThread *thread,\n> >               return;\n> >       }\n> >\n> > +     /* bind the framerate */\n>\n> s/bind/Clamp OR\n> /* Check framerate bounds within  controls::FrameDurationLimits */\n> > +     gst_libcamera_bind_framerate(state->cam_->controls(),\n> state->initControls_);\n>\n> I would just pass the controls::FrameDurationLimits and GValue\n> (containing the asked framerate) and perform the bound check.\n>\n> You can update the GValue's framerate in that function after which\n> simply, set the initControls' ::FrameDurationLimits right here.\n> > +\n> > +     /* Expose the controls in initControls_ throught a new caps event.\n> */\n> > +     for (gsize i = 0; i < state->srcpads_.size(); i++) {\n> > +             GstPad *srcpad = state->srcpads_[i];\n> > +             const StreamConfiguration &stream_cfg =\n> state->config_->at(i);\n> > +\n> > +             g_autoptr(GstCaps) caps =\n> gst_libcamera_init_controls_to_caps(state->initControls_,\n> > +\n>    stream_cfg, framerate_container);\n> > +\n> > +             if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps)))\n> {\n> > +                     flow_ret = GST_FLOW_NOT_NEGOTIATED;\n> > +                     break;\n> > +             }\n> > +     }\n> > +\n> >       self->allocator = gst_libcamera_allocator_new(state->cam_,\n> state->config_.get());\n> >       if (!self->allocator) {\n> >               GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,\n> > @@ -566,7 +590,7 @@ gst_libcamera_src_task_enter(GstTask *task,\n> [[maybe_unused]] GThread *thread,\n> >               gst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n> >       }\n> >\n> > -     ret = state->cam_->start();\n> > +     ret = state->cam_->start(&state->initControls_);\n> >       if (ret) {\n> >               GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> >                                 (\"Failed to start the camera: %s\",\n> g_strerror(-ret)),\n> > @@ -576,6 +600,8 @@ gst_libcamera_src_task_enter(GstTask *task,\n> [[maybe_unused]] GThread *thread,\n> >       }\n> >\n> >   done:\n> > +     state->initControls_.clear();\n> > +     g_free(framerate_container);\n> >       switch (flow_ret) {\n> >       case GST_FLOW_NOT_NEGOTIATED:\n> >               GST_ELEMENT_FLOW_ERROR(self, flow_ret);\n>\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 43C27C0DA4\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  8 Sep 2022 06:06:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C5BC562099;\n\tThu,  8 Sep 2022 08:06:38 +0200 (CEST)","from mail-yb1-xb2b.google.com (mail-yb1-xb2b.google.com\n\t[IPv6:2607:f8b0:4864:20::b2b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EA6A46208F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  8 Sep 2022 08:06:36 +0200 (CEST)","by mail-yb1-xb2b.google.com with SMTP id p200so5792088yba.1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 07 Sep 2022 23:06:36 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1662617198;\n\tbh=9AKj1Xd7Rt/l5W4xFIcafdsr0LzASnTOTJJQifqOpqs=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=u2mQvhk3gbXZH0u5kyuaouiYS0qGfkvSv5sd0L8sPcFhRDe44TD+qKyQpiK3rsglD\n\tx67wIDBcHpX/su1nkDOvQnFYLgf5tdI+G2GwIVoqUphcg53aiCOrgb//OPb3A+oUFr\n\tLq7L/VEROCGdV+KNV+2VG/cwxzjtjjjccO7CKxAZzvc3QOHWlubTzkzNXcbfCqtDQ3\n\txwMDdWiNJZUB3gA7WoIEM6ISjvEfmdkozO5sl64214NGBY2RXbTwDYAazyxKVTst5c\n\tBqHwrfZw8vKgC661LFsY37bFttAUUMUQXu3I/3nDpJ/In3LcetHFJPA/yPg/iKNJRa\n\tMNJtSvTtz/82Q==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date;\n\tbh=R4ReubfLPA4wgiWP0rxE3aFxWDjWL1jLWpLl3bJGj08=;\n\tb=kktU/nApYBWMxPC4ryAqn7ibJL/WqhiDnuJM/6ipAlOoP5oXkkGOb5B6NHOCnox1jx\n\th/4pSGU/o2aM6ghFsCy1jD6guDbIPzZEMKDgVmXohYjs80pAfAKj1lyfoQ2S31286vwO\n\tDlxBAG2MHsrnlSccvlqun77rO0KsnLuh2tWnkUdMhVAOX2s4A0xbm6Lm9i2aGjKS2Na4\n\t/acIiya0sX1ZAx/D1gHGHhvb9Ve6OXZ/nQ25f+fvZ3Gzn/TnPTe3JyvGd82IcBrC7ZQl\n\tKNi9h9mMDoKkjvefTAKxy2ry+nHNcBizTPXHpgaLGbvVWB9neygqKhNaFsY+QpNPqgAn\n\tff8w=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"kktU/nAp\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date;\n\tbh=R4ReubfLPA4wgiWP0rxE3aFxWDjWL1jLWpLl3bJGj08=;\n\tb=csxvz4+KwQ2mpdDCKM/08hVQ0I11D4+3lvcqzUMDtnk9IWBmKld6Ayu9OVlQznNmKk\n\tXzZwsBE2USdytfUBWipuYM+Lw0d+JMUMO9eGmEuKeFAjPaESnRj3zlw5Zq4KKVF7K0sM\n\t1fqR468lgKIXe/vw7kmge1RTS/32WcvsQ6AURKJxDo6FFOkh7jxoiH8LNjNWLpIc3dsv\n\tlUQzJ5BLN6WdtXYPmVZGGGk9Ha7YjoiHdKMvp4Lhw/vJh7BdqozOA3cwJ9L0/tZmIZee\n\tb3Le+1/Irk3kobJjc2eMMioVB/uemkHTL+Sf+uTAdoaOy6a3RwjvK51L7j92MtWnS2rN\n\twnqw==","X-Gm-Message-State":"ACgBeo0UAk3XQfi8tayjeQc8ygweVYGez1dS3vl80e2XlQ/H+TTV+Fjg\n\t6i+6v2dgPUH9yugGNOQImhOYo7bU2dThVU/r+9k=","X-Google-Smtp-Source":"AA6agR7c34cE+om71EWxMoGtB8qIknCIMcYdD7osiTuwKf0dH5JgjzbbHCOqX1daFjLGRjrTiPDEed++PtG1WY/73MI=","X-Received":"by 2002:a25:1844:0:b0:6a9:668a:c138 with SMTP id\n\t65-20020a251844000000b006a9668ac138mr5644456yby.269.1662617195455;\n\tWed, 07 Sep 2022 23:06:35 -0700 (PDT)","MIME-Version":"1.0","References":"<20220907104711.87365-1-rishikeshdonadkar@gmail.com>\n\t<96c8f5d4-a742-f5a9-a9db-8bb1bb30f67d@ideasonboard.com>","In-Reply-To":"<96c8f5d4-a742-f5a9-a9db-8bb1bb30f67d@ideasonboard.com>","Date":"Thu, 8 Sep 2022 11:36:24 +0530","Message-ID":"<CAEQmg0mq8GSkDYLGF4TDXoS4wWTHyrNhFgoismhs3x-5TxB9gQ@mail.gmail.com>","To":"Umang Jain <umang.jain@ideasonboard.com>","Content-Type":"multipart/alternative; boundary=\"000000000000552a4105e8243bb7\"","Subject":"Re: [libcamera-devel] [PATCH v3] gstreamer: Provide framerate\n\tsupport for libcamerasrc.","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Rishikesh Donadkar via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Rishikesh Donadkar <rishikeshdonadkar@gmail.com>","Cc":"libcamera-devel@lists.libcamera.org, vedantparanjape160201@gmail.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":24959,"web_url":"https://patchwork.libcamera.org/comment/24959/","msgid":"<d8b8e0de-c5ad-7742-9a13-16c4d49ce320@ideasonboard.com>","date":"2022-09-08T08:35:12","subject":"Re: [libcamera-devel] [PATCH v3] gstreamer: Provide framerate\n\tsupport for libcamerasrc.","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Rishi,\n\nOn 9/8/22 11:36 AM, Rishikesh Donadkar wrote:\n>\n>     > +\n>     > +     gst_structure_set(s,\n>     > +                       \"width\", G_TYPE_INT, stream_cfg.size.width,\n>     > +                       \"height\", G_TYPE_INT,\n>     stream_cfg.size.height,\n>     > +                       nullptr);\n>     > +\n>     > +     if (ret) {\n>     > +             gint numerator, denominator;\n>     > +             numerator =\n>     gst_value_get_fraction_numerator(framerate_container);\n>     > +             denominator =\n>     gst_value_get_fraction_denominator(framerate_container);\n>     > +             gst_structure_set(s, \"framerate\",\n>     GST_TYPE_FRACTION, numerator, denominator, nullptr);\n>     > +     } else {\n>     > +             gst_structure_set(s, \"framerate\",\n>     GST_TYPE_FRACTION, 0, 1, nullptr);\n>     > +     }\n>     > +\n>     > +     if (stream_cfg.colorSpace) {\n>\n>     ?!\n>\n>     I am not getting why you are introducing code related to colorspace\n>     here? Is it a copy/paste fiasco?\n>\n> As new caps are exposed. Shouldn't we expose everything that has been \n> decided on the libcamera side that includes colorspace, resolutions, \n> format and framerate?\n\nerr yes... It's not very nice though...\n\nI wish gstreamer has/had mechanism to provide / update caps on the fly \n(even partially). I have looked deep into other elements but maybe there \nis a possbile way to do that, however [1] suggests there is no event to \nsupport it.\n\nAnother option I'm actually considering is moving the new caps event for \nloop (just above state->cam_->configure()) after the configure(). So, we \nwould go from:\n\n     streamCfg->validate();\n     // new caps event\n     camera::configure();\n\nto\n\n     streamCfg->validate();\n     camera::configure();\n     // new caps event\n\nWhich seems logical to me i.e. announcing the caps after the camera is \nactually configured. This will also help us with framerate and I don't \nsee any harm in announcing the caps after camera is configured (but I \nmaybe wrong, so requires some testing on RPi)\n\nDoes it makes sense? What do you think?\n\n[1] \nhttps://gstreamer.freedesktop.org/documentation/gstreamer/gstevent.html?gi-language=c#GstEventType\n\n>\n>\n> On Thu, Sep 8, 2022 at 11:15 AM Umang Jain \n> <umang.jain@ideasonboard.com> wrote:\n>\n>     Hi Rishi,\n>\n>     On 9/7/22 4:17 PM, Rishikesh Donadkar wrote:\n>     > Add a field of the type ControlList to GstLibcameraSrcState.\n>     >\n>     > Get the framerate from the caps and convert it to FrameDuration.\n>     > Set the FrameDurationLimits control in the initControls_\n>     >\n>     > Passing initControls_ at camera::start() doesn't tell us if the\n>     controls\n>     > have been applied to the camera successfully or not (or they\n>     have adjusted in\n>     > some fashion). Until that is introduced in camera::start() API,\n>     > we need to carry out such handling on the application side.\n>     >\n>     > Check the frame_duration whether it is in-bound to the max / min\n>     > frame-duration supported by the camera using the function\n>     > gst_libcamera_bind_framerate().\n>     >\n>     > If the frame_duartion is out of bounds, bind the frame_duration\n>     > between the max and min FrameDuration that is supported by the\n>     camera.\n>     >\n>     > Pass the initControls_ at the time of starting camera.\n>     >\n>     > Solve the complications in exposing the correct framerate due to\n>     loss in\n>     > precision as a result of casting the frameduration to\n>     int64_t(which is\n>     > required in libcamera to set the FrameDurationLimits control).\n>     >\n>     > Example -\n>     >\n>     > * Suppose the framerate requested is 35/1. The framerate is read\n>     form\n>     >    the caps in the from of fraction that has a numerator and\n>     >    denominator.\n>     >\n>     > * Converting to FrameDuration (in microseconds)\n>     >      (1 * 10^6) / 35 = 28571.4286\n>     >      int64_t frame_duration = 28571\n>     >      (the precision here is lost.)\n>     > * To expose the framerate in caps, Inverting the frame_duration\n>     to get\n>     >    back the framerate and converting to Seconds.\n>     >      double framerate = 10^6 / 28571\n>     >      and\n>     >      10^6/28571 which is close but not equal to 35/1 will fail\n>     the negotiation.\n>     >\n>     > To solve the above problem, Store the framerate requested through\n>     > negotiated caps in the GValue of the type GST_TYPE_FRACTION.\n>     >\n>     > Try to apply the framerate and check if the floor value of\n>     framerate that gets applied\n>     > closely matches with the floor value framerate requested in the\n>     negotiated caps. If\n>     > the value matches expose the framerate that is stored in the\n>     framerate_container, else expose\n>     > the framerate in the new caps as 0/1.\n>     >\n>     > Pass the initControls_ at camera->start().\n>     >\n>     > ---\n>     > Changes from v1 to v2\n>     > * Use GValue of the type GST_TYPE_FRACTION instead of std::pair\n>     to store the framerate.\n>     > * Don't shift the call to camera->configure(), instead bound the\n>     framerate\n>     >    after the call to camera->configure().\n>     > * Reset the FrameDurationLimits control only if it is out of bounds.\n>     > * Expose the caps containing framerate information with a new\n>     CAPS event\n>     >    after the call to camera->configure().\n>     > ---\n>     >\n>     > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n>\n>     As I haven't contributed to any LOC of this patch, please drop of my\n>     s-o-b tag.\n>\n>     Non-existent s-o-b on behalf of others shouldn't be applied ideally\n>     > Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com>\n>     > ---\n>     >   src/gstreamer/gstlibcamera-utils.cpp | 116\n>     +++++++++++++++++++++++++++\n>     >   src/gstreamer/gstlibcamera-utils.h   |   8 ++\n>     >   src/gstreamer/gstlibcamerasrc.cpp    |  28 ++++++-\n>     >   3 files changed, 151 insertions(+), 1 deletion(-)\n>     >\n>     > diff --git a/src/gstreamer/gstlibcamera-utils.cpp\n>     b/src/gstreamer/gstlibcamera-utils.cpp\n>     > index 4df5dd6c..e862f7ea 100644\n>     > --- a/src/gstreamer/gstlibcamera-utils.cpp\n>     > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n>     > @@ -8,6 +8,7 @@\n>     >\n>     >   #include \"gstlibcamera-utils.h\"\n>     >\n>     > +#include <libcamera/control_ids.h>\n>     >   #include <libcamera/formats.h>\n>     >\n>     >   using namespace libcamera;\n>     > @@ -405,6 +406,121 @@\n>     gst_libcamera_configure_stream_from_caps(StreamConfiguration\n>     &stream_cfg,\n>     >       }\n>     >   }\n>     >\n>     > +void\n>     > +gst_libcamera_get_init_controls_from_caps(ControlList\n>     &controls, [[maybe_unused]] GstCaps *caps,\n>     > +                                       GValue *framerate_container)\n>     > +{\n>     > +     GstStructure *s = gst_caps_get_structure(caps, 0);\n>     > +     gint fps_n = -1, fps_d = -1;\n>     > +\n>     > +     if (gst_structure_has_field_typed(s, \"framerate\",\n>     GST_TYPE_FRACTION)) {\n>     > +             if (!gst_structure_get_fraction(s, \"framerate\",\n>     &fps_n, &fps_d)) {\n>     > +                     GST_WARNING(\"invalid framerate in the caps.\");\n>     > +                     return;\n>     > +             }\n>     > +     }\n>     > +\n>     > +     if (fps_n < 0 || fps_d < 0)\n>     > +             return;\n>     > +\n>     > +     gst_value_set_fraction(framerate_container, fps_n, fps_d);\n>     > +     int64_t frame_duration = (fps_d * 1000000.0) / fps_n;\n>     > +\n>     > +     controls.set(controls::FrameDurationLimits,\n>     > +                  Span<const int64_t, 2>({ frame_duration,\n>     frame_duration }));\n>\n>     setting the frame_duration even before bound-checking is wrong. This\n>     Function should ideally get the 'framerate' and just save it for\n>     later\n>     use/bound-checking after camera::configure()\n>\n>     Once the bound checking is successfully, only then you are allowed to\n>     set frame_duration on initControls_\n>     > +}\n>     > +\n>     > +void\n>     > +gst_libcamera_bind_framerate(const ControlInfoMap\n>     &camera_controls, ControlList &controls)\n>\n>     maybe\n>     gst_libcamera_framerate_clamp()\n>     OR\n>     gst_libcamera_framerate_bounds_check()\n>     > +{\n>     > +     gboolean isBound = true;\n>     > +     auto iterFrameDuration =\n>     camera_controls.find(controls::FrameDurationLimits.id());\n>     > +\n>     > +     if (!(iterFrameDuration != camera_controls.end() &&\n>     > +  controls.contains(controls::FRAME_DURATION_LIMITS)))\n>     > +             return;\n>     > +\n>     > +     int64_t frame_duration =\n>     controls.get(controls::FrameDurationLimits).value()[0];\n>     > +     int64_t min_frame_duration =\n>     iterFrameDuration->second.min().get<int64_t>();\n>     > +     int64_t max_frame_duration =\n>     iterFrameDuration->second.max().get<int64_t>();\n>     > +\n>     > +     if (frame_duration < min_frame_duration) {\n>     > +             frame_duration = min_frame_duration;\n>     > +     } else if (frame_duration > max_frame_duration) {\n>     > +             frame_duration = max_frame_duration;\n>     > +     } else {\n>     > +             isBound = false;\n>     > +     }\n>     > +\n>     > +     if (isBound)\n>     > +             controls.set(controls::FrameDurationLimits,\n>     > +                          Span<const int64_t, 2>({\n>     frame_duration, frame_duration }));\n>     > +}\n>     > +\n>     > +gboolean\n>     > +gst_libcamera_get_framerate_from_init_controls(const\n>     ControlList &controls, GValue *framerate,\n>     > +                                            GValue\n>     *framerate_container)\n>     > +{\n>     > +     gint fps_caps_n, fps_caps_d, numerator, denominator;\n>     > +\n>     > +     fps_caps_n =\n>     gst_value_get_fraction_numerator(framerate_container);\n>     > +     fps_caps_d =\n>     gst_value_get_fraction_denominator(framerate_container);\n>     > +\n>     > +     if (!controls.contains(controls::FRAME_DURATION_LIMITS))\n>     > +             return false;\n>     > +\n>     > +     double framerate_ret = 1000000 /\n>     static_cast<double>(controls.get(controls::FrameDurationLimits).value()[0]);\n>     > +     gst_util_double_to_fraction(framerate_ret, &numerator,\n>     &denominator);\n>     > +     gst_value_set_fraction(framerate, numerator, denominator);\n>     > +\n>     > +     if (fps_caps_n / fps_caps_d == numerator / denominator) {\n>     > +             return true;\n>     > +     }\n>     > +     return false;\n>     > +}\n>     > +\n>     > +GstCaps *\n>     > +gst_libcamera_init_controls_to_caps(const ControlList\n>     &controls, const StreamConfiguration &stream_cfg,\n>     > +                                 GValue *framerate_container)\n>     > +{\n>     > +     GstCaps *caps = gst_caps_new_empty();\n>     > +     GstStructure *s =\n>     bare_structure_from_format(stream_cfg.pixelFormat);\n>     > +     gboolean ret;\n>     > +     GValue *framerate = g_new0(GValue, 1);\n>\n>     Why create a new GValue here?\n>\n>     You have everything you need through framerate_container, a\n>     applied and\n>     bound-checked framerate, you just need to query the fraction from\n>     it and\n>     set the structure?\n>     > +     g_value_init(framerate, GST_TYPE_FRACTION);\n>     > +\n>     > +     ret =\n>     gst_libcamera_get_framerate_from_init_controls(controls,\n>     framerate, framerate_container);\n>\n>     As per the above comment, I am not sure if you really need to do\n>     that.\n>     Seems over-kill\n>     > +\n>     > +     gst_structure_set(s,\n>     > +                       \"width\", G_TYPE_INT, stream_cfg.size.width,\n>     > +                       \"height\", G_TYPE_INT,\n>     stream_cfg.size.height,\n>     > +                       nullptr);\n>     > +\n>     > +     if (ret) {\n>     > +             gint numerator, denominator;\n>     > +             numerator =\n>     gst_value_get_fraction_numerator(framerate_container);\n>     > +             denominator =\n>     gst_value_get_fraction_denominator(framerate_container);\n>     > +             gst_structure_set(s, \"framerate\",\n>     GST_TYPE_FRACTION, numerator, denominator, nullptr);\n>     > +     } else {\n>     > +             gst_structure_set(s, \"framerate\",\n>     GST_TYPE_FRACTION, 0, 1, nullptr);\n>     > +     }\n>     > +\n>     > +     if (stream_cfg.colorSpace) {\n>\n>     ?!\n>\n>     I am not getting why you are introducing code related to colorspace\n>     here? Is it a copy/paste fiasco?\n>     > +             GstVideoColorimetry colorimetry =\n>     colorimetry_from_colorspace(stream_cfg.colorSpace.value());\n>     > +             gchar *colorimetry_str =\n>     gst_video_colorimetry_to_string(&colorimetry);\n>     > +\n>     > +             if (colorimetry_str)\n>     > +                     gst_structure_set(s, \"colorimetry\",\n>     G_TYPE_STRING, colorimetry_str, nullptr);\n>     > +             else\n>     > +                     g_error(\"Got invalid colorimetry from\n>     ColorSpace: %s\",\n>     > +  ColorSpace::toString(stream_cfg.colorSpace).c_str());\n>     > +     }\n>     > +\n>     > +     gst_caps_append_structure(caps, s);\n>     > +     g_free(framerate);\n>     > +     return caps;\n>     > +}\n>     > +\n>     >   #if !GST_CHECK_VERSION(1, 17, 1)\n>     >   gboolean\n>     >   gst_task_resume(GstTask *task)\n>     > diff --git a/src/gstreamer/gstlibcamera-utils.h\n>     b/src/gstreamer/gstlibcamera-utils.h\n>     > index 164189a2..955a717d 100644\n>     > --- a/src/gstreamer/gstlibcamera-utils.h\n>     > +++ b/src/gstreamer/gstlibcamera-utils.h\n>     > @@ -9,6 +9,7 @@\n>     >   #pragma once\n>     >\n>     >   #include <libcamera/camera_manager.h>\n>     > +#include <libcamera/controls.h>\n>     >   #include <libcamera/stream.h>\n>     >\n>     >   #include <gst/gst.h>\n>     > @@ -18,6 +19,13 @@ GstCaps\n>     *gst_libcamera_stream_formats_to_caps(const\n>     libcamera::StreamFormats &fo\n>     >   GstCaps *gst_libcamera_stream_configuration_to_caps(const\n>     libcamera::StreamConfiguration &stream_cfg);\n>     >   void\n>     gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration\n>     &stream_cfg,\n>     >                                             GstCaps *caps);\n>     > +void\n>     gst_libcamera_get_init_controls_from_caps(libcamera::ControlList\n>     &controls,\n>     > +                                            GstCaps *caps,\n>     GValue *framerate_container);\n>     > +void gst_libcamera_bind_framerate(const\n>     libcamera::ControlInfoMap &camera_controls,\n>     > +                               libcamera::ControlList &controls);\n>     > +GstCaps *gst_libcamera_init_controls_to_caps(const\n>     libcamera::ControlList &controls,\n>     > +                                          const\n>     libcamera::StreamConfiguration &streame_cfg, GValue\n>     *framerate_container);\n>     > +\n>     >   #if !GST_CHECK_VERSION(1, 17, 1)\n>     >   gboolean gst_task_resume(GstTask *task);\n>     >   #endif\n>     > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n>     b/src/gstreamer/gstlibcamerasrc.cpp\n>     > index 16d70fea..0c981357 100644\n>     > --- a/src/gstreamer/gstlibcamerasrc.cpp\n>     > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n>     > @@ -131,6 +131,7 @@ struct GstLibcameraSrcState {\n>     >  std::queue<std::unique_ptr<RequestWrap>> completedRequests_\n>     >               LIBCAMERA_TSA_GUARDED_BY(lock_);\n>     >\n>     > +     ControlList initControls_;\n>     >       guint group_id_;\n>     >\n>     >       int queueRequest();\n>     > @@ -461,6 +462,8 @@ gst_libcamera_src_task_enter(GstTask *task,\n>     [[maybe_unused]] GThread *thread,\n>     >       GstLibcameraSrcState *state = self->state;\n>     >       GstFlowReturn flow_ret = GST_FLOW_OK;\n>     >       gint ret;\n>     > +     GValue *framerate_container = g_new0(GValue, 1);\n>     > +     framerate_container = g_value_init(framerate_container,\n>     GST_TYPE_FRACTION);\n>\n>     Since it's a GValue, I would not make it framerate_* specific, so I\n>     would just rename and remove all \"framerate_\" specifics.\n>\n>     In future, when we have to parse more parameters as a part of\n>     initControls_ setting, the same GValue can be used to temporarily\n>     hold\n>     all other values\n>     >\n>     >       GST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n>     >\n>     > @@ -496,6 +499,7 @@ gst_libcamera_src_task_enter(GstTask *task,\n>     [[maybe_unused]] GThread *thread,\n>     >               /* Retrieve the supported caps. */\n>     >               g_autoptr(GstCaps) filter =\n>     gst_libcamera_stream_formats_to_caps(stream_cfg.formats());\n>     >               g_autoptr(GstCaps) caps =\n>     gst_pad_peer_query_caps(srcpad, filter);\n>     > +\n>     >               if (gst_caps_is_empty(caps)) {\n>     >                       flow_ret = GST_FLOW_NOT_NEGOTIATED;\n>     >                       break;\n>     > @@ -504,6 +508,8 @@ gst_libcamera_src_task_enter(GstTask *task,\n>     [[maybe_unused]] GThread *thread,\n>     >               /* Fixate caps and configure the stream. */\n>     >               caps = gst_caps_make_writable(caps);\n>     >  gst_libcamera_configure_stream_from_caps(stream_cfg, caps);\n>     > +\n>      gst_libcamera_get_init_controls_from_caps(state->initControls_, caps,\n>     > +  framerate_container);\n>\n>     passing state->initControls_ is un-necessary here as we don't need to\n>     set frame_duration on it, just right now (it has to go through bound\n>     check first which is below...)\n>     >       }\n>     >\n>     >       if (flow_ret != GST_FLOW_OK)\n>     > @@ -524,6 +530,7 @@ gst_libcamera_src_task_enter(GstTask *task,\n>     [[maybe_unused]] GThread *thread,\n>     >               const StreamConfiguration &stream_cfg =\n>     state->config_->at(i);\n>     >\n>     >               g_autoptr(GstCaps) caps =\n>     gst_libcamera_stream_configuration_to_caps(stream_cfg);\n>     > +\n>     >               if (!gst_pad_push_event(srcpad,\n>     gst_event_new_caps(caps))) {\n>     >                       flow_ret = GST_FLOW_NOT_NEGOTIATED;\n>     >                       break;\n>     > @@ -544,6 +551,23 @@ gst_libcamera_src_task_enter(GstTask *task,\n>     [[maybe_unused]] GThread *thread,\n>     >               return;\n>     >       }\n>     >\n>     > +     /* bind the framerate */\n>\n>     s/bind/Clamp OR\n>     /* Check framerate bounds within controls::FrameDurationLimits */\n>     > +  gst_libcamera_bind_framerate(state->cam_->controls(),\n>     state->initControls_);\n>\n>     I would just pass the controls::FrameDurationLimits and GValue\n>     (containing the asked framerate) and perform the bound check.\n>\n>     You can update the GValue's framerate in that function after which\n>     simply, set the initControls' ::FrameDurationLimits right here.\n>     > +\n>     > +     /* Expose the controls in initControls_ throught a new\n>     caps event. */\n>     > +     for (gsize i = 0; i < state->srcpads_.size(); i++) {\n>     > +             GstPad *srcpad = state->srcpads_[i];\n>     > +             const StreamConfiguration &stream_cfg =\n>     state->config_->at(i);\n>     > +\n>     > +             g_autoptr(GstCaps) caps =\n>     gst_libcamera_init_controls_to_caps(state->initControls_,\n>     > +                    stream_cfg, framerate_container);\n>     > +\n>     > +             if (!gst_pad_push_event(srcpad,\n>     gst_event_new_caps(caps))) {\n>     > +                     flow_ret = GST_FLOW_NOT_NEGOTIATED;\n>     > +                     break;\n>     > +             }\n>     > +     }\n>     > +\n>     >       self->allocator = gst_libcamera_allocator_new(state->cam_,\n>     state->config_.get());\n>     >       if (!self->allocator) {\n>     >               GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,\n>     > @@ -566,7 +590,7 @@ gst_libcamera_src_task_enter(GstTask *task,\n>     [[maybe_unused]] GThread *thread,\n>     >  gst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n>     >       }\n>     >\n>     > -     ret = state->cam_->start();\n>     > +     ret = state->cam_->start(&state->initControls_);\n>     >       if (ret) {\n>     >               GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n>     >                                 (\"Failed to start the camera:\n>     %s\", g_strerror(-ret)),\n>     > @@ -576,6 +600,8 @@ gst_libcamera_src_task_enter(GstTask *task,\n>     [[maybe_unused]] GThread *thread,\n>     >       }\n>     >\n>     >   done:\n>     > +     state->initControls_.clear();\n>     > +     g_free(framerate_container);\n>     >       switch (flow_ret) {\n>     >       case GST_FLOW_NOT_NEGOTIATED:\n>     >               GST_ELEMENT_FLOW_ERROR(self, flow_ret);\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 36A55C0DA4\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  8 Sep 2022 08:35:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C34E620A1;\n\tThu,  8 Sep 2022 10:35:21 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F0A696206C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  8 Sep 2022 10:35:19 +0200 (CEST)","from [IPV6:2401:4900:1f3e:50f3:a89c:93ec:bcd7:8b90] (unknown\n\t[IPv6:2401:4900:1f3e:50f3:a89c:93ec:bcd7:8b90])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D12B26CC;\n\tThu,  8 Sep 2022 10:35:17 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1662626121;\n\tbh=qxQKvA35xMhajUuroM/FRJCUo0akXSDgfKKleKEbzMk=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=G+odhznzmEE5j3ZyU3ukELXuxpyvRx1KnM1aM++SEBbQbYlbd5RqVPxDWSKfOVs0s\n\tzP6FDppc1thZ8SvdDzDYdnzCzRFFyekEYKUn0VO3at2EYwJC6cWwryqcBdVt4AYocf\n\tStTpgRvrZ3mafXDsxJed3K5vnZZlhvdBfiqvDbRkUMEg8huRxFFhHx1VlKGfsPYxOb\n\t8bFsQTjs1mVEQWwgR1BoA7vDxFaGzzn/a8+gHWCOL0QK+WLRxTKsf82JjP1DOe/tCn\n\tTiTS6DIKoOFqkmPUcXwt9Fb5l8mDpgisBR774rPGzNzLC38y6y++L4QsBQLdM5Bpm3\n\texd0dYqPhPqVg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1662626119;\n\tbh=qxQKvA35xMhajUuroM/FRJCUo0akXSDgfKKleKEbzMk=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=agWdHf1VICfqD4G0vFeNBdvJnaaoPKxi3oN4LyUdLzhWktxVyG9Oih3CRNmd1ZGcj\n\tuXBVDqEC+25LbpXrZDzDnkbNQmUQy15bY/4sRWnN8psDS+9qmQCmx0l3xBToD6a/9s\n\ttK0UihRAOzFRAzFcpy7GbyZG0/Eeq1E0aHU2F6LY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"agWdHf1V\"; dkim-atps=neutral","Content-Type":"multipart/alternative;\n\tboundary=\"------------pnxKfMk6AnOMt020Jc8n5ilm\"","Message-ID":"<d8b8e0de-c5ad-7742-9a13-16c4d49ce320@ideasonboard.com>","Date":"Thu, 8 Sep 2022 14:05:12 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.12.0","Content-Language":"en-US","To":"Rishikesh Donadkar <rishikeshdonadkar@gmail.com>","References":"<20220907104711.87365-1-rishikeshdonadkar@gmail.com>\n\t<96c8f5d4-a742-f5a9-a9db-8bb1bb30f67d@ideasonboard.com>\n\t<CAEQmg0mq8GSkDYLGF4TDXoS4wWTHyrNhFgoismhs3x-5TxB9gQ@mail.gmail.com>","In-Reply-To":"<CAEQmg0mq8GSkDYLGF4TDXoS4wWTHyrNhFgoismhs3x-5TxB9gQ@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v3] gstreamer: Provide framerate\n\tsupport for libcamerasrc.","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Umang Jain via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Umang Jain <umang.jain@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org, vedantparanjape160201@gmail.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":24960,"web_url":"https://patchwork.libcamera.org/comment/24960/","msgid":"<CAEQmg0nOBHeLmig6xYm2hKTDHpA+CRJ-ucGf+xeE0QzZs+mQ6Q@mail.gmail.com>","date":"2022-09-08T09:15:20","subject":"Re: [libcamera-devel] [PATCH v3] gstreamer: Provide framerate\n\tsupport for libcamerasrc.","submitter":{"id":118,"url":"https://patchwork.libcamera.org/api/people/118/","name":"Rishikesh Donadkar","email":"rishikeshdonadkar@gmail.com"},"content":">\n> I wish gstreamer has/had mechanism to provide / update caps on the fly\n> (even partially). I have looked deep into other elements but maybe there is\n> a possbile way to do that, however [1] suggests there is no event to\n> support it.\n>\nI don't think gstreamer has/had such a mechanism. GStreamer works with caps\nas a whole.\n\n> Another option I'm actually considering is moving the new caps event for\n> loop (just above state->cam_->configure()) after the configure(). So, we\n> would go from:\n>\n>     streamCfg->validate();\n>     // new caps event\n>     camera::configure();\n>\n> to\n>\n>     streamCfg->validate();\n>     camera::configure();\n>     // new caps event\n>\n> Which seems logical to me i.e. announcing the caps after the camera is\n> actually configured. This will also help us with framerate and I don't see\n> any harm in announcing the caps after camera is configured (but I maybe\n> wrong, so requires some testing on RPi)\n>\nYes, it makes sense. It will help quite a bit with framerate and bound\nchecking. And the framerate can be exposed to gstreamer on the same line as\ncolorimetry is exposed without another new CAPS event.\n\n\nOn Thu, Sep 8, 2022 at 2:05 PM Umang Jain <umang.jain@ideasonboard.com>\nwrote:\n\n> Hi Rishi,\n>\n> On 9/8/22 11:36 AM, Rishikesh Donadkar wrote:\n>\n> > +\n>> > +     gst_structure_set(s,\n>> > +                       \"width\", G_TYPE_INT, stream_cfg.size.width,\n>> > +                       \"height\", G_TYPE_INT, stream_cfg.size.height,\n>> > +                       nullptr);\n>> > +\n>> > +     if (ret) {\n>> > +             gint numerator, denominator;\n>> > +             numerator =\n>> gst_value_get_fraction_numerator(framerate_container);\n>> > +             denominator =\n>> gst_value_get_fraction_denominator(framerate_container);\n>> > +             gst_structure_set(s, \"framerate\", GST_TYPE_FRACTION,\n>> numerator, denominator, nullptr);\n>> > +     } else {\n>> > +             gst_structure_set(s, \"framerate\", GST_TYPE_FRACTION, 0,\n>> 1, nullptr);\n>> > +     }\n>> > +\n>> > +     if (stream_cfg.colorSpace) {\n>>\n>> ?!\n>>\n>> I am not getting why you are introducing code related to colorspace\n>> here? Is it a copy/paste fiasco?\n>>\n> As new caps are exposed. Shouldn't we expose everything that has been\n> decided on the libcamera side that includes colorspace, resolutions,\n> format and framerate?\n>\n>\n> err yes... It's not very nice though...\n>\n> I wish gstreamer has/had mechanism to provide / update caps on the fly\n> (even partially). I have looked deep into other elements but maybe there is\n> a possbile way to do that, however [1] suggests there is no event to\n> support it.\n>\n> Another option I'm actually considering is moving the new caps event for\n> loop (just above state->cam_->configure()) after the configure(). So, we\n> would go from:\n>\n>     streamCfg->validate();\n>     // new caps event\n>     camera::configure();\n>\n> to\n>\n>     streamCfg->validate();\n>     camera::configure();\n>     // new caps event\n>\n> Which seems logical to me i.e. announcing the caps after the camera is\n> actually configured. This will also help us with framerate and I don't see\n> any harm in announcing the caps after camera is configured (but I maybe\n> wrong, so requires some testing on RPi)\n>\n> Does it makes sense? What do you think?\n>\n> [1]\n> https://gstreamer.freedesktop.org/documentation/gstreamer/gstevent.html?gi-language=c#GstEventType\n>\n>\n>\n> On Thu, Sep 8, 2022 at 11:15 AM Umang Jain <umang.jain@ideasonboard.com>\n> wrote:\n>\n>> Hi Rishi,\n>>\n>> On 9/7/22 4:17 PM, Rishikesh Donadkar wrote:\n>> > Add a field of the type ControlList to GstLibcameraSrcState.\n>> >\n>> > Get the framerate from the caps and convert it to FrameDuration.\n>> > Set the FrameDurationLimits control in the initControls_\n>> >\n>> > Passing initControls_ at camera::start() doesn't tell us if the controls\n>> > have been applied to the camera successfully or not (or they have\n>> adjusted in\n>> > some fashion). Until that is introduced in camera::start() API,\n>> > we need to carry out such handling on the application side.\n>> >\n>> > Check the frame_duration whether it is in-bound to the max / min\n>> > frame-duration supported by the camera using the function\n>> > gst_libcamera_bind_framerate().\n>> >\n>> > If the frame_duartion is out of bounds, bind the frame_duration\n>> > between the max and min FrameDuration that is supported by the camera.\n>> >\n>> > Pass the initControls_ at the time of starting camera.\n>> >\n>> > Solve the complications in exposing the correct framerate due to loss in\n>> > precision as a result of casting the frameduration to int64_t(which is\n>> > required in libcamera to set the FrameDurationLimits control).\n>> >\n>> > Example -\n>> >\n>> > * Suppose the framerate requested is 35/1. The framerate is read form\n>> >    the caps in the from of fraction that has a numerator and\n>> >    denominator.\n>> >\n>> > * Converting to FrameDuration (in microseconds)\n>> >      (1 * 10^6) / 35 = 28571.4286\n>> >      int64_t frame_duration = 28571\n>> >      (the precision here is lost.)\n>> > * To expose the framerate in caps, Inverting the frame_duration to get\n>> >    back the framerate and converting to Seconds.\n>> >      double framerate = 10^6 / 28571\n>> >      and\n>> >      10^6/28571 which is close but not equal to 35/1 will fail the\n>> negotiation.\n>> >\n>> > To solve the above problem, Store the framerate requested through\n>> > negotiated caps in the GValue of the type GST_TYPE_FRACTION.\n>> >\n>> > Try to apply the framerate and check if the floor value of framerate\n>> that gets applied\n>> > closely matches with the floor value framerate requested in the\n>> negotiated caps. If\n>> > the value matches expose the framerate that is stored in the\n>> framerate_container, else expose\n>> > the framerate in the new caps as 0/1.\n>> >\n>> > Pass the initControls_ at camera->start().\n>> >\n>> > ---\n>> > Changes from v1 to v2\n>> > * Use GValue of the type GST_TYPE_FRACTION instead of std::pair to\n>> store the framerate.\n>> > * Don't shift the call to camera->configure(), instead bound the\n>> framerate\n>> >    after the call to camera->configure().\n>> > * Reset the FrameDurationLimits control only if it is out of bounds.\n>> > * Expose the caps containing framerate information with a new CAPS event\n>> >    after the call to camera->configure().\n>> > ---\n>> >\n>> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n>>\n>> As I haven't contributed to any LOC of this patch, please drop of my\n>> s-o-b tag.\n>>\n>> Non-existent s-o-b on behalf of others shouldn't be applied ideally\n>> > Signed-off-by: Rishikesh Donadkar <rishikeshdonadkar@gmail.com>\n>> > ---\n>> >   src/gstreamer/gstlibcamera-utils.cpp | 116 +++++++++++++++++++++++++++\n>> >   src/gstreamer/gstlibcamera-utils.h   |   8 ++\n>> >   src/gstreamer/gstlibcamerasrc.cpp    |  28 ++++++-\n>> >   3 files changed, 151 insertions(+), 1 deletion(-)\n>> >\n>> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp\n>> b/src/gstreamer/gstlibcamera-utils.cpp\n>> > index 4df5dd6c..e862f7ea 100644\n>> > --- a/src/gstreamer/gstlibcamera-utils.cpp\n>> > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n>> > @@ -8,6 +8,7 @@\n>> >\n>> >   #include \"gstlibcamera-utils.h\"\n>> >\n>> > +#include <libcamera/control_ids.h>\n>> >   #include <libcamera/formats.h>\n>> >\n>> >   using namespace libcamera;\n>> > @@ -405,6 +406,121 @@\n>> gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n>> >       }\n>> >   }\n>> >\n>> > +void\n>> > +gst_libcamera_get_init_controls_from_caps(ControlList &controls,\n>> [[maybe_unused]] GstCaps *caps,\n>> > +                                       GValue *framerate_container)\n>> > +{\n>> > +     GstStructure *s = gst_caps_get_structure(caps, 0);\n>> > +     gint fps_n = -1, fps_d = -1;\n>> > +\n>> > +     if (gst_structure_has_field_typed(s, \"framerate\",\n>> GST_TYPE_FRACTION)) {\n>> > +             if (!gst_structure_get_fraction(s, \"framerate\", &fps_n,\n>> &fps_d)) {\n>> > +                     GST_WARNING(\"invalid framerate in the caps.\");\n>> > +                     return;\n>> > +             }\n>> > +     }\n>> > +\n>> > +     if (fps_n < 0 || fps_d < 0)\n>> > +             return;\n>> > +\n>> > +     gst_value_set_fraction(framerate_container, fps_n, fps_d);\n>> > +     int64_t frame_duration = (fps_d * 1000000.0) / fps_n;\n>> > +\n>> > +     controls.set(controls::FrameDurationLimits,\n>> > +                  Span<const int64_t, 2>({ frame_duration,\n>> frame_duration }));\n>>\n>> setting the frame_duration even before bound-checking is wrong. This\n>> Function should ideally get the 'framerate' and just save it for later\n>> use/bound-checking after camera::configure()\n>>\n>> Once the bound checking is successfully, only then you are allowed to\n>> set frame_duration on initControls_\n>> > +}\n>> > +\n>> > +void\n>> > +gst_libcamera_bind_framerate(const ControlInfoMap &camera_controls,\n>> ControlList &controls)\n>>\n>> maybe\n>> gst_libcamera_framerate_clamp()\n>> OR\n>> gst_libcamera_framerate_bounds_check()\n>> > +{\n>> > +     gboolean isBound = true;\n>> > +     auto iterFrameDuration =\n>> camera_controls.find(controls::FrameDurationLimits.id());\n>> > +\n>> > +     if (!(iterFrameDuration != camera_controls.end() &&\n>> > +           controls.contains(controls::FRAME_DURATION_LIMITS)))\n>> > +             return;\n>> > +\n>> > +     int64_t frame_duration =\n>> controls.get(controls::FrameDurationLimits).value()[0];\n>> > +     int64_t min_frame_duration =\n>> iterFrameDuration->second.min().get<int64_t>();\n>> > +     int64_t max_frame_duration =\n>> iterFrameDuration->second.max().get<int64_t>();\n>> > +\n>> > +     if (frame_duration < min_frame_duration) {\n>> > +             frame_duration = min_frame_duration;\n>> > +     } else if (frame_duration > max_frame_duration) {\n>> > +             frame_duration = max_frame_duration;\n>> > +     } else {\n>> > +             isBound = false;\n>> > +     }\n>> > +\n>> > +     if (isBound)\n>> > +             controls.set(controls::FrameDurationLimits,\n>> > +                          Span<const int64_t, 2>({ frame_duration,\n>> frame_duration }));\n>> > +}\n>> > +\n>> > +gboolean\n>> > +gst_libcamera_get_framerate_from_init_controls(const ControlList\n>> &controls, GValue *framerate,\n>> > +                                            GValue\n>> *framerate_container)\n>> > +{\n>> > +     gint fps_caps_n, fps_caps_d, numerator, denominator;\n>> > +\n>> > +     fps_caps_n =\n>> gst_value_get_fraction_numerator(framerate_container);\n>> > +     fps_caps_d =\n>> gst_value_get_fraction_denominator(framerate_container);\n>> > +\n>> > +     if (!controls.contains(controls::FRAME_DURATION_LIMITS))\n>> > +             return false;\n>> > +\n>> > +     double framerate_ret = 1000000 /\n>> static_cast<double>(controls.get(controls::FrameDurationLimits).value()[0]);\n>> > +     gst_util_double_to_fraction(framerate_ret, &numerator,\n>> &denominator);\n>> > +     gst_value_set_fraction(framerate, numerator, denominator);\n>> > +\n>> > +     if (fps_caps_n / fps_caps_d == numerator / denominator) {\n>> > +             return true;\n>> > +     }\n>> > +     return false;\n>> > +}\n>> > +\n>> > +GstCaps *\n>> > +gst_libcamera_init_controls_to_caps(const ControlList &controls, const\n>> StreamConfiguration &stream_cfg,\n>> > +                                 GValue *framerate_container)\n>> > +{\n>> > +     GstCaps *caps = gst_caps_new_empty();\n>> > +     GstStructure *s =\n>> bare_structure_from_format(stream_cfg.pixelFormat);\n>> > +     gboolean ret;\n>> > +     GValue *framerate = g_new0(GValue, 1);\n>>\n>> Why create a new GValue here?\n>>\n>> You have everything you need through framerate_container, a applied and\n>> bound-checked framerate, you just need to query the fraction from it and\n>> set the structure?\n>> > +     g_value_init(framerate, GST_TYPE_FRACTION);\n>> > +\n>> > +     ret = gst_libcamera_get_framerate_from_init_controls(controls,\n>> framerate, framerate_container);\n>>\n>> As per the above comment, I am not sure if you really need to do that.\n>> Seems over-kill\n>> > +\n>> > +     gst_structure_set(s,\n>> > +                       \"width\", G_TYPE_INT, stream_cfg.size.width,\n>> > +                       \"height\", G_TYPE_INT, stream_cfg.size.height,\n>> > +                       nullptr);\n>> > +\n>> > +     if (ret) {\n>> > +             gint numerator, denominator;\n>> > +             numerator =\n>> gst_value_get_fraction_numerator(framerate_container);\n>> > +             denominator =\n>> gst_value_get_fraction_denominator(framerate_container);\n>> > +             gst_structure_set(s, \"framerate\", GST_TYPE_FRACTION,\n>> numerator, denominator, nullptr);\n>> > +     } else {\n>> > +             gst_structure_set(s, \"framerate\", GST_TYPE_FRACTION, 0,\n>> 1, nullptr);\n>> > +     }\n>> > +\n>> > +     if (stream_cfg.colorSpace) {\n>>\n>> ?!\n>>\n>> I am not getting why you are introducing code related to colorspace\n>> here? Is it a copy/paste fiasco?\n>> > +             GstVideoColorimetry colorimetry =\n>> colorimetry_from_colorspace(stream_cfg.colorSpace.value());\n>> > +             gchar *colorimetry_str =\n>> gst_video_colorimetry_to_string(&colorimetry);\n>> > +\n>> > +             if (colorimetry_str)\n>> > +                     gst_structure_set(s, \"colorimetry\",\n>> G_TYPE_STRING, colorimetry_str, nullptr);\n>> > +             else\n>> > +                     g_error(\"Got invalid colorimetry from ColorSpace:\n>> %s\",\n>> > +\n>>  ColorSpace::toString(stream_cfg.colorSpace).c_str());\n>> > +     }\n>> > +\n>> > +     gst_caps_append_structure(caps, s);\n>> > +     g_free(framerate);\n>> > +     return caps;\n>> > +}\n>> > +\n>> >   #if !GST_CHECK_VERSION(1, 17, 1)\n>> >   gboolean\n>> >   gst_task_resume(GstTask *task)\n>> > diff --git a/src/gstreamer/gstlibcamera-utils.h\n>> b/src/gstreamer/gstlibcamera-utils.h\n>> > index 164189a2..955a717d 100644\n>> > --- a/src/gstreamer/gstlibcamera-utils.h\n>> > +++ b/src/gstreamer/gstlibcamera-utils.h\n>> > @@ -9,6 +9,7 @@\n>> >   #pragma once\n>> >\n>> >   #include <libcamera/camera_manager.h>\n>> > +#include <libcamera/controls.h>\n>> >   #include <libcamera/stream.h>\n>> >\n>> >   #include <gst/gst.h>\n>> > @@ -18,6 +19,13 @@ GstCaps *gst_libcamera_stream_formats_to_caps(const\n>> libcamera::StreamFormats &fo\n>> >   GstCaps *gst_libcamera_stream_configuration_to_caps(const\n>> libcamera::StreamConfiguration &stream_cfg);\n>> >   void\n>> gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration\n>> &stream_cfg,\n>> >                                             GstCaps *caps);\n>> > +void gst_libcamera_get_init_controls_from_caps(libcamera::ControlList\n>> &controls,\n>> > +                                            GstCaps *caps, GValue\n>> *framerate_container);\n>> > +void gst_libcamera_bind_framerate(const libcamera::ControlInfoMap\n>> &camera_controls,\n>> > +                               libcamera::ControlList &controls);\n>> > +GstCaps *gst_libcamera_init_controls_to_caps(const\n>> libcamera::ControlList &controls,\n>> > +                                          const\n>> libcamera::StreamConfiguration &streame_cfg, GValue *framerate_container);\n>> > +\n>> >   #if !GST_CHECK_VERSION(1, 17, 1)\n>> >   gboolean gst_task_resume(GstTask *task);\n>> >   #endif\n>> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n>> b/src/gstreamer/gstlibcamerasrc.cpp\n>> > index 16d70fea..0c981357 100644\n>> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n>> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n>> > @@ -131,6 +131,7 @@ struct GstLibcameraSrcState {\n>> >       std::queue<std::unique_ptr<RequestWrap>> completedRequests_\n>> >               LIBCAMERA_TSA_GUARDED_BY(lock_);\n>> >\n>> > +     ControlList initControls_;\n>> >       guint group_id_;\n>> >\n>> >       int queueRequest();\n>> > @@ -461,6 +462,8 @@ gst_libcamera_src_task_enter(GstTask *task,\n>> [[maybe_unused]] GThread *thread,\n>> >       GstLibcameraSrcState *state = self->state;\n>> >       GstFlowReturn flow_ret = GST_FLOW_OK;\n>> >       gint ret;\n>> > +     GValue *framerate_container = g_new0(GValue, 1);\n>> > +     framerate_container = g_value_init(framerate_container,\n>> GST_TYPE_FRACTION);\n>>\n>> Since it's a GValue, I would not make it framerate_* specific, so I\n>> would just rename and remove all \"framerate_\" specifics.\n>>\n>> In future, when we have to parse more parameters as a part of\n>> initControls_ setting, the same GValue can be used to temporarily hold\n>> all other values\n>> >\n>> >       GST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n>> >\n>> > @@ -496,6 +499,7 @@ gst_libcamera_src_task_enter(GstTask *task,\n>> [[maybe_unused]] GThread *thread,\n>> >               /* Retrieve the supported caps. */\n>> >               g_autoptr(GstCaps) filter =\n>> gst_libcamera_stream_formats_to_caps(stream_cfg.formats());\n>> >               g_autoptr(GstCaps) caps = gst_pad_peer_query_caps(srcpad,\n>> filter);\n>> > +\n>> >               if (gst_caps_is_empty(caps)) {\n>> >                       flow_ret = GST_FLOW_NOT_NEGOTIATED;\n>> >                       break;\n>> > @@ -504,6 +508,8 @@ gst_libcamera_src_task_enter(GstTask *task,\n>> [[maybe_unused]] GThread *thread,\n>> >               /* Fixate caps and configure the stream. */\n>> >               caps = gst_caps_make_writable(caps);\n>> >               gst_libcamera_configure_stream_from_caps(stream_cfg,\n>> caps);\n>> > +\n>>  gst_libcamera_get_init_controls_from_caps(state->initControls_, caps,\n>> > +\n>>  framerate_container);\n>>\n>> passing state->initControls_ is un-necessary here as we don't need to\n>> set frame_duration on it, just right now (it has to go through bound\n>> check first which is below...)\n>> >       }\n>> >\n>> >       if (flow_ret != GST_FLOW_OK)\n>> > @@ -524,6 +530,7 @@ gst_libcamera_src_task_enter(GstTask *task,\n>> [[maybe_unused]] GThread *thread,\n>> >               const StreamConfiguration &stream_cfg =\n>> state->config_->at(i);\n>> >\n>> >               g_autoptr(GstCaps) caps =\n>> gst_libcamera_stream_configuration_to_caps(stream_cfg);\n>> > +\n>> >               if (!gst_pad_push_event(srcpad,\n>> gst_event_new_caps(caps))) {\n>> >                       flow_ret = GST_FLOW_NOT_NEGOTIATED;\n>> >                       break;\n>> > @@ -544,6 +551,23 @@ gst_libcamera_src_task_enter(GstTask *task,\n>> [[maybe_unused]] GThread *thread,\n>> >               return;\n>> >       }\n>> >\n>> > +     /* bind the framerate */\n>>\n>> s/bind/Clamp OR\n>> /* Check framerate bounds within  controls::FrameDurationLimits */\n>> > +     gst_libcamera_bind_framerate(state->cam_->controls(),\n>> state->initControls_);\n>>\n>> I would just pass the controls::FrameDurationLimits and GValue\n>> (containing the asked framerate) and perform the bound check.\n>>\n>> You can update the GValue's framerate in that function after which\n>> simply, set the initControls' ::FrameDurationLimits right here.\n>> > +\n>> > +     /* Expose the controls in initControls_ throught a new caps\n>> event. */\n>> > +     for (gsize i = 0; i < state->srcpads_.size(); i++) {\n>> > +             GstPad *srcpad = state->srcpads_[i];\n>> > +             const StreamConfiguration &stream_cfg =\n>> state->config_->at(i);\n>> > +\n>> > +             g_autoptr(GstCaps) caps =\n>> gst_libcamera_init_controls_to_caps(state->initControls_,\n>> > +\n>>      stream_cfg, framerate_container);\n>> > +\n>> > +             if (!gst_pad_push_event(srcpad,\n>> gst_event_new_caps(caps))) {\n>> > +                     flow_ret = GST_FLOW_NOT_NEGOTIATED;\n>> > +                     break;\n>> > +             }\n>> > +     }\n>> > +\n>> >       self->allocator = gst_libcamera_allocator_new(state->cam_,\n>> state->config_.get());\n>> >       if (!self->allocator) {\n>> >               GST_ELEMENT_ERROR(self, RESOURCE, NO_SPACE_LEFT,\n>> > @@ -566,7 +590,7 @@ gst_libcamera_src_task_enter(GstTask *task,\n>> [[maybe_unused]] GThread *thread,\n>> >               gst_flow_combiner_add_pad(self->flow_combiner, srcpad);\n>> >       }\n>> >\n>> > -     ret = state->cam_->start();\n>> > +     ret = state->cam_->start(&state->initControls_);\n>> >       if (ret) {\n>> >               GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n>> >                                 (\"Failed to start the camera: %s\",\n>> g_strerror(-ret)),\n>> > @@ -576,6 +600,8 @@ gst_libcamera_src_task_enter(GstTask *task,\n>> [[maybe_unused]] GThread *thread,\n>> >       }\n>> >\n>> >   done:\n>> > +     state->initControls_.clear();\n>> > +     g_free(framerate_container);\n>> >       switch (flow_ret) {\n>> >       case GST_FLOW_NOT_NEGOTIATED:\n>> >               GST_ELEMENT_FLOW_ERROR(self, flow_ret);\n>>\n>>\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C6746C3272\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  8 Sep 2022 09:15:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1006C620A5;\n\tThu,  8 Sep 2022 11:15:35 +0200 (CEST)","from mail-vs1-xe2d.google.com (mail-vs1-xe2d.google.com\n\t[IPv6:2607:f8b0:4864:20::e2d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8A4236206C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  8 Sep 2022 11:15:33 +0200 (CEST)","by mail-vs1-xe2d.google.com with SMTP id u189so7944036vsb.4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 08 Sep 2022 02:15:33 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1662628535;\n\tbh=GOaRMj0KP7TR2BuLh3I4+3FwcPAy7gjV+mz6ItfKuWY=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=4dbK8zf5CSyeXxPuPATujHPNln5IP3qA8cGMn0Zt9uKEmnwfEV//M8l/yAj8Ochav\n\tG+QpgjFd8ji53S0ExLnaD1FwUZIeypMmYG5ckOdFNYICYxg1RJK2Z/ZKJpYawXbM4N\n\tC/f1s2BZF6zrl/4/A8AVd/Ju/nPAHA53Y2tSKPp6b/1TOUyTH5XKeZv27CvV/cmmy0\n\tjqkRRGEgR65z6ylvUXeVA4+QtBuu86I+JAOzWrBp5af7HMVIUp6UFI9BElSFy/SWqr\n\twVCdYwii76Qb+KXwjdWXxC7diIf4iw3OuKeoUaa7mpW/EjDJqOjxOA5BRxqBj4GXFy\n\toCyr0J4xtzGeg==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date;\n\tbh=XPCkxQsQ9mEKHtzv0Tq9+KR5eEwb8F8MTAP5bR6onA4=;\n\tb=HGUXoz4ghSORvEXyMqbHWBggj/WPWQgk83YRkAkTnon9OuxtDejvm2oKCfgZjORieC\n\tYU53FvqdkhuU0UoHiZ2MtpcwUzkAqXJJUzew5LNWicreiDYYIzdoiugHFf9rXkHdx945\n\tVJFnLqaWNkEjHj8NmK5KmMAlYAXvt/VMW12O2IVZT1nCeySa9ia6AfCls3jUgPYHK8k9\n\tTldKl65dapzeLlb8lns8K3wnSZsFGo2ylWjRHdqG1WgLTl9di00d8Dwvh2CUS1NCWpQy\n\t2kZmegzYyjID96YHtwUCKPvsMxBRZK2qbegpKzKOA+jcv9VtmnCGrCAV/uIwePbCANbO\n\t8gjA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"HGUXoz4g\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date;\n\tbh=XPCkxQsQ9mEKHtzv0Tq9+KR5eEwb8F8MTAP5bR6onA4=;\n\tb=UCOTh7jrTTsSahJWNrZi8IUYMd4P3j4U1Fr6pmKtVL5U7L5CQtjySKtyBye0idfLsW\n\tn8jd4O7OpaaoJLfU2hhYfPPGVcj3+VTKZDlEehaIcH8zwWJIoQcKRVHvvJ+Y/ywxoEE5\n\t0uKUJt0ND0roh8D6qjv6fioHr+4AKyBBEtbcVvnYhWAAndN9k1bOGJU+SkgwTXNIOvyq\n\tQ5yit7JbsZhdOEdLe0/L1h5UqzLX8bEFAXjDd3DQEltE3aoHk/zK7+JSn7LYtC8WvAJV\n\to8JDIMdvtQyrdSkmNz8iqmpCBSWZw3E9u3arDqO3fhJ4RhYsSym/LGrLCi8oSLc8upyp\n\tRVkw==","X-Gm-Message-State":"ACgBeo1TBTfUKGcNcen83QZvr/4tJMMnJWkI6kceDeBxTPso9fCu3niN\n\tsFkbrf723svQOclAsEx03gJhz5rb+Juih6nCX1A=","X-Google-Smtp-Source":"AA6agR5ALoqr+32ivwmlvp1bmGhWy9sh6i3qGZr8vSngasypA8K+i+4hgopvOF5nfj146S4/1gAgPoggSsppX0dgDQM=","X-Received":"by 2002:a67:7345:0:b0:398:31d7:d4e0 with SMTP id\n\to66-20020a677345000000b0039831d7d4e0mr471939vsc.11.1662628532198;\n\tThu, 08 Sep 2022 02:15:32 -0700 (PDT)","MIME-Version":"1.0","References":"<20220907104711.87365-1-rishikeshdonadkar@gmail.com>\n\t<96c8f5d4-a742-f5a9-a9db-8bb1bb30f67d@ideasonboard.com>\n\t<CAEQmg0mq8GSkDYLGF4TDXoS4wWTHyrNhFgoismhs3x-5TxB9gQ@mail.gmail.com>\n\t<d8b8e0de-c5ad-7742-9a13-16c4d49ce320@ideasonboard.com>","In-Reply-To":"<d8b8e0de-c5ad-7742-9a13-16c4d49ce320@ideasonboard.com>","Date":"Thu, 8 Sep 2022 14:45:20 +0530","Message-ID":"<CAEQmg0nOBHeLmig6xYm2hKTDHpA+CRJ-ucGf+xeE0QzZs+mQ6Q@mail.gmail.com>","To":"Umang Jain <umang.jain@ideasonboard.com>","Content-Type":"multipart/alternative; boundary=\"0000000000000e227005e826dfa9\"","Subject":"Re: [libcamera-devel] [PATCH v3] gstreamer: Provide framerate\n\tsupport for libcamerasrc.","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Rishikesh Donadkar via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Rishikesh Donadkar <rishikeshdonadkar@gmail.com>","Cc":"libcamera-devel@lists.libcamera.org, vedantparanjape160201@gmail.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]