[libcamera-devel,v3,11/17] py: New event handling

16504 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 | 191 +++++++++++++++++++++++--
 src/py/libcamera/py_camera_manager.h   |  64 ++++++++-
 src/py/libcamera/py_main.cpp           |  45 +++++-
 3 files changed, 276 insertions(+), 24 deletions(-)

Hi Tomi,

Thank you for the patch.

On Fri, Jul 01, 2022 at 11:45:15AM +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.

And we will likely have error events in the not too distant future.

> This patch makes all those events available to the users.
> 
> The get_ready_requests() method is now deprecated, but available to keep
> backward compatibility.

Who are the users of get_ready_requests() that you are concern about,
and when could it be removed ? Other changes in this series break the
API (such as switching to exceptions), so all users will need to be
updated, I'd rather remove get_ready_requests() at the same time.

> 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.

As mentioned in the reply to the cover letter, we could have a more
generic subscription mechanism, but I also think it may not make much
sense to make subscription optional for all the other events, perhaps
with the exception of a future metadata completion event.

If only the buffer and metadata completion events are optional, should
we have a per-camera subscription ?

> TODO: Documentation.

This should be captured in the code. Or documentation could be written
:-)

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  src/py/libcamera/py_camera_manager.cpp | 191 +++++++++++++++++++++++--
>  src/py/libcamera/py_camera_manager.h   |  64 ++++++++-
>  src/py/libcamera/py_main.cpp           |  45 +++++-
>  3 files changed, 276 insertions(+), 24 deletions(-)
> 
> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp
> index 3dd8668e..d1d63690 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 <sys/eventfd.h>
>  #include <system_error>
> @@ -33,11 +34,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()
> @@ -58,6 +65,50 @@ py::list PyCameraManager::cameras()
>  	return l;
>  }
>  
> +PyCameraEvent PyCameraManager::convertEvent(const CameraEvent &event)
> +{
> +	PyCameraEvent pyevent;
> +
> +	pyevent.type_ = event.type_;
> +
> +	/*
> +	 * 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);
> +
> +	pyevent.camera_ = 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;
> +	}

No need for curly braces. But a blank line would be nice.

> +	case CameraEventType::RequestCompleted: {
> +		pyevent.request_ = py::cast(event.request_);
> +
> +		/* Decrease the ref increased in Camera.queue_request() */
> +		pyevent.request_.dec_ref();
> +
> +		break;
> +	}
> +	default:
> +		ASSERT(false);

What if we dropped the undefined event type ? You wouldn't need this
then. It's best to write code that makes errors impossible.

I think I would also move all of this, except the py::cast(this) call,
to a PyCameraEvent constructor that takes the CameraEvent and camera
manager py::object.

> +	}
> +
> +	return pyevent;
> +}
> +
> +/* DEPRECATED */
>  std::vector<py::object> PyCameraManager::getReadyRequests()
>  {
>  	int ret = readFd();
> @@ -70,21 +121,125 @@ 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;

Ouch, this will silently drop all other types of events. I'd really like
to drop getReadyRequests().

> +
> +		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) << "Get " << events.size() << " events";

s/Get/Got/

> +
> +	std::vector<PyCameraEvent> pyevents(events.size());

You'll end up creating events.size() default-constructed elements here,
only to overwrite them just after. I think calling .reserve() and using
a std::back_inserter in the std::transform loop below would be more
efficient (see https://en.cppreference.com/w/cpp/algorithm/transform).

> +
> +	std::transform(events.begin(), events.end(), pyevents.begin(),
> +		       [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);

I don't think you should include disconnect events here. This would
result in stop() removing disconnect events from the queue, while I
think they need to be handled through the normal event handling
mechanism.

> +}
> +
> +std::vector<PyCameraEvent> PyCameraManager::getPyCameraEvents(std::shared_ptr<Camera> camera)
> +{
> +	MutexLocker guard(eventsMutex_);
> +
> +	std::vector<PyCameraEvent> pyevents;
> +
> +	/* Get events related to the given camera */
> +
> +	for (const auto &event : events_) {
> +		if (!isCameraSpecificEvent(event, camera))
> +			continue;
> +
> +		PyCameraEvent pyev = convertEvent(event);
> +		pyevents.push_back(pyev);
> +	}
> +
> +	/* Drop the events from the events_ list */
> +
> +	events_.erase(std::remove_if(events_.begin(), events_.end(),
> +		[&camera](const CameraEvent &ev) {
> +			return isCameraSpecificEvent(ev, camera);
> +		}),
> +		events_.end());

I'd like to minmize the time spent holding the lock. You can move the
events specific to this camera to a different vector without converting
them (and merge the two loops if possible), and the convert the events
without holding the lock.

Also, dropping elements in the middle of a vector is fairly inefficient.
I think you should use a list instead of a vector for events_, which
will allow you to efficiently move elements here with splice().

> +
> +	LOG(Python, Debug) << "Get " << pyevents.size() << " camera events, "

s/Get/Got/ ?

> +			   << events_.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);
> +	ev.request_ = req;
> +	ev.fb_ = fb;
> +
> +	pushEvent(ev);
> +}
> +
> +/* Note: Called from another thread */
> +void PyCameraManager::handleRequestCompleted(std::shared_ptr<Camera> cam, Request *req)
> +{
> +	CameraEvent ev(CameraEventType::RequestCompleted, cam);
> +	ev.request_ = 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()
> @@ -110,16 +265,22 @@ int PyCameraManager::readFd()
>  	return 0;
>  }
>  
> -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();

This can be done without holding the lock.

> +
> +	LOG(Python, Debug) << "Queued events: " << events_.size();

I'm tempted to drop this, it will make the log very chatty. Have you
found it useful to debug applications ?

>  }
>  
> -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..b313ba9b 100644
> --- a/src/py/libcamera/py_camera_manager.h
> +++ b/src/py/libcamera/py_camera_manager.h
> @@ -13,6 +13,47 @@
>  
>  using namespace libcamera;
>  
> +enum class CameraEventType {
> +	Undefined = 0,

Unless we drop this (which would be my preference), maybe Invalid
instead of Undefined ? People don't like undefined behaviours :-D

> +	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)
> +		: type_(type), camera_(camera)
> +	{
> +	}
> +
> +	CameraEventType type_ = CameraEventType::Undefined;
> +
> +	std::shared_ptr<Camera> camera_;
> +
> +	Request *request_ = nullptr;
> +	FrameBuffer *fb_ = nullptr;
> +};
> +
> +/*
> + * 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 {
> +	CameraEventType type_ = CameraEventType::Undefined;
> +
> +	pybind11::object camera_;
> +
> +	pybind11::object request_;
> +	pybind11::object fb_;
> +};
> +
>  class PyCameraManager
>  {
>  public:
> @@ -26,20 +67,29 @@ 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 handleRequestCompleted(Request *req);
> +	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);
>  
> +	bool bufferCompletedEventActive_ = false;

Missing blank line.

>  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 6cd99919..b4f756d7 100644
> --- a/src/py/libcamera/py_main.cpp
> +++ b/src/py/libcamera/py_main.cpp
> @@ -58,6 +58,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>(m, "CameraManager");
>  	auto pyCamera = py::class_<Camera>(m, "Camera");
>  	auto pyCameraConfiguration = py::class_<CameraConfiguration>(m, "CameraConfiguration");
> @@ -90,6 +91,21 @@ PYBIND11_MODULE(_libcamera, m)
>  	m.def("log_set_level", &logSetLevel);
>  
>  	/* Classes */
> +
> +	py::enum_<CameraEventType>(pyEvent, "Type")
> +		.value("Undefined", CameraEventType::Undefined)
> +		.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();
> @@ -107,7 +123,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 */

