[libcamera-devel] [PATCH 2/2] libcamera: v4l2_videodevice: Add support for frame start events

[Jacopo Mondi]

Hi Laurent,

On Sun, Apr 26, 2020 at 08:37:32PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Sun, Apr 26, 2020 at 03:25:22PM +0200, Jacopo Mondi wrote:
> > On Sun, Apr 26, 2020 at 03:43:09AM +0300, Laurent Pinchart wrote:
> > > Extend the V4L2VideoDevice to support notifying of frame start events.
> > > The events are received from the device through the V4L2 event API, and
> > > passed to users of the class through a signal.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > >  src/libcamera/include/v4l2_videodevice.h |  8 +++
> > >  src/libcamera/v4l2_videodevice.cpp       | 75 +++++++++++++++++++++++-
> > >  2 files changed, 82 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> > > index ee63d28da734..a0409e59c08f 100644
> > > --- a/src/libcamera/include/v4l2_videodevice.h
> > > +++ b/src/libcamera/include/v4l2_videodevice.h
> > > @@ -224,6 +224,9 @@ public:
> > >  	int queueBuffer(FrameBuffer *buffer);
> > >  	Signal<FrameBuffer *> bufferReady;
> > >
> > > +	int setFrameStartEnabled(bool enable);
> > > +	Signal<uint32_t> frameStart;
> > > +
> > >  	int streamOn();
> > >  	int streamOff();
> > >
> > > @@ -262,6 +265,8 @@ private:
> > >  	void bufferAvailable(EventNotifier *notifier);
> > >  	FrameBuffer *dequeueBuffer();
> > >
> > > +	void eventAvailable(EventNotifier *notifier);
> > > +
> > >  	V4L2Capability caps_;
> > >
> > >  	enum v4l2_buf_type bufferType_;
> > > @@ -271,6 +276,9 @@ private:
> > >  	std::map<unsigned int, FrameBuffer *> queuedBuffers_;
> > >
> > >  	EventNotifier *fdBufferNotifier_;
> > > +	EventNotifier *fdEventNotifier_;
> > > +
> > > +	bool frameStartEnabled_;
> > >  };
> > >
> > >  class V4L2M2MDevice
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index 18a71e4f8a7f..45aedb1a9490 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -544,7 +544,8 @@ const std::string V4L2DeviceFormat::toString() const
> > >   * \param[in] deviceNode The file-system path to the video device node
> > >   */
> > >  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> > > -	: V4L2Device(deviceNode), cache_(nullptr), fdBufferNotifier_(nullptr)
> > > +	: V4L2Device(deviceNode), cache_(nullptr), fdBufferNotifier_(nullptr),
> > > +	  fdEventNotifier_(nullptr), frameStartEnabled_(false)
> > >  {
> > >  	/*
> > >  	 * We default to an MMAP based CAPTURE video device, however this will
> > > @@ -637,6 +638,10 @@ int V4L2VideoDevice::open()
> > >  	fdBufferNotifier_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);
> > >  	fdBufferNotifier_->setEnabled(false);
> > >
> > > +	fdEventNotifier_ = new EventNotifier(fd(), EventNotifier::Exception);
> > > +	fdEventNotifier_->activated.connect(this, &V4L2VideoDevice::eventAvailable);
> > > +	fdEventNotifier_->setEnabled(false);
> > > +
> > >  	LOG(V4L2, Debug)
> > >  		<< "Opened device " << caps_.bus_info() << ": "
> > >  		<< caps_.driver() << ": " << caps_.card();
> > > @@ -726,6 +731,10 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)
> > >  	fdBufferNotifier_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);
> > >  	fdBufferNotifier_->setEnabled(false);
> > >
> > > +	fdEventNotifier_ = new EventNotifier(fd(), EventNotifier::Exception);
> > > +	fdEventNotifier_->activated.connect(this, &V4L2VideoDevice::eventAvailable);
> > > +	fdEventNotifier_->setEnabled(false);
> > > +
> > >  	LOG(V4L2, Debug)
> > >  		<< "Opened device " << caps_.bus_info() << ": "
> > >  		<< caps_.driver() << ": " << caps_.card();
> > > @@ -743,6 +752,7 @@ void V4L2VideoDevice::close()
> > >
> > >  	releaseBuffers();
> > >  	delete fdBufferNotifier_;
> > > +	delete fdEventNotifier_;
> > >
> > >  	V4L2Device::close();
> > >  }
> > > @@ -1570,11 +1580,74 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> > >  	return buffer;
> > >  }
> > >
> > > +/**
> > > + * \brief Slot to handle V4L2 events from the V4L2 video device
> > > + * \param[in] notifier The event notifier
> > > + *
> > > + * When this slot is called, a V4L2 event is available to be dequeued from the
> > > + * device.
> > > + */
> > > +void V4L2VideoDevice::eventAvailable(EventNotifier *notifier)
> > > +{
> > > +	struct v4l2_event event{};
> > > +	int ret = ioctl(VIDIOC_DQEVENT, &event);
> > > +	if (ret < 0) {
> > > +		LOG(V4L2, Error)
> > > +			<< "Failed to dequeue event, disabling event notifier";
> > > +		fdEventNotifier_->setEnabled(false);
> > > +		return;
> > > +	}
> > > +
> > > +	if (event.type != V4L2_EVENT_FRAME_SYNC) {
> > > +		LOG(V4L2, Error)
> > > +			<< "Spurious event (" << event.type
> > > +			<< "), disabling event notifier";
> > > +		fdEventNotifier_->setEnabled(false);
> >
> > Should we stop emitting events if a spurious one is received ?
>
> This should really never happen, and if it does, there's nothing that
> guarantees we'll only get one spurious event. For all we know there
> could be a flood of them. I'd rather treat this as a hard error for that
> reason, until we realize there's a use case to still continue, and then
> decide what the best recovery process is.
>

Ack

> > > +		return;
> > > +	}
> > > +
> > > +	frameStart.emit(event.u.frame_sync.frame_sequence);
> > > +}
> > > +
> > >  /**
> > >   * \var V4L2VideoDevice::bufferReady
> > >   * \brief A Signal emitted when a framebuffer completes
> > >   */
> > >
> > > +/**
> > > + * \brief Enable or disable frame start event notification
> > > + * \param[in] enable True to enable frame start events, false to disable them
> >
> > to disable 'it' ?
>
> "them" refers to "events", which is a plural.
>

Oh! the final 'm' in them was under the column brake bar in my editor,
I though the statement was left un-complete :)

> > > + *
> > > + * This function enables or disables generation of frame start events. Once
> > > + * enabled, the events are signalled through the frameStart signal.
> > > + *
> > > + * \return 0 on success, a negative error code otherwise
> > > + */
> > > +int V4L2VideoDevice::setFrameStartEnabled(bool enable)
> > > +{
> > > +	if (frameStartEnabled_ == enable)
> > > +		return 0;
> > > +
> > > +	struct v4l2_event_subscription event{};
> > > +	event.type = V4L2_EVENT_FRAME_SYNC;
> > > +
> > > +	unsigned long request = enable ? VIDIOC_SUBSCRIBE_EVENT
> > > +			      : VIDIOC_UNSUBSCRIBE_EVENT;
> > > +	int ret = ioctl(request, &event);
> > > +	if (enable && ret)
> > > +		return ret;
> >
> > There's a very tiny window here that could make us miss a signal.
> > Enabling the notifier before instructing the video device to report
> > events would solve it. It will need handling the error path a bit more
> > carefully though.
>
> All the V4L2VideoDevice functions have to be called from the thread in
> which the V4L2VideoDevice object lives, so this won't be a problem in
> practice. The V4L2 fd will not be polled before control returns to the
> event loop of the thread, which is after this function returns.
>
> There could be use cases for relaxing this constraint, but we should
> then also consider queueuBuffer() as a candidate for thread-safety (and
> possibly other functions), and solve it globally for V4L2VideoDevice.
>

Ack

> > With this considered
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >

Thanks for the explanation

> > > +
> > > +	fdEventNotifier_->setEnabled(enable);
> > > +	frameStartEnabled_ = enable;
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +/**
> > > + * \var V4L2VideoDevice::frameStart
> > > + * \brief A Signal emitted when capture of a frame has started
> > > + */
> > > +
> > >  /**
> > >   * \brief Start the video stream
> > >   * \return 0 on success or a negative error code otherwise
>
> --
> Regards,
>
> Laurent Pinchart