[libcamera-devel] [PATCH v5 04/13] py: cam.py: Use new events support

[Laurent Pinchart]

Hi Tomi,

On Mon, Jun 05, 2023 at 12:37:26PM +0300, Tomi Valkeinen wrote:
> On 05/06/2023 12:10, Tomi Valkeinen wrote:
> > On 05/06/2023 08:10, Laurent Pinchart wrote:
> >> On Sat, Jun 03, 2023 at 10:56:06AM +0300, Tomi Valkeinen wrote:
> >>> Convert cam.py to use the new event dispatching. In addition to handling
> >>> the request-completed event, handle also disconnect, camera-added and
> >>> camera-removed events (which only do a simple print).
> >>>
> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> >>> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>> ---
> >>>   src/py/cam/cam.py | 27 +++++++++++++++++++++------
> >>>   1 file changed, 21 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py
> >>> index a2a115c1..1e2d1f69 100755
> >>> --- a/src/py/cam/cam.py
> >>> +++ b/src/py/cam/cam.py
> >>> @@ -230,11 +230,19 @@ class CaptureState:
> >>>       # Called from renderer when there is a libcamera event
> >>>       def event_handler(self):
> >>>           try:
> >>> -            reqs = self.cm.get_ready_requests()
> >>> -
> >>> -            for req in reqs:
> >>> -                ctx = next(ctx for ctx in self.contexts if ctx.idx == req.cookie)
> >>> -                self.__request_handler(ctx, req)
> >>> +            for ev in self.cm.get_events():
> >>> +                type = ev.type
> >>> +
> >>> +                if type == libcam.Event.Type.CameraAdded:
> >>> +                    print(f'Camera {ev.camera} added')
> >>> +                elif type == libcam.Event.Type.CameraRemoved:
> >>> +                    print(f'Camera {ev.camera} removed')
> >>> +                elif type == libcam.Event.Type.Disconnect:
> >>> +                    print(f'Camera {ev.camera} disconnected')
> >>> +                elif type == libcam.Event.Type.RequestCompleted:
> >>> +                    self.__request_handler(ev.camera, ev.request)
> >>> +                else:
> >>> +                    raise RuntimeError("Bad event type")
> >>
> >> This will cause issues if we later add new event types. Wouldn't it be
> >> better to ignore unknown event types, or possibly print a (one-time)
> >> warning message ?
> > 
> > Right, this error should never happen. Maybe this again shows that we 
> > have some unclear behavior in the bindings. If we make it so that all 
> > the events (including CameraManager's events) must be explicitly 
> > enabled, we can only handle the ones we have enabled, and raise an error 
> > for anything else (or assert). But if we do implicitly enable some 
> > events, we have to ignore any unhandled ones.

Agreed.

> > I'm leaning towards the explicit behavior, as these are somewhat low 
> > level bindings.

Aonther option would be to always emit all events. I think I would
prefer that, an event subscription mechanism seems a bit over-engineered
to me, it's additional complexity for unclear gains. Could we skip it
for now and introduce it later if needed ?

> >>>               running = any(ctx.reqs_completed < ctx.opt_capture for 
> >>> ctx in self.contexts)
> >>>               return running
> >>> @@ -242,7 +250,9 @@ class CaptureState:
> >>>               traceback.print_exc()
> >>>               return False
> >>> -    def __request_handler(self, ctx, req):
> >>> +    def __request_handler(self, cam, req):
> >>> +        ctx = next(ctx for ctx in self.contexts if ctx.camera == cam)
> >>> +
> >>>           if req.status != libcam.Request.Status.Complete:
> >>>               raise Exception('{}: Request failed: {}'.format(ctx.id, req.status))
> >>> @@ -447,6 +457,11 @@ def main():
> >>>           state.do_cmd_capture()
> >>> +        # This is not strictly needed, but it helps to do a proper cleanup as we
> >>> +        # drop any unhandled events, and so makes it easier to use memory leak
> >>> +        # detectors.
> >>> +        cm.get_events()
> >>
> >> Somehow this feels like a hack, outlining a design issue. Is there any
> >> way we could do better ? How about clearing events in the camera manager
> >> destructor ?
> > 
> > If we have events in the queue, the events keep their respective cameras 
> > alive, and the cameras keep the camera manager alive, so there will 
> > never be a destructor call. That said, python gc should detect circular 
> > references, but I'm not sure if it applies here.
> > 
> > But now that I test it, I don't see PyCameraManager destructor being 
> > called at all... So something has gotten broken. Hmm it's probably the 
> > new signal subscription. We keep the requestCompleted subscribed, and we 
> > have passed a shared_ptr<CameraManager> to it...
> 
> Yes, that's the issue. It can be solved by passing a 
> weak_ptr<PyCameraManager> instead. And we also need to disconnect all 
> the signals in the PyCameraManager destructor, as otherwise libcamera 
> complains about "Removing media device /dev/media0 while still in use".

-- 
Regards,

Laurent Pinchart