Is there any way to generate a warning at runtime when this gets used ?
If we drop it (depending on the outcome of the discussion in the commit
message) that will be even easier :-)

> +		.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)
> @@ -131,7 +153,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());
>  
> @@ -152,11 +184,20 @@ PYBIND11_MODULE(_libcamera, m)
>  			int ret = self.stop();
>  
>  			self.requestCompleted.disconnect();
> +			self.bufferCompleted.disconnect();
> +			self.disconnected.disconnect();
> +
> +			auto cm = gCameraManager.lock();
> +			ASSERT(cm);
> +
> +			auto l = cm->getPyCameraEvents(self.shared_from_this());

Please use a more descriptive variable name.

>  
>  			/* \todo Should we just ignore the error? */
>  			if (ret)
>  				throw std::system_error(-ret, std::generic_category(),
>  							"Failed to start camera");
> +
> +			return l;
>  		})
>  
>  		.def("__str__", [](Camera &self) {

On 19/08/2022 01:00, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Fri, Jul 01, 2022 at 11:45:15AM +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.
> 
> And we will likely have error events in the not too distant future.
> 
>> This patch makes all those events available to the users.
>>
>> The get_ready_requests() method is now deprecated, but available to keep
>> backward compatibility.
> 
> Who are the users of get_ready_requests() that you are concern about,
> and when could it be removed ? Other changes in this series break the
> API (such as switching to exceptions), so all users will need to be
> updated, I'd rather remove get_ready_requests() at the same time.

I'm fine with dropping get_ready_requests(). It is just so trivial to 
support it that I kept it, which also helped converting the examples, as 
I didn't have to convert everything in one commit.

>> 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.
> 
> As mentioned in the reply to the cover letter, we could have a more
> generic subscription mechanism, but I also think it may not make much
> sense to make subscription optional for all the other events, perhaps
> with the exception of a future metadata completion event.
> 
> If only the buffer and metadata completion events are optional, should
> we have a per-camera subscription ?

That's one option. I did try multiple different approaches, and 
everything seems to have their drawbacks. When I get some time I'll 
again try a few different things to refresh my memory.

>> TODO: Documentation.
> 
> This should be captured in the code. Or documentation could be written
> :-)

I'll write it before merging, when we're happy with the model. I don't 
want to write it until I'm happy with the code =).

>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   src/py/libcamera/py_camera_manager.cpp | 191 +++++++++++++++++++++++--
>>   src/py/libcamera/py_camera_manager.h   |  64 ++++++++-
>>   src/py/libcamera/py_main.cpp           |  45 +++++-
>>   3 files changed, 276 insertions(+), 24 deletions(-)
>>
>> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp
>> index 3dd8668e..d1d63690 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 <sys/eventfd.h>
>>   #include <system_error>
>> @@ -33,11 +34,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()
>> @@ -58,6 +65,50 @@ py::list PyCameraManager::cameras()
>>   	return l;
>>   }
>>   
>> +PyCameraEvent PyCameraManager::convertEvent(const CameraEvent &event)
>> +{
>> +	PyCameraEvent pyevent;
>> +
>> +	pyevent.type_ = event.type_;
>> +
>> +	/*
>> +	 * 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);
>> +
>> +	pyevent.camera_ = 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;
>> +	}
> 
> No need for curly braces. But a blank line would be nice.

Yep.

>> +	case CameraEventType::RequestCompleted: {
>> +		pyevent.request_ = py::cast(event.request_);
>> +
>> +		/* Decrease the ref increased in Camera.queue_request() */
>> +		pyevent.request_.dec_ref();
>> +
>> +		break;
>> +	}
>> +	default:
>> +		ASSERT(false);
> 
> What if we dropped the undefined event type ? You wouldn't need this
> then. It's best to write code that makes errors impossible.

That wouldn't make errors impossible, it would just hide possible bugs 
=). When something goes bad, event.type_ can contains an illegal value. 
I think it's good to catch it here.

> I think I would also move all of this, except the py::cast(this) call,
> to a PyCameraEvent constructor that takes the CameraEvent and camera
> manager py::object.

We also do the ref count magics for RequestCompleted. I'll think about 
it, but I probably like it better if we keep the PyCameraEvent as a 
dummy container.

>> +	}
>> +
>> +	return pyevent;
>> +}
>> +
>> +/* DEPRECATED */
>>   std::vector<py::object> PyCameraManager::getReadyRequests()
>>   {
>>   	int ret = readFd();
>> @@ -70,21 +121,125 @@ 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;
> 
> Ouch, this will silently drop all other types of events. I'd really like
> to drop getReadyRequests().

That's exactly as it was before.

>> +
>> +		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) << "Get " << events.size() << " events";
> 
> s/Get/Got/

Ok.

