[{"id":22194,"web_url":"https://patchwork.libcamera.org/comment/22194/","msgid":"","date":"2022-02-25T08:47:37","subject":"Re: [libcamera-devel] [PATCH] v4l2: Support setting frame rate in\n\tthe V4L2 Adaptation layer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nejc,\n\nThank you for the patch.\n\nOn Tue, Feb 15, 2022 at 12:42:22PM +0100, Nejc Galof wrote:\n> The V4L2 adaptation layer can already support streaming with components\n> such as OpenCV, however it is not accepting, or handling any requests to\n> configure the frame rate.\n> \n> In V4L2 the frame rate is set by configuring the timeperframe component\n> of the v4l2_streamparm structure through the VIDIOC_S_PARM ioctl.\n> \n> Extend the V4L2 compatibility layer to accept the VIDIOC_S_PARM ioctls\n> and provide an interface for setting controls on the V4L2Camera class to\n> set the requested rate when starting the camera.\n> \n> Signed-off-by: Nejc Galof \n> Signed-off-by: Kieran Bingham \n> ---\n> src/v4l2/v4l2_camera.cpp | 12 +++++++++---\n> src/v4l2/v4l2_camera.h | 7 +++++++\n> src/v4l2/v4l2_camera_proxy.cpp | 25 +++++++++++++++++++++++++\n> src/v4l2/v4l2_camera_proxy.h | 1 +\n> 4 files changed, 42 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> index e922b9e6..e4eb3a2b 100644\n> --- a/src/v4l2/v4l2_camera.cpp\n> +++ b/src/v4l2/v4l2_camera.cpp\n> @@ -12,13 +12,15 @@\n> \n> #include \n> \n> +#include \n> +\n> using namespace libcamera;\n> \n> LOG_DECLARE_CATEGORY(V4L2Compat)\n> \n> V4L2Camera::V4L2Camera(std::shared_ptr camera)\n> -\t: camera_(camera), isRunning_(false), bufferAllocator_(nullptr),\n> -\t efd_(-1), bufferAvailableCount_(0)\n> +\t: camera_(camera), controls_(controls::controls), isRunning_(false),\n> +\t bufferAllocator_(nullptr), efd_(-1), bufferAvailableCount_(0)\n> {\n> \tcamera_->requestCompleted.connect(this, &V4L2Camera::requestComplete);\n> }\n> @@ -203,10 +205,12 @@ int V4L2Camera::streamOn()\n> \tif (isRunning_)\n> \t\treturn 0;\n> \n> -\tint ret = camera_->start();\n> +\tint ret = camera_->start(&controls_);\n> \tif (ret < 0)\n> \t\treturn ret == -EACCES ? -EBUSY : ret;\n> \n> +\tcontrols_.clear();\n> +\n> \tisRunning_ = true;\n> \n> \tfor (Request *req : pendingRequests_) {\n> @@ -266,6 +270,8 @@ int V4L2Camera::qbuf(unsigned int index)\n> \t\treturn 0;\n> \t}\n> \n> +\trequest->controls().merge(std::move(controls_));\n> +\n> \tret = camera_->queueRequest(request);\n> \tif (ret < 0) {\n> \t\tLOG(V4L2Compat, Error) << \"Can't queue request\";\n> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h\n> index 03e74118..9e6c895a 100644\n> --- a/src/v4l2/v4l2_camera.h\n> +++ b/src/v4l2/v4l2_camera.h\n> @@ -10,11 +10,14 @@\n> #include \n> #include \n> \n> +#include \n\nI don't think you need to include this here.\n\n> +\n> #include \n> #include \n> #include \n> \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -49,6 +52,8 @@ public:\n> \t\t\t\t const libcamera::Size &size,\n> \t\t\t\t libcamera::StreamConfiguration *streamConfigOut);\n> \n> +\tlibcamera::ControlList &controls() { return controls_; }\n\nI may have gone for a setControls() function, but this works too.\n\n> +\n> \tint allocBuffers(unsigned int count);\n> \tvoid freeBuffers();\n> \tint getBufferFd(unsigned int index);\n> @@ -69,6 +74,8 @@ private:\n> \tstd::shared_ptr camera_;\n> \tstd::unique_ptr config_;\n> \n> +\tlibcamera::ControlList controls_;\n> +\n> \tbool isRunning_;\n> \n> \tlibcamera::Mutex bufferLock_;\n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index e114d09f..f005b21c 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -18,6 +18,8 @@\n> #include \n> \n> #include \n> +#include \n> +#include \n> #include \n> \n> #include \n> @@ -33,6 +35,7 @@\n> #define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + (c))\n> \n> using namespace libcamera;\n> +using namespace std::literals::chrono_literals;\n> \n> LOG_DECLARE_CATEGORY(V4L2Compat)\n> \n> @@ -755,6 +758,24 @@ int V4L2CameraProxy::vidioc_streamoff(V4L2CameraFile *file, int *arg)\n> \treturn ret;\n> }\n> \n> +int V4L2CameraProxy::vidioc_s_param(V4L2CameraFile *file, struct v4l2_streamparm *arg)\n> +{\n> +\tLOG(V4L2Compat, Debug)\n> +\t\t<< \"[\" << file->description() << \"] \" << __func__ << \"()\";\n> +\n> +\tif (!validateBufferType(arg->type))\n> +\t\treturn -EINVAL;\n> +\n> +\tstruct v4l2_fract *timeperframe = &arg->parm.capture.timeperframe;\n> +\tutils::Duration frameDuration = 1s * (static_cast(timeperframe->numerator) /\n> +\t\t\t\t\t static_cast(timeperframe->denominator));\n\nDoes the following work ?\n\n\tutils::Duration frameDuration = 1.0s * timeperframe->numerator\n\t\t\t\t / timeperframe->denominator;\n\n> +\n> +\tint64_t uDuration = frameDuration.get();\n> +\tvcam_->controls().set(controls::FrameDurationLimits, { uDuration, uDuration });\n\nLooks like we should move the FrameDurationLimits control to using the\nDuration type. Not a candidate for this patch of course.\n\n> +\n> +\treturn 0;\n> +}\n\nGetting the frame rate back from the camera to implement vidioc_g_param\nis a more difficult task, but would it make sense to cache the frame\nrate in the V4L2CameraProxy and implement vidioc_g_param based on that\nalready ? The question of what frame rate to expose by default will be\nan interesting one, but I think you can start with a hardcoded default\nvalue of 1/30. This could be done in this patch if easy, or in a\nseparate patch.\n\n> +\n> const std::set V4L2CameraProxy::supportedIoctls_ = {\n> \tVIDIOC_QUERYCAP,\n> \tVIDIOC_ENUM_FRAMESIZES,\n> @@ -775,6 +796,7 @@ const std::set V4L2CameraProxy::supportedIoctls_ = {\n> \tVIDIOC_EXPBUF,\n> \tVIDIOC_STREAMON,\n> \tVIDIOC_STREAMOFF,\n> +\tVIDIOC_S_PARM,\n> };\n> \n> int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *arg)\n> @@ -852,6 +874,9 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *ar\n> \tcase VIDIOC_STREAMOFF:\n> \t\tret = vidioc_streamoff(file, static_cast(arg));\n> \t\tbreak;\n> +\tcase VIDIOC_S_PARM:\n> +\t\tret = vidioc_s_param(file, static_cast(arg));\n\nvidioc_s_parm, to match the ioctl name.\n\nAll in all it looks fairly good. With the two small changes requested\nabove,\n\nReviewed-by: Laurent Pinchart \n\nWill you send a v2 ?\n\n> +\t\tbreak;\n> \tdefault:\n> \t\tret = -ENOTTY;\n> \t\tbreak;\n> diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h\n> index 76ca2d8a..30a3f492 100644\n> --- a/src/v4l2/v4l2_camera_proxy.h\n> +++ b/src/v4l2/v4l2_camera_proxy.h\n> @@ -65,6 +65,7 @@ private:\n> \tint vidioc_expbuf(V4L2CameraFile *file, struct v4l2_exportbuffer *arg);\n> \tint vidioc_streamon(V4L2CameraFile *file, int *arg);\n> \tint vidioc_streamoff(V4L2CameraFile *file, int *arg);\n> +\tint vidioc_s_param(V4L2CameraFile *file, struct v4l2_streamparm *arg);\n> \n> \tbool hasOwnership(V4L2CameraFile *file);\n> \tint acquire(V4L2CameraFile *file);","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 A74FEBF415\n\tfor ;\n\tFri, 25 Feb 2022 08:47:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CD22E61163;\n\tFri, 25 Feb 2022 09:47:50 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D1DF6604EE\n\tfor ;\n\tFri, 25 Feb 2022 09:47:48 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 429F9DD;\n\tFri, 25 Feb 2022 09:47:48 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"aL8XZKeQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1645778868;\n\tbh=5mNdiI2eQYMXtB9HPPSziWty7N2t+VVcnpAK3I9Eq7o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aL8XZKeQRMXaI09+Msr7CbOiZ57fLhL5hqSpfct+LGgSURVeQ+dzIXB2No0oUL7x4\n\ttKIQsNKNv/w94I/6NOqlXAHQOx7vIy9io19GbYCoNzsW6ZCYtg8AEwdWE6/Slu6qsr\n\t5FjWeOFFU1lqXOD/751jtajvXesHqQtvN986SyGY=","Date":"Fri, 25 Feb 2022 10:47:37 +0200","From":"Laurent Pinchart ","To":"Nejc Galof ","Message-ID":"","References":"<20220215114222.115790-1-galof.nejc@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220215114222.115790-1-galof.nejc@gmail.com>","Subject":"Re: [libcamera-devel] [PATCH] v4l2: Support setting frame rate in\n\tthe V4L2 Adaptation layer","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22196,"web_url":"https://patchwork.libcamera.org/comment/22196/","msgid":"","date":"2022-02-25T14:19:18","subject":"Re: [libcamera-devel] [PATCH] v4l2: Support setting frame rate in\n\tthe V4L2 Adaptation layer","submitter":{"id":113,"url":"https://patchwork.libcamera.org/api/people/113/","name":"Nejc Galof","email":"galof.nejc@gmail.com"},"content":"Hi Laurent.\n\nI something messed up with patches (I think), and it is added like a new\npatch. Can you review what I do wrong, for the next update in the feature?\nSome notes about review:\n- GetParams I recommended to add in the new patch.\n- setControls is not named like that, because there is no getControls (yet).\n- All other recommendations are added and fixed.\n\nRegards,\nNejc Galof\n\nV V pet., 25. feb. 2022 ob 09:47 je oseba Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> napisala:\n\n> Hi Nejc,\n>\n> Thank you for the patch.\n>\n> On Tue, Feb 15, 2022 at 12:42:22PM +0100, Nejc Galof wrote:\n> > The V4L2 adaptation layer can already support streaming with components\n> > such as OpenCV, however it is not accepting, or handling any requests to\n> > configure the frame rate.\n> >\n> > In V4L2 the frame rate is set by configuring the timeperframe component\n> > of the v4l2_streamparm structure through the VIDIOC_S_PARM ioctl.\n> >\n> > Extend the V4L2 compatibility layer to accept the VIDIOC_S_PARM ioctls\n> > and provide an interface for setting controls on the V4L2Camera class to\n> > set the requested rate when starting the camera.\n> >\n> > Signed-off-by: Nejc Galof \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > src/v4l2/v4l2_camera.cpp | 12 +++++++++---\n> > src/v4l2/v4l2_camera.h | 7 +++++++\n> > src/v4l2/v4l2_camera_proxy.cpp | 25 +++++++++++++++++++++++++\n> > src/v4l2/v4l2_camera_proxy.h | 1 +\n> > 4 files changed, 42 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> > index e922b9e6..e4eb3a2b 100644\n> > --- a/src/v4l2/v4l2_camera.cpp\n> > +++ b/src/v4l2/v4l2_camera.cpp\n> > @@ -12,13 +12,15 @@\n> >\n> > #include \n> >\n> > +#include \n> > +\n> > using namespace libcamera;\n> >\n> > LOG_DECLARE_CATEGORY(V4L2Compat)\n> >\n> > V4L2Camera::V4L2Camera(std::shared_ptr camera)\n> > - : camera_(camera), isRunning_(false), bufferAllocator_(nullptr),\n> > - efd_(-1), bufferAvailableCount_(0)\n> > + : camera_(camera), controls_(controls::controls),\n> isRunning_(false),\n> > + bufferAllocator_(nullptr), efd_(-1), bufferAvailableCount_(0)\n> > {\n> > camera_->requestCompleted.connect(this,\n> &V4L2Camera::requestComplete);\n> > }\n> > @@ -203,10 +205,12 @@ int V4L2Camera::streamOn()\n> > if (isRunning_)\n> > return 0;\n> >\n> > - int ret = camera_->start();\n> > + int ret = camera_->start(&controls_);\n> > if (ret < 0)\n> > return ret == -EACCES ? -EBUSY : ret;\n> >\n> > + controls_.clear();\n> > +\n> > isRunning_ = true;\n> >\n> > for (Request *req : pendingRequests_) {\n> > @@ -266,6 +270,8 @@ int V4L2Camera::qbuf(unsigned int index)\n> > return 0;\n> > }\n> >\n> > + request->controls().merge(std::move(controls_));\n> > +\n> > ret = camera_->queueRequest(request);\n> > if (ret < 0) {\n> > LOG(V4L2Compat, Error) << \"Can't queue request\";\n> > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h\n> > index 03e74118..9e6c895a 100644\n> > --- a/src/v4l2/v4l2_camera.h\n> > +++ b/src/v4l2/v4l2_camera.h\n> > @@ -10,11 +10,14 @@\n> > #include \n> > #include \n> >\n> > +#include \n>\n> I don't think you need to include this here.\n>\n> > +\n> > #include \n> > #include \n> > #include \n> >\n> > #include \n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -49,6 +52,8 @@ public:\n> > const libcamera::Size &size,\n> > libcamera::StreamConfiguration\n> *streamConfigOut);\n> >\n> > + libcamera::ControlList &controls() { return controls_; }\n>\n> I may have gone for a setControls() function, but this works too.\n>\n> > +\n> > int allocBuffers(unsigned int count);\n> > void freeBuffers();\n> > int getBufferFd(unsigned int index);\n> > @@ -69,6 +74,8 @@ private:\n> > std::shared_ptr camera_;\n> > std::unique_ptr config_;\n> >\n> > + libcamera::ControlList controls_;\n> > +\n> > bool isRunning_;\n> >\n> > libcamera::Mutex bufferLock_;\n> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp\n> b/src/v4l2/v4l2_camera_proxy.cpp\n> > index e114d09f..f005b21c 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.cpp\n> > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > @@ -18,6 +18,8 @@\n> > #include \n> >\n> > #include \n> > +#include \n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -33,6 +35,7 @@\n> > #define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + (c))\n> >\n> > using namespace libcamera;\n> > +using namespace std::literals::chrono_literals;\n> >\n> > LOG_DECLARE_CATEGORY(V4L2Compat)\n> >\n> > @@ -755,6 +758,24 @@ int\n> V4L2CameraProxy::vidioc_streamoff(V4L2CameraFile *file, int *arg)\n> > return ret;\n> > }\n> >\n> > +int V4L2CameraProxy::vidioc_s_param(V4L2CameraFile *file, struct\n> v4l2_streamparm *arg)\n> > +{\n> > + LOG(V4L2Compat, Debug)\n> > + << \"[\" << file->description() << \"] \" << __func__ << \"()\";\n> > +\n> > + if (!validateBufferType(arg->type))\n> > + return -EINVAL;\n> > +\n> > + struct v4l2_fract *timeperframe = &arg->parm.capture.timeperframe;\n> > + utils::Duration frameDuration = 1s *\n> (static_cast(timeperframe->numerator) /\n> > +\n> static_cast(timeperframe->denominator));\n>\n> Does the following work ?\n>\n> utils::Duration frameDuration = 1.0s * timeperframe->numerator\n> / timeperframe->denominator;\n>\n> > +\n> > + int64_t uDuration = frameDuration.get();\n> > + vcam_->controls().set(controls::FrameDurationLimits, { uDuration,\n> uDuration });\n>\n> Looks like we should move the FrameDurationLimits control to using the\n> Duration type. Not a candidate for this patch of course.\n>\n> > +\n> > + return 0;\n> > +}\n>\n> Getting the frame rate back from the camera to implement vidioc_g_param\n> is a more difficult task, but would it make sense to cache the frame\n> rate in the V4L2CameraProxy and implement vidioc_g_param based on that\n> already ? The question of what frame rate to expose by default will be\n> an interesting one, but I think you can start with a hardcoded default\n> value of 1/30. This could be done in this patch if easy, or in a\n> separate patch.\n>\n> > +\n> > const std::set V4L2CameraProxy::supportedIoctls_ = {\n> > VIDIOC_QUERYCAP,\n> > VIDIOC_ENUM_FRAMESIZES,\n> > @@ -775,6 +796,7 @@ const std::set\n> V4L2CameraProxy::supportedIoctls_ = {\n> > VIDIOC_EXPBUF,\n> > VIDIOC_STREAMON,\n> > VIDIOC_STREAMOFF,\n> > + VIDIOC_S_PARM,\n> > };\n> >\n> > int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request,\n> void *arg)\n> > @@ -852,6 +874,9 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *file,\n> unsigned long request, void *ar\n> > case VIDIOC_STREAMOFF:\n> > ret = vidioc_streamoff(file, static_cast(arg));\n> > break;\n> > + case VIDIOC_S_PARM:\n> > + ret = vidioc_s_param(file, static_cast v4l2_streamparm *>(arg));\n>\n> vidioc_s_parm, to match the ioctl name.\n>\n> All in all it looks fairly good. With the two small changes requested\n> above,\n>\n> Reviewed-by: Laurent Pinchart \n>\n> Will you send a v2 ?\n>\n> > + break;\n> > default:\n> > ret = -ENOTTY;\n> > break;\n> > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h\n> > index 76ca2d8a..30a3f492 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.h\n> > +++ b/src/v4l2/v4l2_camera_proxy.h\n> > @@ -65,6 +65,7 @@ private:\n> > int vidioc_expbuf(V4L2CameraFile *file, struct v4l2_exportbuffer\n> *arg);\n> > int vidioc_streamon(V4L2CameraFile *file, int *arg);\n> > int vidioc_streamoff(V4L2CameraFile *file, int *arg);\n> > + int vidioc_s_param(V4L2CameraFile *file, struct v4l2_streamparm\n> *arg);\n> >\n> > bool hasOwnership(V4L2CameraFile *file);\n> > int acquire(V4L2CameraFile *file);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 46EBCBE08A\n\tfor ;\n\tFri, 25 Feb 2022 14:19:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 75F5261174;\n\tFri, 25 Feb 2022 15:19:33 +0100 (CET)","from mail-io1-xd33.google.com (mail-io1-xd33.google.com\n\t[IPv6:2607:f8b0:4864:20::d33])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7BA07601FE\n\tfor ;\n\tFri, 25 Feb 2022 15:19:31 +0100 (CET)","by mail-io1-xd33.google.com with SMTP id f14so6670754ioz.1\n\tfor ;\n\tFri, 25 Feb 2022 06:19:31 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"occysShy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=X0MHPsg6iilUjsH8KiG0TqnUkuLlNXoP0JUkg4MyjIw=;\n\tb=occysShyfq/hwOy7Qg2MGk0LVKctsvz9OIK4YJU4bY3Q6rbh7V8otfWV1qSrvl4csV\n\tKatzuH6e0SsTzRgbHiQhfb80jXtM/KFjD/PwmOgZaqZOkiX6TclJiIQPCIvE7JGnT5Wb\n\tDeMuroMxWh66p0HuKAVk1zHm1pKQdcepwNMG76kXUE1zjzZFzNmEahHLmMa9YyNTA0AB\n\tVYQyK1ciXAVr7SGtK8JuGt9hbpXgZoeVvBkRlc4W9qUDXKKgfWZna5lN95mZL1/HN1ax\n\tvapUDxV4EEpcMRi/T36P6xmpvxaPpJ+RSzRERTnbp42ylkxHO2gs/7k801RdQEbG+LsQ\n\tBtiw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=X0MHPsg6iilUjsH8KiG0TqnUkuLlNXoP0JUkg4MyjIw=;\n\tb=SEsEYBUAiy33kROhQZbkjjUu0w6FT6BzIGYpxVKD34Dy3gO8j3fVcPkZrgEMGOjfyq\n\tfMLLg/iBRlZo7tNavF46R2hg4zoK8cmR3QqiXtvVnAWb/a2+Pvw3E8FsqGPOW8Gb/uNg\n\tERKKs42fB67sKMP+i03LNp2KcQIVA0HnWTDn7HyPO8EjSDS1ylhX2TV0itvJwJN7thGo\n\teIQm0CE9y9pU99A+vi3yZg5BvFOGZWeFQUbF0L0vZp/hnbkjFSGrvj+00buSbn2faVpP\n\toeEH0hMG/JTkkE8MjmFJwL9rLejRKYzr13Rqzc1a0A3JPpNBv/eIYN5R2QPcyYHRgJXW\n\tSQ6g==","X-Gm-Message-State":"AOAM531jhPIYmHZXaMdYvEvK1B6teXzKNWBX84U5/J2VC8P4HvamzBD6\n\tDcQK4+FcVu1vEr1WIub+//F84SmYq9ow/1qpL6c=","X-Google-Smtp-Source":"ABdhPJzaJ5RIalKLepE91gok6NYyKwaR2uPyHy6TI9Z7ElRlknOS5HkANLoeN47o13wVOJ5JqzLIecPx2NiSC1Qa7Ow=","X-Received":"by 2002:a6b:7a0a:0:b0:641:6419:f5a1 with SMTP id\n\th10-20020a6b7a0a000000b006416419f5a1mr5692712iom.174.1645798769873;\n\tFri, 25 Feb 2022 06:19:29 -0800 (PST)","MIME-Version":"1.0","References":"<20220215114222.115790-1-galof.nejc@gmail.com>\n\t","In-Reply-To":"","From":"Nejc Galof ","Date":"Fri, 25 Feb 2022 15:19:18 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000000cda6605d8d863ea\"","Subject":"Re: [libcamera-devel] [PATCH] v4l2: Support setting frame rate in\n\tthe V4L2 Adaptation layer","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22197,"web_url":"https://patchwork.libcamera.org/comment/22197/","msgid":"","date":"2022-02-25T14:25:22","subject":"Re: [libcamera-devel] [PATCH] v4l2: Support setting frame rate in\n\tthe V4L2 Adaptation layer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nejc,\n\nOn Fri, Feb 25, 2022 at 03:19:18PM +0100, Nejc Galof wrote:\n> Hi Laurent.\n> \n> I something messed up with patches (I think), and it is added like a new\n> patch.\n\nIt should indeed have been a single patch. If the first patch had been\nmerged already, a separate patch to fix issues would have been the right\nthing to do, but as that's not the case, the fixes should have been\nsquashed. \"git commit --amend\" and \"git rebase -i\" are you friends there.\n\n> Can you review what I do wrong, for the next update in the feature?\n> Some notes about review:\n> - GetParams I recommended to add in the new patch.\n> - setControls is not named like that, because there is no getControls (yet).\n> - All other recommendations are added and fixed.\n\nSure, I'll review v2.\n\n> V V pet., 25. feb. 2022 ob 09:47 je oseba Laurent Pinchart napisala:\n> > On Tue, Feb 15, 2022 at 12:42:22PM +0100, Nejc Galof wrote:\n> > > The V4L2 adaptation layer can already support streaming with components\n> > > such as OpenCV, however it is not accepting, or handling any requests to\n> > > configure the frame rate.\n> > >\n> > > In V4L2 the frame rate is set by configuring the timeperframe component\n> > > of the v4l2_streamparm structure through the VIDIOC_S_PARM ioctl.\n> > >\n> > > Extend the V4L2 compatibility layer to accept the VIDIOC_S_PARM ioctls\n> > > and provide an interface for setting controls on the V4L2Camera class to\n> > > set the requested rate when starting the camera.\n> > >\n> > > Signed-off-by: Nejc Galof \n> > > Signed-off-by: Kieran Bingham \n> > > ---\n> > > src/v4l2/v4l2_camera.cpp | 12 +++++++++---\n> > > src/v4l2/v4l2_camera.h | 7 +++++++\n> > > src/v4l2/v4l2_camera_proxy.cpp | 25 +++++++++++++++++++++++++\n> > > src/v4l2/v4l2_camera_proxy.h | 1 +\n> > > 4 files changed, 42 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> > > index e922b9e6..e4eb3a2b 100644\n> > > --- a/src/v4l2/v4l2_camera.cpp\n> > > +++ b/src/v4l2/v4l2_camera.cpp\n> > > @@ -12,13 +12,15 @@\n> > >\n> > > #include \n> > >\n> > > +#include \n> > > +\n> > > using namespace libcamera;\n> > >\n> > > LOG_DECLARE_CATEGORY(V4L2Compat)\n> > >\n> > > V4L2Camera::V4L2Camera(std::shared_ptr camera)\n> > > - : camera_(camera), isRunning_(false), bufferAllocator_(nullptr),\n> > > - efd_(-1), bufferAvailableCount_(0)\n> > > + : camera_(camera), controls_(controls::controls), isRunning_(false),\n> > > + bufferAllocator_(nullptr), efd_(-1), bufferAvailableCount_(0)\n> > > {\n> > > camera_->requestCompleted.connect(this, &V4L2Camera::requestComplete);\n> > > }\n> > > @@ -203,10 +205,12 @@ int V4L2Camera::streamOn()\n> > > if (isRunning_)\n> > > return 0;\n> > >\n> > > - int ret = camera_->start();\n> > > + int ret = camera_->start(&controls_);\n> > > if (ret < 0)\n> > > return ret == -EACCES ? -EBUSY : ret;\n> > >\n> > > + controls_.clear();\n> > > +\n> > > isRunning_ = true;\n> > >\n> > > for (Request *req : pendingRequests_) {\n> > > @@ -266,6 +270,8 @@ int V4L2Camera::qbuf(unsigned int index)\n> > > return 0;\n> > > }\n> > >\n> > > + request->controls().merge(std::move(controls_));\n> > > +\n> > > ret = camera_->queueRequest(request);\n> > > if (ret < 0) {\n> > > LOG(V4L2Compat, Error) << \"Can't queue request\";\n> > > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h\n> > > index 03e74118..9e6c895a 100644\n> > > --- a/src/v4l2/v4l2_camera.h\n> > > +++ b/src/v4l2/v4l2_camera.h\n> > > @@ -10,11 +10,14 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \n> >\n> > I don't think you need to include this here.\n> >\n> > > +\n> > > #include \n> > > #include \n> > > #include \n> > >\n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > @@ -49,6 +52,8 @@ public:\n> > > const libcamera::Size &size,\n> > > libcamera::StreamConfiguration *streamConfigOut);\n> > >\n> > > + libcamera::ControlList &controls() { return controls_; }\n> >\n> > I may have gone for a setControls() function, but this works too.\n> >\n> > > +\n> > > int allocBuffers(unsigned int count);\n> > > void freeBuffers();\n> > > int getBufferFd(unsigned int index);\n> > > @@ -69,6 +74,8 @@ private:\n> > > std::shared_ptr camera_;\n> > > std::unique_ptr config_;\n> > >\n> > > + libcamera::ControlList controls_;\n> > > +\n> > > bool isRunning_;\n> > >\n> > > libcamera::Mutex bufferLock_;\n> > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> > > index e114d09f..f005b21c 100644\n> > > --- a/src/v4l2/v4l2_camera_proxy.cpp\n> > > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > > @@ -18,6 +18,8 @@\n> > > #include \n> > >\n> > > #include \n> > > +#include \n> > > +#include \n> > > #include \n> > >\n> > > #include \n> > > @@ -33,6 +35,7 @@\n> > > #define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + (c))\n> > >\n> > > using namespace libcamera;\n> > > +using namespace std::literals::chrono_literals;\n> > >\n> > > LOG_DECLARE_CATEGORY(V4L2Compat)\n> > >\n> > > @@ -755,6 +758,24 @@ int V4L2CameraProxy::vidioc_streamoff(V4L2CameraFile *file, int *arg)\n> > > return ret;\n> > > }\n> > >\n> > > +int V4L2CameraProxy::vidioc_s_param(V4L2CameraFile *file, struct v4l2_streamparm *arg)\n> > > +{\n> > > + LOG(V4L2Compat, Debug)\n> > > + << \"[\" << file->description() << \"] \" << __func__ << \"()\";\n> > > +\n> > > + if (!validateBufferType(arg->type))\n> > > + return -EINVAL;\n> > > +\n> > > + struct v4l2_fract *timeperframe = &arg->parm.capture.timeperframe;\n> > > + utils::Duration frameDuration = 1s * (static_cast(timeperframe->numerator) /\n> > > + static_cast(timeperframe->denominator));\n> >\n> > Does the following work ?\n> >\n> > utils::Duration frameDuration = 1.0s * timeperframe->numerator\n> > / timeperframe->denominator;\n> >\n> > > +\n> > > + int64_t uDuration = frameDuration.get();\n> > > + vcam_->controls().set(controls::FrameDurationLimits, { uDuration, uDuration });\n> >\n> > Looks like we should move the FrameDurationLimits control to using the\n> > Duration type. Not a candidate for this patch of course.\n> >\n> > > +\n> > > + return 0;\n> > > +}\n> >\n> > Getting the frame rate back from the camera to implement vidioc_g_param\n> > is a more difficult task, but would it make sense to cache the frame\n> > rate in the V4L2CameraProxy and implement vidioc_g_param based on that\n> > already ? The question of what frame rate to expose by default will be\n> > an interesting one, but I think you can start with a hardcoded default\n> > value of 1/30. This could be done in this patch if easy, or in a\n> > separate patch.\n> >\n> > > +\n> > > const std::set V4L2CameraProxy::supportedIoctls_ = {\n> > > VIDIOC_QUERYCAP,\n> > > VIDIOC_ENUM_FRAMESIZES,\n> > > @@ -775,6 +796,7 @@ const std::set V4L2CameraProxy::supportedIoctls_ = {\n> > > VIDIOC_EXPBUF,\n> > > VIDIOC_STREAMON,\n> > > VIDIOC_STREAMOFF,\n> > > + VIDIOC_S_PARM,\n> > > };\n> > >\n> > > int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *arg)\n> > > @@ -852,6 +874,9 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *ar\n> > > case VIDIOC_STREAMOFF:\n> > > ret = vidioc_streamoff(file, static_cast(arg));\n> > > break;\n> > > + case VIDIOC_S_PARM:\n> > > + ret = vidioc_s_param(file, static_cast(arg));\n> >\n> > vidioc_s_parm, to match the ioctl name.\n> >\n> > All in all it looks fairly good. With the two small changes requested\n> > above,\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > Will you send a v2 ?\n> >\n> > > + break;\n> > > default:\n> > > ret = -ENOTTY;\n> > > break;\n> > > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h\n> > > index 76ca2d8a..30a3f492 100644\n> > > --- a/src/v4l2/v4l2_camera_proxy.h\n> > > +++ b/src/v4l2/v4l2_camera_proxy.h\n> > > @@ -65,6 +65,7 @@ private:\n> > > int vidioc_expbuf(V4L2CameraFile *file, struct v4l2_exportbuffer\n> > *arg);\n> > > int vidioc_streamon(V4L2CameraFile *file, int *arg);\n> > > int vidioc_streamoff(V4L2CameraFile *file, int *arg);\n> > > + int vidioc_s_param(V4L2CameraFile *file, struct v4l2_streamparm\n> > *arg);\n> > >\n> > > bool hasOwnership(V4L2CameraFile *file);\n> > > int acquire(V4L2CameraFile *file);","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 BFC95BF415\n\tfor ;\n\tFri, 25 Feb 2022 14:25:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EF72E6116E;\n\tFri, 25 Feb 2022 15:25:35 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 658BE61166\n\tfor ;\n\tFri, 25 Feb 2022 15:25:34 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C8FE9DD;\n\tFri, 25 Feb 2022 15:25:33 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"CBY1dAKP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1645799134;\n\tbh=2S8EYZ90LEAFwc+X6WFlpv3k41Ke8/IBiMfvQ9IIKcQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CBY1dAKP5Y+Vfh+o8je8x8KmJmmBgW74AtCgcV0Xf7XrQb4fX8uozH1CS6dypmgos\n\tbEzcJB1qgf360iCE3vBBS3USFaiVUfJucw64zui41d9cppefvk2esbRqJAhgtIsLiF\n\tGtex8E3dkxJEeXa6ofoBTXZ3+2HkvBku/NJa9T34=","Date":"Fri, 25 Feb 2022 16:25:22 +0200","From":"Laurent Pinchart ","To":"Nejc Galof ","Message-ID":"","References":"<20220215114222.115790-1-galof.nejc@gmail.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] v4l2: Support setting frame rate in\n\tthe V4L2 Adaptation layer","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]