[libcamera-devel] [RFC v1 6/7] py: New event handling
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jun 30 10:28:22 CEST 2022
Hi Tomi,
On Mon, Jun 27, 2022 at 05:40:13PM +0300, Tomi Valkeinen wrote:
> On 24/06/2022 17:44, Laurent Pinchart wrote:
> > On Fri, Jun 24, 2022 at 11:53:43AM +0100, Kieran Bingham wrote:
> >> Quoting Tomi Valkeinen (2022-06-23 15:47:35)
> >>> At the moment the Python bindings only handle the requestCompleted
> >>> events. But we have others, bufferCompleted and disconnec from the
> >>
> >> disconnec/disconnect
> >>
> >>> Camera, and cameraAdded and cameraRemoved from the CameraManager.
> >>>
> >>> This makes all those events available to the users.
> >>
> >> \o/
> >>
> >>> The get_ready_requests() method is now deprecated, but available to keep
> >>> backward compatibility.
> >
> > I'd rather drop it. We're nowhere close to guaranteeing any API in the
> > Python bindings.
>
> It's nice to keep the existing py scripts working. I can drop it
> eventually, but as it's so trivial (at least so far) to keep it, I'd
> rather have it.
I'm OK with that if it can help with the transition to the new API, but
I'd like to drop legacy code sooner than later. How long do you expect
to keep this ?
> >> Aha, once again - answering my questions in the previous patch ;-)
> >> Maybe I need to read patch series backwards ... hehe.
> >>
> >>> The new event handling works as follows:
> >>>
> >>> The user sets callbacks to the CameraManager or Cameras (e.g.
> >>> Camera.buffer_completed). When the event_fd informs of an event, the
> >>> user must call CameraManager.dispatch_events() which gets the queued
> >>> events and calls the relevant callbacks for each queued event.
> >>>
> >>> Additionally there is CameraManager.discard_events() if the user does
> >>> not want to process the events but wants to clear the event queue (e.g.
> >>> after stopping the cameras or when exiting the app).
> >
> > What is this for ? If the user doesn't set any handler for a specific
> > event type, I would expect the corresponding events to be discarded.
> > Anything else will be passed to the event handlers. If it's about
> > ensuring that the event queue is cleared after stop() if the use doesn't
> > call dispatch_events(), I'd rather clear the queue at stop() time, and
> > possible even call dispatch_events() in stop().
>
> It's mainly for exit time. If there are unhandled events in the event
> queue, CameraManager and Cameras are never freed. As the app is exiting
> there should be no other downsides to this but memory leak checkers
> possibly complaining.
>
> The user could alternatively call dispatch_event(), but then he either
> must be able to process the dispatched events or unsubscribe the
> callbacks first.
>
> >> Hrm... in C++ lifetimes, the end of stop() means everything internally
> >> is released, so I'm still weary that if we change that it could be
> >> problematic (i.e. if we have to 'rely' on an application doing clean
> >> up).
> >>
> >>> I'm not very happy with this patch. It works fine, but there's a lot of
> >>> repetition of almost-the-same code. Perhaps some template magics might
> >>> reduce the repetition, but I also fear that it can easily make the code
> >>> more difficult to read.
> >>>
> >>> TODO: Documentation.
> >>>
> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> >>> ---
> >>> src/py/libcamera/py_camera_manager.cpp | 291 +++++++++++++++++++++++--
> >>> src/py/libcamera/py_camera_manager.h | 41 +++-
> >>> src/py/libcamera/py_main.cpp | 87 +++++++-
> >>> 3 files changed, 397 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp
> >>> index ba45f713..bbadb9ee 100644
> >>> --- a/src/py/libcamera/py_camera_manager.cpp
> >>> +++ b/src/py/libcamera/py_camera_manager.cpp
> >>> @@ -15,6 +15,37 @@ namespace py = pybind11;
> >>>
> >>> using namespace libcamera;
> >>>
> >>> +struct CameraEvent {
> >
> > I'd make this a class.
>
> Why?
I'm not sure, I suppose it's the constructor and nested type that makes
it different enough from a C struct, but there's nothing in C++ that
states a struct shouldn't be used.
> >>> + enum class EventType {
> >
> > You can s/EventType/Type/ as you'll need to write CameraEvent::Type
> > anyway.
>
> That's true.
>
> >>> + Undefined = 0,
> >>> + CameraAdded,
> >>> + CameraRemoved,
> >>> + Disconnect,
> >>> + RequestCompleted,
> >>> + BufferCompleted,
> >>> + };
> >>> +
> >>> + CameraEvent(EventType type)
> >>> + : type(type)
> >
> > No variable shadowing warning ? Ah, we disable them. It would still be
> > good to avoid any possible issue here, and use type_ for the member.
>
> Ok.
>
> >>> + {
> >>> + }
> >>> +
> >>> + EventType type;
> >>> +
> >>> + std::shared_ptr<Camera> cam;
> >>> +
> >>> + union {
> >>> + struct {
> >>> + Request *req;
> >>> + FrameBuffer *fb;
> >>> + } buf_completed;
> >>> +
> >>> + struct {
> >>> + Request *req;
> >>> + } req_completed;
> >>> + };
> >
> > As this stores pointers only, I think you could simplify it to just
> >
> > Request *request_;
> > FrameBuffer *fb_;
> >
> > and initialize the two pointers to nullptr in the constructor, to more
> > easily catch bugs.
>
> Yes, I think that's fine. I also thought about std::variant, but it felt
> like going a bit too far.
>
> This could backfire at some point, though, if we get new events with
> data that doesn't quite match the above.
That's true, but we can fix it later in that case.
> >>> +};
> >>> +
> >>> PyCameraManager::PyCameraManager()
> >>> {
> >>> int fd = eventfd(0, 0);
> >>> @@ -56,33 +87,261 @@ py::list PyCameraManager::getCameras()
> >>> return l;
> >>> }
> >>>
> >>> +/* DEPRECATED */
> >
> > Let's drop it :-)
> >
> >>> std::vector<py::object> PyCameraManager::getReadyRequests(bool nonBlocking)
> >>> {
> >>> if (!nonBlocking || hasEvents())
> >>> readFd();
> >>>
> >>> - std::vector<Request *> v;
> >>> - getRequests(v);
> >>> + std::vector<CameraEvent> v;
> >>> + getEvents(v);
> >>>
> >>> std::vector<py::object> ret;
> >>>
> >>> - for (Request *req : v) {
> >>> - py::object o = py::cast(req);
> >>> - /* Decrease the ref increased in Camera.queue_request() */
> >>> - o.dec_ref();
> >>> - ret.push_back(o);
> >>> + for (const auto &ev : v) {
> >>> + switch (ev.type) {
> >>> + case CameraEvent::EventType::RequestCompleted: {
> >>> + Request *req = ev.req_completed.req;
> >>> + py::object o = py::cast(req);
> >>> + /* Decrease the ref increased in Camera.queue_request() */
> >>> + o.dec_ref();
> >>> + ret.push_back(o);
> >>> + }
> >>> + default:
> >>> + /* ignore */
> >>
> >> Does this mean that events will get lost if the deprecated call gets
> >> used? (Ok, so it's deprecated, but will it cause 'bugs' ?)
> >>
> >>
> >>> + break;
> >>> + }
> >>> }
> >>>
> >>> return ret;
> >>> }
> >>>
> >>> /* Note: Called from another thread */
> >>> -void PyCameraManager::handleRequestCompleted(Request *req)
> >>> +void PyCameraManager::handleBufferCompleted(std::shared_ptr<Camera> cam, Request *req, FrameBuffer *fb)
> >>> +{
> >>> + CameraEvent ev(CameraEvent::EventType::BufferCompleted);
> >>> + ev.cam = cam;
> >>> + ev.buf_completed.req = req;
> >>> + ev.buf_completed.fb = fb;
> >>> +
> >>> + pushEvent(ev);
> >>> + writeFd();
> >>
> >> Should writeFd be part of pushEvent? then there should be one entry on
> >> the poll Fd for every event right? And forces them to be kept in sync...
> >
> > Sounds like a good idea.
> >
> >>> +}
> >>> +
> >>> +/* Note: Called from another thread */
> >>> +void PyCameraManager::handleRequestCompleted(std::shared_ptr<Camera> cam, Request *req)
> >>> +{
> >>> + CameraEvent ev(CameraEvent::EventType::RequestCompleted);
> >>> + ev.cam = cam;
> >>> + ev.req_completed.req = req;
> >>> +
> >>> + pushEvent(ev);
> >>> + writeFd();
> >>> +}
> >>> +
> >>> +/* Note: Called from another thread */
> >>> +void PyCameraManager::handleDisconnected(std::shared_ptr<Camera> cam)
> >>> {
> >>> - pushRequest(req);
> >>> + CameraEvent ev(CameraEvent::EventType::Disconnect);
> >>> + ev.cam = cam;
> >>> +
> >>> + pushEvent(ev);
> >>> writeFd();
> >>> }
> >>>
> >>> +/* Note: Called from another thread */
> >>> +void PyCameraManager::handleCameraAdded(std::shared_ptr<Camera> cam)
> >>> +{
> >>> + CameraEvent ev(CameraEvent::EventType::CameraAdded);
> >>> + ev.cam = cam;
> >>> +
> >>> + pushEvent(ev);
> >>> + writeFd();
> >>> +}
> >>> +
> >>> +/* Note: Called from another thread */
> >>> +void PyCameraManager::handleCameraRemoved(std::shared_ptr<Camera> cam)
> >>> +{
> >>> + CameraEvent ev(CameraEvent::EventType::CameraRemoved);
> >>> + ev.cam = cam;
> >>> +
> >>> + pushEvent(ev);
> >>> + writeFd();
> >>> +}
> >>
> >> Is that what you mean about code duplication? I think it's fine.
> >>
> >> These could be templated out to specify the event type I guess ... but
> >> that might be more pain. Especially when the request completed has
> >> different parameters anyway.
> >
> > Yes, generalizing this is probably not really worth it.
> >
> >>> +
> >>> +void PyCameraManager::dispatchEvents(bool nonBlocking)
> >
> > Can we make this unconditionally non-blocking ?
> >
> >>> +{
> >>> + if (!nonBlocking || hasEvents())
> >>> + readFd();
> >>> +
> >>> + std::vector<CameraEvent> v;
> >
> > s/v/events/
> >
> >>> + getEvents(v);
> >
> > std::vector<CameraEvent> events = getEvents();
> >
> >>> +
> >>> + LOG(Python, Debug) << "Dispatch " << v.size() << " events";
> >>> +
> >>> + for (const auto &ev : v) {
> >
> > s/ev/event/
>
> This style of using long variable names and yet insisting on short lines
> is sometimes a bit annoying =).
Conflicting requirements ? :-) I agree, and shortening can make sense,
but I'd do so only when it helps.
> >>> + switch (ev.type) {
> >>> + case CameraEvent::EventType::CameraAdded: {
> >>> + std::shared_ptr<Camera> cam = ev.cam;
> >
> > As there's always a camera in all events, you could move it before the
> > switch.
>
> True. Originally I had the camera separately for each event.
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list