>> +
>> +	std::vector<PyCameraEvent> pyevents(events.size());
> 
> You'll end up creating events.size() default-constructed elements here,
> only to overwrite them just after. I think calling .reserve() and using
> a std::back_inserter in the std::transform loop below would be more
> efficient (see https://en.cppreference.com/w/cpp/algorithm/transform).

True. And changing this allows me to add a more specific constructor for 
PyCameraEvent.

>> +
>> +	std::transform(events.begin(), events.end(), pyevents.begin(),
>> +		       [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);
> 
> I don't think you should include disconnect events here. This would
> result in stop() removing disconnect events from the queue, while I
> think they need to be handled through the normal event handling
> mechanism.

Can you elaborate? The point of this is to drop all the events related 
to the camera. Also note that there's the CameraRemoved event from 
camera manager that is not dropped.

>> +}
>> +
>> +std::vector<PyCameraEvent> PyCameraManager::getPyCameraEvents(std::shared_ptr<Camera> camera)
>> +{
>> +	MutexLocker guard(eventsMutex_);
>> +
>> +	std::vector<PyCameraEvent> pyevents;
>> +
>> +	/* Get events related to the given camera */
>> +
>> +	for (const auto &event : events_) {
>> +		if (!isCameraSpecificEvent(event, camera))
>> +			continue;
>> +
>> +		PyCameraEvent pyev = convertEvent(event);
>> +		pyevents.push_back(pyev);
>> +	}
>> +
>> +	/* Drop the events from the events_ list */
>> +
>> +	events_.erase(std::remove_if(events_.begin(), events_.end(),
>> +		[&camera](const CameraEvent &ev) {
>> +			return isCameraSpecificEvent(ev, camera);
>> +		}),
>> +		events_.end());
> 
> I'd like to minmize the time spent holding the lock. You can move the
> events specific to this camera to a different vector without converting
> them (and merge the two loops if possible), and the convert the events
> without holding the lock.
> 
> Also, dropping elements in the middle of a vector is fairly inefficient.
> I think you should use a list instead of a vector for events_, which
> will allow you to efficiently move elements here with splice().

I'll have a look, but these sound a bit like pointless optimizations. 
This is called only when a camera is stopped, and we're talking about a 
few events.

Taking the conversion part outside the lock sounds feasible.

If we change the events from a vector to a list to speed up the code 
here, where it doesn't matter, are we making the code slower in other 
parts where it matters?

>> +
>> +	LOG(Python, Debug) << "Get " << pyevents.size() << " camera events, "
> 
> s/Get/Got/ ?

Ok.

>> +			   << events_.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);
>> +	ev.request_ = req;
>> +	ev.fb_ = fb;
>> +
>> +	pushEvent(ev);
>> +}
>> +
>> +/* Note: Called from another thread */
>> +void PyCameraManager::handleRequestCompleted(std::shared_ptr<Camera> cam, Request *req)
>> +{
>> +	CameraEvent ev(CameraEventType::RequestCompleted, cam);
>> +	ev.request_ = 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()
>> @@ -110,16 +265,22 @@ int PyCameraManager::readFd()
>>   	return 0;
>>   }
>>   
>> -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();
> 
> This can be done without holding the lock.

The writeFd()? Are you sure? We discussed this, I think, but... Well, 
I'd just rather be safe than sorry =).

>> +
>> +	LOG(Python, Debug) << "Queued events: " << events_.size();
> 
> I'm tempted to drop this, it will make the log very chatty. Have you
> found it useful to debug applications ?

For development, yes. It's very easy to have very obscure issues with 
the Python bindings. I think we can reduce the debug prints when things 
settle down with the bindings.

>>   }
>>   
>> -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..b313ba9b 100644
>> --- a/src/py/libcamera/py_camera_manager.h
>> +++ b/src/py/libcamera/py_camera_manager.h
>> @@ -13,6 +13,47 @@
>>   
>>   using namespace libcamera;
>>   
>> +enum class CameraEventType {
>> +	Undefined = 0,
> 
> Unless we drop this (which would be my preference), maybe Invalid
> instead of Undefined ? People don't like undefined behaviours :-D

And they like invalid behavior? =)

I'll see how I like it if I drop the Undefined value. Usually I like 
those, it's good to be able to initialize things to 0.

>> +	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)
>> +		: type_(type), camera_(camera)
>> +	{
>> +	}
>> +
>> +	CameraEventType type_ = CameraEventType::Undefined;
>> +
>> +	std::shared_ptr<Camera> camera_;
>> +
>> +	Request *request_ = nullptr;
>> +	FrameBuffer *fb_ = nullptr;
>> +};
>> +
>> +/*
>> + * 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 {
>> +	CameraEventType type_ = CameraEventType::Undefined;
>> +
>> +	pybind11::object camera_;
>> +
>> +	pybind11::object request_;
>> +	pybind11::object fb_;
>> +};
>> +
>>   class PyCameraManager
>>   {
>>   public:
>> @@ -26,20 +67,29 @@ 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 handleRequestCompleted(Request *req);
>> +	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);
>>   
>> +	bool bufferCompletedEventActive_ = false;
> 
> Missing blank line.

Yep.

>>   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 6cd99919..b4f756d7 100644
>> --- a/src/py/libcamera/py_main.cpp
>> +++ b/src/py/libcamera/py_main.cpp
>> @@ -58,6 +58,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>(m, "CameraManager");
>>   	auto pyCamera = py::class_<Camera>(m, "Camera");
>>   	auto pyCameraConfiguration = py::class_<CameraConfiguration>(m, "CameraConfiguration");
>> @@ -90,6 +91,21 @@ PYBIND11_MODULE(_libcamera, m)
>>   	m.def("log_set_level", &logSetLevel);
>>   
>>   	/* Classes */
>> +
>> +	py::enum_<CameraEventType>(pyEvent, "Type")
>> +		.value("Undefined", CameraEventType::Undefined)
>> +		.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();
>> @@ -107,7 +123,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 */
> 
> Is there any way to generate a warning at runtime when this gets used ?

The normal libcamera logging facilities, I guess? Well, we can also use 
the standard python side logging, but I'm not sure how easy that is.

In any case, I'm not planning to keep it here forever =).

> If we drop it (depending on the outcome of the discussion in the commit
> message) that will be even easier :-)
> 
>> +		.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)
>> @@ -131,7 +153,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());
>>   
>> @@ -152,11 +184,20 @@ PYBIND11_MODULE(_libcamera, m)
>>   			int ret = self.stop();
>>   
>>   			self.requestCompleted.disconnect();
>> +			self.bufferCompleted.disconnect();
>> +			self.disconnected.disconnect();
>> +
>> +			auto cm = gCameraManager.lock();
>> +			ASSERT(cm);
>> +
>> +			auto l = cm->getPyCameraEvents(self.shared_from_this());
> 
> Please use a more descriptive variable name.

Ok.

  Tomi

Hi Tomi,

