[{"id":24811,"web_url":"https://patchwork.libcamera.org/comment/24811/","msgid":"<8cd28f20-f895-4c23-91cd-a4a63a446bcb@ideasonboard.com>","date":"2022-08-29T06:47:32","subject":"Re: [libcamera-devel] [PATCH v1] 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 Rishikesh,\n\nThank you for the patch.\n\nRegarding the subject, you should have mentioend [RFC] as you mentioned \nyou are still working on it and could've mention the missing pieces below.\n\nOn 8/28/22 8:26 PM, Rishikesh Donadkar wrote:\n> This patch aims to add framerate support to libcamerasrc in the\n> direction gstreamer->libcamera.\n>\n> Add a field of the type ControlList to GstLibcameraSrcState.\n> Get the framerate from the caps and convert it to FrameDuration.\n> Set the FrameDurationLimits control in the initControls_ and pass\n> the initControls_ at the time of starting camera.\n>\n> Signed-off-by: Umang Jain \n> Signed-off-by: Rishikesh Donadkar \n>\n> ---\n> Tested the patch with 3 different framerates with the help of the\n> fpsdisplaysink element.\n>\n> https://paste.debian.net/1251951/\n>\n> https://paste.debian.net/1251952/\n>\n> https://paste.debian.net/1251953/\n\nGreat results, happy to see. Also you could've mentioned the platform on \nwhich you tested.\nIn this case RPi 4 + OV5647\n\n> ---\n> src/gstreamer/gstlibcamera-utils.cpp | 18 ++++++++++++++++++\n> src/gstreamer/gstlibcamera-utils.h | 3 +++\n> src/gstreamer/gstlibcamerasrc.cpp | 7 ++++++-\n> 3 files changed, 27 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> index 5a21a391..7d8519da 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> @@ -236,6 +237,23 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> \tstream_cfg.size.height = height;\n> }\n> \n> +void\n> +gst_libcamera_configure_controls_from_caps(ControlList &controls, [[maybe_unused]] GstCaps *caps)\n> +{\n> +\t/* read framerate from caps - convert to integer and set to frame_time. */\n> +\tGstStructure *s = gst_caps_get_structure(caps, 0);\n> +\tgint fps_n = -1, fps_d = -1;\n> +\tif (gst_structure_has_field(s, \"framerate\"))\n> +\t\tgst_structure_get_fraction(s, \"framerate\", &fps_n, &fps_d);\n\nI think if the user input is malformed, get_fraction() will return a \nFALSE. We should check that for invalid input and log errors.\n> +\n> +\tif (fps_n < 0 || fps_d < 0)\n> +\t\treturn;\n\nCan it happen that only the numerator is mentioned? Have you tested \nsomething like : \"framerate=30\"  instead of \"framerate=30/1\"\n> +\n> +\tgdouble frame_duration = static_cast(fps_d) / static_cast(fps_n) * 1000000.0;\n\nSince frame_duration is int64_t, and fps_d and fps_n are both integers, \ncan we do:\n\n             int64_t frame_duration = (fps_d * 1000000) / fps_n; ?\n> +\tcontrols.set(controls::FrameDurationLimits,\n> +\t\t Span({ static_cast(frame_duration), static_cast(frame_duration) }));\n\nyou could then drop the cast from here  I believe\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..1f737e84 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,8 @@ 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_configure_controls_from_caps(libcamera::ControlList &controls, GstCaps *caps);\n\nnit: To be pedantic, we are only parsing controls needed at \ncamera::StarT(); so maybe\n         gst_libcamera_get_init_ctrls_from_caps(...);\n?\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..d1080271 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> @@ -496,6 +497,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 +506,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_configure_controls_from_caps(state->initControls_, caps);\n> \t}\n> \n> \tif (flow_ret != GST_FLOW_OK)\n> @@ -524,6 +527,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> @@ -566,7 +570,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 +580,7 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,\n> \t}\n> \n> done:\n> +\tstate->initControls_.clear();\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 D3EA9C0DA4\n\tfor ;\n\tMon, 29 Aug 2022 06:47:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2C55561FC0;\n\tMon, 29 Aug 2022 08:47:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 92FE161F9D\n\tfor ;\n\tMon, 29 Aug 2022 08:47:40 +0200 (CEST)","from [IPV6:2401:4900:1f3f:1548:78ac:4a3:edc3:c28a] (unknown\n\t[IPv6:2401:4900:1f3f:1548:78ac:4a3:edc3:c28a])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 941E8481;\n\tMon, 29 Aug 2022 08:47:38 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661755662;\n\tbh=znvqUT3jb/SY1B3lJgbfdWiutxv8dCjaFLX5ClfE/TU=;\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=25vj9YHPrcnCAGlkrUaoLxmqZSfmCnpAKkS5W9mXImFq2PU36v9WjJwnW//b4F7VR\n\tK5ZfSA/oaBHL153kWdLvRoDeR/e87Wgnugg8ud5C2ly4YhBl1FFJZxeogNBQA9beqB\n\tEU2ONM+l4I6hXlh7hsVXazrFja53nmMA+AgmZ/NaGFdJPKhB4L2eDgN2C/ZMGwvmMh\n\tB/zi5dvkr5ngkwLpcO5SkYYJaI2I9nExcXbAftKAMowDZWZoVkZJXm7zGDuveipmGW\n\tu3wK2tCxtXQbHQCy2tZ4IFe3IrH5nDqf25Ll5WPf1FOVIay9Gbw4Q4poDGiWfVjO0l\n\tIakcf2F0s6kgg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661755660;\n\tbh=znvqUT3jb/SY1B3lJgbfdWiutxv8dCjaFLX5ClfE/TU=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=pDL5kCnWiDipxjSjIhHRPpY1UJY10Ne9b1ABAgoeTgk8XnnI0zcmcT9qxWQVJf5Xv\n\tvK5eyxwVGi/9r72Dmen1ooAg1yEXXwrT3VzM1Jo29Y7ksoT9tLhaMjnaQ/HCL/lLlS\n\t6+amE1myFwmSCdYovsu6wjV/8aCzC61hDqNH3hX0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"pDL5kCnW\"; dkim-atps=neutral","Message-ID":"<8cd28f20-f895-4c23-91cd-a4a63a446bcb@ideasonboard.com>","Date":"Mon, 29 Aug 2022 12:17:32 +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 ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220828145630.51618-1-rishikeshdonadkar@gmail.com>","In-Reply-To":"<20220828145630.51618-1-rishikeshdonadkar@gmail.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v1] 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 ","Cc":"nicolas.dufresne@collabora.com, vedantparanjape160201@gmail.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24827,"web_url":"https://patchwork.libcamera.org/comment/24827/","msgid":"","date":"2022-08-30T04:15:47","subject":"Re: [libcamera-devel] [PATCH v1] 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> Regarding the subject, you should have mentioend [RFC] as you mentioned\n> you are still working on it and could've mention the missing pieces below.\n\nNoted.\n\n> Great results, happy to see. Also you could've mentioned the platform on\n> which you tested.\n> In this case RPi 4 + OV5647\n>\nNoted.\n\n> > +void\n> > +gst_libcamera_configure_controls_from_caps(ControlList &controls,\n> [[maybe_unused]] GstCaps *caps)\n> > +{\n> > + /* read framerate from caps - convert to integer and set to\n> frame_time. */\n> > + GstStructure *s = gst_caps_get_structure(caps, 0);\n> > + gint fps_n = -1, fps_d = -1;\n> > + if (gst_structure_has_field(s, \"framerate\"))\n> > + gst_structure_get_fraction(s, \"framerate\", &fps_n, &fps_d);\n>\n> I think if the user input is malformed, get_fraction() will return a\n> FALSE. We should check that for invalid input and log errors.\n>\nOkay.\n\n> > +\n> > + if (fps_n < 0 || fps_d < 0)\n> > + return;\n>\n> Can it happen that only the numerator is mentioned? Have you tested\n> something like : \"framerate=30\" instead of \"framerate=30/1\"\n>\nI tested for the case \"framerate=30\". We cannot accept (int)30 for the\nframerate field that\nis supposed to be a fraction. Passing 30 fails the negotiation as\ngst_pad_peer_query_caps()\nreturns empty caps here.\ng_autoptr(GstCaps) caps = gst_pad_peer_query_caps(srcpad, filter);\n\nif (gst_caps_is_empty(caps)) {\nflow_ret = GST_FLOW_NOT_NEGOTIATED;\nbreak;\n}\n\n> Since frame_duration is int64_t, and fps_d and fps_n are both integers,\n> can we do:\n>\n> int64_t frame_duration = (fps_d * 1000000) / fps_n; ?\n> > + controls.set(controls::FrameDurationLimits,\n> > + Span({\n> static_cast(frame_duration), static_cast(frame_duration)\n> }));\n>\n> you could then drop the cast from here I believe\n>\nRight.\n\n> nit: To be pedantic, we are only parsing controls needed at\n> camera::StarT(); so maybe\n> gst_libcamera_get_init_ctrls_from_caps(...);\n> ?\n\nYes, this will make more sense.","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 6A44BC3272\n\tfor ;\n\tTue, 30 Aug 2022 04:16:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 574F461FC1;\n\tTue, 30 Aug 2022 06:16:02 +0200 (CEST)","from mail-vs1-xe29.google.com (mail-vs1-xe29.google.com\n\t[IPv6:2607:f8b0:4864:20::e29])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0A312603E0\n\tfor ;\n\tTue, 30 Aug 2022 06:16:00 +0200 (CEST)","by mail-vs1-xe29.google.com with SMTP id k66so4720613vsc.11\n\tfor ;\n\tMon, 29 Aug 2022 21:15:59 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661832962;\n\tbh=crRYzVpPpx5hZUrWhZrGFAs5okqkIwCcZeodO9ZFrBI=;\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=fV+g/Rq2vGo+CUCPizSuzzrduZt3WibVdV+luV8GTTOTPMSzOiXXSe00XycY58sEs\n\t7rqSrlf4sIaPj65RGgMPC48Q1bzO76UPa0eJ7ebJ3rUZuSfP/7KV8lUPochqphmZXM\n\txmHn1kCAAAPGIJthNx0mrWmqjNtvLh50esBPv6J0RFbs2rLZ7BtSV1/kKqDtRBC6VV\n\tJSoxrL1n7k30WxqPYxfWwbMKy1H/qMQMCQCbivPTWlAGfYPHK206dMMWRjnCtswE+h\n\tOkPfD0JVaIoc4oiUev+oDj7svwLFG9lWH8Mq9R+JB6Ti+cJW60m38hqvczZhnWO/5A\n\tDlGFR/4otgcdA==","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;\n\tbh=uAab1IwuPNx1Kw0H0HqObYdYG1I81Jgelc5y1jpvET4=;\n\tb=JHCJJIR1AopTcZtlCUxjF6l68dVD7vKwGwx2bVr/NyCHvYL3NxzoPqyYxyWYgydQMD\n\t+aYZea2+l3DbGUCdCITk7OJjRDOSIItU7sF3WmaBBeGofezoyAQP3U6aeQJjxYtCNxB1\n\tgRRDiFupNBM8R8opgTF83fvSdrb/dxXP56N/U9zRoPDC6u9F/oh7ZcmoXlWgbkC0qy6u\n\tFMQzPnV1thY3MV3oN0A0+bIiZUogFyZIIR0HKQMOjIWcSVL1wIkXv2vaylGnlQHemrpI\n\tLITDpusdY3ygk3IlJSJdLsIUqh3OqqrRm/9HlYMrE1bJvYZ27B0mxaFRb1iwK+hpd1e0\n\taQtA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"JHCJJIR1\"; 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;\n\tbh=uAab1IwuPNx1Kw0H0HqObYdYG1I81Jgelc5y1jpvET4=;\n\tb=xsC815fsKBakJSHKd7VUSX9YnhPHUHV4/qOZ88pmndxEpBxQxy62p2ByNV5PolqZBL\n\t++d06D4Yo4fvPbNjJAjmPNQTLGFqYEa1hqRSIgVOE3wtnb+hsVWbqD/o0EmVkex0I42Y\n\tvWutLqAM06dylMU7OiL27+uujhfiRQggmtPiCfolEaxBHIkai/Ofg/AXcq/itxrWNZEW\n\tGKr3Cb+yo0Pi6lCffKt7/Psm4/LFABRasrd6yHPEOMVvbY7JZBDZg6Aj/4I0hiWbV/6Z\n\tGFdtWtUBgRycuMLbL2m5UXOFlbeHowUKr2lkecsZIor/NWfjHkzTOwC/Wug9xMl3K6iA\n\tUM7w==","X-Gm-Message-State":"ACgBeo0rsuIdvZbu1+bBotwE6l+Q2hvfKqBN2LHJu4oWryxn8hHfwbTF\n\t/8PyeZDGV7iLhRuNzflqOEzVhvKV0uqFjKWInlM=","X-Google-Smtp-Source":"AA6agR5Sjn65Hlcrh8NJF+bxKgijQrRkHnkfZpeBr8qMbH7Sw6M7Rwl938vJQgSDb6DLJ21bVlX0+pVhczjDbnFXZkE=","X-Received":"by 2002:a67:d712:0:b0:390:d3b8:3e97 with SMTP id\n\tp18-20020a67d712000000b00390d3b83e97mr3128636vsj.13.1661832958775;\n\tMon, 29 Aug 2022 21:15:58 -0700 (PDT)","MIME-Version":"1.0","References":"<20220828145630.51618-1-rishikeshdonadkar@gmail.com>\n\t<8cd28f20-f895-4c23-91cd-a4a63a446bcb@ideasonboard.com>","In-Reply-To":"<8cd28f20-f895-4c23-91cd-a4a63a446bcb@ideasonboard.com>","Date":"Tue, 30 Aug 2022 09:45:47 +0530","Message-ID":"","To":"Umang Jain ","Content-Type":"multipart/alternative; boundary=\"0000000000002f16cf05e76da3fb\"","Subject":"Re: [libcamera-devel] [PATCH v1] 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":"Rishikesh Donadkar via libcamera-devel\n\t","Reply-To":"Rishikesh Donadkar ","Cc":"libcamera-devel@lists.libcamera.org, vedantparanjape160201@gmail.com,\n\tnicolas.dufresne@collabora.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24842,"web_url":"https://patchwork.libcamera.org/comment/24842/","msgid":"<17a3fa5d-7548-1970-4a90-f0416fbcd180@ideasonboard.com>","date":"2022-08-30T17:56:09","subject":"Re: [libcamera-devel] [PATCH v1] 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 Rishikesh,\none more comment,\n\nOn 8/29/22 12:17 PM, Umang Jain via libcamera-devel wrote:\n> Hi Rishikesh,\n>\n> Thank you for the patch.\n>\n> Regarding the subject, you should have mentioend [RFC] as you \n> mentioned you are still working on it and could've mention the missing \n> pieces below.\n>\n> On 8/28/22 8:26 PM, Rishikesh Donadkar wrote:\n>> This patch aims to add framerate support to libcamerasrc in the\n>> direction gstreamer->libcamera.\n>>\n>> Add a field of the type ControlList to GstLibcameraSrcState.\n>> Get the framerate from the caps and convert it to FrameDuration.\n>> Set the FrameDurationLimits control in the initControls_ and pass\n>> the initControls_ at the time of starting camera.\n>>\n>> Signed-off-by: Umang Jain \n>> Signed-off-by: Rishikesh Donadkar \n>>\n>> ---\n>> Tested the patch with 3 different framerates with the help of the\n>> fpsdisplaysink element.\n>>\n>> https://paste.debian.net/1251951/\n>>\n>> https://paste.debian.net/1251952/\n>>\n>> https://paste.debian.net/1251953/\n>\n> Great results, happy to see. Also you could've mentioned the platform \n> on which you tested.\n> In this case RPi 4 + OV5647\n>\n>> ---\n>>   src/gstreamer/gstlibcamera-utils.cpp | 18 ++++++++++++++++++\n>>   src/gstreamer/gstlibcamera-utils.h   |  3 +++\n>>   src/gstreamer/gstlibcamerasrc.cpp    |  7 ++++++-\n>>   3 files changed, 27 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp \n>> b/src/gstreamer/gstlibcamera-utils.cpp\n>> index 5a21a391..7d8519da 100644\n>> --- a/src/gstreamer/gstlibcamera-utils.cpp\n>> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n>> @@ -8,6 +8,7 @@\n>>     #include \"gstlibcamera-utils.h\"\n>>   +#include \n>>   #include \n>>     using namespace libcamera;\n>> @@ -236,6 +237,23 @@ \n>> gst_libcamera_configure_stream_from_caps(StreamConfiguration \n>> &stream_cfg,\n>>       stream_cfg.size.height = height;\n>>   }\n>>   +void\n>> +gst_libcamera_configure_controls_from_caps(ControlList &controls, \n>> [[maybe_unused]] GstCaps *caps)\n>> +{\n>> +    /* read framerate from caps - convert to integer and set to \n>> frame_time. */\n>> +    GstStructure *s = gst_caps_get_structure(caps, 0);\n>> +    gint fps_n = -1, fps_d = -1;\n>> +    if (gst_structure_has_field(s, \"framerate\"))\n>> +        gst_structure_get_fraction(s, \"framerate\", &fps_n, &fps_d);\n>\n> I think if the user input is malformed, get_fraction() will return a \n> FALSE. We should check that for invalid input and log errors.\n>> +\n>> +    if (fps_n < 0 || fps_d < 0)\n>> +        return;\n>\n> Can it happen that only the numerator is mentioned? Have you tested \n> something like : \"framerate=30\"  instead of \"framerate=30/1\"\n>> +\n>> +    gdouble frame_duration = static_cast(fps_d) / \n>> static_cast(fps_n) * 1000000.0;\n>\n> Since frame_duration is int64_t, and fps_d and fps_n are both \n> integers, can we do:\n>\n>             int64_t frame_duration = (fps_d * 1000000) / fps_n; ?\n\nwe also need to check the frame_duration whether it is in-bound to the \nmax / min frame-duration supported by the camera itself.\n\nIf it's lower than the minimum, we should set the frame_duration to minimum\nIf it's larger than the maximum, we should set the frame duration to \nmaximum.\n\nWe need to do this on the libcamerasrc side, since passing initCtrls_ at \ncamera::start() doesn't tell us if the controls have been applied\nto the camera successfully or not (or they have adjusted in some \nfashion). Until that is introduced in camera::start() API, we need\nto carry out such handling on the application side.\n\n>> + controls.set(controls::FrameDurationLimits,\n>> +             Span({ \n>> static_cast(frame_duration), \n>> static_cast(frame_duration) }));\n>\n> you could then drop the cast from here  I believe\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..1f737e84 100644\n>> --- a/src/gstreamer/gstlibcamera-utils.h\n>> +++ b/src/gstreamer/gstlibcamera-utils.h\n>> @@ -9,6 +9,7 @@\n>>   #pragma once\n>>     #include \n>> +#include \n>>   #include \n>>     #include \n>> @@ -18,6 +19,8 @@ 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 \n>> gst_libcamera_configure_controls_from_caps(libcamera::ControlList \n>> &controls, GstCaps *caps);\n>\n> nit: To be pedantic, we are only parsing controls needed at \n> camera::StarT(); so maybe\n>         gst_libcamera_get_init_ctrls_from_caps(...);\n> ?\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..d1080271 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>>   +    ControlList initControls_;\n>>       guint group_id_;\n>>         int queueRequest();\n>> @@ -496,6 +497,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 +506,7 @@ 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>> + gst_libcamera_configure_controls_from_caps(state->initControls_, \n>> caps);\n>>       }\n>>         if (flow_ret != GST_FLOW_OK)\n>> @@ -524,6 +527,7 @@ gst_libcamera_src_task_enter(GstTask *task, \n>> [[maybe_unused]] GThread *thread,\n>>           const StreamConfiguration &stream_cfg = state->config_->at(i);\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>>               flow_ret = GST_FLOW_NOT_NEGOTIATED;\n>>               break;\n>> @@ -566,7 +570,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>>   -    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 +580,7 @@ gst_libcamera_src_task_enter(GstTask *task, \n>> [[maybe_unused]] GThread *thread,\n>>       }\n>>     done:\n>> +    state->initControls_.clear();\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 9B61DC3272\n\tfor ;\n\tTue, 30 Aug 2022 17:56:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1607761FBD;\n\tTue, 30 Aug 2022 19:56:18 +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 D353561F9C\n\tfor ;\n\tTue, 30 Aug 2022 19:56:16 +0200 (CEST)","from [IPV6:2401:4900:1f3f:1548:78ac:4a3:edc3:c28a] (unknown\n\t[IPv6:2401:4900:1f3f:1548:78ac:4a3:edc3:c28a])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3C4BE481;\n\tTue, 30 Aug 2022 19:56:15 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661882178;\n\tbh=kd9cKlJ0inYvbH/BQ5XWW/hru5adTU1wOZ2cDbazd+o=;\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=NQmQNaNT2SvrFlR0nOm8rK010hsUDI+u4BR0JwnxY3vYD0vtTwE1ix2qk6HzOvCn8\n\tI8ktZYEFZjGXYWvJJX/Hc2f6iplgP5SSdiwbISz0rHvqkHqB0rnuqDwFOsFZRAvYLx\n\tfMAhnCGrIKgnPazHWWMCFfjVE0hnbMmKROOw+d1D/AmroixJxZ2No78xDQ/5SaqE1+\n\tWXvDfQQzNkWlTZrYoRVs/U5h8SXk56idI3N4x0dnEGoL8bckFdI5Dg4k+J/2Wgqwby\n\t+ULkOggzFZ8eYlNq7/jem60fy/5B+kUGqy0meAGl7XFA1uOhFH/jbhwHuloa2u7AfL\n\tEuWn2D+xojK7g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661882176;\n\tbh=kd9cKlJ0inYvbH/BQ5XWW/hru5adTU1wOZ2cDbazd+o=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=D78h/jcKTbICmHKO8hZ4PaFiA4r/Ns1SNN66KPPwdWUBzb05fTGJJyLrqOtuOt5Uh\n\tup1XVzusyAqIkvBDjBqKSjBLHzp9T6Pw2KeYWUXR0iwSMFKN5VXwJSFEoI8xF0sTcQ\n\tWFRGCP4p1qQNMCSsK/Tkw7fr9LBLictjWhyG6T7g="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"D78h/jcK\"; dkim-atps=neutral","Message-ID":"<17a3fa5d-7548-1970-4a90-f0416fbcd180@ideasonboard.com>","Date":"Tue, 30 Aug 2022 23:26:09 +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 ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220828145630.51618-1-rishikeshdonadkar@gmail.com>\n\t<8cd28f20-f895-4c23-91cd-a4a63a446bcb@ideasonboard.com>","In-Reply-To":"<8cd28f20-f895-4c23-91cd-a4a63a446bcb@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v1] 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 ","Cc":"nicolas.dufresne@collabora.com, vedantparanjape160201@gmail.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]