[libcamera-devel] [PATCH 01/21] qcam: Remove custom event dispatcher
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Mar 23 16:46:25 CET 2020
Hi Kieran,
On Mon, Mar 23, 2020 at 02:33:45PM +0000, Kieran Bingham wrote:
> On 23/03/2020 14:21, Laurent Pinchart wrote:
> > The qcam application installs a custom event dispatcher based on the Qt
> > event loop. As the camera manager now creates an internal thread, it
> > doesn't use that event dispatcher of the application thread at all.
> >
> > Furthermore, the custom event dispatcher is buggy, as it doesn't
> > dispatch messages posted to the main thread's event loop. This isn't an
> > issue as no messages are posted there in the first place, but would
> > cause incorrect behaviour if we were to use that feature (for instance
> > to deliver signals from the camera manager thread to the application
> > thread).
>
> (for topics such as hotplug notifications ...)
Or any topic. The buffer and request completion signals are already
delivered from the camera manager *thread*. Hotplug will (likely) be
delivered from the CameraManager *class*.
> > Fixing the event dispatcher requires a change in the libcamera public
> > API, as there's currently no way to dispatch messages using the public
> > API (Thread::dispatchMessages() is not exposed). This isn't worth it at
> > the moment, so just remove the custom event dispatcher. If qcam later
> > needs the libcamera request and buffer completion signals to be
> > delivered in the application thread, it will need to handle that
> > internally, using Qt's cross-thread signal delivery.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> Code removal is always good right? :-)
As long as it's not rm -rf :-)
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > ---
> > src/qcam/main.cpp | 3 -
> > src/qcam/meson.build | 1 -
> > src/qcam/qt_event_dispatcher.cpp | 152 -------------------------------
> > src/qcam/qt_event_dispatcher.h | 62 -------------
> > 4 files changed, 218 deletions(-)
> > delete mode 100644 src/qcam/qt_event_dispatcher.cpp
> > delete mode 100644 src/qcam/qt_event_dispatcher.h
> >
> > diff --git a/src/qcam/main.cpp b/src/qcam/main.cpp
> > index a7ff5c52663b..297453914ae9 100644
> > --- a/src/qcam/main.cpp
> > +++ b/src/qcam/main.cpp
> > @@ -15,7 +15,6 @@
> >
> > #include "main_window.h"
> > #include "../cam/options.h"
> > -#include "qt_event_dispatcher.h"
> >
> > void signalHandler(int signal)
> > {
> > @@ -62,9 +61,7 @@ int main(int argc, char **argv)
> > sa.sa_handler = &signalHandler;
> > sigaction(SIGINT, &sa, nullptr);
> >
> > - std::unique_ptr<EventDispatcher> dispatcher(new QtEventDispatcher());
> > CameraManager *cm = new CameraManager();
> > - cm->setEventDispatcher(std::move(dispatcher));
> >
> > ret = cm->start();
> > if (ret) {
> > diff --git a/src/qcam/meson.build b/src/qcam/meson.build
> > index 5150631b55c8..214bfb12aabb 100644
> > --- a/src/qcam/meson.build
> > +++ b/src/qcam/meson.build
> > @@ -3,7 +3,6 @@ qcam_sources = files([
> > 'main.cpp',
> > 'main_window.cpp',
> > '../cam/options.cpp',
> > - 'qt_event_dispatcher.cpp',
> > 'viewfinder.cpp',
> > ])
> >
> > diff --git a/src/qcam/qt_event_dispatcher.cpp b/src/qcam/qt_event_dispatcher.cpp
> > deleted file mode 100644
> > index 2780c9123ac3..000000000000
> > --- a/src/qcam/qt_event_dispatcher.cpp
> > +++ /dev/null
> > @@ -1,152 +0,0 @@
> > -/* SPDX-License-Identifier: GPL-2.0-or-later */
> > -/*
> > - * Copyright (C) 2019, Google Inc.
> > - *
> > - * qt_event_dispatcher.cpp - qcam - Qt-based event dispatcher
> > - */
> > -
> > -#include <chrono>
> > -#include <iostream>
> > -
> > -#include <QAbstractEventDispatcher>
> > -#include <QCoreApplication>
> > -#include <QSocketNotifier>
> > -#include <QTimerEvent>
> > -
> > -#include <libcamera/event_notifier.h>
> > -#include <libcamera/timer.h>
> > -
> > -#include "qt_event_dispatcher.h"
> > -
> > -using namespace libcamera;
> > -
> > -QtEventDispatcher::QtEventDispatcher()
> > -{
> > -}
> > -
> > -QtEventDispatcher::~QtEventDispatcher()
> > -{
> > - for (auto &it : notifiers_) {
> > - NotifierSet &set = it.second;
> > - delete set.read.qnotifier;
> > - delete set.write.qnotifier;
> > - delete set.exception.qnotifier;
> > - }
> > -}
> > -
> > -void QtEventDispatcher::registerEventNotifier(EventNotifier *notifier)
> > -{
> > - NotifierSet &set = notifiers_[notifier->fd()];
> > - QSocketNotifier::Type qtype;
> > - void (QtEventDispatcher::*method)(int);
> > - NotifierPair *pair;
> > -
> > - switch (notifier->type()) {
> > - case EventNotifier::Read:
> > - default:
> > - qtype = QSocketNotifier::Read;
> > - method = &QtEventDispatcher::readNotifierActivated;
> > - pair = &set.read;
> > - break;
> > -
> > - case EventNotifier::Write:
> > - qtype = QSocketNotifier::Write;
> > - method = &QtEventDispatcher::writeNotifierActivated;
> > - pair = &set.write;
> > - break;
> > -
> > - case EventNotifier::Exception:
> > - qtype = QSocketNotifier::Exception;
> > - method = &QtEventDispatcher::exceptionNotifierActivated;
> > - pair = &set.exception;
> > - break;
> > - }
> > -
> > - QSocketNotifier *qnotifier = new QSocketNotifier(notifier->fd(), qtype);
> > - connect(qnotifier, &QSocketNotifier::activated, this, method);
> > - pair->notifier = notifier;
> > - pair->qnotifier = qnotifier;
> > -}
> > -
> > -void QtEventDispatcher::unregisterEventNotifier(EventNotifier *notifier)
> > -{
> > - NotifierSet &set = notifiers_[notifier->fd()];
> > - NotifierPair *pair;
> > -
> > - switch (notifier->type()) {
> > - case EventNotifier::Read:
> > - default:
> > - pair = &set.read;
> > - break;
> > -
> > - case EventNotifier::Write:
> > - pair = &set.write;
> > - break;
> > -
> > - case EventNotifier::Exception:
> > - pair = &set.exception;
> > - break;
> > - }
> > -
> > - delete pair->qnotifier;
> > - pair->qnotifier = nullptr;
> > - pair->notifier = nullptr;
> > -}
> > -
> > -void QtEventDispatcher::readNotifierActivated(int socket)
> > -{
> > - EventNotifier *notifier = notifiers_[socket].read.notifier;
> > - notifier->activated.emit(notifier);
> > -}
> > -
> > -void QtEventDispatcher::writeNotifierActivated(int socket)
> > -{
> > - EventNotifier *notifier = notifiers_[socket].write.notifier;
> > - notifier->activated.emit(notifier);
> > -}
> > -
> > -void QtEventDispatcher::exceptionNotifierActivated(int socket)
> > -{
> > - EventNotifier *notifier = notifiers_[socket].exception.notifier;
> > - notifier->activated.emit(notifier);
> > -}
> > -
> > -void QtEventDispatcher::registerTimer(Timer *timer)
> > -{
> > - std::chrono::steady_clock::time_point now = std::chrono::steady_clock::now();
> > - std::chrono::steady_clock::duration duration = timer->deadline() - now;
> > - std::chrono::milliseconds msec =
> > - std::chrono::duration_cast<std::chrono::milliseconds>(duration);
> > - int timerId = startTimer(msec.count());
> > - timers_[timerId] = timer;
> > - timerIds_[timer] = timerId;
> > -}
> > -
> > -void QtEventDispatcher::unregisterTimer(Timer *timer)
> > -{
> > - auto it = timerIds_.find(timer);
> > - if (it == timerIds_.end())
> > - return;
> > -
> > - timers_.erase(it->second);
> > - killTimer(it->second);
> > - timerIds_.erase(it);
> > -}
> > -
> > -void QtEventDispatcher::timerEvent(QTimerEvent *event)
> > -{
> > - Timer *timer = timers_[event->timerId()];
> > - timer->stop();
> > - timer->timeout.emit(timer);
> > -}
> > -
> > -void QtEventDispatcher::processEvents()
> > -{
> > - std::cout << "QtEventDispatcher::processEvents() should not be called"
> > - << std::endl;
> > -}
> > -
> > -void QtEventDispatcher::interrupt()
> > -{
> > - QCoreApplication::eventDispatcher()->interrupt();
> > -}
> > diff --git a/src/qcam/qt_event_dispatcher.h b/src/qcam/qt_event_dispatcher.h
> > deleted file mode 100644
> > index b0f123e52d06..000000000000
> > --- a/src/qcam/qt_event_dispatcher.h
> > +++ /dev/null
> > @@ -1,62 +0,0 @@
> > -/* SPDX-License-Identifier: GPL-2.0-or-later */
> > -/*
> > - * Copyright (C) 2019, Google Inc.
> > - *
> > - * qt_event_dispatcher.h - qcam - Qt-based event dispatcher
> > - */
> > -#ifndef __QCAM_QT_EVENT_DISPATCHER_H__
> > -#define __QCAM_QT_EVENT_DISPATCHER_H__
> > -
> > -#include <map>
> > -
> > -#include <libcamera/event_dispatcher.h>
> > -
> > -using namespace libcamera;
> > -
> > -class QSocketNotifier;
> > -
> > -class QtEventDispatcher final : public EventDispatcher, public QObject
> > -{
> > -public:
> > - QtEventDispatcher();
> > - ~QtEventDispatcher();
> > -
> > - void registerEventNotifier(EventNotifier *notifier);
> > - void unregisterEventNotifier(EventNotifier *notifier);
> > -
> > - void registerTimer(Timer *timer);
> > - void unregisterTimer(Timer *timer);
> > -
> > - void processEvents();
> > -
> > - void interrupt();
> > -
> > -protected:
> > - void timerEvent(QTimerEvent *event);
> > -
> > -private:
> > - void readNotifierActivated(int socket);
> > - void writeNotifierActivated(int socket);
> > - void exceptionNotifierActivated(int socket);
> > -
> > - struct NotifierPair {
> > - NotifierPair()
> > - : notifier(nullptr), qnotifier(nullptr)
> > - {
> > - }
> > - EventNotifier *notifier;
> > - QSocketNotifier *qnotifier;
> > - };
> > -
> > - struct NotifierSet {
> > - NotifierPair read;
> > - NotifierPair write;
> > - NotifierPair exception;
> > - };
> > -
> > - std::map<int, NotifierSet> notifiers_;
> > - std::map<int, Timer *> timers_;
> > - std::map<Timer *, int> timerIds_;
> > -};
> > -
> > -#endif /* __QCAM_QT_EVENT_DISPATCHER_H__ */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list