On Fri, Aug 26, 2022 at 04:24:46PM +0300, Tomi Valkeinen wrote:
> On 19/08/2022 01:00, Laurent Pinchart wrote:
> > On Fri, Jul 01, 2022 at 11:45:15AM +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.
> > 
> > And we will likely have error events in the not too distant future.
> > 
> >> This patch makes all those events available to the users.
> >>
> >> The get_ready_requests() method is now deprecated, but available to keep
> >> backward compatibility.
> > 
> > Who are the users of get_ready_requests() that you are concern about,
> > and when could it be removed ? Other changes in this series break the
> > API (such as switching to exceptions), so all users will need to be
> > updated, I'd rather remove get_ready_requests() at the same time.
> 
> I'm fine with dropping get_ready_requests(). It is just so trivial to 
> support it that I kept it, which also helped converting the examples, as 
> I didn't have to convert everything in one commit.
> 
> >> 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.
> > 
> > As mentioned in the reply to the cover letter, we could have a more
> > generic subscription mechanism, but I also think it may not make much
> > sense to make subscription optional for all the other events, perhaps
> > with the exception of a future metadata completion event.
> > 
> > If only the buffer and metadata completion events are optional, should
> > we have a per-camera subscription ?
> 
> That's one option. I did try multiple different approaches, and 
> everything seems to have their drawbacks. When I get some time I'll 
> again try a few different things to refresh my memory.
> 
> >> TODO: Documentation.
> > 
> > This should be captured in the code. Or documentation could be written
> > :-)
> 
> I'll write it before merging, when we're happy with the model. I don't 
> want to write it until I'm happy with the code =).
> 
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >> ---
> >>   src/py/libcamera/py_camera_manager.cpp | 191 +++++++++++++++++++++++--
> >>   src/py/libcamera/py_camera_manager.h   |  64 ++++++++-
> >>   src/py/libcamera/py_main.cpp           |  45 +++++-
> >>   3 files changed, 276 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp
> >> index 3dd8668e..d1d63690 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 <sys/eventfd.h>
> >>   #include <system_error>
> >> @@ -33,11 +34,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()
> >> @@ -58,6 +65,50 @@ py::list PyCameraManager::cameras()
> >>   	return l;
> >>   }
> >>   
> >> +PyCameraEvent PyCameraManager::convertEvent(const CameraEvent &event)
> >> +{
> >> +	PyCameraEvent pyevent;
> >> +
> >> +	pyevent.type_ = event.type_;
> >> +
> >> +	/*
> >> +	 * 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);
> >> +
> >> +	pyevent.camera_ = 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;
> >> +	}
> > 
> > No need for curly braces. But a blank line would be nice.
> 
> Yep.
> 
> >> +	case CameraEventType::RequestCompleted: {
> >> +		pyevent.request_ = py::cast(event.request_);
> >> +
> >> +		/* Decrease the ref increased in Camera.queue_request() */
> >> +		pyevent.request_.dec_ref();
> >> +
> >> +		break;
> >> +	}
> >> +	default:
> >> +		ASSERT(false);
> > 
> > What if we dropped the undefined event type ? You wouldn't need this
> > then. It's best to write code that makes errors impossible.
> 
> That wouldn't make errors impossible, it would just hide possible bugs 
> =). When something goes bad, event.type_ can contains an illegal value. 
> I think it's good to catch it here.

CameraEventType is a scoped enum, so the compiler will not allow you
setting an invalid value. It will also enforce that all valid values
have a case in a switch statement. Of course if we have a buffer
overflow somewhere that happens to overwrite a CameraEventType variable
with an invalid value we won't catch that, but I don't think the assert
here is a good answer to that kind of problem.

> > I think I would also move all of this, except the py::cast(this) call,
> > to a PyCameraEvent constructor that takes the CameraEvent and camera
> > manager py::object.
> 
> We also do the ref count magics for RequestCompleted. I'll think about 
> it, but I probably like it better if we keep the PyCameraEvent as a 
> dummy container.
> 
> >> +	}
> >> +
> >> +	return pyevent;
> >> +}
> >> +
> >> +/* DEPRECATED */
> >>   std::vector<py::object> PyCameraManager::getReadyRequests()
> >>   {
> >>   	int ret = readFd();
> >> @@ -70,21 +121,125 @@ 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;
> > 
> > Ouch, this will silently drop all other types of events. I'd really like
> > to drop getReadyRequests().
> 
> That's exactly as it was before.

Yes, but it doesn't make it nice :-)

> >> +
> >> +		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) << "Get " << events.size() << " events";
> > 
> > s/Get/Got/
> 
> Ok.
> 
> >> +
> >> +	std::vector<PyCameraEvent> pyevents(events.size());
> > 
> > You'll end up creating events.size() default-constructed elements here,
> > only to overwrite them just after. I think calling .reserve() and using
> > a std::back_inserter in the std::transform loop below would be more
> > efficient (see https://en.cppreference.com/w/cpp/algorithm/transform).
> 
> True. And changing this allows me to add a more specific constructor for 
> PyCameraEvent.
> 
> >> +
> >> +	std::transform(events.begin(), events.end(), pyevents.begin(),
> >> +		       [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);
> > 
> > I don't think you should include disconnect events here. This would
> > result in stop() removing disconnect events from the queue, while I
> > think they need to be handled through the normal event handling
> > mechanism.
> 
> Can you elaborate? The point of this is to drop all the events related 
> to the camera. Also note that there's the CameraRemoved event from 
> camera manager that is not dropped.

Dropping the request completion and buffer completion events is fine.
The camera disconnection event should stay I think. Those are unrelated
to a start/stop cycle. If an application handles disconnection events
originating from a camera, it will be very confusing (and very hard to
debug) if those events are randomly dropped because they happen to
arrive right at stop() time. Behaviour that depends on race conditions
should be minimized if not avoided completely.

> >> +}
> >> +
> >> +std::vector<PyCameraEvent> PyCameraManager::getPyCameraEvents(std::shared_ptr<Camera> camera)
> >> +{
> >> +	MutexLocker guard(eventsMutex_);
> >> +
> >> +	std::vector<PyCameraEvent> pyevents;
> >> +
> >> +	/* Get events related to the given camera */
> >> +
> >> +	for (const auto &event : events_) {
> >> +		if (!isCameraSpecificEvent(event, camera))
> >> +			continue;
> >> +
> >> +		PyCameraEvent pyev = convertEvent(event);
> >> +		pyevents.push_back(pyev);
> >> +	}
> >> +
> >> +	/* Drop the events from the events_ list */
> >> +
> >> +	events_.erase(std::remove_if(events_.begin(), events_.end(),
> >> +		[&camera](const CameraEvent &ev) {
> >> +			return isCameraSpecificEvent(ev, camera);
> >> +		}),
> >> +		events_.end());
> > 
> > I'd like to minmize the time spent holding the lock. You can move the
> > events specific to this camera to a different vector without converting
> > them (and merge the two loops if possible), and the convert the events
> > without holding the lock.
> > 
> > Also, dropping elements in the middle of a vector is fairly inefficient.
> > I think you should use a list instead of a vector for events_, which
> > will allow you to efficiently move elements here with splice().
> 
> I'll have a look, but these sound a bit like pointless optimizations. 
> This is called only when a camera is stopped, and we're talking about a 
> few events.
> 
> Taking the conversion part outside the lock sounds feasible.
> 
> If we change the events from a vector to a list to speed up the code 
> here, where it doesn't matter, are we making the code slower in other 
> parts where it matters?

I don't think so (could be wrong of course).

> >> +
> >> +	LOG(Python, Debug) << "Get " << pyevents.size() << " camera events, "
> > 
> > s/Get/Got/ ?
> 
> Ok.
> 
> >> +			   << events_.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);
> >> +	ev.request_ = req;
> >> +	ev.fb_ = fb;
> >> +
> >> +	pushEvent(ev);
> >> +}
> >> +
> >> +/* Note: Called from another thread */
> >> +void PyCameraManager::handleRequestCompleted(std::shared_ptr<Camera> cam, Request *req)
> >> +{
> >> +	CameraEvent ev(CameraEventType::RequestCompleted, cam);
> >> +	ev.request_ = 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()
> >> @@ -110,16 +265,22 @@ int PyCameraManager::readFd()
> >>   	return 0;
> >>   }
> >>   
> >> -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();
> > 
> > This can be done without holding the lock.
> 
> The writeFd()? Are you sure? We discussed this, I think, but... Well, 
> I'd just rather be safe than sorry =).

