[libcamera-devel] [PATCH v3 11/17] py: New event handling
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Aug 26 16:29:51 CEST 2022
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 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 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.
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list