[{"id":13500,"web_url":"https://patchwork.libcamera.org/comment/13500/","msgid":"<20201027014201.GR3756@pendragon.ideasonboard.com>","date":"2020-10-27T01:42:01","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_device: Move start of\n\tframe detection to V4L2Device","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Sun, Oct 18, 2020 at 03:57:54AM +0200, Niklas Söderlund wrote:\n> The V4L2_EVENT_FRAME_SYNC event may occur on both V4L2 video-devices\n> (V4L2VideoDevice) and sub-devices (V4L2Subdevice). Move the start of\n> frame detection to the common base class of the two, V4L2Device.\n> \n> There is no functional change.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/internal/v4l2_device.h | 13 ++++\n> include/libcamera/internal/v4l2_videodevice.h | 8 --\n> src/libcamera/v4l2_device.cpp | 78 ++++++++++++++++++-\n> src/libcamera/v4l2_videodevice.cpp | 75 +-----------------\n> 4 files changed, 91 insertions(+), 83 deletions(-)\n> \n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index 722fb72207d74111..b0a9ed0a1d3557e6 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -13,11 +13,15 @@\n> \n> #include \n> \n> +#include \n> +\n> #include \"libcamera/internal/log.h\"\n> #include \"libcamera/internal/v4l2_controls.h\"\n> \n> namespace libcamera {\n> \n> +class EventNotifier;\n> +\n> class V4L2Device : protected Loggable\n> {\n> public:\n> @@ -34,6 +38,9 @@ public:\n> \tconst std::string &deviceNode() const { return deviceNode_; }\n> \tstd::string devicePath() const;\n> \n> +\tint setFrameStartEnabled(bool enable);\n> +\tSignal frameStart;\n> +\n> protected:\n> \tV4L2Device(const std::string &deviceNode);\n> \t~V4L2Device();\n> @@ -45,6 +52,8 @@ protected:\n> \n> \tint fd() { return fd_; }\n> \n> +\tvoid eventAvailable(EventNotifier *notifier);\n\nIs there a reason why this is protected and not private ?\n\n> +\n> private:\n> \tvoid listControls();\n> \tvoid updateControls(ControlList *ctrls,\n> @@ -56,6 +65,10 @@ private:\n> \tControlInfoMap controls_;\n> \tstd::string deviceNode_;\n> \tint fd_;\n> +\n> +\tEventNotifier *fdEventNotifier_;\n> +\n\nI'd drop this blank line.\n\n> +\tbool frameStartEnabled_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index 40ed87e17cfa6d3c..2fd09a1343566efa 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -204,9 +204,6 @@ public:\n> \tint queueBuffer(FrameBuffer *buffer);\n> \tSignal bufferReady;\n> \n> -\tint setFrameStartEnabled(bool enable);\n> -\tSignal frameStart;\n> -\n> \tint streamOn();\n> \tint streamOff();\n> \n> @@ -240,8 +237,6 @@ private:\n> \tvoid bufferAvailable(EventNotifier *notifier);\n> \tFrameBuffer *dequeueBuffer();\n> \n> -\tvoid eventAvailable(EventNotifier *notifier);\n> -\n> \tV4L2Capability caps_;\n> \n> \tenum v4l2_buf_type bufferType_;\n> @@ -251,9 +246,6 @@ private:\n> \tstd::map queuedBuffers_;\n> \n> \tEventNotifier *fdBufferNotifier_;\n> -\tEventNotifier *fdEventNotifier_;\n> -\n> -\tbool frameStartEnabled_;\n> };\n> \n> class V4L2M2MDevice\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 31d4dad0ee47b734..36a2c21e6544af22 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -16,6 +16,8 @@\n> #include \n> #include \n> \n> +#include \n> +\n> #include \"libcamera/internal/log.h\"\n> #include \"libcamera/internal/sysfs.h\"\n> #include \"libcamera/internal/utils.h\"\n> @@ -52,7 +54,8 @@ LOG_DEFINE_CATEGORY(V4L2)\n> * at open() time, and the \\a logTag to prefix log messages with.\n> */\n> V4L2Device::V4L2Device(const std::string &deviceNode)\n> -\t: deviceNode_(deviceNode), fd_(-1)\n> +\t: deviceNode_(deviceNode), fd_(-1), fdEventNotifier_(nullptr),\n> +\t frameStartEnabled_(false)\n> {\n> }\n> \n> @@ -91,6 +94,10 @@ int V4L2Device::open(unsigned int flags)\n> \n> \tlistControls();\n> \n> +\tfdEventNotifier_ = new EventNotifier(fd_, EventNotifier::Exception);\n> +\tfdEventNotifier_->activated.connect(this, &V4L2Device::eventAvailable);\n> +\tfdEventNotifier_->setEnabled(false);\n\nYou could alternatively call setFd() (replacing the fd_ = ret; line).\n\nReviewed-by: Laurent Pinchart \n\n> +\n> \treturn 0;\n> }\n> \n> @@ -117,6 +124,10 @@ int V4L2Device::setFd(int fd)\n> \n> \tfd_ = fd;\n> \n> +\tfdEventNotifier_ = new EventNotifier(fd_, EventNotifier::Exception);\n> +\tfdEventNotifier_->activated.connect(this, &V4L2Device::eventAvailable);\n> +\tfdEventNotifier_->setEnabled(false);\n> +\n> \treturn 0;\n> }\n> \n> @@ -130,6 +141,8 @@ void V4L2Device::close()\n> \tif (!isOpen())\n> \t\treturn;\n> \n> +\tdelete fdEventNotifier_;\n> +\n> \tif (::close(fd_) < 0)\n> \t\tLOG(V4L2, Error) << \"Failed to close V4L2 device: \"\n> \t\t\t\t << strerror(errno);\n> @@ -396,6 +409,40 @@ std::string V4L2Device::devicePath() const\n> \treturn path;\n> }\n> \n> +/**\n> + * \\brief Enable or disable frame start event notification\n> + * \\param[in] enable True to enable frame start events, false to disable them\n> + *\n> + * This function enables or disables generation of frame start events. Once\n> + * enabled, the events are signalled through the frameStart signal.\n> + *\n> + * \\return 0 on success, a negative error code otherwise\n> + */\n> +int V4L2Device::setFrameStartEnabled(bool enable)\n> +{\n> +\tif (frameStartEnabled_ == enable)\n> +\t\treturn 0;\n> +\n> +\tstruct v4l2_event_subscription event{};\n> +\tevent.type = V4L2_EVENT_FRAME_SYNC;\n> +\n> +\tunsigned long request = enable ? VIDIOC_SUBSCRIBE_EVENT\n> +\t\t\t : VIDIOC_UNSUBSCRIBE_EVENT;\n> +\tint ret = ioctl(request, &event);\n> +\tif (enable && ret)\n> +\t\treturn ret;\n> +\n> +\tfdEventNotifier_->setEnabled(enable);\n> +\tframeStartEnabled_ = enable;\n> +\n> +\treturn ret;\n> +}\n> +\n> +/**\n> + * \\var V4L2Device::frameStart\n> + * \\brief A Signal emitted when capture of a frame has started\n> + */\n> +\n> /**\n> * \\brief Perform an IOCTL system call on the device node\n> * \\param[in] request The IOCTL request code\n> @@ -414,6 +461,35 @@ int V4L2Device::ioctl(unsigned long request, void *argp)\n> \treturn 0;\n> }\n> \n> +/**\n> + * \\brief Slot to handle V4L2 events from the V4L2 device\n> + * \\param[in] notifier The event notifier\n> + *\n> + * When this slot is called, a V4L2 event is available to be dequeued from the\n> + * device.\n> + */\n> +void V4L2Device::eventAvailable([[maybe_unused]] EventNotifier *notifier)\n> +{\n> +\tstruct v4l2_event event{};\n> +\tint ret = ioctl(VIDIOC_DQEVENT, &event);\n> +\tif (ret < 0) {\n> +\t\tLOG(V4L2, Error)\n> +\t\t\t<< \"Failed to dequeue event, disabling event notifier\";\n> +\t\tfdEventNotifier_->setEnabled(false);\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (event.type != V4L2_EVENT_FRAME_SYNC) {\n> +\t\tLOG(V4L2, Error)\n> +\t\t\t<< \"Spurious event (\" << event.type\n> +\t\t\t<< \"), disabling event notifier\";\n> +\t\tfdEventNotifier_->setEnabled(false);\n> +\t\treturn;\n> +\t}\n> +\n> +\tframeStart.emit(event.u.frame_sync.frame_sequence);\n> +}\n> +\n> /**\n> * \\fn V4L2Device::deviceNode()\n> * \\brief Retrieve the device node path\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 508522ef42bb52cb..6e86849b0013c373 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -471,8 +471,7 @@ const std::string V4L2DeviceFormat::toString() const\n> * \\param[in] deviceNode The file-system path to the video device node\n> */\n> V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)\n> -\t: V4L2Device(deviceNode), cache_(nullptr), fdBufferNotifier_(nullptr),\n> -\t fdEventNotifier_(nullptr), frameStartEnabled_(false)\n> +\t: V4L2Device(deviceNode), cache_(nullptr), fdBufferNotifier_(nullptr)\n> {\n> \t/*\n> \t * We default to an MMAP based CAPTURE video device, however this will\n> @@ -565,10 +564,6 @@ int V4L2VideoDevice::open()\n> \tfdBufferNotifier_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);\n> \tfdBufferNotifier_->setEnabled(false);\n> \n> -\tfdEventNotifier_ = new EventNotifier(fd(), EventNotifier::Exception);\n> -\tfdEventNotifier_->activated.connect(this, &V4L2VideoDevice::eventAvailable);\n> -\tfdEventNotifier_->setEnabled(false);\n> -\n> \tLOG(V4L2, Debug)\n> \t\t<< \"Opened device \" << caps_.bus_info() << \": \"\n> \t\t<< caps_.driver() << \": \" << caps_.card();\n> @@ -658,10 +653,6 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)\n> \tfdBufferNotifier_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);\n> \tfdBufferNotifier_->setEnabled(false);\n> \n> -\tfdEventNotifier_ = new EventNotifier(fd(), EventNotifier::Exception);\n> -\tfdEventNotifier_->activated.connect(this, &V4L2VideoDevice::eventAvailable);\n> -\tfdEventNotifier_->setEnabled(false);\n> -\n> \tLOG(V4L2, Debug)\n> \t\t<< \"Opened device \" << caps_.bus_info() << \": \"\n> \t\t<< caps_.driver() << \": \" << caps_.card();\n> @@ -679,7 +670,6 @@ void V4L2VideoDevice::close()\n> \n> \treleaseBuffers();\n> \tdelete fdBufferNotifier_;\n> -\tdelete fdEventNotifier_;\n> \n> \tV4L2Device::close();\n> }\n> @@ -1532,74 +1522,11 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> \treturn buffer;\n> }\n> \n> -/**\n> - * \\brief Slot to handle V4L2 events from the V4L2 video device\n> - * \\param[in] notifier The event notifier\n> - *\n> - * When this slot is called, a V4L2 event is available to be dequeued from the\n> - * device.\n> - */\n> -void V4L2VideoDevice::eventAvailable([[maybe_unused]] EventNotifier *notifier)\n> -{\n> -\tstruct v4l2_event event{};\n> -\tint ret = ioctl(VIDIOC_DQEVENT, &event);\n> -\tif (ret < 0) {\n> -\t\tLOG(V4L2, Error)\n> -\t\t\t<< \"Failed to dequeue event, disabling event notifier\";\n> -\t\tfdEventNotifier_->setEnabled(false);\n> -\t\treturn;\n> -\t}\n> -\n> -\tif (event.type != V4L2_EVENT_FRAME_SYNC) {\n> -\t\tLOG(V4L2, Error)\n> -\t\t\t<< \"Spurious event (\" << event.type\n> -\t\t\t<< \"), disabling event notifier\";\n> -\t\tfdEventNotifier_->setEnabled(false);\n> -\t\treturn;\n> -\t}\n> -\n> -\tframeStart.emit(event.u.frame_sync.frame_sequence);\n> -}\n> -\n> /**\n> * \\var V4L2VideoDevice::bufferReady\n> * \\brief A Signal emitted when a framebuffer completes\n> */\n> \n> -/**\n> - * \\brief Enable or disable frame start event notification\n> - * \\param[in] enable True to enable frame start events, false to disable them\n> - *\n> - * This function enables or disables generation of frame start events. Once\n> - * enabled, the events are signalled through the frameStart signal.\n> - *\n> - * \\return 0 on success, a negative error code otherwise\n> - */\n> -int V4L2VideoDevice::setFrameStartEnabled(bool enable)\n> -{\n> -\tif (frameStartEnabled_ == enable)\n> -\t\treturn 0;\n> -\n> -\tstruct v4l2_event_subscription event{};\n> -\tevent.type = V4L2_EVENT_FRAME_SYNC;\n> -\n> -\tunsigned long request = enable ? VIDIOC_SUBSCRIBE_EVENT\n> -\t\t\t : VIDIOC_UNSUBSCRIBE_EVENT;\n> -\tint ret = ioctl(request, &event);\n> -\tif (enable && ret)\n> -\t\treturn ret;\n> -\n> -\tfdEventNotifier_->setEnabled(enable);\n> -\tframeStartEnabled_ = enable;\n> -\n> -\treturn ret;\n> -}\n> -\n> -/**\n> - * \\var V4L2VideoDevice::frameStart\n> - * \\brief A Signal emitted when capture of a frame has started\n> - */\n> -\n> /**\n> * \\brief Start the video stream\n> * \\return 0 on success or a negative error code otherwise","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 DB020BDB1E\n\tfor ;\n\tTue, 27 Oct 2020 01:42:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 522F662069;\n\tTue, 27 Oct 2020 02:42: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 D27C76034C\n\tfor ;\n\tTue, 27 Oct 2020 02:42: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 47EF33D;\n\tTue, 27 Oct 2020 02:42:48 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"eWODcXRg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603762968;\n\tbh=r5ORHzxDPv85fkZoPwUKVag85XVHEoYjWEHxNmg6xm0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eWODcXRghEGtgNn0xhU97fHh0uwhocsM49X8naIUqoHs06f1JP+MRQGRRgikKfngL\n\t5iPxNsUQooFyJWMSPmLelhpBnHnWyAe4nxeOVm/Ogo9kjUYZ9EEcjdx/luqhGc0lfF\n\tSulNVAuErzopgVK3CzGD6jyJmXBfpf4R/Hnd8r9c=","Date":"Tue, 27 Oct 2020 03:42:01 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20201027014201.GR3756@pendragon.ideasonboard.com>","References":"<20201018015754.3784352-1-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201018015754.3784352-1-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_device: Move start of\n\tframe detection to V4L2Device","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]