writeFd() only accesses eventFd_, and calls ::write() (plus logging a
fatal message in case of error). The eventsMutex_ lock doesn't cover
anything that would make eventFd_ invalid as far as I can tell, so
there's only the write() remaining. The corresponding read() is called
without the lock held. Do you recall a reason why write would need to be
protected by the lock ?

> >> +
> >> +	LOG(Python, Debug) << "Queued events: " << events_.size();
> > 
> > I'm tempted to drop this, it will make the log very chatty. Have you
> > found it useful to debug applications ?
> 
> For development, yes. It's very easy to have very obscure issues with 
> the Python bindings. I think we can reduce the debug prints when things 
> settle down with the bindings.

Do you mean for development of the bindings, or for development of
applications (once the bindings are working fine) ?

> >>   }
> >>   
> >> -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..b313ba9b 100644
> >> --- a/src/py/libcamera/py_camera_manager.h
> >> +++ b/src/py/libcamera/py_camera_manager.h
> >> @@ -13,6 +13,47 @@
> >>   
> >>   using namespace libcamera;
> >>   
> >> +enum class CameraEventType {
> >> +	Undefined = 0,
> > 
> > Unless we drop this (which would be my preference), maybe Invalid
> > instead of Undefined ? People don't like undefined behaviours :-D
> 
> And they like invalid behavior? =)
> 
> I'll see how I like it if I drop the Undefined value. Usually I like 
> those, it's good to be able to initialize things to 0.

I don't think I agree. I'd rather enforce that all variables of
CameraEventType have a valid value to remove the possibility of bugs
where a variable would be initialized to Undefined and mistakenly left
at that value.

> >> +	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)
> >> +		: type_(type), camera_(camera)
> >> +	{
> >> +	}
> >> +
> >> +	CameraEventType type_ = CameraEventType::Undefined;
> >> +
> >> +	std::shared_ptr<Camera> camera_;
> >> +
> >> +	Request *request_ = nullptr;
> >> +	FrameBuffer *fb_ = nullptr;
> >> +};
> >> +
> >> +/*
> >> + * 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 {
> >> +	CameraEventType type_ = CameraEventType::Undefined;
> >> +
> >> +	pybind11::object camera_;
> >> +
> >> +	pybind11::object request_;
> >> +	pybind11::object fb_;
> >> +};
> >> +
> >>   class PyCameraManager
> >>   {
> >>   public:
> >> @@ -26,20 +67,29 @@ 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 handleRequestCompleted(Request *req);
> >> +	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);
> >>   
> >> +	bool bufferCompletedEventActive_ = false;
> > 
> > Missing blank line.
> 
> Yep.
> 
> >>   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 6cd99919..b4f756d7 100644
> >> --- a/src/py/libcamera/py_main.cpp
> >> +++ b/src/py/libcamera/py_main.cpp
> >> @@ -58,6 +58,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>(m, "CameraManager");
> >>   	auto pyCamera = py::class_<Camera>(m, "Camera");
> >>   	auto pyCameraConfiguration = py::class_<CameraConfiguration>(m, "CameraConfiguration");
> >> @@ -90,6 +91,21 @@ PYBIND11_MODULE(_libcamera, m)
> >>   	m.def("log_set_level", &logSetLevel);
> >>   
> >>   	/* Classes */
> >> +
> >> +	py::enum_<CameraEventType>(pyEvent, "Type")
> >> +		.value("Undefined", CameraEventType::Undefined)
> >> +		.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();
> >> @@ -107,7 +123,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 */
> > 
> > Is there any way to generate a warning at runtime when this gets used ?
> 
> The normal libcamera logging facilities, I guess? Well, we can also use 
> the standard python side logging, but I'm not sure how easy that is.
> 
> In any case, I'm not planning to keep it here forever =).
> 
> > If we drop it (depending on the outcome of the discussion in the commit
> > message) that will be even easier :-)
> > 
> >> +		.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)
> >> @@ -131,7 +153,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());
> >>   
> >> @@ -152,11 +184,20 @@ PYBIND11_MODULE(_libcamera, m)
> >>   			int ret = self.stop();
> >>   
> >>   			self.requestCompleted.disconnect();
> >> +			self.bufferCompleted.disconnect();
> >> +			self.disconnected.disconnect();
> >> +
> >> +			auto cm = gCameraManager.lock();
> >> +			ASSERT(cm);
> >> +
> >> +			auto l = cm->getPyCameraEvents(self.shared_from_this());
> > 
> > Please use a more descriptive variable name.
> 
> Ok.

Hi,

On 26/08/2022 17:29, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Fri, Aug 26, 2022 at 04:24:46PM +0300, Tomi Valkeinen wrote:
>> On 19/08/2022 01:00, Laurent Pinchart wrote:
>>> On Fri, Jul 01, 2022 at 11:45:15AM +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.
>>>
>>> And we will likely have error events in the not too distant future.
>>>
>>>> This patch makes all those events available to the users.
>>>>
>>>> The get_ready_requests() method is now deprecated, but available to keep
>>>> backward compatibility.
>>>
>>> Who are the users of get_ready_requests() that you are concern about,
>>> and when could it be removed ? Other changes in this series break the
>>> API (such as switching to exceptions), so all users will need to be
>>> updated, I'd rather remove get_ready_requests() at the same time.
>>
>> I'm fine with dropping get_ready_requests(). It is just so trivial to
>> support it that I kept it, which also helped converting the examples, as
>> I didn't have to convert everything in one commit.
>>
>>>> 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.
>>>
>>> As mentioned in the reply to the cover letter, we could have a more
>>> generic subscription mechanism, but I also think it may not make much
>>> sense to make subscription optional for all the other events, perhaps
>>> with the exception of a future metadata completion event.
>>>
>>> If only the buffer and metadata completion events are optional, should
>>> we have a per-camera subscription ?
>>
>> That's one option. I did try multiple different approaches, and
>> everything seems to have their drawbacks. When I get some time I'll
>> again try a few different things to refresh my memory.
>>
>>>> TODO: Documentation.
>>>
>>> This should be captured in the code. Or documentation could be written
>>> :-)
>>
>> I'll write it before merging, when we're happy with the model. I don't
>> want to write it until I'm happy with the code =).
>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>> ---
>>>>    src/py/libcamera/py_camera_manager.cpp | 191 +++++++++++++++++++++++--
>>>>    src/py/libcamera/py_camera_manager.h   |  64 ++++++++-
>>>>    src/py/libcamera/py_main.cpp           |  45 +++++-
>>>>    3 files changed, 276 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp
>>>> index 3dd8668e..d1d63690 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 <sys/eventfd.h>
>>>>    #include <system_error>
>>>> @@ -33,11 +34,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()
>>>> @@ -58,6 +65,50 @@ py::list PyCameraManager::cameras()
>>>>    	return l;
>>>>    }
>>>>    
>>>> +PyCameraEvent PyCameraManager::convertEvent(const CameraEvent &event)
>>>> +{
>>>> +	PyCameraEvent pyevent;
>>>> +
>>>> +	pyevent.type_ = event.type_;
>>>> +
>>>> +	/*
>>>> +	 * 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);
>>>> +
>>>> +	pyevent.camera_ = 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;
>>>> +	}
>>>
>>> No need for curly braces. But a blank line would be nice.
>>
>> Yep.
>>
>>>> +	case CameraEventType::RequestCompleted: {
>>>> +		pyevent.request_ = py::cast(event.request_);
>>>> +
>>>> +		/* Decrease the ref increased in Camera.queue_request() */
>>>> +		pyevent.request_.dec_ref();
>>>> +
>>>> +		break;
>>>> +	}
>>>> +	default:
>>>> +		ASSERT(false);
>>>
>>> What if we dropped the undefined event type ? You wouldn't need this
>>> then. It's best to write code that makes errors impossible.
>>
>> That wouldn't make errors impossible, it would just hide possible bugs
>> =). When something goes bad, event.type_ can contains an illegal value.
>> I think it's good to catch it here.
> 
> CameraEventType is a scoped enum, so the compiler will not allow you
> setting an invalid value. It will also enforce that all valid values
> have a case in a switch statement. Of course if we have a buffer
> overflow somewhere that happens to overwrite a CameraEventType variable
> with an invalid value we won't catch that, but I don't think the assert
> here is a good answer to that kind of problem.

