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

[Tomi Valkeinen]

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 at ideasonboard.com>
>> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart at 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
>