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

[Tomi Valkeinen]

On 05/06/2023 12:10, Tomi Valkeinen wrote:
> On 05/06/2023 08:10, Laurent Pinchart wrote:
>> Hi Tomi,
>>
>> Thank you for the patch.
>>
>> 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.
> 
> I'm leaning towards the explicit behavior, as these are somewhat low 
> level bindings.
> 
>>>               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".

  Tomi