[libcamera-devel,v4,02/15] py: New event handling

18363 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
camera. This serves two purposes: first, it removes the requestCompleted
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.

All other event types are always collected and returned, except
bufferCompleted which needs to be enabled by setting the
CameraManager.buffer_completed_active to True. This is to reduce the
overhead a bit. It is not clear if this is a necessary optimization or
not.

TODO: Documentation.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 src/py/libcamera/py_camera_manager.cpp | 195 +++++++++++++++++++++++--
 src/py/libcamera/py_camera_manager.h   |  66 ++++++++-
 src/py/libcamera/py_main.cpp           |  44 +++++-
 3 files changed, 281 insertions(+), 24 deletions(-)

Hi Tomi,

Thank you for the patch.

On Thu, Mar 09, 2023 at 04:25:48PM +0200, Tomi Valkeinen via libcamera-devel 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
> camera. This serves two purposes: first, it removes the requestCompleted
> 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.
> 
> All other event types are always collected and returned, except
> bufferCompleted which needs to be enabled by setting the
> CameraManager.buffer_completed_active to True. This is to reduce the
> overhead a bit. It is not clear if this is a necessary optimization or
> not.
> 
> TODO: Documentation.

Looks like we're converging on the API, so it's time to write
documentation :-)

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  src/py/libcamera/py_camera_manager.cpp | 195 +++++++++++++++++++++++--
>  src/py/libcamera/py_camera_manager.h   |  66 ++++++++-
>  src/py/libcamera/py_main.cpp           |  44 +++++-
>  3 files changed, 281 insertions(+), 24 deletions(-)
> 
> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp
> index 9ccb7aad..7d6dded4 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,17 @@ 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();
>  }
>  
>  py::list PyCameraManager::cameras()
> @@ -60,6 +67,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 +116,134 @@ 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);

Could you check my latest reply to the v3 mail thread for this function
(message ID Yw5ki0Tmycz9Uk6A@pendragon.ideasonboard.com) ? I think we
haven't finished the discussion.

> +}
> +
> +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);

Erasing elements in the middle of a vactor is quite inefficient. We've
discussed this in the review of v4. Could you confirm you have deviced
to not switch to a std::list, and not just forgotten about 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();
> +	if (!bufferCompletedEventActive_)
> +		return;
> +
> +	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);
>  }
>  
>  void PyCameraManager::writeFd()
> @@ -116,16 +273,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 3525057d..757f6d8e 100644
> --- a/src/py/libcamera/py_camera_manager.h
> +++ b/src/py/libcamera/py_camera_manager.h
> @@ -13,6 +13,48 @@
>  
>  using namespace libcamera;
>  
> +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:
> @@ -26,20 +68,30 @@ 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 bufferCompletedEventActive_ = false;
>  
>  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);
>  };
> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> index 1d4c23b3..0fffc030 100644
> --- a/src/py/libcamera/py_main.cpp
> +++ b/src/py/libcamera/py_main.cpp
> @@ -61,6 +61,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");

Alphabetical order ?

>  	auto pyCameraManager = py::class_<PyCameraManager>(m, "CameraManager");
>  	auto pyCamera = py::class_<Camera>(m, "Camera");
>  	auto pyCameraConfiguration = py::class_<CameraConfiguration>(m, "CameraConfiguration");
> @@ -93,6 +94,20 @@ PYBIND11_MODULE(_libcamera, m)
>  	m.def("log_set_level", &logSetLevel);
>  
>  	/* Classes */
> +

Extra blank line ?

> +	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();
> @@ -110,7 +125,13 @@ 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)
> +
> +		.def_readwrite("buffer_completed_active", &PyCameraManager::bufferCompletedEventActive_);
>  
>  	pyCamera
>  		.def_property_readonly("id", &Camera::id)
> @@ -133,7 +154,17 @@ PYBIND11_MODULE(_libcamera, m)
>  			auto cm = gCameraManager.lock();
>  			ASSERT(cm);
>  
> -			self.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted);
> +			self.requestCompleted.connect(&self, [cm, camera=self.shared_from_this()](Request *req) {
> +				cm->handleRequestCompleted(camera, req);
> +			});
> +
> +			self.bufferCompleted.connect(&self, [cm, camera=self.shared_from_this()](Request *req, FrameBuffer *fb) {
> +				cm->handleBufferCompleted(camera, req, fb);
> +			});
> +
> +			self.disconnected.connect(&self, [cm, camera=self.shared_from_this()]() {
> +				cm->handleDisconnected(camera);
> +			});
>  
>  			ControlList controlList(self.controls());
>  
> @@ -154,10 +185,19 @@ PYBIND11_MODULE(_libcamera, m)
>  			int ret = self.stop();
>  
>  			self.requestCompleted.disconnect();
> +			self.bufferCompleted.disconnect();
> +			self.disconnected.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) {

Hi Laurent,

On 12/03/2023 17:44, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Thu, Mar 09, 2023 at 04:25:48PM +0200, Tomi Valkeinen via libcamera-devel 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
>> camera. This serves two purposes: first, it removes the requestCompleted
>> 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.
>>
>> All other event types are always collected and returned, except
>> bufferCompleted which needs to be enabled by setting the
>> CameraManager.buffer_completed_active to True. This is to reduce the
>> overhead a bit. It is not clear if this is a necessary optimization or
>> not.
>>
>> TODO: Documentation.
> 
> Looks like we're converging on the API, so it's time to write
> documentation :-)

