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

[Tomi Valkeinen]

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

This is odd, I'm pretty sure I tested this. And the unittests think that 
CameraManager does get cleaned up. Sigh...

  Tomi