[{"id":25716,"web_url":"https://patchwork.libcamera.org/comment/25716/","msgid":"","date":"2022-11-02T14:13:13","subject":"Re: [libcamera-devel] [PATCH v6 2/2] 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":"Hello,\n\nOn 11/2/22 7:26 PM, Umang Jain wrote:\n> From: Rishikesh Donadkar \n>\n> Control the framerate by passing the controls::FrameDurationLimits\n> during Camera::start(). Framerate in gstreamer is expressed as\n> GST_TYPE_FRACTION so we maximise on maintaining it as a fraction\n> throughout and only do arithematic computations as and when required\n> (to compute frame-duration and vice-versa).\n>\n> To weed out abritrary framerate as input, place the clamping via the\n> controls::FrameDurationLimits provided after camera::configure() phase.\n> This is handled by a helper function\n> gst_libcamera_clamp_and_set_frameduration().\n>\n> Set the bound checked framerate (done in the above mentioned helper)\n> into the caps and pass the ControlList containing the frame-duration\n> to Camera::start(ctrls).\n>\n> Signed-off-by: Rishikesh Donadkar \n> Signed-off-by: Umang Jain \n> Tested-by: Umang Jain \n> ---\n> src/gstreamer/gstlibcamera-utils.cpp | 78 ++++++++++++++++++++++++++++\n> src/gstreamer/gstlibcamera-utils.h | 6 +++\n> src/gstreamer/gstlibcamerasrc.cpp | 15 +++++-\n> 3 files changed, 98 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> index 244a4a79..c14f72ec 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 \n> #include \n> \n> using namespace libcamera;\n> @@ -407,6 +408,83 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> \t}\n> }\n> \n> +void gst_libcamera_get_framerate_from_caps(GstCaps *caps,\n> +\t\t\t\t\t GstStructure *container)\n> +{\n> +\tGstStructure *s = gst_caps_get_structure(caps, 0);\n> +\t/*\n> +\t * Default to 30 fps. If the \"framerate\" fraction is invalid below,\n> +\t * libcamerasrc will set 30fps as the framerate.\n> +\t */\n> +\tgint fps_n = 30, 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}\n> +\n> +\tgst_structure_set(container, \"framerate\", GST_TYPE_FRACTION,\n> +\t\t\t fps_n, fps_d, nullptr);\n> +}\n> +\n> +void gst_libcamera_clamp_and_set_frameduration(ControlList &initCtrls,\n> +\t\t\t\t\t const ControlInfoMap &cam_ctrls,\n> +\t\t\t\t\t GstStructure *container)\n> +{\n> +\tgboolean in_bounds = false;\n> +\tgint fps_caps_n, fps_caps_d;\n> +\n> +\tif (!gst_structure_has_field_typed(container, \"framerate\", GST_TYPE_FRACTION))\n> +\t\treturn;\n> +\n> +\tauto iterFrameDuration = cam_ctrls.find(controls::FrameDurationLimits.id());\n> +\tif (iterFrameDuration == cam_ctrls.end()) {\n> +\t\tGST_WARNING(\"FrameDurationLimits not found in camera controls.\");\n> +\t\treturn;\n> +\t}\n> +\n> +\tconst GValue *framerate = gst_structure_get_value(container, \"framerate\");\n> +\n> +\tfps_caps_n = gst_value_get_fraction_numerator(framerate);\n> +\tfps_caps_d = gst_value_get_fraction_denominator(framerate);\n> +\n> +\tint64_t frame_duration = (fps_caps_d * 1000000.0) / fps_caps_n;\n> +\tint64_t min_frame_duration = iterFrameDuration->second.min().get();\n> +\tint64_t max_frame_duration = iterFrameDuration->second.max().get();\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\tin_bounds = true;\n> +\t}\n> +\n> +\tif (!in_bounds) {\n> +\t\tgint framerate_clamped = 1000000 / frame_duration;\n> +\n> +\t\t/* Update the framerate which then will be exposed in caps. */\n> +\t\tgst_structure_set(container, \"framerate\", GST_TYPE_FRACTION,\n> +\t\t\t\t framerate_clamped, 1, nullptr);\n\nSo the logic here is that, if the input is too high/low fps - it will be \nbound to min/max FrameDuration and exposed it caps.\n\nIt doesn't mean, the camera will go on to stream ...  it means that \nbound-limit will be exposed in caps but the negotiation will probably \nfail (representing that the fps requested was too high/low and not \nsupported by the camera)\n\nFor e.g.\n\nInput: framerate=100/1\n     [[capped to framerate=43/1 by this patch on OV5647 ]]\nExposed in caps : framerate=43/1\n     [[negotation failed]]\n     Expected framerate=100/1 but got framerate=43/1\n\nIt seems to me this is expected. But I am not 100% sure here.\n\nCan the expectation be to stream the camera somehow (not sure how to \nsurpass negotitaiton in caps) with the bound-limit set?  Probably worth \nexploring other gstreamer plugins specific to framerate handling...\n\n\n> +\t}\n> +\n> +\tinitCtrls.set(controls::FrameDurationLimits,\n> +\t\t { frame_duration, frame_duration });\n> +}\n> +\n> +void gst_libcamera_framerate_to_caps(GstCaps *caps, const GValue *container)\n> +{\n> +\tif (!GST_VALUE_HOLDS_FRACTION(container))\n> +\t\treturn;\n> +\n> +\tGstStructure *s = gst_caps_get_structure(caps, 0);\n> +\tgint fps_caps_n, fps_caps_d;\n> +\n> +\tfps_caps_n = gst_value_get_fraction_numerator(container);\n> +\tfps_caps_d = gst_value_get_fraction_denominator(container);\n> +\tgst_structure_set(s, \"framerate\", GST_TYPE_FRACTION, fps_caps_n, fps_caps_d, nullptr);\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..3d217fcf 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 \n> +#include \n> #include \n> \n> #include \n> @@ -18,6 +19,11 @@ 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_framerate_from_caps(GstCaps *caps, GstStructure *container);\n> +void gst_libcamera_clamp_and_set_frameduration(libcamera::ControlList &controls,\n> +\t\t\t\t\t const libcamera::ControlInfoMap &camera_controls, GstStructure *container);\n> +void gst_libcamera_framerate_to_caps(GstCaps *caps, const GValue *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 60032236..955d6eac 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -131,6 +131,7 @@ struct GstLibcameraSrcState {\n> \tstd::queue> completedRequests_\n> \t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n> \n> +\tControlList initControls_;\n> \tguint group_id_;\n> \n> \tint queueRequest();\n> @@ -462,6 +463,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> \tGstFlowReturn flow_ret = GST_FLOW_OK;\n> \tgint ret;\n> \n> +\tGstStructure *container = gst_structure_new_empty(\"container\");\n> +\n> \tGST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n> \n> \tgint stream_id_num = 0;\n> @@ -504,6 +507,7 @@ 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_framerate_from_caps(caps, container);\n> \t}\n> \n> \tif (flow_ret != GST_FLOW_OK)\n> @@ -525,6 +529,10 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> \t\tgoto done;\n> \t}\n> \n> +\t/* Check frame duration bounds within controls::FrameDurationLimits */\n> +\tgst_libcamera_clamp_and_set_frameduration(state->initControls_,\n> +\t\t\t\t\t\t state->cam_->controls(), container);\n> +\n> \t/*\n> \t * Regardless if it has been modified, create clean caps and push the\n> \t * caps event. Downstream will decide if the caps are acceptable.\n> @@ -532,8 +540,11 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\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> +\t\tconst GValue *framerate = gst_structure_get_value(container, \"framerate\");\n> \n> \t\tg_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg);\n> +\t\tgst_libcamera_framerate_to_caps(caps, framerate);\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> @@ -567,7 +578,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> @@ -577,6 +588,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> \t}\n> \n> done:\n> +\tstate->initControls_.clear();\n> +\tg_free(container);\n> \tswitch (flow_ret) {\n> \tcase GST_FLOW_NOT_NEGOTIATED:\n> \t\tGST_ELEMENT_FLOW_ERROR(self, flow_ret);","headers":{"Return-Path":"","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 19CCDBDB16\n\tfor ;\n\tWed, 2 Nov 2022 14:13:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 741916307E;\n\tWed, 2 Nov 2022 15:13:22 +0100 (CET)","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 14DA563075\n\tfor ;\n\tWed, 2 Nov 2022 15:13:21 +0100 (CET)","from [192.168.1.108] (unknown [103.251.226.107])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A98D98B;\n\tWed, 2 Nov 2022 15:13:19 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1667398402;\n\tbh=S3IGT3Xwk0YYkTvp4HoShL1YxGRuKsAkbhzBrGRYWdY=;\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:\n\tFrom;\n\tb=Qvl7n7kD9okRw2qfuITDVO5LGaqUL3bykJOKD7rpZZdqnTnfxxhuv8b+Ii+6mc3f0\n\tT4//HflN+ySsPeNpdzxv1uM5ltM/QRsvrFn1BNCXBFPYGiFVTTLt4SG3dfBCj+V/P3\n\tdJEby87Yxr0JjSEEDnOmCLTyE52UOJOXCqlhBCYY35eK4vBQMcEJuDbVjYL/jKZPFu\n\tfowZXWQ8NWJ+iQg4zEGsKMluP8UZ/NBKyewI5GUZIu6cfrHgSJbamDkUaMmSpRryTO\n\t1vaqjk98UKNk2l1oMjy6+8yv66yEuUSe50h8xj+GC1kDz5G7p7QxMywHEs4UaPk+o4\n\tDzk7NDtKNAcGg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1667398400;\n\tbh=S3IGT3Xwk0YYkTvp4HoShL1YxGRuKsAkbhzBrGRYWdY=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Zx4e9DxS5XL5flkJshfG5mGKw4j6FpPlj3KtpOrAiX/4AJZAOTQXn9IoHx2PO8siF\n\tY9PDGtmiFJIrvkpag3JYLUrXZk0hp094plpnMRZ2ZvvLqjrPbJ/2cwiPL290Z6jSG9\n\tV9+Qf755X+N0QqSRQc4CIymep07UlfYLtZVz01zM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Zx4e9DxS\"; dkim-atps=neutral","Message-ID":"","Date":"Wed, 2 Nov 2022 19:43:13 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.2.1","To":"libcamera-devel@lists.libcamera.org","References":"<20221102135614.657444-1-umang.jain@ideasonboard.com>\n\t<20221102135614.657444-3-umang.jain@ideasonboard.com>","Content-Language":"en-US","In-Reply-To":"<20221102135614.657444-3-umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v6 2/2] gstreamer: Provide framerate\n\tsupport for libcamerasrc","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25844,"web_url":"https://patchwork.libcamera.org/comment/25844/","msgid":"","date":"2022-11-21T18:30:01","subject":"Re: [libcamera-devel] [PATCH v6 2/2] gstreamer: Provide framerate\n\tsupport for libcamerasrc","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le vendredi 04 novembre 2022 à 11:25 +0000, Kieran Bingham via libcamera-devel a\nécrit :\n> Quoting Umang Jain via libcamera-devel (2022-11-02 14:13:13)\n> > Hello,\n> > \n> > On 11/2/22 7:26 PM, Umang Jain wrote:\n> > > From: Rishikesh Donadkar \n> > > \n> > > Control the framerate by passing the controls::FrameDurationLimits\n> > > during Camera::start(). Framerate in gstreamer is expressed as\n> > > GST_TYPE_FRACTION so we maximise on maintaining it as a fraction\n> > > throughout and only do arithematic computations as and when required\n> > > (to compute frame-duration and vice-versa).\n> > > \n> > > To weed out abritrary framerate as input, place the clamping via the\n> > > controls::FrameDurationLimits provided after camera::configure() phase.\n> > > This is handled by a helper function\n> > > gst_libcamera_clamp_and_set_frameduration().\n> > > \n> > > Set the bound checked framerate (done in the above mentioned helper)\n> > > into the caps and pass the ControlList containing the frame-duration\n> > > to Camera::start(ctrls).\n> > > \n> > > Signed-off-by: Rishikesh Donadkar \n> > > Signed-off-by: Umang Jain \n> > > Tested-by: Umang Jain \n> > > ---\n> > > src/gstreamer/gstlibcamera-utils.cpp | 78 ++++++++++++++++++++++++++++\n> > > src/gstreamer/gstlibcamera-utils.h | 6 +++\n> > > src/gstreamer/gstlibcamerasrc.cpp | 15 +++++-\n> > > 3 files changed, 98 insertions(+), 1 deletion(-)\n> > > \n> > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> > > index 244a4a79..c14f72ec 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 \n> > > #include \n> > > \n> > > using namespace libcamera;\n> > > @@ -407,6 +408,83 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> > > }\n> > > }\n> > > \n> > > +void gst_libcamera_get_framerate_from_caps(GstCaps *caps,\n> > > + GstStructure *container)\n> > > +{\n> > > + GstStructure *s = gst_caps_get_structure(caps, 0);\n> > > + /*\n> > > + * Default to 30 fps. If the \"framerate\" fraction is invalid below,\n> > > + * libcamerasrc will set 30fps as the framerate.\n> > > + */\n> > > + gint fps_n = 30, fps_d = 1;\n> > > +\n> > > + if (gst_structure_has_field_typed(s, \"framerate\", GST_TYPE_FRACTION)) {\n> > > + if (!gst_structure_get_fraction(s, \"framerate\", &fps_n, &fps_d))\n> > > + GST_WARNING(\"Invalid framerate in the caps\");\n> \n> I wonder if this should fail negotiation - but I don't actually mind if\n> it continues. It's a bit like the negotiation failure below though.. ?\n\nYou may have multiple sturctures like:\n\n video/x-raw,framerate=30/1;video/x-raw\n\nThat indicates a preference for 30fps, but does not limite to other framerate.\nThis is always what you'd get by using a rate adapter like videorate. In theory,\na better approach would be to iterate all the structure, not just the very first\none.\n\n> \n> > > + }\n> > > +\n> > > + gst_structure_set(container, \"framerate\", GST_TYPE_FRACTION,\n> > > + fps_n, fps_d, nullptr);\n> > > +}\n> > > +\n> > > +void gst_libcamera_clamp_and_set_frameduration(ControlList &initCtrls,\n> > > + const ControlInfoMap &cam_ctrls,\n> > > + GstStructure *container)\n> > > +{\n> > > + gboolean in_bounds = false;\n> > > + gint fps_caps_n, fps_caps_d;\n> > > +\n> > > + if (!gst_structure_has_field_typed(container, \"framerate\", GST_TYPE_FRACTION))\n> > > + return;\n> > > +\n> > > + auto iterFrameDuration = cam_ctrls.find(controls::FrameDurationLimits.id());\n> > > + if (iterFrameDuration == cam_ctrls.end()) {\n> > > + GST_WARNING(\"FrameDurationLimits not found in camera controls.\");\n> > > + return;\n> > > + }\n> > > +\n> > > + const GValue *framerate = gst_structure_get_value(container, \"framerate\");\n> > > +\n> > > + fps_caps_n = gst_value_get_fraction_numerator(framerate);\n> > > + fps_caps_d = gst_value_get_fraction_denominator(framerate);\n> > > +\n> > > + int64_t frame_duration = (fps_caps_d * 1000000.0) / fps_caps_n;\n> > > + int64_t min_frame_duration = iterFrameDuration->second.min().get();\n> > > + int64_t max_frame_duration = iterFrameDuration->second.max().get();\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> > > + in_bounds = true;\n> > > + }\n> > > +\n> > > + if (!in_bounds) {\n> \n> I don't think we need to open code clamp() here:\n> \n> \tint64_t target_duration = (fps_caps_d * 1000000.0) / fps_caps_n;\n> \tint64_t min_frame_duration = iterFrameDuration->second.min().get();\n> \tint64_t max_frame_duration = iterFrameDuration->second.max().get();\n> \n> \tint64_t frame_duration = std::clamp(target_duration,\n> \t\t\t\t\t min_frame_duration,\n> \t\t\t\t\t max_frame_duration);\n> \t\n> \tif (frame_duration != target_duration) {\n> \n> \n> > > + gint framerate_clamped = 1000000 / frame_duration;\n> > > +\n> > > + /* Update the framerate which then will be exposed in caps. */\n> > > + gst_structure_set(container, \"framerate\", GST_TYPE_FRACTION,\n> > > + framerate_clamped, 1, nullptr);\n> > \n> > So the logic here is that, if the input is too high/low fps - it will be \n> > bound to min/max FrameDuration and exposed it caps.\n> > \n> > It doesn't mean, the camera will go on to stream ...  it means that \n> > bound-limit will be exposed in caps but the negotiation will probably \n> > fail (representing that the fps requested was too high/low and not \n> > supported by the camera)\n> > \n> > For e.g.\n> > \n> > Input: framerate=100/1\n> >     [[capped to framerate=43/1 by this patch on OV5647 ]]\n> > Exposed in caps : framerate=43/1\n> >     [[negotation failed]]\n> >     Expected framerate=100/1 but got framerate=43/1\n> > \n> > It seems to me this is expected. But I am not 100% sure here.\n> > \n> > Can the expectation be to stream the camera somehow (not sure how to \n> > surpass negotitaiton in caps) with the bound-limit set?  Probably worth \n> > exploring other gstreamer plugins specific to framerate handling...\n> \n> Indeed, this seems to be how the situation is reported by a user on\n> github:\n> - https://github.com/raspberrypi/libcamera/issues/30#issuecomment-1301117923\n> \n> \n> I believe it's the correct behaviour. If a pipeline explicitly requests 1000 FPS, and\n> the camera can not deliver, it is a failure to negotiate. But it's worth\n> checking on #gstreamer or hearing from Nicolas ?\n> \n> - I've asked on #gstreamer ...\n> \n> \n> \n> > > + }\n> > > +\n> > > + initCtrls.set(controls::FrameDurationLimits,\n> > > + { frame_duration, frame_duration });\n> > > +}\n> > > +\n> > > +void gst_libcamera_framerate_to_caps(GstCaps *caps, const GValue *container)\n> > > +{\n> > > + if (!GST_VALUE_HOLDS_FRACTION(container))\n> > > + return;\n> > > +\n> > > + GstStructure *s = gst_caps_get_structure(caps, 0);\n> > > + gint fps_caps_n, fps_caps_d;\n> > > +\n> > > + fps_caps_n = gst_value_get_fraction_numerator(container);\n> > > + fps_caps_d = gst_value_get_fraction_denominator(container);\n> > > + gst_structure_set(s, \"framerate\", GST_TYPE_FRACTION, fps_caps_n, fps_caps_d, nullptr);\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..3d217fcf 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 \n> > > +#include \n> > > #include \n> > > \n> > > #include \n> > > @@ -18,6 +19,11 @@ 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> > > GstCaps *caps);\n> > > +void gst_libcamera_get_framerate_from_caps(GstCaps *caps, GstStructure *container);\n> > > +void gst_libcamera_clamp_and_set_frameduration(libcamera::ControlList &controls,\n> > > + const libcamera::ControlInfoMap &camera_controls, GstStructure *container);\n> > > +void gst_libcamera_framerate_to_caps(GstCaps *caps, const GValue *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 60032236..955d6eac 100644\n> > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > @@ -131,6 +131,7 @@ struct GstLibcameraSrcState {\n> > > std::queue> completedRequests_\n> > > LIBCAMERA_TSA_GUARDED_BY(lock_);\n> > > \n> > > + ControlList initControls_;\n> > > guint group_id_;\n> > > \n> > > int queueRequest();\n> > > @@ -462,6 +463,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> > > GstFlowReturn flow_ret = GST_FLOW_OK;\n> > > gint ret;\n> > > \n> > > + GstStructure *container = gst_structure_new_empty(\"container\");\n> > > +\n> \n> Container seems to be a really non-descript term here ...\n> \n> Is this only used for framerate or is it expected to be used to contain\n> 'anything' ?\n> \n> > > GST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n> > > \n> > > gint stream_id_num = 0;\n> > > @@ -504,6 +507,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[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> > > + gst_libcamera_get_framerate_from_caps(caps, container);\n> \n> maybe container could be called 'framerate' then ? It's going to store\n> the framerate right ?\n> \n> > > }\n> > > \n> > > if (flow_ret != GST_FLOW_OK)\n> > > @@ -525,6 +529,10 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> > > goto done;\n> > > }\n> > > \n> > > + /* Check frame duration bounds within controls::FrameDurationLimits */\n> > > + gst_libcamera_clamp_and_set_frameduration(state->initControls_,\n> > > + state->cam_->controls(), container);\n> > > +\n> > > /*\n> > > * Regardless if it has been modified, create clean caps and push the\n> > > * caps event. Downstream will decide if the caps are acceptable.\n> > > @@ -532,8 +540,11 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> > > for (gsize i = 0; i < state->srcpads_.size(); i++) {\n> > > GstPad *srcpad = state->srcpads_[i];\n> > > const StreamConfiguration &stream_cfg = state->config_->at(i);\n> > > + const GValue *framerate = gst_structure_get_value(container, \"framerate\");\n> > > \n> > > g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg);\n> > > + gst_libcamera_framerate_to_caps(caps, framerate);\n> \n> If gst_libcamera_framerate_to_caps took a GstStructure *, it can do the\n> conversion in that function, and you can use the name 'framerate' for\n> the structure variable in this scope.\n> \n> > > +\n> > > if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) {\n> > > flow_ret = GST_FLOW_NOT_NEGOTIATED;\n> > > break;\n> > > @@ -567,7 +578,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[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\", g_strerror(-ret)),\n> > > @@ -577,6 +588,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> > > }\n> > > \n> > > done:\n> > > + state->initControls_.clear();\n> > > + g_free(container);\n> \n> Does the gstreamer framework provide any smart unique pointer to hold\n> the container that would free automatically when this function goes out\n> of scope ?\n> \n> Aside from all that, which really are solveable /minors /bikeshedding:\n> \n> \n> Reviewed-by: Kieran Bingham \n> \n> \n> \n> > > switch (flow_ret) {\n> > > case GST_FLOW_NOT_NEGOTIATED:\n> > > GST_ELEMENT_FLOW_ERROR(self, flow_ret);\n> >","headers":{"Return-Path":"","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 83826BE08B\n\tfor ;\n\tMon, 21 Nov 2022 18:30:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DF4BA6331A;\n\tMon, 21 Nov 2022 19:30:05 +0100 (CET)","from mail-qt1-x831.google.com (mail-qt1-x831.google.com\n\t[IPv6:2607:f8b0:4864:20::831])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3FF4A63097\n\tfor ;\n\tMon, 21 Nov 2022 19:30:04 +0100 (CET)","by mail-qt1-x831.google.com with SMTP id h24so7807083qta.9\n\tfor ;\n\tMon, 21 Nov 2022 10:30:04 -0800 (PST)","from nicolas-tpx395.localdomain (192-222-136-102.qc.cable.ebox.net.\n\t[192.222.136.102]) by smtp.gmail.com with ESMTPSA id\n\tl25-20020a37f919000000b006fb38ff190bsm8668068qkj.34.2022.11.21.10.30.02\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 21 Nov 2022 10:30:02 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669055405;\n\tbh=EOJOOr6nHx4vjdlOiqu5qlz48jxnY0OyhyIEolpEiX8=;\n\th=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=b7BAuYy4FdzVvQC9iNg5JymErgLz5QAIPzOC0qH7K0kvIHiYRTBksrskIHi3oOs5J\n\twuHJXNWOlfXr+F+QeOyqstCzF7cSqv5184KcPdI95IMDDWFhzoCdZKCaewu+ByfTWX\n\tWRI6IyqVwNBo5Exq4bmVWtfxRE/bRM9MnE1UNhnOJpLCbrT7d+LLYLJpKoLnYh6u7E\n\tsAgGI8mYlTBnyzPDU9hZZpD5KjnbgKCHirUfN/q/e0ifkca7srweGeDQKrP75Qmcdc\n\tLHvkpJQDdNBnHGjP+qGmy7wWLd3RK1eHemVyUsc1MM/lWR/JmPDqidQAMe8Vw1nzSa\n\t/MGUp12Hp8LxA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20210112.gappssmtp.com; s=20210112;\n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:to:from:subject:message-id:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=3CXjVnXdlrrFrr6lqUs40B7n1OQuqQjzylkfrXEKfgA=;\n\tb=7Vd/MW0M/Tldzf9DKSTjKoFqnR2yLkz5uDuMGM5DW4X73xxcqhTwnXFORM7YMXXred\n\tlur79XCYr4Gb6CV0jifsm1WIUUDxd6urDuFiCeJ0GnbdupLc+Dw9Ll+9PZ9d3itIVLYs\n\tj3EW4ruNfP57AdanJ1hxZnCsNnuqPQh1zt+g0my12biQ/qPel2qp15QvEhywtuViQz96\n\tt/YSF2VL08z0ChOHkv/A0p/sf2dniQ/m2AheBTsHQUsVI3v6EKzJ+UOVVPluqFjF71Wc\n\tDpg+/o+QLLnvphTF8EqmoGFLIbvwuviKfjVqOYLJy63LLisDTEopTZDSLoCUVd5WAbG2\n\tiFYg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ndufresne-ca.20210112.gappssmtp.com\n\theader.i=@ndufresne-ca.20210112.gappssmtp.com header.b=\"7Vd/MW0M\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:to:from:subject:message-id:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=3CXjVnXdlrrFrr6lqUs40B7n1OQuqQjzylkfrXEKfgA=;\n\tb=sdXCwxM90GldMO2DN71IrKkJ1WISpmMUaFq7dugzFMlrMpmW1uQr5ABYGacoen+YF2\n\tROvIFGWqrknB4ga2ijFCOAMleH6isbB0OvD9xn3GsYVGI4lhl4oWwEFTR5AnI6VqgqMK\n\tTpgnM2/3h9cXI/OzdQ3myytM0XX7dL2YCMKNIigRkquS16T1e5bBilUe93u7sZZtSAyV\n\t4YbCZ32/QVLG/sW5oztK7hg2SsPtMfgHeBnAuYlVNNRKzcS9jBLMr7R7ig41HxmHY8n1\n\t5ACYD41ICMksFZdOErJQkq647RQtEnkENQDB3JZbrC7cZ2Ggshfjx/dK0H7eSs0Wnptd\n\tLbZA==","X-Gm-Message-State":"ANoB5plxm/El99JJddJh/62dOO59hHRhw8ETO+4DKhptotp9p3P+Ck46\n\th04P6EBJ/OBf55KFM8RJ9GnUZwiyGlVwuA==","X-Google-Smtp-Source":"AA0mqf5ROdnVOrGEta6hC+QCX25E9X2gOSkUS+QWnxYuix9I1uMVW8LWIDBDrcMI/ZjPUM3uqAhUoA==","X-Received":"by 2002:a05:622a:514e:b0:3a5:2988:c67c with SMTP id\n\tew14-20020a05622a514e00b003a52988c67cmr6300829qtb.487.1669055402913; \n\tMon, 21 Nov 2022 10:30:02 -0800 (PST)","Message-ID":"","To":"Kieran Bingham , Umang Jain\n\t, libcamera-devel@lists.libcamera.org","Date":"Mon, 21 Nov 2022 13:30:01 -0500","In-Reply-To":"<166756111421.15935.7749295504550053866@Monstersaurus>","References":"<20221102135614.657444-1-umang.jain@ideasonboard.com>\n\t<20221102135614.657444-3-umang.jain@ideasonboard.com>\n\t\n\t<166756111421.15935.7749295504550053866@Monstersaurus>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.44.4 (3.44.4-2.fc36) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v6 2/2] gstreamer: Provide framerate\n\tsupport for libcamerasrc","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Nicolas Dufresne via libcamera-devel\n\t","Reply-To":"Nicolas Dufresne ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25845,"web_url":"https://patchwork.libcamera.org/comment/25845/","msgid":"<166906624707.512294.12808042651517244144@Monstersaurus>","date":"2022-11-21T21:30:47","subject":"Re: [libcamera-devel] [PATCH v6 2/2] gstreamer: Provide framerate\n\tsupport for libcamerasrc","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Nicolas Dufresne (2022-11-21 18:30:01)\n> Le vendredi 04 novembre 2022 à 11:25 +0000, Kieran Bingham via libcamera-devel a\n> écrit :\n> > Quoting Umang Jain via libcamera-devel (2022-11-02 14:13:13)\n> > > Hello,\n> > > \n> > > On 11/2/22 7:26 PM, Umang Jain wrote:\n> > > > From: Rishikesh Donadkar \n> > > > \n> > > > Control the framerate by passing the controls::FrameDurationLimits\n> > > > during Camera::start(). Framerate in gstreamer is expressed as\n> > > > GST_TYPE_FRACTION so we maximise on maintaining it as a fraction\n> > > > throughout and only do arithematic computations as and when required\n> > > > (to compute frame-duration and vice-versa).\n> > > > \n> > > > To weed out abritrary framerate as input, place the clamping via the\n> > > > controls::FrameDurationLimits provided after camera::configure() phase.\n> > > > This is handled by a helper function\n> > > > gst_libcamera_clamp_and_set_frameduration().\n> > > > \n> > > > Set the bound checked framerate (done in the above mentioned helper)\n> > > > into the caps and pass the ControlList containing the frame-duration\n> > > > to Camera::start(ctrls).\n> > > > \n> > > > Signed-off-by: Rishikesh Donadkar \n> > > > Signed-off-by: Umang Jain \n> > > > Tested-by: Umang Jain \n> > > > ---\n> > > > src/gstreamer/gstlibcamera-utils.cpp | 78 ++++++++++++++++++++++++++++\n> > > > src/gstreamer/gstlibcamera-utils.h | 6 +++\n> > > > src/gstreamer/gstlibcamerasrc.cpp | 15 +++++-\n> > > > 3 files changed, 98 insertions(+), 1 deletion(-)\n> > > > \n> > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> > > > index 244a4a79..c14f72ec 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 \n> > > > #include \n> > > > \n> > > > using namespace libcamera;\n> > > > @@ -407,6 +408,83 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> > > > }\n> > > > }\n> > > > \n> > > > +void gst_libcamera_get_framerate_from_caps(GstCaps *caps,\n> > > > + GstStructure *container)\n> > > > +{\n> > > > + GstStructure *s = gst_caps_get_structure(caps, 0);\n> > > > + /*\n> > > > + * Default to 30 fps. If the \"framerate\" fraction is invalid below,\n> > > > + * libcamerasrc will set 30fps as the framerate.\n> > > > + */\n> > > > + gint fps_n = 30, fps_d = 1;\n> > > > +\n> > > > + if (gst_structure_has_field_typed(s, \"framerate\", GST_TYPE_FRACTION)) {\n> > > > + if (!gst_structure_get_fraction(s, \"framerate\", &fps_n, &fps_d))\n> > > > + GST_WARNING(\"Invalid framerate in the caps\");\n> > \n> > I wonder if this should fail negotiation - but I don't actually mind if\n> > it continues. It's a bit like the negotiation failure below though.. ?\n> \n> You may have multiple sturctures like:\n> \n> video/x-raw,framerate=30/1;video/x-raw\n> \n> That indicates a preference for 30fps, but does not limite to other framerate.\n> This is always what you'd get by using a rate adapter like videorate. In theory,\n> a better approach would be to iterate all the structure, not just the very first\n> one.\n\nIs there any existing code that parses the structures that would be a\nhelpful reference here ?\n\nDo we just keep trying to get a structure until we either, get a\nnullpointer, or find a satisfactory/satisfiable frame rate result ?\n\n\n> > \n> > > > + }\n> > > > +\n> > > > + gst_structure_set(container, \"framerate\", GST_TYPE_FRACTION,\n> > > > + fps_n, fps_d, nullptr);\n> > > > +}\n> > > > +\n> > > > +void gst_libcamera_clamp_and_set_frameduration(ControlList &initCtrls,\n> > > > + const ControlInfoMap &cam_ctrls,\n> > > > + GstStructure *container)\n> > > > +{\n> > > > + gboolean in_bounds = false;\n> > > > + gint fps_caps_n, fps_caps_d;\n> > > > +\n> > > > + if (!gst_structure_has_field_typed(container, \"framerate\", GST_TYPE_FRACTION))\n> > > > + return;\n> > > > +\n> > > > + auto iterFrameDuration = cam_ctrls.find(controls::FrameDurationLimits.id());\n> > > > + if (iterFrameDuration == cam_ctrls.end()) {\n> > > > + GST_WARNING(\"FrameDurationLimits not found in camera controls.\");\n> > > > + return;\n> > > > + }\n> > > > +\n> > > > + const GValue *framerate = gst_structure_get_value(container, \"framerate\");\n> > > > +\n> > > > + fps_caps_n = gst_value_get_fraction_numerator(framerate);\n> > > > + fps_caps_d = gst_value_get_fraction_denominator(framerate);\n> > > > +\n> > > > + int64_t frame_duration = (fps_caps_d * 1000000.0) / fps_caps_n;\n> > > > + int64_t min_frame_duration = iterFrameDuration->second.min().get();\n> > > > + int64_t max_frame_duration = iterFrameDuration->second.max().get();\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> > > > + in_bounds = true;\n> > > > + }\n> > > > +\n> > > > + if (!in_bounds) {\n> > \n> > I don't think we need to open code clamp() here:\n> > \n> > int64_t target_duration = (fps_caps_d * 1000000.0) / fps_caps_n;\n> > int64_t min_frame_duration = iterFrameDuration->second.min().get();\n> > int64_t max_frame_duration = iterFrameDuration->second.max().get();\n> > \n> > int64_t frame_duration = std::clamp(target_duration,\n> > min_frame_duration,\n> > max_frame_duration);\n> > \n> > if (frame_duration != target_duration) {\n> > \n> > \n> > > > + gint framerate_clamped = 1000000 / frame_duration;\n> > > > +\n> > > > + /* Update the framerate which then will be exposed in caps. */\n> > > > + gst_structure_set(container, \"framerate\", GST_TYPE_FRACTION,\n> > > > + framerate_clamped, 1, nullptr);\n> > > \n> > > So the logic here is that, if the input is too high/low fps - it will be \n> > > bound to min/max FrameDuration and exposed it caps.\n> > > \n> > > It doesn't mean, the camera will go on to stream ...  it means that \n> > > bound-limit will be exposed in caps but the negotiation will probably \n> > > fail (representing that the fps requested was too high/low and not \n> > > supported by the camera)\n> > > \n> > > For e.g.\n> > > \n> > > Input: framerate=100/1\n> > >     [[capped to framerate=43/1 by this patch on OV5647 ]]\n> > > Exposed in caps : framerate=43/1\n> > >     [[negotation failed]]\n> > >     Expected framerate=100/1 but got framerate=43/1\n> > > \n> > > It seems to me this is expected. But I am not 100% sure here.\n> > > \n> > > Can the expectation be to stream the camera somehow (not sure how to \n> > > surpass negotitaiton in caps) with the bound-limit set?  Probably worth \n> > > exploring other gstreamer plugins specific to framerate handling...\n> > \n> > Indeed, this seems to be how the situation is reported by a user on\n> > github:\n> > - https://github.com/raspberrypi/libcamera/issues/30#issuecomment-1301117923\n> > \n> > \n> > I believe it's the correct behaviour. If a pipeline explicitly requests 1000 FPS, and\n> > the camera can not deliver, it is a failure to negotiate. But it's worth\n> > checking on #gstreamer or hearing from Nicolas ?\n> > \n> > - I've asked on #gstreamer ...\n> > \n> > \n> > \n> > > > + }\n> > > > +\n> > > > + initCtrls.set(controls::FrameDurationLimits,\n> > > > + { frame_duration, frame_duration });\n> > > > +}\n> > > > +\n> > > > +void gst_libcamera_framerate_to_caps(GstCaps *caps, const GValue *container)\n> > > > +{\n> > > > + if (!GST_VALUE_HOLDS_FRACTION(container))\n> > > > + return;\n> > > > +\n> > > > + GstStructure *s = gst_caps_get_structure(caps, 0);\n> > > > + gint fps_caps_n, fps_caps_d;\n> > > > +\n> > > > + fps_caps_n = gst_value_get_fraction_numerator(container);\n> > > > + fps_caps_d = gst_value_get_fraction_denominator(container);\n> > > > + gst_structure_set(s, \"framerate\", GST_TYPE_FRACTION, fps_caps_n, fps_caps_d, nullptr);\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..3d217fcf 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 \n> > > > +#include \n> > > > #include \n> > > > \n> > > > #include \n> > > > @@ -18,6 +19,11 @@ 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> > > > GstCaps *caps);\n> > > > +void gst_libcamera_get_framerate_from_caps(GstCaps *caps, GstStructure *container);\n> > > > +void gst_libcamera_clamp_and_set_frameduration(libcamera::ControlList &controls,\n> > > > + const libcamera::ControlInfoMap &camera_controls, GstStructure *container);\n> > > > +void gst_libcamera_framerate_to_caps(GstCaps *caps, const GValue *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 60032236..955d6eac 100644\n> > > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > @@ -131,6 +131,7 @@ struct GstLibcameraSrcState {\n> > > > std::queue> completedRequests_\n> > > > LIBCAMERA_TSA_GUARDED_BY(lock_);\n> > > > \n> > > > + ControlList initControls_;\n> > > > guint group_id_;\n> > > > \n> > > > int queueRequest();\n> > > > @@ -462,6 +463,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> > > > GstFlowReturn flow_ret = GST_FLOW_OK;\n> > > > gint ret;\n> > > > \n> > > > + GstStructure *container = gst_structure_new_empty(\"container\");\n> > > > +\n> > \n> > Container seems to be a really non-descript term here ...\n> > \n> > Is this only used for framerate or is it expected to be used to contain\n> > 'anything' ?\n> > \n> > > > GST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n> > > > \n> > > > gint stream_id_num = 0;\n> > > > @@ -504,6 +507,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[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> > > > + gst_libcamera_get_framerate_from_caps(caps, container);\n> > \n> > maybe container could be called 'framerate' then ? It's going to store\n> > the framerate right ?\n> > \n> > > > }\n> > > > \n> > > > if (flow_ret != GST_FLOW_OK)\n> > > > @@ -525,6 +529,10 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> > > > goto done;\n> > > > }\n> > > > \n> > > > + /* Check frame duration bounds within controls::FrameDurationLimits */\n> > > > + gst_libcamera_clamp_and_set_frameduration(state->initControls_,\n> > > > + state->cam_->controls(), container);\n> > > > +\n> > > > /*\n> > > > * Regardless if it has been modified, create clean caps and push the\n> > > > * caps event. Downstream will decide if the caps are acceptable.\n> > > > @@ -532,8 +540,11 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> > > > for (gsize i = 0; i < state->srcpads_.size(); i++) {\n> > > > GstPad *srcpad = state->srcpads_[i];\n> > > > const StreamConfiguration &stream_cfg = state->config_->at(i);\n> > > > + const GValue *framerate = gst_structure_get_value(container, \"framerate\");\n> > > > \n> > > > g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg);\n> > > > + gst_libcamera_framerate_to_caps(caps, framerate);\n> > \n> > If gst_libcamera_framerate_to_caps took a GstStructure *, it can do the\n> > conversion in that function, and you can use the name 'framerate' for\n> > the structure variable in this scope.\n> > \n> > > > +\n> > > > if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) {\n> > > > flow_ret = GST_FLOW_NOT_NEGOTIATED;\n> > > > break;\n> > > > @@ -567,7 +578,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[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\", g_strerror(-ret)),\n> > > > @@ -577,6 +588,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> > > > }\n> > > > \n> > > > done:\n> > > > + state->initControls_.clear();\n> > > > + g_free(container);\n> > \n> > Does the gstreamer framework provide any smart unique pointer to hold\n> > the container that would free automatically when this function goes out\n> > of scope ?\n> > \n> > Aside from all that, which really are solveable /minors /bikeshedding:\n> > \n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> > \n> > \n> > > > switch (flow_ret) {\n> > > > case GST_FLOW_NOT_NEGOTIATED:\n> > > > GST_ELEMENT_FLOW_ERROR(self, flow_ret);\n> > > \n>","headers":{"Return-Path":"","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 2D7B3BD16B\n\tfor ;\n\tMon, 21 Nov 2022 21:30:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8B8F863311;\n\tMon, 21 Nov 2022 22:30:51 +0100 (CET)","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 D28DC63097\n\tfor ;\n\tMon, 21 Nov 2022 22:30:49 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7A7E874C;\n\tMon, 21 Nov 2022 22:30:49 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669066251;\n\tbh=/61ut02mNf9iXD80La2npzYoOrZuP7qm8nYuUpYWARw=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=pKMndF4v9OpatbxQq3yXVkROAXrlbVoeAeMwT6eqZ0A7Gn7fIGgzm31TAMWt8CrA3\n\tgE6tYw+XJYJDBgNtYBWPNZOyiDWE/1ErZ7HSP/DQ0GDdrPXn5+wJpJdZwuBl+vQ4Wk\n\tgQndTK0FkBqSpjDBKGYt9k12OIBa7udE9PUw4Waupr7k5ldGBr8rOH9NtL0nDfqvpX\n\tDsk1qKzyiQUe0FZ0FhkQGH35AjjwqO/qnkk1Kn2PHVgPH2hKV9rzPsYqijKucdBA3S\n\trJTAQ65es48b1L9tAhYqa91F9zUPTrJG6LQVPsReB36CCBi2nqOSy/2W6iTa8RL12q\n\t4XFg7hUN9RGZg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669066249;\n\tbh=/61ut02mNf9iXD80La2npzYoOrZuP7qm8nYuUpYWARw=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=V6TDYsabhKI/asWqHNH/deUTVVcgURtDMPK/6HLqVxiaJD5+BDEmkUOTR4ay+24Ok\n\tlE6sWFuKgimjbLFKR0PkmDJTqYwn+hqUeUAxoA4dps5UwRzSOiC4fbGAfqxmkTH8Jl\n\tmBN9MJV4q1Q7H9TzVffvbtatAZUZXYOC/t4Ld3TQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"V6TDYsab\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20221102135614.657444-1-umang.jain@ideasonboard.com>\n\t<20221102135614.657444-3-umang.jain@ideasonboard.com>\n\t\n\t<166756111421.15935.7749295504550053866@Monstersaurus>\n\t","To":"Nicolas Dufresne ,\n\tUmang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 21 Nov 2022 21:30:47 +0000","Message-ID":"<166906624707.512294.12808042651517244144@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v6 2/2] gstreamer: Provide framerate\n\tsupport for libcamerasrc","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25918,"web_url":"https://patchwork.libcamera.org/comment/25918/","msgid":"","date":"2022-11-28T14:29:56","subject":"Re: [libcamera-devel] [PATCH v6 2/2] gstreamer: Provide framerate\n\tsupport for libcamerasrc","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le lundi 21 novembre 2022 à 21:30 +0000, Kieran Bingham a écrit :\n> Quoting Nicolas Dufresne (2022-11-21 18:30:01)\n> > Le vendredi 04 novembre 2022 à 11:25 +0000, Kieran Bingham via libcamera-devel a\n> > écrit :\n> > > Quoting Umang Jain via libcamera-devel (2022-11-02 14:13:13)\n> > > > Hello,\n> > > > \n> > > > On 11/2/22 7:26 PM, Umang Jain wrote:\n> > > > > From: Rishikesh Donadkar \n> > > > > \n> > > > > Control the framerate by passing the controls::FrameDurationLimits\n> > > > > during Camera::start(). Framerate in gstreamer is expressed as\n> > > > > GST_TYPE_FRACTION so we maximise on maintaining it as a fraction\n> > > > > throughout and only do arithematic computations as and when required\n> > > > > (to compute frame-duration and vice-versa).\n> > > > > \n> > > > > To weed out abritrary framerate as input, place the clamping via the\n> > > > > controls::FrameDurationLimits provided after camera::configure() phase.\n> > > > > This is handled by a helper function\n> > > > > gst_libcamera_clamp_and_set_frameduration().\n> > > > > \n> > > > > Set the bound checked framerate (done in the above mentioned helper)\n> > > > > into the caps and pass the ControlList containing the frame-duration\n> > > > > to Camera::start(ctrls).\n> > > > > \n> > > > > Signed-off-by: Rishikesh Donadkar \n> > > > > Signed-off-by: Umang Jain \n> > > > > Tested-by: Umang Jain \n> > > > > ---\n> > > > > src/gstreamer/gstlibcamera-utils.cpp | 78 ++++++++++++++++++++++++++++\n> > > > > src/gstreamer/gstlibcamera-utils.h | 6 +++\n> > > > > src/gstreamer/gstlibcamerasrc.cpp | 15 +++++-\n> > > > > 3 files changed, 98 insertions(+), 1 deletion(-)\n> > > > > \n> > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> > > > > index 244a4a79..c14f72ec 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 \n> > > > > #include \n> > > > > \n> > > > > using namespace libcamera;\n> > > > > @@ -407,6 +408,83 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> > > > > }\n> > > > > }\n> > > > > \n> > > > > +void gst_libcamera_get_framerate_from_caps(GstCaps *caps,\n> > > > > + GstStructure *container)\n> > > > > +{\n> > > > > + GstStructure *s = gst_caps_get_structure(caps, 0);\n> > > > > + /*\n> > > > > + * Default to 30 fps. If the \"framerate\" fraction is invalid below,\n> > > > > + * libcamerasrc will set 30fps as the framerate.\n> > > > > + */\n> > > > > + gint fps_n = 30, fps_d = 1;\n> > > > > +\n> > > > > + if (gst_structure_has_field_typed(s, \"framerate\", GST_TYPE_FRACTION)) {\n> > > > > + if (!gst_structure_get_fraction(s, \"framerate\", &fps_n, &fps_d))\n> > > > > + GST_WARNING(\"Invalid framerate in the caps\");\n> > > \n> > > I wonder if this should fail negotiation - but I don't actually mind if\n> > > it continues. It's a bit like the negotiation failure below though.. ?\n> > \n> > You may have multiple sturctures like:\n> > \n> > video/x-raw,framerate=30/1;video/x-raw\n> > \n> > That indicates a preference for 30fps, but does not limite to other framerate.\n> > This is always what you'd get by using a rate adapter like videorate. In theory,\n> > a better approach would be to iterate all the structure, not just the very first\n> > one.\n> \n> Is there any existing code that parses the structures that would be a\n> helpful reference here ?\n> \n> Do we just keep trying to get a structure until we either, get a\n> nullpointer, or find a satisfactory/satisfiable frame rate result ?\n\nThe exact algorithm here will be libcamera specific. There is no parsing\ninvolved, GstCaps is an array of GstStructure, the length is:\n\n gst_caps_get_size()\n\nAnd you can index the structures with (its a borrow pointer, owned by GstCaps):\n\n gst_caps_get_structure()\n\nAs you often endup doing the same routine over all the structures, you can also\nuse gst_caps_foreach() and pass a negotiation callback with user data. No match\nin any of the structures is EMPTY or GST_FLOW_NOT_NEGOTIATED (depending on the\ncontexte and the algo you have designed).\n\n> \n> \n> > > \n> > > > > + }\n> > > > > +\n> > > > > + gst_structure_set(container, \"framerate\", GST_TYPE_FRACTION,\n> > > > > + fps_n, fps_d, nullptr);\n> > > > > +}\n> > > > > +\n> > > > > +void gst_libcamera_clamp_and_set_frameduration(ControlList &initCtrls,\n> > > > > + const ControlInfoMap &cam_ctrls,\n> > > > > + GstStructure *container)\n> > > > > +{\n> > > > > + gboolean in_bounds = false;\n> > > > > + gint fps_caps_n, fps_caps_d;\n> > > > > +\n> > > > > + if (!gst_structure_has_field_typed(container, \"framerate\", GST_TYPE_FRACTION))\n> > > > > + return;\n> > > > > +\n> > > > > + auto iterFrameDuration = cam_ctrls.find(controls::FrameDurationLimits.id());\n> > > > > + if (iterFrameDuration == cam_ctrls.end()) {\n> > > > > + GST_WARNING(\"FrameDurationLimits not found in camera controls.\");\n> > > > > + return;\n> > > > > + }\n> > > > > +\n> > > > > + const GValue *framerate = gst_structure_get_value(container, \"framerate\");\n> > > > > +\n> > > > > + fps_caps_n = gst_value_get_fraction_numerator(framerate);\n> > > > > + fps_caps_d = gst_value_get_fraction_denominator(framerate);\n> > > > > +\n> > > > > + int64_t frame_duration = (fps_caps_d * 1000000.0) / fps_caps_n;\n> > > > > + int64_t min_frame_duration = iterFrameDuration->second.min().get();\n> > > > > + int64_t max_frame_duration = iterFrameDuration->second.max().get();\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> > > > > + in_bounds = true;\n> > > > > + }\n> > > > > +\n> > > > > + if (!in_bounds) {\n> > > \n> > > I don't think we need to open code clamp() here:\n> > > \n> > > int64_t target_duration = (fps_caps_d * 1000000.0) / fps_caps_n;\n> > > int64_t min_frame_duration = iterFrameDuration->second.min().get();\n> > > int64_t max_frame_duration = iterFrameDuration->second.max().get();\n> > > \n> > > int64_t frame_duration = std::clamp(target_duration,\n> > > min_frame_duration,\n> > > max_frame_duration);\n> > > \n> > > if (frame_duration != target_duration) {\n> > > \n> > > \n> > > > > + gint framerate_clamped = 1000000 / frame_duration;\n> > > > > +\n> > > > > + /* Update the framerate which then will be exposed in caps. */\n> > > > > + gst_structure_set(container, \"framerate\", GST_TYPE_FRACTION,\n> > > > > + framerate_clamped, 1, nullptr);\n> > > > \n> > > > So the logic here is that, if the input is too high/low fps - it will be \n> > > > bound to min/max FrameDuration and exposed it caps.\n> > > > \n> > > > It doesn't mean, the camera will go on to stream ...  it means that \n> > > > bound-limit will be exposed in caps but the negotiation will probably \n> > > > fail (representing that the fps requested was too high/low and not \n> > > > supported by the camera)\n> > > > \n> > > > For e.g.\n> > > > \n> > > > Input: framerate=100/1\n> > > >     [[capped to framerate=43/1 by this patch on OV5647 ]]\n> > > > Exposed in caps : framerate=43/1\n> > > >     [[negotation failed]]\n> > > >     Expected framerate=100/1 but got framerate=43/1\n> > > > \n> > > > It seems to me this is expected. But I am not 100% sure here.\n> > > > \n> > > > Can the expectation be to stream the camera somehow (not sure how to \n> > > > surpass negotitaiton in caps) with the bound-limit set?  Probably worth \n> > > > exploring other gstreamer plugins specific to framerate handling...\n> > > \n> > > Indeed, this seems to be how the situation is reported by a user on\n> > > github:\n> > > - https://github.com/raspberrypi/libcamera/issues/30#issuecomment-1301117923\n> > > \n> > > \n> > > I believe it's the correct behaviour. If a pipeline explicitly requests 1000 FPS, and\n> > > the camera can not deliver, it is a failure to negotiate. But it's worth\n> > > checking on #gstreamer or hearing from Nicolas ?\n> > > \n> > > - I've asked on #gstreamer ...\n> > > \n> > > \n> > > \n> > > > > + }\n> > > > > +\n> > > > > + initCtrls.set(controls::FrameDurationLimits,\n> > > > > + { frame_duration, frame_duration });\n> > > > > +}\n> > > > > +\n> > > > > +void gst_libcamera_framerate_to_caps(GstCaps *caps, const GValue *container)\n> > > > > +{\n> > > > > + if (!GST_VALUE_HOLDS_FRACTION(container))\n> > > > > + return;\n> > > > > +\n> > > > > + GstStructure *s = gst_caps_get_structure(caps, 0);\n> > > > > + gint fps_caps_n, fps_caps_d;\n> > > > > +\n> > > > > + fps_caps_n = gst_value_get_fraction_numerator(container);\n> > > > > + fps_caps_d = gst_value_get_fraction_denominator(container);\n> > > > > + gst_structure_set(s, \"framerate\", GST_TYPE_FRACTION, fps_caps_n, fps_caps_d, nullptr);\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..3d217fcf 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 \n> > > > > +#include \n> > > > > #include \n> > > > > \n> > > > > #include \n> > > > > @@ -18,6 +19,11 @@ 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> > > > > GstCaps *caps);\n> > > > > +void gst_libcamera_get_framerate_from_caps(GstCaps *caps, GstStructure *container);\n> > > > > +void gst_libcamera_clamp_and_set_frameduration(libcamera::ControlList &controls,\n> > > > > + const libcamera::ControlInfoMap &camera_controls, GstStructure *container);\n> > > > > +void gst_libcamera_framerate_to_caps(GstCaps *caps, const GValue *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 60032236..955d6eac 100644\n> > > > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > @@ -131,6 +131,7 @@ struct GstLibcameraSrcState {\n> > > > > std::queue> completedRequests_\n> > > > > LIBCAMERA_TSA_GUARDED_BY(lock_);\n> > > > > \n> > > > > + ControlList initControls_;\n> > > > > guint group_id_;\n> > > > > \n> > > > > int queueRequest();\n> > > > > @@ -462,6 +463,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> > > > > GstFlowReturn flow_ret = GST_FLOW_OK;\n> > > > > gint ret;\n> > > > > \n> > > > > + GstStructure *container = gst_structure_new_empty(\"container\");\n> > > > > +\n> > > \n> > > Container seems to be a really non-descript term here ...\n> > > \n> > > Is this only used for framerate or is it expected to be used to contain\n> > > 'anything' ?\n> > > \n> > > > > GST_DEBUG_OBJECT(self, \"Streaming thread has started\");\n> > > > > \n> > > > > gint stream_id_num = 0;\n> > > > > @@ -504,6 +507,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[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> > > > > + gst_libcamera_get_framerate_from_caps(caps, container);\n> > > \n> > > maybe container could be called 'framerate' then ? It's going to store\n> > > the framerate right ?\n> > > \n> > > > > }\n> > > > > \n> > > > > if (flow_ret != GST_FLOW_OK)\n> > > > > @@ -525,6 +529,10 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> > > > > goto done;\n> > > > > }\n> > > > > \n> > > > > + /* Check frame duration bounds within controls::FrameDurationLimits */\n> > > > > + gst_libcamera_clamp_and_set_frameduration(state->initControls_,\n> > > > > + state->cam_->controls(), container);\n> > > > > +\n> > > > > /*\n> > > > > * Regardless if it has been modified, create clean caps and push the\n> > > > > * caps event. Downstream will decide if the caps are acceptable.\n> > > > > @@ -532,8 +540,11 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> > > > > for (gsize i = 0; i < state->srcpads_.size(); i++) {\n> > > > > GstPad *srcpad = state->srcpads_[i];\n> > > > > const StreamConfiguration &stream_cfg = state->config_->at(i);\n> > > > > + const GValue *framerate = gst_structure_get_value(container, \"framerate\");\n> > > > > \n> > > > > g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg);\n> > > > > + gst_libcamera_framerate_to_caps(caps, framerate);\n> > > \n> > > If gst_libcamera_framerate_to_caps took a GstStructure *, it can do the\n> > > conversion in that function, and you can use the name 'framerate' for\n> > > the structure variable in this scope.\n> > > \n> > > > > +\n> > > > > if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps))) {\n> > > > > flow_ret = GST_FLOW_NOT_NEGOTIATED;\n> > > > > break;\n> > > > > @@ -567,7 +578,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[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\", g_strerror(-ret)),\n> > > > > @@ -577,6 +588,8 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> > > > > }\n> > > > > \n> > > > > done:\n> > > > > + state->initControls_.clear();\n> > > > > + g_free(container);\n> > > \n> > > Does the gstreamer framework provide any smart unique pointer to hold\n> > > the container that would free automatically when this function goes out\n> > > of scope ?\n> > > \n> > > Aside from all that, which really are solveable /minors /bikeshedding:\n> > > \n> > > \n> > > Reviewed-by: Kieran Bingham \n> > > \n> > > \n> > > \n> > > > > switch (flow_ret) {\n> > > > > case GST_FLOW_NOT_NEGOTIATED:\n> > > > > GST_ELEMENT_FLOW_ERROR(self, flow_ret);\n> > > > \n> >","headers":{"Return-Path":"","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 CB168BE08B\n\tfor ;\n\tMon, 28 Nov 2022 14:30:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0F5EB63335;\n\tMon, 28 Nov 2022 15:30:02 +0100 (CET)","from mail-qk1-x72a.google.com (mail-qk1-x72a.google.com\n\t[IPv6:2607:f8b0:4864:20::72a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B8F4A61F2A\n\tfor ;\n\tMon, 28 Nov 2022 15:29:59 +0100 (CET)","by mail-qk1-x72a.google.com with SMTP id d8so7281971qki.13\n\tfor ;\n\tMon, 28 Nov 2022 06:29:59 -0800 (PST)","from nicolas-tpx395.localdomain (192-222-136-102.qc.cable.ebox.net.\n\t[192.222.136.102]) by smtp.gmail.com with ESMTPSA id\n\tm19-20020a05620a291300b006fa12a74c53sm8505243qkp.61.2022.11.28.06.29.57\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 28 Nov 2022 06:29:57 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669645802;\n\tbh=39BqlqudqUIiX4Se4DZ6UELP6nr+UxHPT80wy80QZw8=;\n\th=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=JlCpnB5Nfes+U6CYDBKbZWHfVc8NXJ8C9RD/coXEpAC6wnnzxRwGQa1VS4zbnEgxU\n\txJV7UdHZuo44jpwdR+gJaN+uehq1qMhfMrIyJ+oUq403t1oDrpDdxqofQqeqRmsFOD\n\tVwMfBJi9/hcmWZN8KaA17Xa3Xq4LEoDeMvJ1hLU3hUtSfWlP67tBsravB2uIf2ldbV\n\tH3Hpb+U2PX0yasdyewpyBeVB/Os5GdEIE/Bx+hnmZt7WPOyWEg5dFFqWzx8CnaDGVh\n\tNIad/P5xNgIvzbKRHDgtlFKJfUS6OO/0NeeHJ7SPCZC17HnDKVNmRwaq65NHBmLwZ+\n\tL+TbBW+Nu7fow==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20210112.gappssmtp.com; s=20210112;\n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:to:from:subject:message-id:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=ubQkqagakv2agF8vRfG9fszhy9CxjHX3DWeUzuWz+ug=;\n\tb=QS+wsq2cuqLOE/jtVzjcTtKyaEnw+v7I0LtJGSfiFIgAmg0+6U2t2xNogWpc56Sive\n\tYtOwi8aYub4DTwlk8WFYbqGqKMxeZsYKcQWIynL3KG4bnVzw93ldnyhyAHC23JliRrE/\n\tuyQk5tEPoQ+ljtMSLZXJ5Krm92ZFMHedejE6BP6gCxDMaz8wA4xQ0rqyOqmD7j+1+4Rk\n\tFd4RbDubjj5rDSOyQhqZHskXjPw7JfbcMuoWg0Z6h3QmBGSf2Ctbrf2aJyM8QlyAMQOz\n\tB/NuUAGEvVPJppjZsNlwwPPTe6mdEFAtrtbZPUeC31Q1rJObKqC9HoOTET1s1YK78tCt\n\ts2hw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ndufresne-ca.20210112.gappssmtp.com\n\theader.i=@ndufresne-ca.20210112.gappssmtp.com header.b=\"QS+wsq2c\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:to:from:subject:message-id:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=ubQkqagakv2agF8vRfG9fszhy9CxjHX3DWeUzuWz+ug=;\n\tb=yFgNwz0J402MeQnrb/REcljJNrqBaJgIN+sv56UznBISMAgBoEPInDIx6mjtZL7N5D\n\tjEnF/mhssBxAIrhOVAy8SgnavakBIDj0hb45xDxDuEf5id6QyyEEnzrUbAeSjb0OmR7R\n\tp4Fi59b3rQUBikcqMnk+a56fX38zePAPfOnrezZz/IAT1lczEDm6g8akJuJiQLekytAP\n\tda2K5MZXSArO2BPlqDdNUWDVYj5fhIxwiVuPT7Z7p0lI8zlmxMQWsv1jjxOjzjhVg3mT\n\tjscqLaQmKKKOI2HYHYuihoEadjTiu/UivP9xV1LSWeKnh8NMrso1xAG6h5Hr6+2cxtjm\n\tvyfQ==","X-Gm-Message-State":"ANoB5pni2hxKKy4XOhUEK7ywa++HhHOc7jqPVqzOh/pL0Rfb+qh0TStQ\n\tLXzCIAZ4DwNYKeTgwwyQKBXfGA==","X-Google-Smtp-Source":"AA0mqf6vBnZ458lgfGSmhOEQieR7KVMvzuvucYzpLf2At5HMeVJmpj4SqUeaD9Bvqfj76bRfS9ffSQ==","X-Received":"by 2002:a37:589:0:b0:6fc:5fcf:5634 with SMTP id\n\t131-20020a370589000000b006fc5fcf5634mr11503141qkf.480.1669645798306; \n\tMon, 28 Nov 2022 06:29:58 -0800 (PST)","Message-ID":"","To":"Kieran Bingham , Umang Jain\n\t, libcamera-devel@lists.libcamera.org","Date":"Mon, 28 Nov 2022 09:29:56 -0500","In-Reply-To":"<166906624707.512294.12808042651517244144@Monstersaurus>","References":"<20221102135614.657444-1-umang.jain@ideasonboard.com>\n\t<20221102135614.657444-3-umang.jain@ideasonboard.com>\n\t\n\t<166756111421.15935.7749295504550053866@Monstersaurus>\n\t\n\t<166906624707.512294.12808042651517244144@Monstersaurus>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.44.4 (3.44.4-2.fc36) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH v6 2/2] gstreamer: Provide framerate\n\tsupport for libcamerasrc","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Nicolas Dufresne via libcamera-devel\n\t","Reply-To":"Nicolas Dufresne ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]