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

[Laurent Pinchart]

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.

> > +		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.

> > + *
> > + * 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.

> With this considered
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> > +
> > +	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