I think it's fine to drop the Undefined value, although we do expose the 
enum to Python, which makes things more complex to consider. However, I 
can't right now think of a case where Python would need Undefined value, 
and it can always use None to represent undefined.

>>> I think I would also move all of this, except the py::cast(this) call,
>>> to a PyCameraEvent constructor that takes the CameraEvent and camera
>>> manager py::object.
>>
>> We also do the ref count magics for RequestCompleted. I'll think about
>> it, but I probably like it better if we keep the PyCameraEvent as a
>> dummy container.
>>
>>>> +	}
>>>> +
>>>> +	return pyevent;
>>>> +}
>>>> +
>>>> +/* DEPRECATED */
>>>>    std::vector<py::object> PyCameraManager::getReadyRequests()
>>>>    {
>>>>    	int ret = readFd();
>>>> @@ -70,21 +121,125 @@ 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;
>>>
>>> Ouch, this will silently drop all other types of events. I'd really like
>>> to drop getReadyRequests().
>>
>> That's exactly as it was before.
> 
> Yes, but it doesn't make it nice :-)
> 
>>>> +
>>>> +		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) << "Get " << events.size() << " events";
>>>
>>> s/Get/Got/
>>
>> Ok.
>>
>>>> +
>>>> +	std::vector<PyCameraEvent> pyevents(events.size());
>>>
>>> You'll end up creating events.size() default-constructed elements here,
>>> only to overwrite them just after. I think calling .reserve() and using
>>> a std::back_inserter in the std::transform loop below would be more
>>> efficient (see https://en.cppreference.com/w/cpp/algorithm/transform).
>>
>> True. And changing this allows me to add a more specific constructor for
>> PyCameraEvent.
>>
>>>> +
>>>> +	std::transform(events.begin(), events.end(), pyevents.begin(),
>>>> +		       [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);
>>>
>>> I don't think you should include disconnect events here. This would
>>> result in stop() removing disconnect events from the queue, while I
>>> think they need to be handled through the normal event handling
>>> mechanism.
>>
>> Can you elaborate? The point of this is to drop all the events related
>> to the camera. Also note that there's the CameraRemoved event from
>> camera manager that is not dropped.
> 
> Dropping the request completion and buffer completion events is fine.
> The camera disconnection event should stay I think. Those are unrelated
> to a start/stop cycle. If an application handles disconnection events
> originating from a camera, it will be very confusing (and very hard to
> debug) if those events are randomly dropped because they happen to
> arrive right at stop() time. Behaviour that depends on race conditions
> should be minimized if not avoided completely.

But we're not dropping the events. We are returning them from stop() and 
the app can handle them.

The idea here is that we subscribe the events at start(), unsubscribe at 
stop(), and after stop() there will be no events originating from the 
camera.

I can't really say if not handling Disconnect here would be a real 
problem, but the current behavior feels logical and clear to me.

>>>> +}
>>>> +
>>>> +std::vector<PyCameraEvent> PyCameraManager::getPyCameraEvents(std::shared_ptr<Camera> camera)
>>>> +{
>>>> +	MutexLocker guard(eventsMutex_);
>>>> +
>>>> +	std::vector<PyCameraEvent> pyevents;
>>>> +
>>>> +	/* Get events related to the given camera */
>>>> +
>>>> +	for (const auto &event : events_) {
>>>> +		if (!isCameraSpecificEvent(event, camera))
>>>> +			continue;
>>>> +
>>>> +		PyCameraEvent pyev = convertEvent(event);
>>>> +		pyevents.push_back(pyev);
>>>> +	}
>>>> +
>>>> +	/* Drop the events from the events_ list */
>>>> +
>>>> +	events_.erase(std::remove_if(events_.begin(), events_.end(),
>>>> +		[&camera](const CameraEvent &ev) {
>>>> +			return isCameraSpecificEvent(ev, camera);
>>>> +		}),
>>>> +		events_.end());
>>>
>>> I'd like to minmize the time spent holding the lock. You can move the
>>> events specific to this camera to a different vector without converting
>>> them (and merge the two loops if possible), and the convert the events
>>> without holding the lock.
>>>
>>> Also, dropping elements in the middle of a vector is fairly inefficient.
>>> I think you should use a list instead of a vector for events_, which
>>> will allow you to efficiently move elements here with splice().
>>
>> I'll have a look, but these sound a bit like pointless optimizations.
>> This is called only when a camera is stopped, and we're talking about a
>> few events.
>>
>> Taking the conversion part outside the lock sounds feasible.
>>
>> If we change the events from a vector to a list to speed up the code
>> here, where it doesn't matter, are we making the code slower in other
>> parts where it matters?
> 
> I don't think so (could be wrong of course).

99.9% of the time we're just appending to the events vector when we're 
doing a camera capture. With list, that would mean a heap alloc for 
every event, wouldn't it?

This would (probably) look much nicer with a list, and the code here 
does bug me a bit =). But I don't think it's worth using a list 
considering the worse performance in the main use case.

However, I can move the conversion to Python events to be outside the 
lock. That might be somewhat costly operation.

Also, I believe it would be possible to do the filtering and erasing 
with a single loop, instead of first going through the events vector twice.

>>>> +
>>>> +	LOG(Python, Debug) << "Get " << pyevents.size() << " camera events, "
>>>
>>> s/Get/Got/ ?
>>
>> Ok.
>>
>>>> +			   << events_.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);
>>>> +	ev.request_ = req;
>>>> +	ev.fb_ = fb;
>>>> +
>>>> +	pushEvent(ev);
>>>> +}
>>>> +
>>>> +/* Note: Called from another thread */
>>>> +void PyCameraManager::handleRequestCompleted(std::shared_ptr<Camera> cam, Request *req)
>>>> +{
>>>> +	CameraEvent ev(CameraEventType::RequestCompleted, cam);
>>>> +	ev.request_ = 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()
>>>> @@ -110,16 +265,22 @@ int PyCameraManager::readFd()
>>>>    	return 0;
>>>>    }
>>>>    
>>>> -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();
>>>
>>> This can be done without holding the lock.
>>
>> The writeFd()? Are you sure? We discussed this, I think, but... Well,
>> I'd just rather be safe than sorry =).
> 
> writeFd() only accesses eventFd_, and calls ::write() (plus logging a
> fatal message in case of error). The eventsMutex_ lock doesn't cover
> anything that would make eventFd_ invalid as far as I can tell, so
> there's only the write() remaining. The corresponding read() is called
> without the lock held. Do you recall a reason why write would need to be
> protected by the lock ?

