[libcamera-devel,v5,04/13] py: cam.py: Use new events support
diff mbox series

Message ID 20230603075615.20663-5-tomi.valkeinen@ideasonboard.com
State New
Headers show
Series
  • py: New python bindings event handling
Related show

Commit Message

Tomi Valkeinen June 3, 2023, 7:56 a.m. UTC 
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@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/py/cam/cam.py | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

Comments

Laurent Pinchart June 5, 2023, 5:10 a.m. UTC | #1 
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@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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 ?

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

> +
>      return 0
>  
>
Tomi Valkeinen June 5, 2023, 9:10 a.m. UTC | #2 
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@ideasonboard.com>
>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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
Tomi Valkeinen June 5, 2023, 9:37 a.m. UTC | #3 
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@ideasonboard.com>
>>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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
Laurent Pinchart June 7, 2023, 7:31 a.m. UTC | #4 
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@ideasonboard.com>
> >>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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".
Tomi Valkeinen June 7, 2023, 9:35 a.m. UTC | #5 
On 07/06/2023 10:31, Laurent Pinchart wrote:
> 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@ideasonboard.com>
>>>>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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 ?

Perhaps that's the best option. It does rub me the wrong way, though, 
doing at least a double amount of allocations (presuming the buffer 
completed event is not used), but in the bigger picture it's perhaps in 
the who-cares category.

  Tomi

Patch
diff mbox series 

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")
 
             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()
+
     return 0