Well, I don't know about that. I haven't really gotten any comments 
about the API. And the RFC hack patch at the end of the series 
introduces another style. But probably I just have to pick an approach, 
write the docs, and send a pull req, as otherwise we're still here the 
next year =).

>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   src/py/libcamera/py_camera_manager.cpp | 195 +++++++++++++++++++++++--
>>   src/py/libcamera/py_camera_manager.h   |  66 ++++++++-
>>   src/py/libcamera/py_main.cpp           |  44 +++++-
>>   3 files changed, 281 insertions(+), 24 deletions(-)
>>
>> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp
>> index 9ccb7aad..7d6dded4 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,17 @@ 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();
>>   }
>>   
>>   py::list PyCameraManager::cameras()
>> @@ -60,6 +67,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 +116,134 @@ 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);
> 
> Could you check my latest reply to the v3 mail thread for this function
> (message ID Yw5ki0Tmycz9Uk6A@pendragon.ideasonboard.com) ? I think we
> haven't finished the discussion.

Yes, I mentioned this in the intro letter.

If we use the style used in this patch, I still think it makes sense to 
handle all three event types here. cam.start() connects those events, 
cam.stop() disconnects, so it makes sense that cam.stop() also returns 
all those events.

If we go with something like in the last patch, where the event 
"subscription" is outside the start and stop methods, it's not so clear. 
Maybe stop() shouldn't return any events in that case, although then we 
might want to have another method to get and remove the events for that 
camera, so that the user can flush the events.

If I remember right, I have never managed to get a Disconnect event, so 
I don't quite know when they come or how one would use them. So I don't 
have very strong feelings here.

>> +}
>> +
>> +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);
> 
> Erasing elements in the middle of a vactor is quite inefficient. We've
> discussed this in the review of v4. Could you confirm you have deviced
> to not switch to a std::list, and not just forgotten about it ?

I did move the conversion to py events to be outside the lock.

As for the vector, yes, erasing is inefficient, but so is allocating and 
freeing memory. The number of events is small (I get 4 even if using 
multiple cameras). Afaics, we have to options:

- Use vector. There is some overhead when camera.stop() is called.
- Use list. There is some overhead while capturing frames.

In both cases I think the overhead is unmeasurable in practice, but 
choosing the overhead for .stop() sounds better to me.

Possibly the code could be faster if we just create a new vector for the 
unhandled events, instead of using erase(). This would be the case at 
least in the normal case (at least for me) where all the events are for 
the camera being stopped, as there would be no unhandled events. But I 
think the difference is again unmeasurable.

In any case, this is something we can easily tune later.

>> +			} 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();
>> +	if (!bufferCompletedEventActive_)
>> +		return;
>> +
>> +	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);
>>   }
>>   
>>   void PyCameraManager::writeFd()
>> @@ -116,16 +273,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 3525057d..757f6d8e 100644
>> --- a/src/py/libcamera/py_camera_manager.h
>> +++ b/src/py/libcamera/py_camera_manager.h
>> @@ -13,6 +13,48 @@
>>   
>>   using namespace libcamera;
>>   
>> +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:
>> @@ -26,20 +68,30 @@ 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 bufferCompletedEventActive_ = false;
>>   
>>   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);
>>   };
>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
>> index 1d4c23b3..0fffc030 100644
>> --- a/src/py/libcamera/py_main.cpp
>> +++ b/src/py/libcamera/py_main.cpp
>> @@ -61,6 +61,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");
> 
> Alphabetical order ?

They're not in alphabetical order. Now that we use the style where we 
declare the types before we define them we might order everything 
alphabetically. But that will be a big change, causing big conflicts. So 
I'd rather do that at some other time.

>>   	auto pyCameraManager = py::class_<PyCameraManager>(m, "CameraManager");
>>   	auto pyCamera = py::class_<Camera>(m, "Camera");
>>   	auto pyCameraConfiguration = py::class_<CameraConfiguration>(m, "CameraConfiguration");
>> @@ -93,6 +94,20 @@ PYBIND11_MODULE(_libcamera, m)
>>   	m.def("log_set_level", &logSetLevel);
>>   
>>   	/* Classes */
>> +
> 
> Extra blank line ?

I think it looks better with a blank line.

>> +	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();
>> @@ -110,7 +125,13 @@ 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)
>> +
>> +		.def_readwrite("buffer_completed_active", &PyCameraManager::bufferCompletedEventActive_);
>>   
>>   	pyCamera
>>   		.def_property_readonly("id", &Camera::id)
>> @@ -133,7 +154,17 @@ PYBIND11_MODULE(_libcamera, m)
>>   			auto cm = gCameraManager.lock();
>>   			ASSERT(cm);
>>   
>> -			self.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted);
>> +			self.requestCompleted.connect(&self, [cm, camera=self.shared_from_this()](Request *req) {
>> +				cm->handleRequestCompleted(camera, req);
>> +			});
>> +
>> +			self.bufferCompleted.connect(&self, [cm, camera=self.shared_from_this()](Request *req, FrameBuffer *fb) {
>> +				cm->handleBufferCompleted(camera, req, fb);
>> +			});
>> +
>> +			self.disconnected.connect(&self, [cm, camera=self.shared_from_this()]() {
>> +				cm->handleDisconnected(camera);
>> +			});
>>   
>>   			ControlList controlList(self.controls());
>>   
>> @@ -154,10 +185,19 @@ PYBIND11_MODULE(_libcamera, m)
>>   			int ret = self.stop();
>>   
>>   			self.requestCompleted.disconnect();
>> +			self.bufferCompleted.disconnect();
>> +			self.disconnected.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) {
>