I think you're right. I can't think of a reason to keep the lock during 
writeFd().

>>>> +
>>>> +	LOG(Python, Debug) << "Queued events: " << events_.size();
>>>
>>> I'm tempted to drop this, it will make the log very chatty. Have you
>>> found it useful to debug applications ?
>>
>> For development, yes. It's very easy to have very obscure issues with
>> the Python bindings. I think we can reduce the debug prints when things
>> settle down with the bindings.
> 
> Do you mean for development of the bindings, or for development of
> applications (once the bindings are working fine) ?

Development of the bindings.

  Tomi

Hi Tomi,

On Mon, Aug 29, 2022 at 01:19:54PM +0300, Tomi Valkeinen wrote:
> On 26/08/2022 17:29, Laurent Pinchart wrote:
> > On Fri, Aug 26, 2022 at 04:24:46PM +0300, Tomi Valkeinen wrote:
> >> On 19/08/2022 01:00, Laurent Pinchart wrote:
> >>> On Fri, Jul 01, 2022 at 11:45:15AM +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.
> >>>
> >>> And we will likely have error events in the not too distant future.
> >>>
> >>>> This patch makes all those events available to the users.
> >>>>
> >>>> The get_ready_requests() method is now deprecated, but available to keep
> >>>> backward compatibility.
> >>>
> >>> Who are the users of get_ready_requests() that you are concern about,
> >>> and when could it be removed ? Other changes in this series break the
> >>> API (such as switching to exceptions), so all users will need to be
> >>> updated, I'd rather remove get_ready_requests() at the same time.
> >>
> >> I'm fine with dropping get_ready_requests(). It is just so trivial to
> >> support it that I kept it, which also helped converting the examples, as
> >> I didn't have to convert everything in one commit.
> >>
> >>>> 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.
> >>>
> >>> As mentioned in the reply to the cover letter, we could have a more
> >>> generic subscription mechanism, but I also think it may not make much
> >>> sense to make subscription optional for all the other events, perhaps
> >>> with the exception of a future metadata completion event.
> >>>
> >>> If only the buffer and metadata completion events are optional, should
> >>> we have a per-camera subscription ?
> >>
> >> That's one option. I did try multiple different approaches, and
> >> everything seems to have their drawbacks. When I get some time I'll
> >> again try a few different things to refresh my memory.
> >>
> >>>> TODO: Documentation.
> >>>
> >>> This should be captured in the code. Or documentation could be written
> >>> :-)
> >>
> >> I'll write it before merging, when we're happy with the model. I don't
> >> want to write it until I'm happy with the code =).
> >>
> >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >>>> ---
> >>>>    src/py/libcamera/py_camera_manager.cpp | 191 +++++++++++++++++++++++--
> >>>>    src/py/libcamera/py_camera_manager.h   |  64 ++++++++-
> >>>>    src/py/libcamera/py_main.cpp           |  45 +++++-
> >>>>    3 files changed, 276 insertions(+), 24 deletions(-)
> >>>>
> >>>> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp
> >>>> index 3dd8668e..d1d63690 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 <sys/eventfd.h>
> >>>>    #include <system_error>
> >>>> @@ -33,11 +34,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()
> >>>> @@ -58,6 +65,50 @@ py::list PyCameraManager::cameras()
> >>>>    	return l;
> >>>>    }
> >>>>    
> >>>> +PyCameraEvent PyCameraManager::convertEvent(const CameraEvent &event)
> >>>> +{
> >>>> +	PyCameraEvent pyevent;
> >>>> +
> >>>> +	pyevent.type_ = event.type_;
> >>>> +
> >>>> +	/*
> >>>> +	 * 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);
> >>>> +
> >>>> +	pyevent.camera_ = 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;
> >>>> +	}
> >>>
> >>> No need for curly braces. But a blank line would be nice.
> >>
> >> Yep.
> >>
> >>>> +	case CameraEventType::RequestCompleted: {
> >>>> +		pyevent.request_ = py::cast(event.request_);
> >>>> +
> >>>> +		/* Decrease the ref increased in Camera.queue_request() */
> >>>> +		pyevent.request_.dec_ref();
> >>>> +
> >>>> +		break;
> >>>> +	}
> >>>> +	default:
> >>>> +		ASSERT(false);
> >>>
> >>> What if we dropped the undefined event type ? You wouldn't need this
> >>> then. It's best to write code that makes errors impossible.
> >>
> >> That wouldn't make errors impossible, it would just hide possible bugs
> >> =). When something goes bad, event.type_ can contains an illegal value.
> >> I think it's good to catch it here.
> > 
> > CameraEventType is a scoped enum, so the compiler will not allow you
> > setting an invalid value. It will also enforce that all valid values
> > have a case in a switch statement. Of course if we have a buffer
> > overflow somewhere that happens to overwrite a CameraEventType variable
> > with an invalid value we won't catch that, but I don't think the assert
> > here is a good answer to that kind of problem.
> 
> I think it's fine to drop the Undefined value, although we do expose the 
> enum to Python, which makes things more complex to consider. However, I 
> can't right now think of a case where Python would need Undefined value, 
> and it can always use None to represent undefined.
> 
> >>> I think I would also move all of this, except the py::cast(this) call,
> >>> to a PyCameraEvent constructor that takes the CameraEvent and camera
> >>> manager py::object.
> >>
> >> We also do the ref count magics for RequestCompleted. I'll think about
> >> it, but I probably like it better if we keep the PyCameraEvent as a
> >> dummy container.
> >>
> >>>> +	}
> >>>> +
> >>>> +	return pyevent;
> >>>> +}
> >>>> +
> >>>> +/* DEPRECATED */
> >>>>    std::vector<py::object> PyCameraManager::getReadyRequests()
> >>>>    {
> >>>>    	int ret = readFd();
> >>>> @@ -70,21 +121,125 @@ 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;
> >>>
> >>> Ouch, this will silently drop all other types of events. I'd really like
> >>> to drop getReadyRequests().
> >>
> >> That's exactly as it was before.
> > 
> > Yes, but it doesn't make it nice :-)
> > 
> >>>> +
> >>>> +		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) << "Get " << events.size() << " events";
> >>>
> >>> s/Get/Got/
> >>
> >> Ok.
> >>
> >>>> +
> >>>> +	std::vector<PyCameraEvent> pyevents(events.size());
> >>>
> >>> You'll end up creating events.size() default-constructed elements here,
> >>> only to overwrite them just after. I think calling .reserve() and using
> >>> a std::back_inserter in the std::transform loop below would be more
> >>> efficient (see https://en.cppreference.com/w/cpp/algorithm/transform).
> >>
> >> True. And changing this allows me to add a more specific constructor for
> >> PyCameraEvent.
> >>
> >>>> +
> >>>> +	std::transform(events.begin(), events.end(), pyevents.begin(),
> >>>> +		       [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);
> >>>
> >>> I don't think you should include disconnect events here. This would
> >>> result in stop() removing disconnect events from the queue, while I
> >>> think they need to be handled through the normal event handling
> >>> mechanism.
> >>
> >> Can you elaborate? The point of this is to drop all the events related
> >> to the camera. Also note that there's the CameraRemoved event from
> >> camera manager that is not dropped.
> > 
> > Dropping the request completion and buffer completion events is fine.
> > The camera disconnection event should stay I think. Those are unrelated
> > to a start/stop cycle. If an application handles disconnection events
> > originating from a camera, it will be very confusing (and very hard to
> > debug) if those events are randomly dropped because they happen to
> > arrive right at stop() time. Behaviour that depends on race conditions
> > should be minimized if not avoided completely.
> 
> But we're not dropping the events. We are returning them from stop() and 
> the app can handle them.

That's right, I forgot about that for a moment.

> The idea here is that we subscribe the events at start(), unsubscribe at 
> stop(), and after stop() there will be no events originating from the 
> camera.

start() and stop() are meant to start and stop video capture. The
disconnect event can occur while the camera is stopped. If you don't
want to handle the disconnect event when the camera is stopped, then it
shouldn't be handled at all and not be exposed to Python applications.

> I can't really say if not handling Disconnect here would be a real 
> problem, but the current behavior feels logical and clear to me.
> 
> >>>> +}
> >>>> +
> >>>> +std::vector<PyCameraEvent> PyCameraManager::getPyCameraEvents(std::shared_ptr<Camera> camera)
> >>>> +{
> >>>> +	MutexLocker guard(eventsMutex_);
> >>>> +
> >>>> +	std::vector<PyCameraEvent> pyevents;
> >>>> +
> >>>> +	/* Get events related to the given camera */
> >>>> +
> >>>> +	for (const auto &event : events_) {
> >>>> +		if (!isCameraSpecificEvent(event, camera))
> >>>> +			continue;
> >>>> +
> >>>> +		PyCameraEvent pyev = convertEvent(event);
> >>>> +		pyevents.push_back(pyev);
> >>>> +	}
> >>>> +
> >>>> +	/* Drop the events from the events_ list */
> >>>> +
> >>>> +	events_.erase(std::remove_if(events_.begin(), events_.end(),
> >>>> +		[&camera](const CameraEvent &ev) {
> >>>> +			return isCameraSpecificEvent(ev, camera);
> >>>> +		}),
> >>>> +		events_.end());
> >>>
> >>> I'd like to minmize the time spent holding the lock. You can move the
> >>> events specific to this camera to a different vector without converting
> >>> them (and merge the two loops if possible), and the convert the events
> >>> without holding the lock.
> >>>
> >>> Also, dropping elements in the middle of a vector is fairly inefficient.
> >>> I think you should use a list instead of a vector for events_, which
> >>> will allow you to efficiently move elements here with splice().
> >>
> >> I'll have a look, but these sound a bit like pointless optimizations.
> >> This is called only when a camera is stopped, and we're talking about a
> >> few events.
> >>
> >> Taking the conversion part outside the lock sounds feasible.
> >>
> >> If we change the events from a vector to a list to speed up the code
> >> here, where it doesn't matter, are we making the code slower in other
> >> parts where it matters?
> > 
> > I don't think so (could be wrong of course).
> 
> 99.9% of the time we're just appending to the events vector when we're 
> doing a camera capture. With list, that would mean a heap alloc for 
> every event, wouldn't it?

But appending to a vector may mean reallocating the whole vector if the
size is too small :-) Implementations don't shrink the storage size
though, so after some time the storage should get big enough, but once
in a while you risk full reallocation.

> This would (probably) look much nicer with a list, and the code here 
> does bug me a bit =). But I don't think it's worth using a list 
> considering the worse performance in the main use case.
> 
> However, I can move the conversion to Python events to be outside the 
> lock. That might be somewhat costly operation.

