[libcamera-devel,v5,03/13] py: New event handling

18688 diff mbox series

At the moment the Python bindings only handle the requestCompleted
events. But we have others, bufferCompleted and disconnect from the
Camera, and cameraAdded and cameraRemoved from the CameraManager.

This patch makes all those events available to the users.

The get_ready_requests() method is now deprecated, but available to keep
backward compatibility.

The new event handling works like get_ready_requests(), but instead of
returning only Requests from requestCompleted events, it returns all
event types using a new Event class.

Additionally camera.stop() has been changed to return events for that
specific camera. This serves two purposes: first, it removes the
RequestCompleted and BufferCompleted events from the event queue, thus
preventing the old events being returned when the camera is started
again, and second, it allows the user to process those events if it so
wants.

The CameraManager events (CameraAdded and CameraRemoved) are always
enabled, but of the Camera events only RequestCompleted is enabled by
default. Other Camera events can be enabled
Camera.enable_camera_event().

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 Documentation/python-bindings.rst      |  24 ++-
 src/py/libcamera/py_camera_manager.cpp | 275 +++++++++++++++++++++++--
 src/py/libcamera/py_camera_manager.h   |  73 ++++++-
 src/py/libcamera/py_main.cpp           |  54 ++++-
 4 files changed, 386 insertions(+), 40 deletions(-)

Hi Tomi,

Thank you for the patch.

On Sat, Jun 03, 2023 at 10:56:05AM +0300, Tomi Valkeinen wrote:
> At the moment the Python bindings only handle the requestCompleted
> events. But we have others, bufferCompleted and disconnect from the
> Camera, and cameraAdded and cameraRemoved from the CameraManager.
> 
> This patch makes all those events available to the users.
> 
> The get_ready_requests() method is now deprecated, but available to keep
> backward compatibility.
> 
> The new event handling works like get_ready_requests(), but instead of
> returning only Requests from requestCompleted events, it returns all
> event types using a new Event class.
> 
> Additionally camera.stop() has been changed to return events for that
> specific camera. This serves two purposes: first, it removes the
> RequestCompleted and BufferCompleted events from the event queue, thus
> preventing the old events being returned when the camera is started
> again, and second, it allows the user to process those events if it so
> wants.

Returning events from stop() is a bit of a weird API, and I'm a bit
worried it will confuse users, but I don't have any better option to
propose. I think it's good enough for now.

> The CameraManager events (CameraAdded and CameraRemoved) are always
> enabled, but of the Camera events only RequestCompleted is enabled by
> default. Other Camera events can be enabled
> Camera.enable_camera_event().
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---

Note for the future, per-patch changelogs would be useful.

