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

[Tomi Valkeinen]

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