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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart