[libcamera-devel,v5,06/13] py: simple-capture.py: Use new events support

18691 diff mbox series

Update simple-capture.py to the new event model.

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/examples/simple-capture.py | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

Hi Tomi,

Thank you for the patch.

On Sat, Jun 03, 2023 at 10:56:08AM +0300, Tomi Valkeinen wrote:
> Update simple-capture.py to the new event model.
> 
> 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/examples/simple-capture.py | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/src/py/examples/simple-capture.py b/src/py/examples/simple-capture.py
> index 4b85408f..4fa36d6f 100755
> --- a/src/py/examples/simple-capture.py
> +++ b/src/py/examples/simple-capture.py
> @@ -107,17 +107,22 @@ def main():
>      sel.register(cm.event_fd, selectors.EVENT_READ)
>  
>      while frames_done < TOTAL_FRAMES:
> -        # cm.get_ready_requests() does not block, so we use a Selector to wait
> -        # for a camera event. Here we should almost always get a single
> -        # Request, but in some cases there could be multiple or none.
> +        # cm.get_events() does not block, so we use a Selector to wait for a
> +        # camera event. Here we should almost always get a single request
> +        # completion event, but in some cases there could be multiple ones,
> +        # other events, or no events at all.

I think this will be confusing to people reading the code, as you
mention there can be cases that depart from the norm, but don't explain
why. simple-capture.py is meant as a simple example so I expect
developers to read the code. Let' make it crystal clear :-)

>  
>          events = sel.select()
>          if not events:
>              continue

Also, you're handling two types of events, the ones returned by the
selector, and the ones returned by get_events(). It's not clear which of
those two the comment above relates to. For instance, when you say "no
events at all", is that for selector events, camera manager events, or
both ? This needs to be clarified.

>  
> -        reqs = cm.get_ready_requests()
> +        for ev in cm.get_events():
> +            # We are only interested in RequestCompleted events
> +            if ev.type != libcam.Event.Type.RequestCompleted:
> +                continue
> +
> +            req = ev.request
>  
> -        for req in reqs:
>              frames_done += 1
>  
>              buffers = req.buffers

On 05/06/2023 08:15, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Sat, Jun 03, 2023 at 10:56:08AM +0300, Tomi Valkeinen wrote:
>> Update simple-capture.py to the new event model.
>>
>> 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/examples/simple-capture.py | 15 ++++++++++-----
>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/py/examples/simple-capture.py b/src/py/examples/simple-capture.py
>> index 4b85408f..4fa36d6f 100755
>> --- a/src/py/examples/simple-capture.py
>> +++ b/src/py/examples/simple-capture.py
>> @@ -107,17 +107,22 @@ def main():
>>       sel.register(cm.event_fd, selectors.EVENT_READ)
>>   
>>       while frames_done < TOTAL_FRAMES:
>> -        # cm.get_ready_requests() does not block, so we use a Selector to wait
>> -        # for a camera event. Here we should almost always get a single
>> -        # Request, but in some cases there could be multiple or none.
>> +        # cm.get_events() does not block, so we use a Selector to wait for a
>> +        # camera event. Here we should almost always get a single request
>> +        # completion event, but in some cases there could be multiple ones,
>> +        # other events, or no events at all.
> 
> I think this will be confusing to people reading the code, as you
> mention there can be cases that depart from the norm, but don't explain
> why. simple-capture.py is meant as a simple example so I expect
> developers to read the code. Let' make it crystal clear :-)

Yep. A bad comment is worse than no comment at all =).

>>   
>>           events = sel.select()
>>           if not events:
>>               continue
> 
> Also, you're handling two types of events, the ones returned by the
> selector, and the ones returned by get_events(). It's not clear which of
> those two the comment above relates to. For instance, when you say "no
> events at all", is that for selector events, camera manager events, or
> both ? This needs to be clarified.

Right, annoyingly we have two types of (very different) events. I'll try 
to make it clear which I'm referring to.

>>   
>> -        reqs = cm.get_ready_requests()
>> +        for ev in cm.get_events():
>> +            # We are only interested in RequestCompleted events
>> +            if ev.type != libcam.Event.Type.RequestCompleted:
>> +                continue
>> +
>> +            req = ev.request
>>   
>> -        for req in reqs:
>>               frames_done += 1
>>   
>>               buffers = req.buffers
>