>  Documentation/python-bindings.rst      |  24 ++-
>  src/py/libcamera/py_camera_manager.cpp | 275 +++++++++++++++++++++++--
>  src/py/libcamera/py_camera_manager.h   |  73 ++++++-
>  src/py/libcamera/py_main.cpp           |  54 ++++-
>  4 files changed, 386 insertions(+), 40 deletions(-)
> 
> diff --git a/Documentation/python-bindings.rst b/Documentation/python-bindings.rst
> index bac5cd34..8f2d76c3 100644
> --- a/Documentation/python-bindings.rst
> +++ b/Documentation/python-bindings.rst
> @@ -43,17 +43,23 @@ CameraManager
>  The Python API provides a singleton CameraManager via ``CameraManager.singleton()``.
>  There is no need to start or stop the CameraManager.
>  
> -Handling Completed Requests
> ----------------------------
> +Event Handling
> +--------------
>  
> -The Python bindings do not expose the ``Camera::requestCompleted`` signal
> -directly as the signal is invoked from another thread and it has real-time
> -constraints. Instead the bindings queue the completed requests internally and
> -use an eventfd to inform the user that there are completed requests.
> +The Python bindings do not expose the signals from the C++ side directly as the
> +signals are invoked from another thread and they may have real-time
> +constraints. Instead the bindings queue the received events internally and use
> +an eventfd to inform the user that there are events to be handled.
>  
> -The user can wait on the eventfd, and upon getting an event, use
> -``CameraManager.get_ready_requests()`` to clear the eventfd event and to get
> -the completed requests.
> +The user can wait on the eventfd (e.g. by using Python Selector), and use
> +``CameraManager.get_events()`` to reset the eventfd and get the events.
> +
> +The CameraManager events (CameraAdded and CameraRemoved) are always enabled, but
> +of the Camera events only RequestCompleted is enabled by default. To enable
> +Disconnect or BufferCompleted event, use ``Camera.enable_camera_event()``.
> +
> +The ``Camera.stop()`` method will return all events related to that Camera from
> +the event queue.
>  
>  Controls & Properties
>  ---------------------
> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp
> index 9ccb7aad..83c2b063 100644
> --- a/src/py/libcamera/py_camera_manager.cpp
> +++ b/src/py/libcamera/py_camera_manager.cpp
> @@ -5,6 +5,7 @@
>  
>  #include "py_camera_manager.h"
>  
> +#include <algorithm>
>  #include <errno.h>
>  #include <memory>
>  #include <sys/eventfd.h>
> @@ -35,11 +36,24 @@ PyCameraManager::PyCameraManager()
>  	if (ret)
>  		throw std::system_error(-ret, std::generic_category(),
>  					"Failed to start CameraManager");
> +
> +	cameraManager_->cameraAdded.connect(this, &PyCameraManager::handleCameraAdded);
> +	cameraManager_->cameraRemoved.connect(this, &PyCameraManager::handleCameraRemoved);
>  }
>  
>  PyCameraManager::~PyCameraManager()
>  {
>  	LOG(Python, Debug) << "~PyCameraManager()";
> +
> +	cameraManager_->cameraAdded.disconnect();
> +	cameraManager_->cameraRemoved.disconnect();
> +}
> +
> +void PyCameraManager::init()
> +{
> +	/* Always enable RequestCompleted for all cameras by default */
> +	for (auto cam : cameraManager_->cameras())
> +		setCameraEventFlag(cam, CameraEventType::RequestCompleted, true);

You need to also enable the event when a new camera is detected.

>  }
>  
>  py::list PyCameraManager::cameras()
> @@ -60,6 +74,43 @@ py::list PyCameraManager::cameras()
>  	return l;
>  }
>  
> +PyCameraEvent PyCameraManager::convertEvent(const CameraEvent &event)
> +{
> +	/*
> +	 * We need to set a keep-alive here so that the camera keeps the
> +	 * camera manager alive.
> +	 */
> +	py::object py_cm = py::cast(this);
> +	py::object py_cam = py::cast(event.camera_);
> +	py::detail::keep_alive_impl(py_cam, py_cm);
> +
> +	PyCameraEvent pyevent(event.type_, py_cam);
> +
> +	switch (event.type_) {
> +	case CameraEventType::CameraAdded:
> +	case CameraEventType::CameraRemoved:
> +	case CameraEventType::Disconnect:
> +		/* No additional parameters to add */
> +		break;
> +
> +	case CameraEventType::BufferCompleted:
> +		pyevent.request_ = py::cast(event.request_);
> +		pyevent.fb_ = py::cast(event.fb_);
> +		break;
> +
> +	case CameraEventType::RequestCompleted:
> +		pyevent.request_ = py::cast(event.request_);
> +
> +		/* Decrease the ref increased in Camera.queue_request() */
> +		pyevent.request_.dec_ref();
> +
> +		break;
> +	}
> +
> +	return pyevent;
> +}
> +
> +/* DEPRECATED */
>  std::vector<py::object> PyCameraManager::getReadyRequests()
>  {
>  	int ret = readFd();
> @@ -72,21 +123,207 @@ std::vector<py::object> PyCameraManager::getReadyRequests()
>  
>  	std::vector<py::object> py_reqs;
>  
> -	for (Request *request : getCompletedRequests()) {
> -		py::object o = py::cast(request);
> -		/* Decrease the ref increased in Camera.queue_request() */
> -		o.dec_ref();
> -		py_reqs.push_back(o);
> +	for (const auto &ev : getEvents()) {
> +		if (ev.type_ != CameraEventType::RequestCompleted)
> +			continue;
> +
> +		PyCameraEvent pyev = convertEvent(ev);
> +		py_reqs.push_back(pyev.request_);
>  	}
>  
>  	return py_reqs;
>  }
>  
> +std::vector<PyCameraEvent> PyCameraManager::getPyEvents()
> +{
> +	int ret = readFd();
> +
> +	if (ret == EAGAIN) {
> +		LOG(Python, Debug) << "No events";
> +		return {};
> +	}
> +
> +	if (ret != 0)
> +		throw std::system_error(ret, std::generic_category());
> +
> +	std::vector<CameraEvent> events = getEvents();
> +
> +	LOG(Python, Debug) << "Got " << events.size() << " events";
> +
> +	std::vector<PyCameraEvent> pyevents;
> +	pyevents.reserve(events.size());
> +
> +	std::transform(events.begin(), events.end(), std::back_inserter(pyevents),
> +		       [this](const CameraEvent &ev) {
> +			       return convertEvent(ev);
> +		       });
> +
> +	return pyevents;
> +}
> +
> +static bool isCameraSpecificEvent(const CameraEvent &event, std::shared_ptr<Camera> &camera)
> +{
> +	return event.camera_ == camera &&
> +	       (event.type_ == CameraEventType::RequestCompleted ||
> +		event.type_ == CameraEventType::BufferCompleted ||
> +		event.type_ == CameraEventType::Disconnect);
> +}
> +
> +std::vector<PyCameraEvent> PyCameraManager::getPyCameraEvents(std::shared_ptr<Camera> camera)
> +{
> +	std::vector<CameraEvent> events;
> +	size_t unhandled_size;
> +
> +	{
> +		MutexLocker guard(eventsMutex_);
> +
> +		/*
> +		 * Collect events related to the given camera and remove them
> +		 * from the events_ vector.
> +		 */
> +
> +		auto it = events_.begin();
> +		while (it != events_.end()) {
> +			if (isCameraSpecificEvent(*it, camera)) {
> +				events.push_back(*it);
> +				it = events_.erase(it);
> +			} else {
> +				it++;
> +			}
> +		}
> +
> +		unhandled_size = events_.size();
> +	}
> +
> +	/* Convert events to Python events */
> +
> +	std::vector<PyCameraEvent> pyevents;
> +
> +	for (const auto &event : events) {
> +		PyCameraEvent pyev = convertEvent(event);
> +		pyevents.push_back(pyev);
> +	}
> +
> +	LOG(Python, Debug) << "Got " << pyevents.size() << " camera events, "
> +			   << unhandled_size << " unhandled events left";
> +
> +	return pyevents;
> +}
> +
>  /* Note: Called from another thread */
> -void PyCameraManager::handleRequestCompleted(Request *req)
> +void PyCameraManager::handleBufferCompleted(std::shared_ptr<Camera> cam, Request *req, FrameBuffer *fb)
>  {
> -	pushRequest(req);
> -	writeFd();
> +	CameraEvent ev(CameraEventType::BufferCompleted, cam, req, fb);
> +
> +	pushEvent(ev);
> +}
> +
> +/* Note: Called from another thread */
> +void PyCameraManager::handleRequestCompleted(std::shared_ptr<Camera> cam, Request *req)
> +{
> +	CameraEvent ev(CameraEventType::RequestCompleted, cam, req);
> +
> +	pushEvent(ev);
> +}
> +
> +/* Note: Called from another thread */
> +void PyCameraManager::handleDisconnected(std::shared_ptr<Camera> cam)
> +{
> +	CameraEvent ev(CameraEventType::Disconnect, cam);
> +
> +	pushEvent(ev);
> +}
> +
> +/* Note: Called from another thread */
> +void PyCameraManager::handleCameraAdded(std::shared_ptr<Camera> cam)
> +{
> +	CameraEvent ev(CameraEventType::CameraAdded, cam);
> +
> +	pushEvent(ev);
> +}
> +
> +/* Note: Called from another thread */
> +void PyCameraManager::handleCameraRemoved(std::shared_ptr<Camera> cam)
> +{
> +	CameraEvent ev(CameraEventType::CameraRemoved, cam);
> +
> +	pushEvent(ev);
> +}
> +
> +bool PyCameraManager::getCameraEventFlag(std::shared_ptr<Camera> camera, CameraEventType event_type)
> +{
> +	const uint32_t evbit = 1 << (uint32_t)event_type;
> +	uint32_t mask = 0;
> +
> +	if (auto it = camera_event_masks_.find(camera); it != camera_event_masks_.end())

That's a weird style.

	auto it = camera_event_masks_.find(camera);
	if (it != camera_event_masks_.end())

> +		mask = it->second;
> +
> +	return !!(mask & evbit);
> +}
> +
> +void PyCameraManager::setCameraEventFlag(std::shared_ptr<Camera> camera, CameraEventType event_type, bool value)
> +{
> +	switch (event_type) {
> +	case CameraEventType::RequestCompleted:
> +	case CameraEventType::BufferCompleted:
> +	case CameraEventType::Disconnect:
> +		break;
> +
> +	default:
> +		throw std::runtime_error("Bad camera event type");
> +	}
> +
> +	const uint32_t evbit = 1 << (uint32_t)event_type;
> +	uint32_t mask = 0;
> +
> +	if (auto it = camera_event_masks_.find(camera); it != camera_event_masks_.end())
> +		mask = it->second;

Same here.

> +
> +	bool old_val = !!(mask & evbit);
> +
> +	if (old_val == value)
> +		return;
> +
> +	if (value)
> +		mask |= evbit;
> +	else
> +		mask &= ~evbit;
> +
> +	camera_event_masks_[camera] = mask;
> +
> +	switch (event_type) {
> +	case CameraEventType::RequestCompleted:
> +		if (value) {
> +			camera->requestCompleted.connect(camera.get(), [cm = this->shared_from_this(), camera](Request *req) {
> +				cm->handleRequestCompleted(camera, req);
> +			});
> +		} else {
> +			camera->requestCompleted.disconnect();
> +		}
> +		break;

This is racy. PyCameraManager::handleRequestCompleted() could be called
before you disconnect the signal, with the event then stored in the
events_ queue. The application would then receive the event after
disabling it.

One option would be to connect the signals unconditionally and instead
filter events out when retrieving them in getPyEvents().

> +
> +	case CameraEventType::BufferCompleted:
> +		if (value) {
> +			camera->bufferCompleted.connect(camera.get(), [cm = this->shared_from_this(), camera](Request *req, FrameBuffer *fb) {
> +				cm->handleBufferCompleted(camera, req, fb);
> +			});
> +		} else {
> +			camera->bufferCompleted.disconnect();
> +		}
> +		break;
> +
> +	case CameraEventType::Disconnect:
> +		if (value) {
> +			camera->disconnected.connect(camera.get(), [cm = this->shared_from_this(), camera]() {
> +				cm->handleDisconnected(camera);
> +			});
> +		} else {
> +			camera->disconnected.disconnect();
> +		}
> +		break;
> +	default:
> +		ASSERT(false);
> +	}
>  }
>  
>  void PyCameraManager::writeFd()
> @@ -116,16 +353,24 @@ int PyCameraManager::readFd()
>  		return -EIO;
>  }
>  
> -void PyCameraManager::pushRequest(Request *req)
> +void PyCameraManager::pushEvent(const CameraEvent &ev)
>  {
> -	MutexLocker guard(completedRequestsMutex_);
> -	completedRequests_.push_back(req);
> +	{
> +		MutexLocker guard(eventsMutex_);
> +		events_.push_back(ev);
> +	}
> +
> +	writeFd();
> +
> +	LOG(Python, Debug) << "Queued events: " << events_.size();
>  }
>  
> -std::vector<Request *> PyCameraManager::getCompletedRequests()
> +std::vector<CameraEvent> PyCameraManager::getEvents()
>  {
> -	std::vector<Request *> v;
> -	MutexLocker guard(completedRequestsMutex_);
> -	swap(v, completedRequests_);
> +	std::vector<CameraEvent> v;
> +
> +	MutexLocker guard(eventsMutex_);
> +	swap(v, events_);
> +
>  	return v;
>  }
> diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h
> index 3574db23..31747547 100644
> --- a/src/py/libcamera/py_camera_manager.h
> +++ b/src/py/libcamera/py_camera_manager.h
> @@ -13,12 +13,56 @@
>  
>  using namespace libcamera;
>  
> -class PyCameraManager
> +enum class CameraEventType {
> +	CameraAdded,
> +	CameraRemoved,
> +	Disconnect,
> +	RequestCompleted,
> +	BufferCompleted,
> +};
> +
> +/*
> + * This event struct is used internally to queue the events we receive from
> + * other threads.
> + */
> +struct CameraEvent {
> +	CameraEvent(CameraEventType type, std::shared_ptr<Camera> camera,
> +		    Request *request = nullptr, FrameBuffer *fb = nullptr)
> +		: type_(type), camera_(camera), request_(request), fb_(fb)
> +	{
> +	}
> +
> +	CameraEventType type_;
> +	std::shared_ptr<Camera> camera_;
> +	Request *request_;
> +	FrameBuffer *fb_;
> +};
> +
> +/*
> + * This event struct is passed to Python. We need to use pybind11::object here
> + * instead of a C++ pointer so that we keep a ref to the Request, and a
> + * keep-alive from the camera to the camera manager.
> + */
> +struct PyCameraEvent {
> +	PyCameraEvent(CameraEventType type, pybind11::object camera)
> +		: type_(type), camera_(camera)
> +	{
> +	}
> +
> +	CameraEventType type_;
> +	pybind11::object camera_;
> +	pybind11::object request_;
> +	pybind11::object fb_;
> +};
> +
> +class PyCameraManager : public std::enable_shared_from_this<PyCameraManager>
>  {
>  public:
>  	PyCameraManager();
>  	~PyCameraManager();
>  
> +	void init();
> +
>  	pybind11::list cameras();
>  	std::shared_ptr<Camera> get(const std::string &name) { return cameraManager_->get(name); }
>  
> @@ -26,20 +70,33 @@ public:
>  
>  	int eventFd() const { return eventFd_.get(); }
>  
> -	std::vector<pybind11::object> getReadyRequests();
> +	std::vector<pybind11::object> getReadyRequests(); /* DEPRECATED */
> +	std::vector<PyCameraEvent> getPyEvents();
> +	std::vector<PyCameraEvent> getPyCameraEvents(std::shared_ptr<Camera> camera);
> +
> +	void handleBufferCompleted(std::shared_ptr<Camera> cam, Request *req, FrameBuffer *fb);
> +	void handleRequestCompleted(std::shared_ptr<Camera> cam, Request *req);
> +	void handleDisconnected(std::shared_ptr<Camera> cam);
> +	void handleCameraAdded(std::shared_ptr<Camera> cam);
> +	void handleCameraRemoved(std::shared_ptr<Camera> cam);
>  
> -	void handleRequestCompleted(Request *req);
> +	bool getCameraEventFlag(std::shared_ptr<Camera> camera, CameraEventType event_type);
> +	void setCameraEventFlag(std::shared_ptr<Camera> camera, CameraEventType event_type, bool value);
>  
>  private:
>  	std::unique_ptr<CameraManager> cameraManager_;
>  
>  	UniqueFD eventFd_;
> -	libcamera::Mutex completedRequestsMutex_;
> -	std::vector<Request *> completedRequests_
> -		LIBCAMERA_TSA_GUARDED_BY(completedRequestsMutex_);
> +	libcamera::Mutex eventsMutex_;
> +	std::vector<CameraEvent> events_
> +		LIBCAMERA_TSA_GUARDED_BY(eventsMutex_);
>  
>  	void writeFd();
>  	int readFd();
> -	void pushRequest(Request *req);
> -	std::vector<Request *> getCompletedRequests();
> +	void pushEvent(const CameraEvent &ev);
> +	std::vector<CameraEvent> getEvents();
> +
> +	PyCameraEvent convertEvent(const CameraEvent &event);
> +
> +	std::map<std::shared_ptr<Camera>, uint32_t> camera_event_masks_;
>  };
> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> index 5a5f1a37..981a3070 100644
> --- a/src/py/libcamera/py_main.cpp
> +++ b/src/py/libcamera/py_main.cpp
> @@ -110,6 +110,7 @@ PYBIND11_MODULE(_libcamera, m)
>  	 * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings
>  	 */
>  
> +	auto pyEvent = py::class_<PyCameraEvent>(m, "Event");
>  	auto pyCameraManager = py::class_<PyCameraManager, std::shared_ptr<PyCameraManager>>(m, "CameraManager");
>  	auto pyCamera = py::class_<Camera, PyCameraSmartPtr<Camera>>(m, "Camera");
>  	auto pyCameraConfiguration = py::class_<CameraConfiguration>(m, "CameraConfiguration");
> @@ -136,12 +137,27 @@ PYBIND11_MODULE(_libcamera, m)
>  	m.def("log_set_level", &logSetLevel);
>  
>  	/* Classes */
> +
> +	py::enum_<CameraEventType>(pyEvent, "Type")
> +		.value("CameraAdded", CameraEventType::CameraAdded)
> +		.value("CameraRemoved", CameraEventType::CameraRemoved)
> +		.value("Disconnect", CameraEventType::Disconnect)
> +		.value("RequestCompleted", CameraEventType::RequestCompleted)
> +		.value("BufferCompleted", CameraEventType::BufferCompleted);
> +
> +	pyEvent
> +		.def_readonly("type", &PyCameraEvent::type_)
> +		.def_readonly("camera", &PyCameraEvent::camera_)
> +		.def_readonly("request", &PyCameraEvent::request_)
> +		.def_readonly("fb", &PyCameraEvent::fb_);
> +
>  	pyCameraManager
>  		.def_static("singleton", []() {
>  			std::shared_ptr<PyCameraManager> cm = gCameraManager.lock();
>  
>  			if (!cm) {
>  				cm = std::make_shared<PyCameraManager>();
> +				cm->init();
>  				gCameraManager = cm;
>  			}
>  
> @@ -153,10 +169,33 @@ PYBIND11_MODULE(_libcamera, m)
>  		.def_property_readonly("cameras", &PyCameraManager::cameras)
>  
>  		.def_property_readonly("event_fd", &PyCameraManager::eventFd)
> -		.def("get_ready_requests", &PyCameraManager::getReadyRequests);
> +
> +		/* DEPRECATED */
> +		.def("get_ready_requests", &PyCameraManager::getReadyRequests)
> +
> +		.def("get_events", &PyCameraManager::getPyEvents);
>  
>  	pyCamera
>  		.def_property_readonly("id", &Camera::id)
> +
> +		.def("get_camera_event_enabled",
> +			[](Camera &self, CameraEventType type) {
> +				auto cm = gCameraManager.lock();
> +				return cm->getCameraEventFlag(self.shared_from_this(), type);
> +			})
> +
> +		.def("enable_camera_event",
> +			[](Camera &self, CameraEventType type) {
> +				auto cm = gCameraManager.lock();
> +				cm->setCameraEventFlag(self.shared_from_this(), type, true);
> +			})
> +
> +		.def("disable_camera_event",
> +			[](Camera &self, CameraEventType type) {
> +				auto cm = gCameraManager.lock();
> +				cm->setCameraEventFlag(self.shared_from_this(), type, false);
> +			})
> +
>  		.def("acquire", [](Camera &self) {
>  			int ret = self.acquire();
>  			if (ret)
> @@ -173,11 +212,6 @@ PYBIND11_MODULE(_libcamera, m)
>  		                 const std::unordered_map<const ControlId *, py::object> &controls) {
>  			/* \todo What happens if someone calls start() multiple times? */
>  
> -			auto cm = gCameraManager.lock();
> -			ASSERT(cm);
> -
> -			self.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted);
> -
>  			ControlList controlList(self.controls());
>  
>  			for (const auto &[id, obj] : controls) {
> @@ -187,7 +221,6 @@ PYBIND11_MODULE(_libcamera, m)
>  
>  			int ret = self.start(&controlList);
>  			if (ret) {
> -				self.requestCompleted.disconnect();
>  				throw std::system_error(-ret, std::generic_category(),
>  							"Failed to start camera");
>  			}

You can drop the curly braces.

> @@ -196,11 +229,16 @@ PYBIND11_MODULE(_libcamera, m)
>  		.def("stop", [](Camera &self) {
>  			int ret = self.stop();
>  
> -			self.requestCompleted.disconnect();
> +			auto cm = gCameraManager.lock();
> +			ASSERT(cm);
> +
> +			auto events = cm->getPyCameraEvents(self.shared_from_this());
>  
>  			if (ret)
>  				throw std::system_error(-ret, std::generic_category(),
>  							"Failed to stop camera");
> +
> +			return events;
>  		})
>  
>  		.def("__str__", [](Camera &self) {

On 05/06/2023 09:47, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Sat, Jun 03, 2023 at 10:56:05AM +0300, Tomi Valkeinen wrote:
>> At the moment the Python bindings only handle the requestCompleted
>> events. But we have others, bufferCompleted and disconnect from the
>> Camera, and cameraAdded and cameraRemoved from the CameraManager.
>>
>> This patch makes all those events available to the users.
>>
>> The get_ready_requests() method is now deprecated, but available to keep
>> backward compatibility.
>>
>> The new event handling works like get_ready_requests(), but instead of
>> returning only Requests from requestCompleted events, it returns all
>> event types using a new Event class.
>>
>> Additionally camera.stop() has been changed to return events for that
>> specific camera. This serves two purposes: first, it removes the
>> RequestCompleted and BufferCompleted events from the event queue, thus
>> preventing the old events being returned when the camera is started
>> again, and second, it allows the user to process those events if it so
>> wants.
> 
> Returning events from stop() is a bit of a weird API, and I'm a bit
> worried it will confuse users, but I don't have any better option to
> propose. I think it's good enough for now.

It was added to solve the reported user confusion. So... Confusing both 
ways? =).

It's, of course, not a must. The user can get the events the normal way, 
thus flushing the event queue.

But I agree that it's a bit weird, and I kind of would like to drop it. 
Maybe just documenting it clearly that one needs to take care of the 
events after calling stop would be enough?

>> The CameraManager events (CameraAdded and CameraRemoved) are always
>> enabled, but of the Camera events only RequestCompleted is enabled by
>> default. Other Camera events can be enabled
>> Camera.enable_camera_event().
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
> 
> Note for the future, per-patch changelogs would be useful.

I have never learned to use those... Maybe it's time.

>>   Documentation/python-bindings.rst      |  24 ++-
>>   src/py/libcamera/py_camera_manager.cpp | 275 +++++++++++++++++++++++--
>>   src/py/libcamera/py_camera_manager.h   |  73 ++++++-
>>   src/py/libcamera/py_main.cpp           |  54 ++++-
>>   4 files changed, 386 insertions(+), 40 deletions(-)
>>
>> diff --git a/Documentation/python-bindings.rst b/Documentation/python-bindings.rst
>> index bac5cd34..8f2d76c3 100644
>> --- a/Documentation/python-bindings.rst
>> +++ b/Documentation/python-bindings.rst
>> @@ -43,17 +43,23 @@ CameraManager
>>   The Python API provides a singleton CameraManager via ``CameraManager.singleton()``.
>>   There is no need to start or stop the CameraManager.
>>   
>> -Handling Completed Requests
>> ----------------------------
>> +Event Handling
>> +--------------
>>   
>> -The Python bindings do not expose the ``Camera::requestCompleted`` signal
>> -directly as the signal is invoked from another thread and it has real-time
>> -constraints. Instead the bindings queue the completed requests internally and
>> -use an eventfd to inform the user that there are completed requests.
>> +The Python bindings do not expose the signals from the C++ side directly as the
>> +signals are invoked from another thread and they may have real-time
>> +constraints. Instead the bindings queue the received events internally and use
>> +an eventfd to inform the user that there are events to be handled.
>>   
>> -The user can wait on the eventfd, and upon getting an event, use
>> -``CameraManager.get_ready_requests()`` to clear the eventfd event and to get
>> -the completed requests.
>> +The user can wait on the eventfd (e.g. by using Python Selector), and use
>> +``CameraManager.get_events()`` to reset the eventfd and get the events.
>> +
>> +The CameraManager events (CameraAdded and CameraRemoved) are always enabled, but
>> +of the Camera events only RequestCompleted is enabled by default. To enable
>> +Disconnect or BufferCompleted event, use ``Camera.enable_camera_event()``.
>> +
>> +The ``Camera.stop()`` method will return all events related to that Camera from
>> +the event queue.
>>   
>>   Controls & Properties
>>   ---------------------
>> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp
>> index 9ccb7aad..83c2b063 100644
>> --- a/src/py/libcamera/py_camera_manager.cpp
>> +++ b/src/py/libcamera/py_camera_manager.cpp
>> @@ -5,6 +5,7 @@
>>   
>>   #include "py_camera_manager.h"
>>   
>> +#include <algorithm>
>>   #include <errno.h>
>>   #include <memory>
>>   #include <sys/eventfd.h>
>> @@ -35,11 +36,24 @@ PyCameraManager::PyCameraManager()
>>   	if (ret)
>>   		throw std::system_error(-ret, std::generic_category(),
>>   					"Failed to start CameraManager");
>> +
>> +	cameraManager_->cameraAdded.connect(this, &PyCameraManager::handleCameraAdded);
>> +	cameraManager_->cameraRemoved.connect(this, &PyCameraManager::handleCameraRemoved);
>>   }
>>   
>>   PyCameraManager::~PyCameraManager()
>>   {
>>   	LOG(Python, Debug) << "~PyCameraManager()";
>> +
>> +	cameraManager_->cameraAdded.disconnect();
>> +	cameraManager_->cameraRemoved.disconnect();
>> +}
>> +
>> +void PyCameraManager::init()
>> +{
>> +	/* Always enable RequestCompleted for all cameras by default */
>> +	for (auto cam : cameraManager_->cameras())
>> +		setCameraEventFlag(cam, CameraEventType::RequestCompleted, true);
> 
> You need to also enable the event when a new camera is detected.

Ah, good point. But I also wonder, should this just be dropped too? I'm 
not sure if the return-events-from-stop and this one are just hacks that 
seemingly make life easier for the users, but in the end just add 
confusion for both the user and in the codebase. Or are they actually 
good features...

>>   }
>>   
>>   py::list PyCameraManager::cameras()
>> @@ -60,6 +74,43 @@ py::list PyCameraManager::cameras()
>>   	return l;
>>   }
>>   
>> +PyCameraEvent PyCameraManager::convertEvent(const CameraEvent &event)
>> +{
>> +	/*
>> +	 * We need to set a keep-alive here so that the camera keeps the
>> +	 * camera manager alive.
>> +	 */
>> +	py::object py_cm = py::cast(this);
>> +	py::object py_cam = py::cast(event.camera_);
>> +	py::detail::keep_alive_impl(py_cam, py_cm);
>> +
>> +	PyCameraEvent pyevent(event.type_, py_cam);
>> +
>> +	switch (event.type_) {
>> +	case CameraEventType::CameraAdded:
>> +	case CameraEventType::CameraRemoved:
>> +	case CameraEventType::Disconnect:
>> +		/* No additional parameters to add */
>> +		break;
>> +
>> +	case CameraEventType::BufferCompleted:
>> +		pyevent.request_ = py::cast(event.request_);
>> +		pyevent.fb_ = py::cast(event.fb_);
>> +		break;
>> +
>> +	case CameraEventType::RequestCompleted:
>> +		pyevent.request_ = py::cast(event.request_);
>> +
>> +		/* Decrease the ref increased in Camera.queue_request() */
>> +		pyevent.request_.dec_ref();
>> +
>> +		break;
>> +	}
>> +
>> +	return pyevent;
>> +}
>> +
>> +/* DEPRECATED */
>>   std::vector<py::object> PyCameraManager::getReadyRequests()
>>   {
>>   	int ret = readFd();
>> @@ -72,21 +123,207 @@ std::vector<py::object> PyCameraManager::getReadyRequests()
>>   
>>   	std::vector<py::object> py_reqs;
>>   
>> -	for (Request *request : getCompletedRequests()) {
>> -		py::object o = py::cast(request);
>> -		/* Decrease the ref increased in Camera.queue_request() */
>> -		o.dec_ref();
>> -		py_reqs.push_back(o);
>> +	for (const auto &ev : getEvents()) {
>> +		if (ev.type_ != CameraEventType::RequestCompleted)
>> +			continue;
>> +
>> +		PyCameraEvent pyev = convertEvent(ev);
>> +		py_reqs.push_back(pyev.request_);
>>   	}
>>   
>>   	return py_reqs;
>>   }
>>   
>> +std::vector<PyCameraEvent> PyCameraManager::getPyEvents()
>> +{
>> +	int ret = readFd();
>> +
>> +	if (ret == EAGAIN) {
>> +		LOG(Python, Debug) << "No events";
>> +		return {};
>> +	}
>> +
>> +	if (ret != 0)
>> +		throw std::system_error(ret, std::generic_category());
>> +
>> +	std::vector<CameraEvent> events = getEvents();
>> +
>> +	LOG(Python, Debug) << "Got " << events.size() << " events";
>> +
>> +	std::vector<PyCameraEvent> pyevents;
>> +	pyevents.reserve(events.size());
>> +
>> +	std::transform(events.begin(), events.end(), std::back_inserter(pyevents),
>> +		       [this](const CameraEvent &ev) {
>> +			       return convertEvent(ev);
>> +		       });
>> +
>> +	return pyevents;
>> +}
>> +
>> +static bool isCameraSpecificEvent(const CameraEvent &event, std::shared_ptr<Camera> &camera)
>> +{
>> +	return event.camera_ == camera &&
>> +	       (event.type_ == CameraEventType::RequestCompleted ||
>> +		event.type_ == CameraEventType::BufferCompleted ||
>> +		event.type_ == CameraEventType::Disconnect);
>> +}
>> +
>> +std::vector<PyCameraEvent> PyCameraManager::getPyCameraEvents(std::shared_ptr<Camera> camera)
>> +{
>> +	std::vector<CameraEvent> events;
>> +	size_t unhandled_size;
>> +
>> +	{
>> +		MutexLocker guard(eventsMutex_);
>> +
>> +		/*
>> +		 * Collect events related to the given camera and remove them
>> +		 * from the events_ vector.
>> +		 */
>> +
>> +		auto it = events_.begin();
>> +		while (it != events_.end()) {
>> +			if (isCameraSpecificEvent(*it, camera)) {
>> +				events.push_back(*it);
>> +				it = events_.erase(it);
>> +			} else {
>> +				it++;
>> +			}
>> +		}
>> +
>> +		unhandled_size = events_.size();
>> +	}
>> +
>> +	/* Convert events to Python events */
>> +
>> +	std::vector<PyCameraEvent> pyevents;
>> +
>> +	for (const auto &event : events) {
>> +		PyCameraEvent pyev = convertEvent(event);
>> +		pyevents.push_back(pyev);
>> +	}
>> +
>> +	LOG(Python, Debug) << "Got " << pyevents.size() << " camera events, "
>> +			   << unhandled_size << " unhandled events left";
>> +
>> +	return pyevents;
>> +}
>> +
>>   /* Note: Called from another thread */
>> -void PyCameraManager::handleRequestCompleted(Request *req)
>> +void PyCameraManager::handleBufferCompleted(std::shared_ptr<Camera> cam, Request *req, FrameBuffer *fb)
>>   {
>> -	pushRequest(req);
>> -	writeFd();
>> +	CameraEvent ev(CameraEventType::BufferCompleted, cam, req, fb);
>> +
>> +	pushEvent(ev);
>> +}
>> +
>> +/* Note: Called from another thread */
>> +void PyCameraManager::handleRequestCompleted(std::shared_ptr<Camera> cam, Request *req)
>> +{
>> +	CameraEvent ev(CameraEventType::RequestCompleted, cam, req);
>> +
>> +	pushEvent(ev);
>> +}
>> +
>> +/* Note: Called from another thread */
>> +void PyCameraManager::handleDisconnected(std::shared_ptr<Camera> cam)
>> +{
>> +	CameraEvent ev(CameraEventType::Disconnect, cam);
>> +
>> +	pushEvent(ev);
>> +}
>> +
>> +/* Note: Called from another thread */
>> +void PyCameraManager::handleCameraAdded(std::shared_ptr<Camera> cam)
>> +{
>> +	CameraEvent ev(CameraEventType::CameraAdded, cam);
>> +
>> +	pushEvent(ev);
>> +}
>> +
>> +/* Note: Called from another thread */
>> +void PyCameraManager::handleCameraRemoved(std::shared_ptr<Camera> cam)
>> +{
>> +	CameraEvent ev(CameraEventType::CameraRemoved, cam);
>> +
>> +	pushEvent(ev);
>> +}
>> +
>> +bool PyCameraManager::getCameraEventFlag(std::shared_ptr<Camera> camera, CameraEventType event_type)
>> +{
>> +	const uint32_t evbit = 1 << (uint32_t)event_type;
>> +	uint32_t mask = 0;
>> +
>> +	if (auto it = camera_event_masks_.find(camera); it != camera_event_masks_.end())
> 
> That's a weird style.

Weird how? That's standard c++ style... It prevents the 'it' from 
leaking outside the if block.

> 	auto it = camera_event_masks_.find(camera);
> 	if (it != camera_event_masks_.end())
> 
>> +		mask = it->second;
>> +
>> +	return !!(mask & evbit);
>> +}
>> +
>> +void PyCameraManager::setCameraEventFlag(std::shared_ptr<Camera> camera, CameraEventType event_type, bool value)
>> +{
>> +	switch (event_type) {
>> +	case CameraEventType::RequestCompleted:
>> +	case CameraEventType::BufferCompleted:
>> +	case CameraEventType::Disconnect:
>> +		break;
>> +
>> +	default:
>> +		throw std::runtime_error("Bad camera event type");
>> +	}
>> +
>> +	const uint32_t evbit = 1 << (uint32_t)event_type;
>> +	uint32_t mask = 0;
>> +
>> +	if (auto it = camera_event_masks_.find(camera); it != camera_event_masks_.end())
>> +		mask = it->second;
> 
> Same here.
> 
>> +
>> +	bool old_val = !!(mask & evbit);
>> +
>> +	if (old_val == value)
>> +		return;
>> +
>> +	if (value)
>> +		mask |= evbit;
>> +	else
>> +		mask &= ~evbit;
>> +
>> +	camera_event_masks_[camera] = mask;
>> +
>> +	switch (event_type) {
>> +	case CameraEventType::RequestCompleted:
>> +		if (value) {
>> +			camera->requestCompleted.connect(camera.get(), [cm = this->shared_from_this(), camera](Request *req) {
>> +				cm->handleRequestCompleted(camera, req);
>> +			});
>> +		} else {
>> +			camera->requestCompleted.disconnect();
>> +		}
>> +		break;
> 
> This is racy. PyCameraManager::handleRequestCompleted() could be called
> before you disconnect the signal, with the event then stored in the
> events_ queue. The application would then receive the event after
> disabling it.

I don't think it's racy as such, that's "normal" for single threaded 
application. The event could have happened at some point much before 
this function is even called.

We could drop here all the events from the event queue for the specific 
event being disabled. It's perhaps a question of how these 
enable/disable flags are defined.

Are they enabling/disabling the low-level event as such (this is how I 
have been thinking about it), but they don't affect the event queue 
(i.e. you can get now-disabled events from the event queue).

Or are they enabling/disabling what the get_events() returns.

To me the former feels more natural, but I can see the logic in the 
latter option too.

> One option would be to connect the signals unconditionally and instead
> filter events out when retrieving them in getPyEvents().

The whole point of these flags is to avoid all the extra allocations and 
storing of the events that the user doesn't want.

>> +
>> +	case CameraEventType::BufferCompleted:
>> +		if (value) {
>> +			camera->bufferCompleted.connect(camera.get(), [cm = this->shared_from_this(), camera](Request *req, FrameBuffer *fb) {
>> +				cm->handleBufferCompleted(camera, req, fb);
>> +			});
>> +		} else {
>> +			camera->bufferCompleted.disconnect();
>> +		}
>> +		break;
>> +
>> +	case CameraEventType::Disconnect:
>> +		if (value) {
>> +			camera->disconnected.connect(camera.get(), [cm = this->shared_from_this(), camera]() {
>> +				cm->handleDisconnected(camera);
>> +			});
>> +		} else {
>> +			camera->disconnected.disconnect();
>> +		}
>> +		break;
>> +	default:
>> +		ASSERT(false);
>> +	}
>>   }
>>   
>>   void PyCameraManager::writeFd()
>> @@ -116,16 +353,24 @@ int PyCameraManager::readFd()
>>   		return -EIO;
>>   }
>>   
>> -void PyCameraManager::pushRequest(Request *req)
>> +void PyCameraManager::pushEvent(const CameraEvent &ev)
>>   {
>> -	MutexLocker guard(completedRequestsMutex_);
>> -	completedRequests_.push_back(req);
>> +	{
>> +		MutexLocker guard(eventsMutex_);
>> +		events_.push_back(ev);
>> +	}
>> +
>> +	writeFd();
>> +
>> +	LOG(Python, Debug) << "Queued events: " << events_.size();
>>   }
>>   
>> -std::vector<Request *> PyCameraManager::getCompletedRequests()
>> +std::vector<CameraEvent> PyCameraManager::getEvents()
>>   {
>> -	std::vector<Request *> v;
>> -	MutexLocker guard(completedRequestsMutex_);
>> -	swap(v, completedRequests_);
>> +	std::vector<CameraEvent> v;
>> +
>> +	MutexLocker guard(eventsMutex_);
>> +	swap(v, events_);
>> +
>>   	return v;
>>   }
>> diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h
>> index 3574db23..31747547 100644
>> --- a/src/py/libcamera/py_camera_manager.h
>> +++ b/src/py/libcamera/py_camera_manager.h
>> @@ -13,12 +13,56 @@
>>   
>>   using namespace libcamera;
>>   
>> -class PyCameraManager
>> +enum class CameraEventType {
>> +	CameraAdded,
>> +	CameraRemoved,
>> +	Disconnect,
>> +	RequestCompleted,
>> +	BufferCompleted,
>> +};
>> +
>> +/*
>> + * This event struct is used internally to queue the events we receive from
>> + * other threads.
>> + */
>> +struct CameraEvent {
>> +	CameraEvent(CameraEventType type, std::shared_ptr<Camera> camera,
>> +		    Request *request = nullptr, FrameBuffer *fb = nullptr)
>> +		: type_(type), camera_(camera), request_(request), fb_(fb)
>> +	{
>> +	}
>> +
>> +	CameraEventType type_;
>> +	std::shared_ptr<Camera> camera_;
>> +	Request *request_;
>> +	FrameBuffer *fb_;
>> +};
>> +
>> +/*
>> + * This event struct is passed to Python. We need to use pybind11::object here
>> + * instead of a C++ pointer so that we keep a ref to the Request, and a
>> + * keep-alive from the camera to the camera manager.
>> + */
>> +struct PyCameraEvent {
>> +	PyCameraEvent(CameraEventType type, pybind11::object camera)
>> +		: type_(type), camera_(camera)
>> +	{
>> +	}
>> +
>> +	CameraEventType type_;
>> +	pybind11::object camera_;
>> +	pybind11::object request_;
>> +	pybind11::object fb_;
>> +};
>> +
>> +class PyCameraManager : public std::enable_shared_from_this<PyCameraManager>
>>   {
>>   public:
>>   	PyCameraManager();
>>   	~PyCameraManager();
>>   
>> +	void init();
>> +
>>   	pybind11::list cameras();
>>   	std::shared_ptr<Camera> get(const std::string &name) { return cameraManager_->get(name); }
>>   
>> @@ -26,20 +70,33 @@ public:
>>   
>>   	int eventFd() const { return eventFd_.get(); }
>>   
>> -	std::vector<pybind11::object> getReadyRequests();
>> +	std::vector<pybind11::object> getReadyRequests(); /* DEPRECATED */
>> +	std::vector<PyCameraEvent> getPyEvents();
>> +	std::vector<PyCameraEvent> getPyCameraEvents(std::shared_ptr<Camera> camera);
>> +
>> +	void handleBufferCompleted(std::shared_ptr<Camera> cam, Request *req, FrameBuffer *fb);
>> +	void handleRequestCompleted(std::shared_ptr<Camera> cam, Request *req);
>> +	void handleDisconnected(std::shared_ptr<Camera> cam);
>> +	void handleCameraAdded(std::shared_ptr<Camera> cam);
>> +	void handleCameraRemoved(std::shared_ptr<Camera> cam);
>>   
>> -	void handleRequestCompleted(Request *req);
>> +	bool getCameraEventFlag(std::shared_ptr<Camera> camera, CameraEventType event_type);
>> +	void setCameraEventFlag(std::shared_ptr<Camera> camera, CameraEventType event_type, bool value);
>>   
>>   private:
>>   	std::unique_ptr<CameraManager> cameraManager_;
>>   
>>   	UniqueFD eventFd_;
>> -	libcamera::Mutex completedRequestsMutex_;
>> -	std::vector<Request *> completedRequests_
>> -		LIBCAMERA_TSA_GUARDED_BY(completedRequestsMutex_);
>> +	libcamera::Mutex eventsMutex_;
>> +	std::vector<CameraEvent> events_
>> +		LIBCAMERA_TSA_GUARDED_BY(eventsMutex_);
>>   
>>   	void writeFd();
>>   	int readFd();
>> -	void pushRequest(Request *req);
>> -	std::vector<Request *> getCompletedRequests();
>> +	void pushEvent(const CameraEvent &ev);
>> +	std::vector<CameraEvent> getEvents();
>> +
>> +	PyCameraEvent convertEvent(const CameraEvent &event);
>> +
>> +	std::map<std::shared_ptr<Camera>, uint32_t> camera_event_masks_;
>>   };
>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
>> index 5a5f1a37..981a3070 100644
>> --- a/src/py/libcamera/py_main.cpp
>> +++ b/src/py/libcamera/py_main.cpp
>> @@ -110,6 +110,7 @@ PYBIND11_MODULE(_libcamera, m)
>>   	 * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings
>>   	 */
>>   
>> +	auto pyEvent = py::class_<PyCameraEvent>(m, "Event");
>>   	auto pyCameraManager = py::class_<PyCameraManager, std::shared_ptr<PyCameraManager>>(m, "CameraManager");
>>   	auto pyCamera = py::class_<Camera, PyCameraSmartPtr<Camera>>(m, "Camera");
>>   	auto pyCameraConfiguration = py::class_<CameraConfiguration>(m, "CameraConfiguration");
>> @@ -136,12 +137,27 @@ PYBIND11_MODULE(_libcamera, m)
>>   	m.def("log_set_level", &logSetLevel);
>>   
>>   	/* Classes */
>> +
>> +	py::enum_<CameraEventType>(pyEvent, "Type")
>> +		.value("CameraAdded", CameraEventType::CameraAdded)
>> +		.value("CameraRemoved", CameraEventType::CameraRemoved)
>> +		.value("Disconnect", CameraEventType::Disconnect)
>> +		.value("RequestCompleted", CameraEventType::RequestCompleted)
>> +		.value("BufferCompleted", CameraEventType::BufferCompleted);
>> +
>> +	pyEvent
>> +		.def_readonly("type", &PyCameraEvent::type_)
>> +		.def_readonly("camera", &PyCameraEvent::camera_)
>> +		.def_readonly("request", &PyCameraEvent::request_)
>> +		.def_readonly("fb", &PyCameraEvent::fb_);
>> +
>>   	pyCameraManager
>>   		.def_static("singleton", []() {
>>   			std::shared_ptr<PyCameraManager> cm = gCameraManager.lock();
>>   
>>   			if (!cm) {
>>   				cm = std::make_shared<PyCameraManager>();
>> +				cm->init();
>>   				gCameraManager = cm;
>>   			}
>>   
>> @@ -153,10 +169,33 @@ PYBIND11_MODULE(_libcamera, m)
>>   		.def_property_readonly("cameras", &PyCameraManager::cameras)
>>   
>>   		.def_property_readonly("event_fd", &PyCameraManager::eventFd)
>> -		.def("get_ready_requests", &PyCameraManager::getReadyRequests);
>> +
>> +		/* DEPRECATED */
>> +		.def("get_ready_requests", &PyCameraManager::getReadyRequests)
>> +
>> +		.def("get_events", &PyCameraManager::getPyEvents);
>>   
>>   	pyCamera
>>   		.def_property_readonly("id", &Camera::id)
>> +
>> +		.def("get_camera_event_enabled",
>> +			[](Camera &self, CameraEventType type) {
>> +				auto cm = gCameraManager.lock();
>> +				return cm->getCameraEventFlag(self.shared_from_this(), type);
>> +			})
>> +
>> +		.def("enable_camera_event",
>> +			[](Camera &self, CameraEventType type) {
>> +				auto cm = gCameraManager.lock();
>> +				cm->setCameraEventFlag(self.shared_from_this(), type, true);
>> +			})
>> +
>> +		.def("disable_camera_event",
>> +			[](Camera &self, CameraEventType type) {
>> +				auto cm = gCameraManager.lock();
>> +				cm->setCameraEventFlag(self.shared_from_this(), type, false);
>> +			})
>> +
>>   		.def("acquire", [](Camera &self) {
>>   			int ret = self.acquire();
>>   			if (ret)
>> @@ -173,11 +212,6 @@ PYBIND11_MODULE(_libcamera, m)
>>   		                 const std::unordered_map<const ControlId *, py::object> &controls) {
>>   			/* \todo What happens if someone calls start() multiple times? */
>>   
>> -			auto cm = gCameraManager.lock();
>> -			ASSERT(cm);
>> -
>> -			self.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted);
>> -
>>   			ControlList controlList(self.controls());
>>   
>>   			for (const auto &[id, obj] : controls) {
>> @@ -187,7 +221,6 @@ PYBIND11_MODULE(_libcamera, m)
>>   
>>   			int ret = self.start(&controlList);
>>   			if (ret) {
>> -				self.requestCompleted.disconnect();
>>   				throw std::system_error(-ret, std::generic_category(),
>>   							"Failed to start camera");
>>   			}
> 
> You can drop the curly braces.

Indeed.

>> @@ -196,11 +229,16 @@ PYBIND11_MODULE(_libcamera, m)
>>   		.def("stop", [](Camera &self) {
>>   			int ret = self.stop();
>>   
>> -			self.requestCompleted.disconnect();
>> +			auto cm = gCameraManager.lock();
>> +			ASSERT(cm);
>> +
>> +			auto events = cm->getPyCameraEvents(self.shared_from_this());
>>   
>>   			if (ret)
>>   				throw std::system_error(-ret, std::generic_category(),
>>   							"Failed to stop camera");
>> +
>> +			return events;
>>   		})
>>   
>>   		.def("__str__", [](Camera &self) {
> 

  Tomi

Hi Tomi,

On Mon, Jun 05, 2023 at 10:31:00AM +0300, Tomi Valkeinen wrote:
> On 05/06/2023 09:47, Laurent Pinchart wrote:
> > On Sat, Jun 03, 2023 at 10:56:05AM +0300, Tomi Valkeinen wrote:
> >> At the moment the Python bindings only handle the requestCompleted
> >> events. But we have others, bufferCompleted and disconnect from the
> >> Camera, and cameraAdded and cameraRemoved from the CameraManager.
> >>
> >> This patch makes all those events available to the users.
> >>
> >> The get_ready_requests() method is now deprecated, but available to keep
> >> backward compatibility.
> >>
> >> The new event handling works like get_ready_requests(), but instead of
> >> returning only Requests from requestCompleted events, it returns all
> >> event types using a new Event class.
> >>
> >> Additionally camera.stop() has been changed to return events for that
> >> specific camera. This serves two purposes: first, it removes the
> >> RequestCompleted and BufferCompleted events from the event queue, thus
> >> preventing the old events being returned when the camera is started
> >> again, and second, it allows the user to process those events if it so
> >> wants.
> > 
> > Returning events from stop() is a bit of a weird API, and I'm a bit
> > worried it will confuse users, but I don't have any better option to
> > propose. I think it's good enough for now.
> 
> It was added to solve the reported user confusion. So... Confusing both 
> ways? =).
> 
> It's, of course, not a must. The user can get the events the normal way, 
> thus flushing the event queue.
> 
> But I agree that it's a bit weird, and I kind of would like to drop it. 
> Maybe just documenting it clearly that one needs to take care of the 
> events after calling stop would be enough?

It would be great if people didn't ignore documentation :-)

> >> The CameraManager events (CameraAdded and CameraRemoved) are always
> >> enabled, but of the Camera events only RequestCompleted is enabled by
> >> default. Other Camera events can be enabled
> >> Camera.enable_camera_event().
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >> ---
> > 
> > Note for the future, per-patch changelogs would be useful.
> 
> I have never learned to use those... Maybe it's time.

Yes please :-) I've meant to ask you for a long time already. It doesn't
have to be difficult, you can just record the changes every time you do
a `git commit --amend`.

> >>   Documentation/python-bindings.rst      |  24 ++-
> >>   src/py/libcamera/py_camera_manager.cpp | 275 +++++++++++++++++++++++--
> >>   src/py/libcamera/py_camera_manager.h   |  73 ++++++-
> >>   src/py/libcamera/py_main.cpp           |  54 ++++-
> >>   4 files changed, 386 insertions(+), 40 deletions(-)
> >>
> >> diff --git a/Documentation/python-bindings.rst b/Documentation/python-bindings.rst
> >> index bac5cd34..8f2d76c3 100644
> >> --- a/Documentation/python-bindings.rst
> >> +++ b/Documentation/python-bindings.rst
> >> @@ -43,17 +43,23 @@ CameraManager
> >>   The Python API provides a singleton CameraManager via ``CameraManager.singleton()``.
> >>   There is no need to start or stop the CameraManager.
> >>   
> >> -Handling Completed Requests
> >> ----------------------------
> >> +Event Handling
> >> +--------------
> >>   
> >> -The Python bindings do not expose the ``Camera::requestCompleted`` signal
> >> -directly as the signal is invoked from another thread and it has real-time
> >> -constraints. Instead the bindings queue the completed requests internally and
> >> -use an eventfd to inform the user that there are completed requests.
> >> +The Python bindings do not expose the signals from the C++ side directly as the
> >> +signals are invoked from another thread and they may have real-time
> >> +constraints. Instead the bindings queue the received events internally and use
> >> +an eventfd to inform the user that there are events to be handled.
> >>   
> >> -The user can wait on the eventfd, and upon getting an event, use
> >> -``CameraManager.get_ready_requests()`` to clear the eventfd event and to get
> >> -the completed requests.
> >> +The user can wait on the eventfd (e.g. by using Python Selector), and use
> >> +``CameraManager.get_events()`` to reset the eventfd and get the events.
> >> +
> >> +The CameraManager events (CameraAdded and CameraRemoved) are always enabled, but
> >> +of the Camera events only RequestCompleted is enabled by default. To enable
> >> +Disconnect or BufferCompleted event, use ``Camera.enable_camera_event()``.
> >> +
> >> +The ``Camera.stop()`` method will return all events related to that Camera from
> >> +the event queue.
> >>   
> >>   Controls & Properties
> >>   ---------------------
> >> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp
> >> index 9ccb7aad..83c2b063 100644
> >> --- a/src/py/libcamera/py_camera_manager.cpp
> >> +++ b/src/py/libcamera/py_camera_manager.cpp
> >> @@ -5,6 +5,7 @@
> >>   
> >>   #include "py_camera_manager.h"
> >>   
> >> +#include <algorithm>
> >>   #include <errno.h>
> >>   #include <memory>
> >>   #include <sys/eventfd.h>
> >> @@ -35,11 +36,24 @@ PyCameraManager::PyCameraManager()
> >>   	if (ret)
> >>   		throw std::system_error(-ret, std::generic_category(),
> >>   					"Failed to start CameraManager");
> >> +
> >> +	cameraManager_->cameraAdded.connect(this, &PyCameraManager::handleCameraAdded);
> >> +	cameraManager_->cameraRemoved.connect(this, &PyCameraManager::handleCameraRemoved);
> >>   }
> >>   
> >>   PyCameraManager::~PyCameraManager()
> >>   {
> >>   	LOG(Python, Debug) << "~PyCameraManager()";
> >> +
> >> +	cameraManager_->cameraAdded.disconnect();
> >> +	cameraManager_->cameraRemoved.disconnect();
> >> +}
> >> +
> >> +void PyCameraManager::init()
> >> +{
> >> +	/* Always enable RequestCompleted for all cameras by default */
> >> +	for (auto cam : cameraManager_->cameras())
> >> +		setCameraEventFlag(cam, CameraEventType::RequestCompleted, true);
> > 
> > You need to also enable the event when a new camera is detected.
> 
> Ah, good point. But I also wonder, should this just be dropped too? I'm 
> not sure if the return-events-from-stop and this one are just hacks that 
> seemingly make life easier for the users, but in the end just add 
> confusion for both the user and in the codebase. Or are they actually 
> good features...

Would enabling all events unconditionally have a big impact on
performance ?

> >>   }
> >>   
> >>   py::list PyCameraManager::cameras()
> >> @@ -60,6 +74,43 @@ py::list PyCameraManager::cameras()
> >>   	return l;
> >>   }
> >>   
> >> +PyCameraEvent PyCameraManager::convertEvent(const CameraEvent &event)
> >> +{
> >> +	/*
> >> +	 * We need to set a keep-alive here so that the camera keeps the
> >> +	 * camera manager alive.
> >> +	 */
> >> +	py::object py_cm = py::cast(this);
> >> +	py::object py_cam = py::cast(event.camera_);
> >> +	py::detail::keep_alive_impl(py_cam, py_cm);
> >> +
> >> +	PyCameraEvent pyevent(event.type_, py_cam);
> >> +
> >> +	switch (event.type_) {
> >> +	case CameraEventType::CameraAdded:
> >> +	case CameraEventType::CameraRemoved:
> >> +	case CameraEventType::Disconnect:
> >> +		/* No additional parameters to add */
> >> +		break;
> >> +
> >> +	case CameraEventType::BufferCompleted:
> >> +		pyevent.request_ = py::cast(event.request_);
> >> +		pyevent.fb_ = py::cast(event.fb_);
> >> +		break;
> >> +
> >> +	case CameraEventType::RequestCompleted:
> >> +		pyevent.request_ = py::cast(event.request_);
> >> +
> >> +		/* Decrease the ref increased in Camera.queue_request() */
> >> +		pyevent.request_.dec_ref();
> >> +
> >> +		break;
> >> +	}
> >> +
> >> +	return pyevent;
> >> +}
> >> +
> >> +/* DEPRECATED */
> >>   std::vector<py::object> PyCameraManager::getReadyRequests()
> >>   {
> >>   	int ret = readFd();
> >> @@ -72,21 +123,207 @@ std::vector<py::object> PyCameraManager::getReadyRequests()
> >>   
> >>   	std::vector<py::object> py_reqs;
> >>   
> >> -	for (Request *request : getCompletedRequests()) {
> >> -		py::object o = py::cast(request);
> >> -		/* Decrease the ref increased in Camera.queue_request() */
> >> -		o.dec_ref();
> >> -		py_reqs.push_back(o);
> >> +	for (const auto &ev : getEvents()) {
> >> +		if (ev.type_ != CameraEventType::RequestCompleted)
> >> +			continue;
> >> +
> >> +		PyCameraEvent pyev = convertEvent(ev);
> >> +		py_reqs.push_back(pyev.request_);
> >>   	}
> >>   
> >>   	return py_reqs;
> >>   }
> >>   
> >> +std::vector<PyCameraEvent> PyCameraManager::getPyEvents()
> >> +{
> >> +	int ret = readFd();
> >> +
> >> +	if (ret == EAGAIN) {
> >> +		LOG(Python, Debug) << "No events";
> >> +		return {};
> >> +	}
> >> +
> >> +	if (ret != 0)
> >> +		throw std::system_error(ret, std::generic_category());
> >> +
> >> +	std::vector<CameraEvent> events = getEvents();
> >> +
> >> +	LOG(Python, Debug) << "Got " << events.size() << " events";
> >> +
> >> +	std::vector<PyCameraEvent> pyevents;
> >> +	pyevents.reserve(events.size());
> >> +
> >> +	std::transform(events.begin(), events.end(), std::back_inserter(pyevents),
> >> +		       [this](const CameraEvent &ev) {
> >> +			       return convertEvent(ev);
> >> +		       });
> >> +
> >> +	return pyevents;
> >> +}
> >> +
> >> +static bool isCameraSpecificEvent(const CameraEvent &event, std::shared_ptr<Camera> &camera)
> >> +{
> >> +	return event.camera_ == camera &&
> >> +	       (event.type_ == CameraEventType::RequestCompleted ||
> >> +		event.type_ == CameraEventType::BufferCompleted ||
> >> +		event.type_ == CameraEventType::Disconnect);
> >> +}
> >> +
> >> +std::vector<PyCameraEvent> PyCameraManager::getPyCameraEvents(std::shared_ptr<Camera> camera)
> >> +{
> >> +	std::vector<CameraEvent> events;
> >> +	size_t unhandled_size;
> >> +
> >> +	{
> >> +		MutexLocker guard(eventsMutex_);
> >> +
> >> +		/*
> >> +		 * Collect events related to the given camera and remove them
> >> +		 * from the events_ vector.
> >> +		 */
> >> +
> >> +		auto it = events_.begin();
> >> +		while (it != events_.end()) {
> >> +			if (isCameraSpecificEvent(*it, camera)) {
> >> +				events.push_back(*it);
> >> +				it = events_.erase(it);
> >> +			} else {
> >> +				it++;
> >> +			}
> >> +		}
> >> +
> >> +		unhandled_size = events_.size();
> >> +	}
> >> +
> >> +	/* Convert events to Python events */
> >> +
> >> +	std::vector<PyCameraEvent> pyevents;
> >> +
> >> +	for (const auto &event : events) {
> >> +		PyCameraEvent pyev = convertEvent(event);
> >> +		pyevents.push_back(pyev);
> >> +	}
> >> +
> >> +	LOG(Python, Debug) << "Got " << pyevents.size() << " camera events, "
> >> +			   << unhandled_size << " unhandled events left";
> >> +
> >> +	return pyevents;
> >> +}
> >> +
> >>   /* Note: Called from another thread */
> >> -void PyCameraManager::handleRequestCompleted(Request *req)
> >> +void PyCameraManager::handleBufferCompleted(std::shared_ptr<Camera> cam, Request *req, FrameBuffer *fb)
> >>   {
> >> -	pushRequest(req);
> >> -	writeFd();
> >> +	CameraEvent ev(CameraEventType::BufferCompleted, cam, req, fb);
> >> +
> >> +	pushEvent(ev);
> >> +}
> >> +
> >> +/* Note: Called from another thread */
> >> +void PyCameraManager::handleRequestCompleted(std::shared_ptr<Camera> cam, Request *req)
> >> +{
> >> +	CameraEvent ev(CameraEventType::RequestCompleted, cam, req);
> >> +
> >> +	pushEvent(ev);
> >> +}
> >> +
> >> +/* Note: Called from another thread */
> >> +void PyCameraManager::handleDisconnected(std::shared_ptr<Camera> cam)
> >> +{
> >> +	CameraEvent ev(CameraEventType::Disconnect, cam);
> >> +
> >> +	pushEvent(ev);
> >> +}
> >> +
> >> +/* Note: Called from another thread */
> >> +void PyCameraManager::handleCameraAdded(std::shared_ptr<Camera> cam)
> >> +{
> >> +	CameraEvent ev(CameraEventType::CameraAdded, cam);
> >> +
> >> +	pushEvent(ev);
> >> +}
> >> +
> >> +/* Note: Called from another thread */
> >> +void PyCameraManager::handleCameraRemoved(std::shared_ptr<Camera> cam)
> >> +{
> >> +	CameraEvent ev(CameraEventType::CameraRemoved, cam);
> >> +
> >> +	pushEvent(ev);
> >> +}
> >> +
> >> +bool PyCameraManager::getCameraEventFlag(std::shared_ptr<Camera> camera, CameraEventType event_type)
> >> +{
> >> +	const uint32_t evbit = 1 << (uint32_t)event_type;
> >> +	uint32_t mask = 0;
> >> +
> >> +	if (auto it = camera_event_masks_.find(camera); it != camera_event_masks_.end())
> > 
> > That's a weird style.
> 
> Weird how? That's standard c++ style... It prevents the 'it' from 
> leaking outside the if block.

s/weird/unusual in libcamera/

> > 	auto it = camera_event_masks_.find(camera);
> > 	if (it != camera_event_masks_.end())
> > 
> >> +		mask = it->second;
> >> +
> >> +	return !!(mask & evbit);
> >> +}
> >> +
> >> +void PyCameraManager::setCameraEventFlag(std::shared_ptr<Camera> camera, CameraEventType event_type, bool value)
> >> +{
> >> +	switch (event_type) {
> >> +	case CameraEventType::RequestCompleted:
> >> +	case CameraEventType::BufferCompleted:
> >> +	case CameraEventType::Disconnect:
> >> +		break;
> >> +
> >> +	default:
> >> +		throw std::runtime_error("Bad camera event type");
> >> +	}
> >> +
> >> +	const uint32_t evbit = 1 << (uint32_t)event_type;
> >> +	uint32_t mask = 0;
> >> +
> >> +	if (auto it = camera_event_masks_.find(camera); it != camera_event_masks_.end())
> >> +		mask = it->second;
> > 
> > Same here.
> > 
> >> +
> >> +	bool old_val = !!(mask & evbit);
> >> +
> >> +	if (old_val == value)
> >> +		return;
> >> +
> >> +	if (value)
> >> +		mask |= evbit;
> >> +	else
> >> +		mask &= ~evbit;
> >> +
> >> +	camera_event_masks_[camera] = mask;
> >> +
> >> +	switch (event_type) {
> >> +	case CameraEventType::RequestCompleted:
> >> +		if (value) {
> >> +			camera->requestCompleted.connect(camera.get(), [cm = this->shared_from_this(), camera](Request *req) {
> >> +				cm->handleRequestCompleted(camera, req);
> >> +			});
> >> +		} else {
> >> +			camera->requestCompleted.disconnect();
> >> +		}
> >> +		break;
> > 
> > This is racy. PyCameraManager::handleRequestCompleted() could be called
> > before you disconnect the signal, with the event then stored in the
> > events_ queue. The application would then receive the event after
> > disabling it.
> 
> I don't think it's racy as such, that's "normal" for single threaded 
> application. The event could have happened at some point much before 
> this function is even called.

Consider the following code flow:

	cm.enable_camera_event(libcamera.Event.BufferCompleted)

	for event in cm.get_events():
		if event.type == libcamera.Event.BufferCompleted:
			raise RuntimeError("We have disabled this!")

Should a user really expect this to raise an exception ?

> We could drop here all the events from the event queue for the specific 
> event being disabled. It's perhaps a question of how these 
> enable/disable flags are defined.
> 
> Are they enabling/disabling the low-level event as such (this is how I 
> have been thinking about it), but they don't affect the event queue 
> (i.e. you can get now-disabled events from the event queue).
> 
> Or are they enabling/disabling what the get_events() returns.

Whichever option we pick, it has to be clearly documented.

> To me the former feels more natural, but I can see the logic in the 
> latter option too.
> 
> > One option would be to connect the signals unconditionally and instead
> > filter events out when retrieving them in getPyEvents().
> 
> The whole point of these flags is to avoid all the extra allocations and 
> storing of the events that the user doesn't want.

What's the performance impact, are we over-engineering something here ?

> >> +
> >> +	case CameraEventType::BufferCompleted:
> >> +		if (value) {
> >> +			camera->bufferCompleted.connect(camera.get(), [cm = this->shared_from_this(), camera](Request *req, FrameBuffer *fb) {
> >> +				cm->handleBufferCompleted(camera, req, fb);
> >> +			});
> >> +		} else {
> >> +			camera->bufferCompleted.disconnect();
> >> +		}
> >> +		break;
> >> +
> >> +	case CameraEventType::Disconnect:
> >> +		if (value) {
> >> +			camera->disconnected.connect(camera.get(), [cm = this->shared_from_this(), camera]() {
> >> +				cm->handleDisconnected(camera);
> >> +			});
> >> +		} else {
> >> +			camera->disconnected.disconnect();
> >> +		}
> >> +		break;
> >> +	default:
> >> +		ASSERT(false);
> >> +	}
> >>   }
> >>   
> >>   void PyCameraManager::writeFd()
> >> @@ -116,16 +353,24 @@ int PyCameraManager::readFd()
> >>   		return -EIO;
> >>   }
> >>   
> >> -void PyCameraManager::pushRequest(Request *req)
> >> +void PyCameraManager::pushEvent(const CameraEvent &ev)
> >>   {
> >> -	MutexLocker guard(completedRequestsMutex_);
> >> -	completedRequests_.push_back(req);
> >> +	{
> >> +		MutexLocker guard(eventsMutex_);
> >> +		events_.push_back(ev);
> >> +	}
> >> +
> >> +	writeFd();
> >> +
> >> +	LOG(Python, Debug) << "Queued events: " << events_.size();
> >>   }
> >>   
> >> -std::vector<Request *> PyCameraManager::getCompletedRequests()
> >> +std::vector<CameraEvent> PyCameraManager::getEvents()
> >>   {
> >> -	std::vector<Request *> v;
> >> -	MutexLocker guard(completedRequestsMutex_);
> >> -	swap(v, completedRequests_);
> >> +	std::vector<CameraEvent> v;
> >> +
> >> +	MutexLocker guard(eventsMutex_);
> >> +	swap(v, events_);
> >> +
> >>   	return v;
> >>   }
> >> diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h
> >> index 3574db23..31747547 100644
> >> --- a/src/py/libcamera/py_camera_manager.h
> >> +++ b/src/py/libcamera/py_camera_manager.h
> >> @@ -13,12 +13,56 @@
> >>   
> >>   using namespace libcamera;
> >>   
> >> -class PyCameraManager
> >> +enum class CameraEventType {
> >> +	CameraAdded,
> >> +	CameraRemoved,
> >> +	Disconnect,
> >> +	RequestCompleted,
> >> +	BufferCompleted,
> >> +};
> >> +
> >> +/*
> >> + * This event struct is used internally to queue the events we receive from
> >> + * other threads.
> >> + */
> >> +struct CameraEvent {
> >> +	CameraEvent(CameraEventType type, std::shared_ptr<Camera> camera,
> >> +		    Request *request = nullptr, FrameBuffer *fb = nullptr)
> >> +		: type_(type), camera_(camera), request_(request), fb_(fb)
> >> +	{
> >> +	}
> >> +
> >> +	CameraEventType type_;
> >> +	std::shared_ptr<Camera> camera_;
> >> +	Request *request_;
> >> +	FrameBuffer *fb_;
> >> +};
> >> +
> >> +/*
> >> + * This event struct is passed to Python. We need to use pybind11::object here
> >> + * instead of a C++ pointer so that we keep a ref to the Request, and a
> >> + * keep-alive from the camera to the camera manager.
> >> + */
> >> +struct PyCameraEvent {
> >> +	PyCameraEvent(CameraEventType type, pybind11::object camera)
> >> +		: type_(type), camera_(camera)
> >> +	{
> >> +	}
> >> +
> >> +	CameraEventType type_;
> >> +	pybind11::object camera_;
> >> +	pybind11::object request_;
> >> +	pybind11::object fb_;
> >> +};
> >> +
> >> +class PyCameraManager : public std::enable_shared_from_this<PyCameraManager>
> >>   {
> >>   public:
> >>   	PyCameraManager();
> >>   	~PyCameraManager();
> >>   
> >> +	void init();
> >> +
> >>   	pybind11::list cameras();
> >>   	std::shared_ptr<Camera> get(const std::string &name) { return cameraManager_->get(name); }
> >>   
> >> @@ -26,20 +70,33 @@ public:
> >>   
> >>   	int eventFd() const { return eventFd_.get(); }
> >>   
> >> -	std::vector<pybind11::object> getReadyRequests();
> >> +	std::vector<pybind11::object> getReadyRequests(); /* DEPRECATED */
> >> +	std::vector<PyCameraEvent> getPyEvents();
> >> +	std::vector<PyCameraEvent> getPyCameraEvents(std::shared_ptr<Camera> camera);
> >> +
> >> +	void handleBufferCompleted(std::shared_ptr<Camera> cam, Request *req, FrameBuffer *fb);
> >> +	void handleRequestCompleted(std::shared_ptr<Camera> cam, Request *req);
> >> +	void handleDisconnected(std::shared_ptr<Camera> cam);
> >> +	void handleCameraAdded(std::shared_ptr<Camera> cam);
> >> +	void handleCameraRemoved(std::shared_ptr<Camera> cam);
> >>   
> >> -	void handleRequestCompleted(Request *req);
> >> +	bool getCameraEventFlag(std::shared_ptr<Camera> camera, CameraEventType event_type);
> >> +	void setCameraEventFlag(std::shared_ptr<Camera> camera, CameraEventType event_type, bool value);
> >>   
> >>   private:
> >>   	std::unique_ptr<CameraManager> cameraManager_;
> >>   
> >>   	UniqueFD eventFd_;
> >> -	libcamera::Mutex completedRequestsMutex_;
> >> -	std::vector<Request *> completedRequests_
> >> -		LIBCAMERA_TSA_GUARDED_BY(completedRequestsMutex_);
> >> +	libcamera::Mutex eventsMutex_;
> >> +	std::vector<CameraEvent> events_
> >> +		LIBCAMERA_TSA_GUARDED_BY(eventsMutex_);
> >>   
> >>   	void writeFd();
> >>   	int readFd();
> >> -	void pushRequest(Request *req);
> >> -	std::vector<Request *> getCompletedRequests();
> >> +	void pushEvent(const CameraEvent &ev);
> >> +	std::vector<CameraEvent> getEvents();
> >> +
> >> +	PyCameraEvent convertEvent(const CameraEvent &event);
> >> +
> >> +	std::map<std::shared_ptr<Camera>, uint32_t> camera_event_masks_;
> >>   };
> >> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> >> index 5a5f1a37..981a3070 100644
> >> --- a/src/py/libcamera/py_main.cpp
> >> +++ b/src/py/libcamera/py_main.cpp
> >> @@ -110,6 +110,7 @@ PYBIND11_MODULE(_libcamera, m)
> >>   	 * https://pybind11.readthedocs.io/en/latest/advanced/misc.html#avoiding-c-types-in-docstrings
> >>   	 */
> >>   
> >> +	auto pyEvent = py::class_<PyCameraEvent>(m, "Event");
> >>   	auto pyCameraManager = py::class_<PyCameraManager, std::shared_ptr<PyCameraManager>>(m, "CameraManager");
> >>   	auto pyCamera = py::class_<Camera, PyCameraSmartPtr<Camera>>(m, "Camera");
> >>   	auto pyCameraConfiguration = py::class_<CameraConfiguration>(m, "CameraConfiguration");
> >> @@ -136,12 +137,27 @@ PYBIND11_MODULE(_libcamera, m)
> >>   	m.def("log_set_level", &logSetLevel);
> >>   
> >>   	/* Classes */
> >> +
> >> +	py::enum_<CameraEventType>(pyEvent, "Type")
> >> +		.value("CameraAdded", CameraEventType::CameraAdded)
> >> +		.value("CameraRemoved", CameraEventType::CameraRemoved)
> >> +		.value("Disconnect", CameraEventType::Disconnect)
> >> +		.value("RequestCompleted", CameraEventType::RequestCompleted)
> >> +		.value("BufferCompleted", CameraEventType::BufferCompleted);
> >> +
> >> +	pyEvent
> >> +		.def_readonly("type", &PyCameraEvent::type_)
> >> +		.def_readonly("camera", &PyCameraEvent::camera_)
> >> +		.def_readonly("request", &PyCameraEvent::request_)
> >> +		.def_readonly("fb", &PyCameraEvent::fb_);
> >> +
> >>   	pyCameraManager
> >>   		.def_static("singleton", []() {
> >>   			std::shared_ptr<PyCameraManager> cm = gCameraManager.lock();
> >>   
> >>   			if (!cm) {
> >>   				cm = std::make_shared<PyCameraManager>();
> >> +				cm->init();
> >>   				gCameraManager = cm;
> >>   			}
> >>   
> >> @@ -153,10 +169,33 @@ PYBIND11_MODULE(_libcamera, m)
> >>   		.def_property_readonly("cameras", &PyCameraManager::cameras)
> >>   
> >>   		.def_property_readonly("event_fd", &PyCameraManager::eventFd)
> >> -		.def("get_ready_requests", &PyCameraManager::getReadyRequests);
> >> +
> >> +		/* DEPRECATED */
> >> +		.def("get_ready_requests", &PyCameraManager::getReadyRequests)
> >> +
> >> +		.def("get_events", &PyCameraManager::getPyEvents);
> >>   
> >>   	pyCamera
> >>   		.def_property_readonly("id", &Camera::id)
> >> +
> >> +		.def("get_camera_event_enabled",
> >> +			[](Camera &self, CameraEventType type) {
> >> +				auto cm = gCameraManager.lock();
> >> +				return cm->getCameraEventFlag(self.shared_from_this(), type);
> >> +			})
> >> +
> >> +		.def("enable_camera_event",
> >> +			[](Camera &self, CameraEventType type) {
> >> +				auto cm = gCameraManager.lock();
> >> +				cm->setCameraEventFlag(self.shared_from_this(), type, true);
> >> +			})
> >> +
> >> +		.def("disable_camera_event",
> >> +			[](Camera &self, CameraEventType type) {
> >> +				auto cm = gCameraManager.lock();
> >> +				cm->setCameraEventFlag(self.shared_from_this(), type, false);
> >> +			})
> >> +
> >>   		.def("acquire", [](Camera &self) {
> >>   			int ret = self.acquire();
> >>   			if (ret)
> >> @@ -173,11 +212,6 @@ PYBIND11_MODULE(_libcamera, m)
> >>   		                 const std::unordered_map<const ControlId *, py::object> &controls) {
> >>   			/* \todo What happens if someone calls start() multiple times? */
> >>   
> >> -			auto cm = gCameraManager.lock();
> >> -			ASSERT(cm);
> >> -
> >> -			self.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted);
> >> -
> >>   			ControlList controlList(self.controls());
> >>   
> >>   			for (const auto &[id, obj] : controls) {
> >> @@ -187,7 +221,6 @@ PYBIND11_MODULE(_libcamera, m)
> >>   
> >>   			int ret = self.start(&controlList);
> >>   			if (ret) {
> >> -				self.requestCompleted.disconnect();
> >>   				throw std::system_error(-ret, std::generic_category(),
> >>   							"Failed to start camera");
> >>   			}
> > 
> > You can drop the curly braces.
> 
> Indeed.
> 
> >> @@ -196,11 +229,16 @@ PYBIND11_MODULE(_libcamera, m)
> >>   		.def("stop", [](Camera &self) {
> >>   			int ret = self.stop();
> >>   
> >> -			self.requestCompleted.disconnect();
> >> +			auto cm = gCameraManager.lock();
> >> +			ASSERT(cm);
> >> +
> >> +			auto events = cm->getPyCameraEvents(self.shared_from_this());
> >>   
> >>   			if (ret)
> >>   				throw std::system_error(-ret, std::generic_category(),
> >>   							"Failed to stop camera");
> >> +
> >> +			return events;
> >>   		})
> >>   
> >>   		.def("__str__", [](Camera &self) {