[libcamera-devel] [PATCH v1 6/6] libcamera: Drop emitter object pointer from signal arguments
Umang Jain
umang.jain at ideasonboard.com
Mon Aug 30 14:05:37 CEST 2021
Hi Laurent,
On 8/27/21 8:08 AM, Laurent Pinchart wrote:
> Many signals used in internal and public APIs carry the emitter pointer
> as a signal argument. This was done to allow slots connected to multiple
> signal instances to differentiate between emitters. While starting from
> a good intention of facilitating the implementation of slots, it turned
> out to be a bad API design as the signal isn't meant to know what it
> will be connected to, and thus shouldn't carry parameters that are
> solely meant to support a use case specific to the connected slot.
>
> These pointers turn out to be unused in all slots but one. In the only
> case where it is needed, it can be obtained by wrapping the slot in a
> lambda function when connecting the signal. Do so, and drop the emitter
> pointer from all signals.
hmm, When you said, the emitter object pointer is not required during
signal emission, i initially thought, it's not required to be explicitly
to be mentioned the argument list - during the signal emission for e.g.
class XYZ:
....
Signal<XYZ *, Arg A, Arg B> sig;
becomes
class XYZ:
....
Signal<Arg A, Arg B> sig;
But the slot essentially remained unchanged for both cases i.e. for
every signal, the passing of emitter pointer can be automatic/implicit to:
signalSlot(XYZ *, Arg A, Arg B)
{}
Well, you are right about how the pointers mostly turned out to be
unused, so I am not opposed with series as such. I suspect some cases
will still pop up, where we do need to emitter pointer in the slot, but
as I see below, we can address those with a lamda func.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
> include/libcamera/base/event_notifier.h | 2 +-
> include/libcamera/base/thread.h | 2 +-
> include/libcamera/base/timer.h | 2 +-
> include/libcamera/camera.h | 2 +-
> include/libcamera/internal/device_enumerator_udev.h | 2 +-
> include/libcamera/internal/ipc_pipe_unixsocket.h | 2 +-
> include/libcamera/internal/ipc_unixsocket.h | 4 ++--
> include/libcamera/internal/media_device.h | 2 +-
> include/libcamera/internal/process.h | 4 ++--
> include/libcamera/internal/v4l2_device.h | 2 +-
> include/libcamera/internal/v4l2_videodevice.h | 2 +-
> src/libcamera/base/event_dispatcher_poll.cpp | 4 ++--
> src/libcamera/base/thread.cpp | 2 +-
> src/libcamera/camera.cpp | 2 +-
> src/libcamera/device_enumerator.cpp | 2 +-
> src/libcamera/device_enumerator_udev.cpp | 2 +-
> src/libcamera/ipc_pipe_unixsocket.cpp | 2 +-
> src/libcamera/ipc_unixsocket.cpp | 4 ++--
> src/libcamera/pipeline_handler.cpp | 2 +-
> src/libcamera/process.cpp | 4 ++--
> src/libcamera/v4l2_device.cpp | 3 +--
> src/libcamera/v4l2_videodevice.cpp | 3 +--
> test/event-thread.cpp | 2 +-
> test/event.cpp | 2 +-
> test/ipa/ipa_interface_test.cpp | 2 +-
> test/ipc/unixsocket.cpp | 4 ++--
> test/ipc/unixsocket_ipc.cpp | 2 +-
> test/log/log_process.cpp | 3 +--
> test/process/process_test.cpp | 3 +--
> test/timer-thread.cpp | 2 +-
> test/timer.cpp | 2 +-
> .../libcamera_templates/module_ipa_proxy_worker.cpp.tmpl | 2 +-
> 32 files changed, 38 insertions(+), 42 deletions(-)
>
> diff --git a/include/libcamera/base/event_notifier.h b/include/libcamera/base/event_notifier.h
> index 5055ccbf21ca..f7722a32ef55 100644
> --- a/include/libcamera/base/event_notifier.h
> +++ b/include/libcamera/base/event_notifier.h
> @@ -34,7 +34,7 @@ public:
> bool enabled() const { return enabled_; }
> void setEnabled(bool enable);
>
> - Signal<EventNotifier *> activated;
> + Signal<> activated;
>
> protected:
> void message(Message *msg) override;
> diff --git a/include/libcamera/base/thread.h b/include/libcamera/base/thread.h
> index 762beab2a360..e0ca0aeaa761 100644
> --- a/include/libcamera/base/thread.h
> +++ b/include/libcamera/base/thread.h
> @@ -41,7 +41,7 @@ public:
>
> bool isRunning();
>
> - Signal<Thread *> finished;
> + Signal<> finished;
>
> static Thread *current();
> static pid_t currentId();
> diff --git a/include/libcamera/base/timer.h b/include/libcamera/base/timer.h
> index 798821611c6c..44876a85dc0a 100644
> --- a/include/libcamera/base/timer.h
> +++ b/include/libcamera/base/timer.h
> @@ -33,7 +33,7 @@ public:
>
> std::chrono::steady_clock::time_point deadline() const { return deadline_; }
>
> - Signal<Timer *> timeout;
> + Signal<> timeout;
>
> protected:
> void message(Message *msg) override;
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 05cdab724b10..601ee46e415b 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -86,7 +86,7 @@ public:
>
> Signal<Request *, FrameBuffer *> bufferCompleted;
> Signal<Request *> requestCompleted;
> - Signal<Camera *> disconnected;
> + Signal<> disconnected;
>
> int acquire();
> int release();
> diff --git a/include/libcamera/internal/device_enumerator_udev.h b/include/libcamera/internal/device_enumerator_udev.h
> index 58e64a297b1d..c035298081b4 100644
> --- a/include/libcamera/internal/device_enumerator_udev.h
> +++ b/include/libcamera/internal/device_enumerator_udev.h
> @@ -59,7 +59,7 @@ private:
> std::string lookupDeviceNode(dev_t devnum);
>
> int addV4L2Device(dev_t devnum);
> - void udevNotify(EventNotifier *notifier);
> + void udevNotify();
>
> struct udev *udev_;
> struct udev_monitor *monitor_;
> diff --git a/include/libcamera/internal/ipc_pipe_unixsocket.h b/include/libcamera/internal/ipc_pipe_unixsocket.h
> index 4ffdddcc7f92..ad2927fed7f0 100644
> --- a/include/libcamera/internal/ipc_pipe_unixsocket.h
> +++ b/include/libcamera/internal/ipc_pipe_unixsocket.h
> @@ -35,7 +35,7 @@ private:
> bool done;
> };
>
> - void readyRead(IPCUnixSocket *socket);
> + void readyRead();
> int call(const IPCUnixSocket::Payload &message,
> IPCUnixSocket::Payload *response, uint32_t seq);
>
> diff --git a/include/libcamera/internal/ipc_unixsocket.h b/include/libcamera/internal/ipc_unixsocket.h
> index 9f5b06773ced..2b87196c4851 100644
> --- a/include/libcamera/internal/ipc_unixsocket.h
> +++ b/include/libcamera/internal/ipc_unixsocket.h
> @@ -37,7 +37,7 @@ public:
> int send(const Payload &payload);
> int receive(Payload *payload);
>
> - Signal<IPCUnixSocket *> readyRead;
> + Signal<> readyRead;
>
> private:
> struct Header {
> @@ -48,7 +48,7 @@ private:
> int sendData(const void *buffer, size_t length, const int32_t *fds, unsigned int num);
> int recvData(void *buffer, size_t length, int32_t *fds, unsigned int num);
>
> - void dataNotifier(EventNotifier *notifier);
> + void dataNotifier();
>
> int fd_;
> bool headerReceived_;
> diff --git a/include/libcamera/internal/media_device.h b/include/libcamera/internal/media_device.h
> index 3a7722c2a215..1f2304c19281 100644
> --- a/include/libcamera/internal/media_device.h
> +++ b/include/libcamera/internal/media_device.h
> @@ -53,7 +53,7 @@ public:
> MediaLink *link(const MediaPad *source, const MediaPad *sink);
> int disableLinks();
>
> - Signal<MediaDevice *> disconnected;
> + Signal<> disconnected;
>
> protected:
> std::string logPrefix() const override;
> diff --git a/include/libcamera/internal/process.h b/include/libcamera/internal/process.h
> index c4d5d9c1c009..300e0521eb03 100644
> --- a/include/libcamera/internal/process.h
> +++ b/include/libcamera/internal/process.h
> @@ -38,7 +38,7 @@ public:
>
> void kill();
>
> - Signal<Process *, enum ExitStatus, int> finished;
> + Signal<enum ExitStatus, int> finished;
>
> private:
> void closeAllFdsExcept(const std::vector<int> &fds);
> @@ -70,7 +70,7 @@ public:
> private:
> static ProcessManager *self_;
>
> - void sighandler(EventNotifier *notifier);
> + void sighandler();
>
> std::list<Process *> processes_;
>
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index 423c8fb11845..f21bc3701ca0 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -65,7 +65,7 @@ private:
> void updateControls(ControlList *ctrls,
> Span<const v4l2_ext_control> v4l2Ctrls);
>
> - void eventAvailable(EventNotifier *notifier);
> + void eventAvailable();
>
> std::map<unsigned int, struct v4l2_query_ext_ctrl> controlInfo_;
> std::vector<std::unique_ptr<ControlId>> controlIds_;
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index e767ec84c4da..400d4490d016 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -238,7 +238,7 @@ private:
> std::unique_ptr<FrameBuffer> createBuffer(unsigned int index);
> FileDescriptor exportDmabufFd(unsigned int index, unsigned int plane);
>
> - void bufferAvailable(EventNotifier *notifier);
> + void bufferAvailable();
> FrameBuffer *dequeueBuffer();
>
> V4L2Capability caps_;
> diff --git a/src/libcamera/base/event_dispatcher_poll.cpp b/src/libcamera/base/event_dispatcher_poll.cpp
> index 4f22f5793bb9..3c9a126c0bd6 100644
> --- a/src/libcamera/base/event_dispatcher_poll.cpp
> +++ b/src/libcamera/base/event_dispatcher_poll.cpp
> @@ -278,7 +278,7 @@ void EventDispatcherPoll::processNotifiers(const std::vector<struct pollfd> &pol
> }
>
> if (pfd.revents & event.events)
> - notifier->activated.emit(notifier);
> + notifier->activated.emit();
> }
>
> /* Erase the notifiers_ entry if it is now empty. */
> @@ -300,7 +300,7 @@ void EventDispatcherPoll::processTimers()
>
> timers_.pop_front();
> timer->stop();
> - timer->timeout.emit(timer);
> + timer->timeout.emit();
> }
> }
>
> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp
> index bd7b73911d12..d0ca30e3d522 100644
> --- a/src/libcamera/base/thread.cpp
> +++ b/src/libcamera/base/thread.cpp
> @@ -384,7 +384,7 @@ void Thread::finishThread()
> data_->running_ = false;
> data_->mutex_.unlock();
>
> - finished.emit(this);
> + finished.emit();
> data_->cv_.notify_all();
> }
>
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index c20e05014fb9..b5849d4d401a 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -688,7 +688,7 @@ void Camera::disconnect()
> LOG(Camera, Debug) << "Disconnecting camera " << id();
>
> _d()->disconnect();
> - disconnected.emit(this);
> + disconnected.emit();
> }
>
> int Camera::exportFrameBuffers(Stream *stream,
> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp
> index ec59927eaf34..d12580505303 100644
> --- a/src/libcamera/device_enumerator.cpp
> +++ b/src/libcamera/device_enumerator.cpp
> @@ -288,7 +288,7 @@ void DeviceEnumerator::removeDevice(const std::string &deviceNode)
> LOG(DeviceEnumerator, Debug)
> << "Media device for node " << deviceNode << " removed.";
>
> - media->disconnected.emit(media.get());
> + media->disconnected.emit();
> }
>
> /**
> diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp
> index 37a2c5aa55db..5317afbd77ce 100644
> --- a/src/libcamera/device_enumerator_udev.cpp
> +++ b/src/libcamera/device_enumerator_udev.cpp
> @@ -327,7 +327,7 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum)
> return 0;
> }
>
> -void DeviceEnumeratorUdev::udevNotify([[maybe_unused]] EventNotifier *notifier)
> +void DeviceEnumeratorUdev::udevNotify()
> {
> struct udev_device *dev = udev_monitor_receive_device(monitor_);
> std::string action(udev_device_get_action(dev));
> diff --git a/src/libcamera/ipc_pipe_unixsocket.cpp b/src/libcamera/ipc_pipe_unixsocket.cpp
> index 38bcc30a21ed..533560cf95d3 100644
> --- a/src/libcamera/ipc_pipe_unixsocket.cpp
> +++ b/src/libcamera/ipc_pipe_unixsocket.cpp
> @@ -82,7 +82,7 @@ int IPCPipeUnixSocket::sendAsync(const IPCMessage &data)
> return 0;
> }
>
> -void IPCPipeUnixSocket::readyRead([[maybe_unused]] IPCUnixSocket *socket)
> +void IPCPipeUnixSocket::readyRead()
> {
> IPCUnixSocket::Payload payload;
> int ret = socket_->receive(&payload);
> diff --git a/src/libcamera/ipc_unixsocket.cpp b/src/libcamera/ipc_unixsocket.cpp
> index 7188cf29e56a..bd32fca3a678 100644
> --- a/src/libcamera/ipc_unixsocket.cpp
> +++ b/src/libcamera/ipc_unixsocket.cpp
> @@ -311,7 +311,7 @@ int IPCUnixSocket::recvData(void *buffer, size_t length,
> return 0;
> }
>
> -void IPCUnixSocket::dataNotifier([[maybe_unused]] EventNotifier *notifier)
> +void IPCUnixSocket::dataNotifier()
> {
> int ret;
>
> @@ -342,7 +342,7 @@ void IPCUnixSocket::dataNotifier([[maybe_unused]] EventNotifier *notifier)
> return;
>
> notifier_->setEnabled(false);
> - readyRead.emit(this);
> + readyRead.emit();
> }
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 597d4c6a578a..f69c4f03b80f 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -448,7 +448,7 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera)
> */
> void PipelineHandler::hotplugMediaDevice(MediaDevice *media)
> {
> - media->disconnected.connect(this, &PipelineHandler::mediaDeviceDisconnected);
> + media->disconnected.connect(this, [=]() { mediaDeviceDisconnected(media); });
> }
>
> /**
> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
> index 998d08c2d88a..eca1b30039b8 100644
> --- a/src/libcamera/process.cpp
> +++ b/src/libcamera/process.cpp
> @@ -66,7 +66,7 @@ void sigact(int signal, siginfo_t *info, void *ucontext)
>
> } /* namespace */
>
> -void ProcessManager::sighandler([[maybe_unused]] EventNotifier *notifier)
> +void ProcessManager::sighandler()
> {
> char data;
> ssize_t ret = read(pipe_[0], &data, sizeof(data));
> @@ -326,7 +326,7 @@ void Process::died(int wstatus)
> exitStatus_ = WIFEXITED(wstatus) ? NormalExit : SignalExit;
> exitCode_ = exitStatus_ == NormalExit ? WEXITSTATUS(wstatus) : -1;
>
> - finished.emit(this, exitStatus_, exitCode_);
> + finished.emit(exitStatus_, exitCode_);
> }
>
> /**
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 951592c698f7..9c783c9cbed1 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -705,12 +705,11 @@ void V4L2Device::updateControls(ControlList *ctrls,
>
> /**
> * \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)
> +void V4L2Device::eventAvailable()
> {
> struct v4l2_event event{};
> int ret = ioctl(VIDIOC_DQEVENT, &event);
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index ce60dff6cdfd..1684d0e9c8dd 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1467,7 +1467,6 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
>
> /**
> * \brief Slot to handle completed buffer events from the V4L2 video device
> - * \param[in] notifier The event notifier
> *
> * When this slot is called, a Buffer has become available from the device, and
> * will be emitted through the bufferReady Signal.
> @@ -1475,7 +1474,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> * For Capture video devices the FrameBuffer will contain valid data.
> * For Output video devices the FrameBuffer can be considered empty.
> */
> -void V4L2VideoDevice::bufferAvailable([[maybe_unused]] EventNotifier *notifier)
> +void V4L2VideoDevice::bufferAvailable()
> {
> FrameBuffer *buffer = dequeueBuffer();
> if (!buffer)
> diff --git a/test/event-thread.cpp b/test/event-thread.cpp
> index 12021710ef41..ef8a52c3de55 100644
> --- a/test/event-thread.cpp
> +++ b/test/event-thread.cpp
> @@ -66,7 +66,7 @@ public:
> }
>
> private:
> - void readReady([[maybe_unused]] EventNotifier *notifier)
> + void readReady()
> {
> size_ = read(notifier_->fd(), data_, sizeof(data_));
> notified_ = true;
> diff --git a/test/event.cpp b/test/event.cpp
> index e338335c11e8..d4765eb14d12 100644
> --- a/test/event.cpp
> +++ b/test/event.cpp
> @@ -22,7 +22,7 @@ using namespace libcamera;
> class EventTest : public Test
> {
> protected:
> - void readReady([[maybe_unused]] EventNotifier *notifier)
> + void readReady()
> {
> size_ = read(notifier_->fd(), data_, sizeof(data_));
> notified_ = true;
> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> index 0ee51f71fd87..43562e608506 100644
> --- a/test/ipa/ipa_interface_test.cpp
> +++ b/test/ipa/ipa_interface_test.cpp
> @@ -153,7 +153,7 @@ protected:
> }
>
> private:
> - void readTrace([[maybe_unused]] EventNotifier *notifier)
> + void readTrace()
> {
> ssize_t s = read(notifier_->fd(), &trace_, sizeof(trace_));
> if (s < 0) {
> diff --git a/test/ipc/unixsocket.cpp b/test/ipc/unixsocket.cpp
> index 6507fb12d74b..7270bf4d2fe7 100644
> --- a/test/ipc/unixsocket.cpp
> +++ b/test/ipc/unixsocket.cpp
> @@ -68,7 +68,7 @@ public:
> }
>
> private:
> - void readyRead([[maybe_unused]] IPCUnixSocket *ipc)
> + void readyRead()
> {
> IPCUnixSocket::Payload message, response;
> int ret;
> @@ -447,7 +447,7 @@ private:
> return 0;
> }
>
> - void readyRead([[maybe_unused]] IPCUnixSocket *ipc)
> + void readyRead()
> {
> if (!callResponse_) {
> cerr << "Read ready without expecting data, fail." << endl;
> diff --git a/test/ipc/unixsocket_ipc.cpp b/test/ipc/unixsocket_ipc.cpp
> index 60317a4956b8..ab5d25572d83 100644
> --- a/test/ipc/unixsocket_ipc.cpp
> +++ b/test/ipc/unixsocket_ipc.cpp
> @@ -65,7 +65,7 @@ public:
> }
>
> private:
> - void readyRead([[maybe_unused]] IPCUnixSocket *ipc)
> + void readyRead()
> {
> IPCUnixSocket::Payload message;
> int ret;
> diff --git a/test/log/log_process.cpp b/test/log/log_process.cpp
> index d138aa7ff562..a56a399848a7 100644
> --- a/test/log/log_process.cpp
> +++ b/test/log/log_process.cpp
> @@ -126,8 +126,7 @@ protected:
> }
>
> private:
> - void procFinished([[maybe_unused]] Process *proc,
> - enum Process::ExitStatus exitStatus, int exitCode)
> + void procFinished(enum Process::ExitStatus exitStatus, int exitCode)
> {
> exitStatus_ = exitStatus;
> exitCode_ = exitCode;
> diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp
> index 8f7a1f05f681..378d680bf4ef 100644
> --- a/test/process/process_test.cpp
> +++ b/test/process/process_test.cpp
> @@ -81,8 +81,7 @@ protected:
> }
>
> private:
> - void procFinished([[maybe_unused]] Process *proc,
> - enum Process::ExitStatus exitStatus, int exitCode)
> + void procFinished(enum Process::ExitStatus exitStatus, int exitCode)
> {
> exitStatus_ = exitStatus;
> exitCode_ = exitCode;
> diff --git a/test/timer-thread.cpp b/test/timer-thread.cpp
> index 2c14865b74d5..f7e8743da9e6 100644
> --- a/test/timer-thread.cpp
> +++ b/test/timer-thread.cpp
> @@ -39,7 +39,7 @@ public:
> }
>
> private:
> - void timeoutHandler([[maybe_unused]] Timer *timer)
> + void timeoutHandler()
> {
> timeout_ = true;
> }
> diff --git a/test/timer.cpp b/test/timer.cpp
> index 88f226e79f5f..be79d0100a58 100644
> --- a/test/timer.cpp
> +++ b/test/timer.cpp
> @@ -56,7 +56,7 @@ public:
> }
>
> private:
> - void timeoutHandler([[maybe_unused]] Timer *timer)
> + void timeoutHandler()
> {
> expiration_ = std::chrono::steady_clock::now();
> count_++;
> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> index b4cd1aa9e823..c54ecdb90a1a 100644
> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy_worker.cpp.tmpl
> @@ -57,7 +57,7 @@ public:
>
> ~{{proxy_worker_name}}() {}
>
> - void readyRead([[maybe_unused]] IPCUnixSocket *socket)
> + void readyRead()
> {
> IPCUnixSocket::Payload _message;
> int _retRecv = socket_.receive(&_message);
More information about the libcamera-devel
mailing list