[libcamera-devel] [PATCH] libcamera: v4l2_device: Move start of frame detection to V4L2Device
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Oct 27 02:42:01 CET 2020
Hi Niklas,
Thank you for the patch.
On Sun, Oct 18, 2020 at 03:57:54AM +0200, Niklas Söderlund wrote:
> The V4L2_EVENT_FRAME_SYNC event may occur on both V4L2 video-devices
> (V4L2VideoDevice) and sub-devices (V4L2Subdevice). Move the start of
> frame detection to the common base class of the two, V4L2Device.
>
> There is no functional change.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> include/libcamera/internal/v4l2_device.h | 13 ++++
> include/libcamera/internal/v4l2_videodevice.h | 8 --
> src/libcamera/v4l2_device.cpp | 78 ++++++++++++++++++-
> src/libcamera/v4l2_videodevice.cpp | 75 +-----------------
> 4 files changed, 91 insertions(+), 83 deletions(-)
>
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index 722fb72207d74111..b0a9ed0a1d3557e6 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -13,11 +13,15 @@
>
> #include <linux/videodev2.h>
>
> +#include <libcamera/signal.h>
> +
> #include "libcamera/internal/log.h"
> #include "libcamera/internal/v4l2_controls.h"
>
> namespace libcamera {
>
> +class EventNotifier;
> +
> class V4L2Device : protected Loggable
> {
> public:
> @@ -34,6 +38,9 @@ public:
> const std::string &deviceNode() const { return deviceNode_; }
> std::string devicePath() const;
>
> + int setFrameStartEnabled(bool enable);
> + Signal<uint32_t> frameStart;
> +
> protected:
> V4L2Device(const std::string &deviceNode);
> ~V4L2Device();
> @@ -45,6 +52,8 @@ protected:
>
> int fd() { return fd_; }
>
> + void eventAvailable(EventNotifier *notifier);
Is there a reason why this is protected and not private ?
> +
> private:
> void listControls();
> void updateControls(ControlList *ctrls,
> @@ -56,6 +65,10 @@ private:
> ControlInfoMap controls_;
> std::string deviceNode_;
> int fd_;
> +
> + EventNotifier *fdEventNotifier_;
> +
I'd drop this blank line.
> + bool frameStartEnabled_;
> };
>
> } /* namespace libcamera */
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index 40ed87e17cfa6d3c..2fd09a1343566efa 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -204,9 +204,6 @@ public:
> int queueBuffer(FrameBuffer *buffer);
> Signal<FrameBuffer *> bufferReady;
>
> - int setFrameStartEnabled(bool enable);
> - Signal<uint32_t> frameStart;
> -
> int streamOn();
> int streamOff();
>
> @@ -240,8 +237,6 @@ private:
> void bufferAvailable(EventNotifier *notifier);
> FrameBuffer *dequeueBuffer();
>
> - void eventAvailable(EventNotifier *notifier);
> -
> V4L2Capability caps_;
>
> enum v4l2_buf_type bufferType_;
> @@ -251,9 +246,6 @@ private:
> std::map<unsigned int, FrameBuffer *> queuedBuffers_;
>
> EventNotifier *fdBufferNotifier_;
> - EventNotifier *fdEventNotifier_;
> -
> - bool frameStartEnabled_;
> };
>
> class V4L2M2MDevice
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 31d4dad0ee47b734..36a2c21e6544af22 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -16,6 +16,8 @@
> #include <sys/syscall.h>
> #include <unistd.h>
>
> +#include <libcamera/event_notifier.h>
> +
> #include "libcamera/internal/log.h"
> #include "libcamera/internal/sysfs.h"
> #include "libcamera/internal/utils.h"
> @@ -52,7 +54,8 @@ LOG_DEFINE_CATEGORY(V4L2)
> * at open() time, and the \a logTag to prefix log messages with.
> */
> V4L2Device::V4L2Device(const std::string &deviceNode)
> - : deviceNode_(deviceNode), fd_(-1)
> + : deviceNode_(deviceNode), fd_(-1), fdEventNotifier_(nullptr),
> + frameStartEnabled_(false)
> {
> }
>
> @@ -91,6 +94,10 @@ int V4L2Device::open(unsigned int flags)
>
> listControls();
>
> + fdEventNotifier_ = new EventNotifier(fd_, EventNotifier::Exception);
> + fdEventNotifier_->activated.connect(this, &V4L2Device::eventAvailable);
> + fdEventNotifier_->setEnabled(false);
You could alternatively call setFd() (replacing the fd_ = ret; line).
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> +
> return 0;
> }
>
> @@ -117,6 +124,10 @@ int V4L2Device::setFd(int fd)
>
> fd_ = fd;
>
> + fdEventNotifier_ = new EventNotifier(fd_, EventNotifier::Exception);
> + fdEventNotifier_->activated.connect(this, &V4L2Device::eventAvailable);
> + fdEventNotifier_->setEnabled(false);
> +
> return 0;
> }
>
> @@ -130,6 +141,8 @@ void V4L2Device::close()
> if (!isOpen())
> return;
>
> + delete fdEventNotifier_;
> +
> if (::close(fd_) < 0)
> LOG(V4L2, Error) << "Failed to close V4L2 device: "
> << strerror(errno);
> @@ -396,6 +409,40 @@ std::string V4L2Device::devicePath() const
> return path;
> }
>
> +/**
> + * \brief Enable or disable frame start event notification
> + * \param[in] enable True to enable frame start events, false to disable them
> + *
> + * 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 V4L2Device::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;
> +
> + fdEventNotifier_->setEnabled(enable);
> + frameStartEnabled_ = enable;
> +
> + return ret;
> +}
> +
> +/**
> + * \var V4L2Device::frameStart
> + * \brief A Signal emitted when capture of a frame has started
> + */
> +
> /**
> * \brief Perform an IOCTL system call on the device node
> * \param[in] request The IOCTL request code
> @@ -414,6 +461,35 @@ int V4L2Device::ioctl(unsigned long request, void *argp)
> return 0;
> }
>
> +/**
> + * \brief Slot to handle V4L2 events from the V4L2 device
> + * \param[in] notifier The event notifier
> + *
> + * When this slot is called, a V4L2 event is available to be dequeued from the
> + * device.
> + */
> +void V4L2Device::eventAvailable([[maybe_unused]] 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);
> + return;
> + }
> +
> + frameStart.emit(event.u.frame_sync.frame_sequence);
> +}
> +
> /**
> * \fn V4L2Device::deviceNode()
> * \brief Retrieve the device node path
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 508522ef42bb52cb..6e86849b0013c373 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -471,8 +471,7 @@ 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),
> - fdEventNotifier_(nullptr), frameStartEnabled_(false)
> + : V4L2Device(deviceNode), cache_(nullptr), fdBufferNotifier_(nullptr)
> {
> /*
> * We default to an MMAP based CAPTURE video device, however this will
> @@ -565,10 +564,6 @@ 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();
> @@ -658,10 +653,6 @@ 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();
> @@ -679,7 +670,6 @@ void V4L2VideoDevice::close()
>
> releaseBuffers();
> delete fdBufferNotifier_;
> - delete fdEventNotifier_;
>
> V4L2Device::close();
> }
> @@ -1532,74 +1522,11 @@ 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([[maybe_unused]] 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);
> - 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
> - *
> - * 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;
> -
> - 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
More information about the libcamera-devel
mailing list