I'd like that if possible, yes.

> Also, I believe it would be possible to do the filtering and erasing 
> with a single loop, instead of first going through the events vector twice.
> 
> >>>> +
> >>>> +	LOG(Python, Debug) << "Get " << pyevents.size() << " camera events, "
> >>>
> >>> s/Get/Got/ ?
> >>
> >> Ok.
> >>
> >>>> +			   << events_.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);
> >>>> +	ev.request_ = req;
> >>>> +	ev.fb_ = fb;
> >>>> +
> >>>> +	pushEvent(ev);
> >>>> +}
> >>>> +
> >>>> +/* Note: Called from another thread */
> >>>> +void PyCameraManager::handleRequestCompleted(std::shared_ptr<Camera> cam, Request *req)
> >>>> +{
> >>>> +	CameraEvent ev(CameraEventType::RequestCompleted, cam);
> >>>> +	ev.request_ = 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()
> >>>> @@ -110,16 +265,22 @@ int PyCameraManager::readFd()
> >>>>    	return 0;
> >>>>    }
> >>>>    
> >>>> -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();
> >>>
> >>> This can be done without holding the lock.
> >>
> >> The writeFd()? Are you sure? We discussed this, I think, but... Well,
> >> I'd just rather be safe than sorry =).
> > 
> > writeFd() only accesses eventFd_, and calls ::write() (plus logging a
> > fatal message in case of error). The eventsMutex_ lock doesn't cover
> > anything that would make eventFd_ invalid as far as I can tell, so
> > there's only the write() remaining. The corresponding read() is called
> > without the lock held. Do you recall a reason why write would need to be
> > protected by the lock ?
> 
> I think you're right. I can't think of a reason to keep the lock during 
> writeFd().
> 
> >>>> +
> >>>> +	LOG(Python, Debug) << "Queued events: " << events_.size();
> >>>
> >>> I'm tempted to drop this, it will make the log very chatty. Have you
> >>> found it useful to debug applications ?
> >>
> >> For development, yes. It's very easy to have very obscure issues with
> >> the Python bindings. I think we can reduce the debug prints when things
> >> settle down with the bindings.
> > 
> > Do you mean for development of the bindings, or for development of
> > applications (once the bindings are working fine) ?
> 
> Development of the bindings.