[{"id":4527,"web_url":"https://patchwork.libcamera.org/comment/4527/","msgid":"<20200426132522.tqbf6t2snabbg2f2@uno.localdomain>","date":"2020-04-26T13:25:22","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_videodevice: Add\n\tsupport for frame start events","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sun, Apr 26, 2020 at 03:43:09AM +0300, Laurent Pinchart wrote:\n> Extend the V4L2VideoDevice to support notifying of frame start events.\n> The events are received from the device through the V4L2 event API, and\n> passed to users of the class through a signal.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/include/v4l2_videodevice.h | 8 +++\n> src/libcamera/v4l2_videodevice.cpp | 75 +++++++++++++++++++++++-\n> 2 files changed, 82 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> index ee63d28da734..a0409e59c08f 100644\n> --- a/src/libcamera/include/v4l2_videodevice.h\n> +++ b/src/libcamera/include/v4l2_videodevice.h\n> @@ -224,6 +224,9 @@ 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> @@ -262,6 +265,8 @@ 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> @@ -271,6 +276,9 @@ 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_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 18a71e4f8a7f..45aedb1a9490 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -544,7 +544,8 @@ 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: V4L2Device(deviceNode), cache_(nullptr), fdBufferNotifier_(nullptr),\n> +\t fdEventNotifier_(nullptr), frameStartEnabled_(false)\n> {\n> \t/*\n> \t * We default to an MMAP based CAPTURE video device, however this will\n> @@ -637,6 +638,10 @@ 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> @@ -726,6 +731,10 @@ 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> @@ -743,6 +752,7 @@ void V4L2VideoDevice::close()\n>\n> \treleaseBuffers();\n> \tdelete fdBufferNotifier_;\n> +\tdelete fdEventNotifier_;\n>\n> \tV4L2Device::close();\n> }\n> @@ -1570,11 +1580,74 @@ 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(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\nShould we stop emitting events if a spurious one is received ?\n\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\nto disable 'it' ?\n\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\nThere's a very tiny window here that could make us miss a signal.\nEnabling the notifier before instructing the video device to report\nevents would solve it. It will need handling the error path a bit more\ncarefully though.\n\nWith this considered\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\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\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 659E1603FA\n\tfor ;\n\tSun, 26 Apr 2020 15:22:39 +0200 (CEST)","from uno.localdomain\n\t(host170-48-dynamic.3-87-r.retail.telecomitalia.it [87.3.48.170])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id F039C60004;\n\tSun, 26 Apr 2020 13:22:35 +0000 (UTC)"],"X-Originating-IP":"87.3.48.170","Date":"Sun, 26 Apr 2020 15:25:22 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200426132522.tqbf6t2snabbg2f2@uno.localdomain>","References":"<20200426004309.2743-1-laurent.pinchart@ideasonboard.com>\n\t<20200426004309.2743-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200426004309.2743-2-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_videodevice: Add\n\tsupport for frame start events","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","X-List-Received-Date":"Sun, 26 Apr 2020 13:22:39 -0000"}},{"id":4539,"web_url":"https://patchwork.libcamera.org/comment/4539/","msgid":"<20200426173732.GG5950@pendragon.ideasonboard.com>","date":"2020-04-26T17:37:32","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_videodevice: Add\n\tsupport for frame start events","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Sun, Apr 26, 2020 at 03:25:22PM +0200, Jacopo Mondi wrote:\n> On Sun, Apr 26, 2020 at 03:43:09AM +0300, Laurent Pinchart wrote:\n> > Extend the V4L2VideoDevice to support notifying of frame start events.\n> > The events are received from the device through the V4L2 event API, and\n> > passed to users of the class through a signal.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/include/v4l2_videodevice.h | 8 +++\n> > src/libcamera/v4l2_videodevice.cpp | 75 +++++++++++++++++++++++-\n> > 2 files changed, 82 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> > index ee63d28da734..a0409e59c08f 100644\n> > --- a/src/libcamera/include/v4l2_videodevice.h\n> > +++ b/src/libcamera/include/v4l2_videodevice.h\n> > @@ -224,6 +224,9 @@ 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> > @@ -262,6 +265,8 @@ 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> > @@ -271,6 +276,9 @@ 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_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index 18a71e4f8a7f..45aedb1a9490 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -544,7 +544,8 @@ 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: V4L2Device(deviceNode), cache_(nullptr), fdBufferNotifier_(nullptr),\n> > +\t fdEventNotifier_(nullptr), frameStartEnabled_(false)\n> > {\n> > \t/*\n> > \t * We default to an MMAP based CAPTURE video device, however this will\n> > @@ -637,6 +638,10 @@ 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> > @@ -726,6 +731,10 @@ 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> > @@ -743,6 +752,7 @@ void V4L2VideoDevice::close()\n> >\n> > \treleaseBuffers();\n> > \tdelete fdBufferNotifier_;\n> > +\tdelete fdEventNotifier_;\n> >\n> > \tV4L2Device::close();\n> > }\n> > @@ -1570,11 +1580,74 @@ 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(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> \n> Should we stop emitting events if a spurious one is received ?\n\nThis should really never happen, and if it does, there's nothing that\nguarantees we'll only get one spurious event. For all we know there\ncould be a flood of them. I'd rather treat this as a hard error for that\nreason, until we realize there's a use case to still continue, and then\ndecide what the best recovery process is.\n\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> to disable 'it' ?\n\n\"them\" refers to \"events\", which is a plural.\n\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> There's a very tiny window here that could make us miss a signal.\n> Enabling the notifier before instructing the video device to report\n> events would solve it. It will need handling the error path a bit more\n> carefully though.\n\nAll the V4L2VideoDevice functions have to be called from the thread in\nwhich the V4L2VideoDevice object lives, so this won't be a problem in\npractice. The V4L2 fd will not be polled before control returns to the\nevent loop of the thread, which is after this function returns.\n\nThere could be use cases for relaxing this constraint, but we should\nthen also consider queueuBuffer() as a candidate for thread-safety (and\npossibly other functions), and solve it globally for V4L2VideoDevice.\n\n> With this considered\n> Reviewed-by: Jacopo Mondi \n> \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":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ACF0F62E55\n\tfor ;\n\tSun, 26 Apr 2020 19:37:47 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 218684F7;\n\tSun, 26 Apr 2020 19:37:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"lBISqWcc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587922667;\n\tbh=IYLh+0Y9QheyBx4vYK7WAQJcWF5TAoOQZmuIvIlh9Ys=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lBISqWccWXsBGkv70eyu89L9gcVR6vTmhm5ONduzhNtwH4tn2tPJJbUPaQ6VhW2D6\n\t0Y276y6Tjym4h+76iL/WDvuTB0NT7TBFLifll0EIXX2fSD8I05w8Yypx9K+Mz+b1eS\n\tjAI9iBmfS3hLo3hosOJBPNSstEursbZL58zIOW7s=","Date":"Sun, 26 Apr 2020 20:37:32 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200426173732.GG5950@pendragon.ideasonboard.com>","References":"<20200426004309.2743-1-laurent.pinchart@ideasonboard.com>\n\t<20200426004309.2743-2-laurent.pinchart@ideasonboard.com>\n\t<20200426132522.tqbf6t2snabbg2f2@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200426132522.tqbf6t2snabbg2f2@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_videodevice: Add\n\tsupport for frame start events","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","X-List-Received-Date":"Sun, 26 Apr 2020 17:37:47 -0000"}},{"id":4541,"web_url":"https://patchwork.libcamera.org/comment/4541/","msgid":"<20200426194002.thyzvjo7zoggx4le@uno.localdomain>","date":"2020-04-26T19:40:02","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_videodevice: Add\n\tsupport for frame start events","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sun, Apr 26, 2020 at 08:37:32PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Sun, Apr 26, 2020 at 03:25:22PM +0200, Jacopo Mondi wrote:\n> > On Sun, Apr 26, 2020 at 03:43:09AM +0300, Laurent Pinchart wrote:\n> > > Extend the V4L2VideoDevice to support notifying of frame start events.\n> > > The events are received from the device through the V4L2 event API, and\n> > > passed to users of the class through a signal.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/libcamera/include/v4l2_videodevice.h | 8 +++\n> > > src/libcamera/v4l2_videodevice.cpp | 75 +++++++++++++++++++++++-\n> > > 2 files changed, 82 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h\n> > > index ee63d28da734..a0409e59c08f 100644\n> > > --- a/src/libcamera/include/v4l2_videodevice.h\n> > > +++ b/src/libcamera/include/v4l2_videodevice.h\n> > > @@ -224,6 +224,9 @@ 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> > > @@ -262,6 +265,8 @@ 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> > > @@ -271,6 +276,9 @@ 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_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index 18a71e4f8a7f..45aedb1a9490 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -544,7 +544,8 @@ 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: V4L2Device(deviceNode), cache_(nullptr), fdBufferNotifier_(nullptr),\n> > > +\t fdEventNotifier_(nullptr), frameStartEnabled_(false)\n> > > {\n> > > \t/*\n> > > \t * We default to an MMAP based CAPTURE video device, however this will\n> > > @@ -637,6 +638,10 @@ 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> > > @@ -726,6 +731,10 @@ 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> > > @@ -743,6 +752,7 @@ void V4L2VideoDevice::close()\n> > >\n> > > \treleaseBuffers();\n> > > \tdelete fdBufferNotifier_;\n> > > +\tdelete fdEventNotifier_;\n> > >\n> > > \tV4L2Device::close();\n> > > }\n> > > @@ -1570,11 +1580,74 @@ 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(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> >\n> > Should we stop emitting events if a spurious one is received ?\n>\n> This should really never happen, and if it does, there's nothing that\n> guarantees we'll only get one spurious event. For all we know there\n> could be a flood of them. I'd rather treat this as a hard error for that\n> reason, until we realize there's a use case to still continue, and then\n> decide what the best recovery process is.\n>\n\nAck\n\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> > to disable 'it' ?\n>\n> \"them\" refers to \"events\", which is a plural.\n>\n\nOh! the final 'm' in them was under the column brake bar in my editor,\nI though the statement was left un-complete :)\n\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> > There's a very tiny window here that could make us miss a signal.\n> > Enabling the notifier before instructing the video device to report\n> > events would solve it. It will need handling the error path a bit more\n> > carefully though.\n>\n> All the V4L2VideoDevice functions have to be called from the thread in\n> which the V4L2VideoDevice object lives, so this won't be a problem in\n> practice. The V4L2 fd will not be polled before control returns to the\n> event loop of the thread, which is after this function returns.\n>\n> There could be use cases for relaxing this constraint, but we should\n> then also consider queueuBuffer() as a candidate for thread-safety (and\n> possibly other functions), and solve it globally for V4L2VideoDevice.\n>\n\nAck\n\n> > With this considered\n> > Reviewed-by: Jacopo Mondi \n> >\n\nThanks for the explanation\n\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\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C5B9962E55\n\tfor ;\n\tSun, 26 Apr 2020 21:36:57 +0200 (CEST)","from uno.localdomain (a-ur1-85.tin.it [212.216.150.148])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id D6EDA1BF208;\n\tSun, 26 Apr 2020 19:36:52 +0000 (UTC)"],"X-Originating-IP":"212.216.150.148","Date":"Sun, 26 Apr 2020 21:40:02 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200426194002.thyzvjo7zoggx4le@uno.localdomain>","References":"<20200426004309.2743-1-laurent.pinchart@ideasonboard.com>\n\t<20200426004309.2743-2-laurent.pinchart@ideasonboard.com>\n\t<20200426132522.tqbf6t2snabbg2f2@uno.localdomain>\n\t<20200426173732.GG5950@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200426173732.GG5950@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_videodevice: Add\n\tsupport for frame start events","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","X-List-Received-Date":"Sun, 26 Apr 2020 19:36:57 -0000"}}]