[libcamera-devel] [RFC v1 5/7] py: Add 'nonblocking' argument to get_ready_requests()

[Tomi Valkeinen]

On 27/06/2022 13:16, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Mon, Jun 27, 2022 at 12:49:45PM +0300, Tomi Valkeinen wrote:
>> On 24/06/2022 16:53, Laurent Pinchart wrote:
>>> On Fri, Jun 24, 2022 at 11:26:18AM +0100, David Plowman wrote:
>>>> Hi everyone
>>>>
>>>> Just to add some background to this one...
>>>>
>>>> This has been causing me a little trouble in Picamera2 lately.
>>>> get_ready_requests used to be non-blocking so I would always call it
>>>> after stopping the camera to clear out any "lurking" requests. Since
>>>> it became blocking I've had to stop that but it does mean that a stray
>>>> request can be read out at an awkward time. This can lead to us
>>>> getting the "wrong" image (and probably falling over when it's the
>>>> wrong size or something), or even trying to queue it back to libcamera
>>>> (while the camera is still stopped).
>>>>
>>>> Anyway, either solution works for me. Either I can flush out those
>>>> requests after calling stop(), which is what I used to do. Or they
>>>> could disappear "spontaneously". Both work for me!!
>>>
>>> I'd like to make get_ready_requests() non-blocking unconditionally,
>>> could that be done ?
>>
>> I think we should have a blocking version too.
> 
> Why is that ?

It's the easiest and the most natural way to write a small script to get 
something captured. Maybe we'll do it with a separate function, but 
somehow we need to offer a way to capture without dealing with Selectors 
or such.

>>>> On Fri, 24 Jun 2022 at 11:13, Kieran Bingham wrote:
>>>>> Quoting Tomi Valkeinen (2022-06-23 15:47:34)
>>>>>> Add 'nonblocking' argument to get_ready_requests() which allows the user
>>>>>> to ensure get_ready_requests() never blocks. This can be used e.g. after
>>>>>> calling camera.stop(), to process or discard any ready or cancelled
>>>>>> Requests.
>>>>>
>>>>> I guess I needed to read ahead for the comments I posted on the previous
>>>>> patch ;-)
>>>>>
>>>>>> In fact, it probably should always be used after stopping the cameras,
>>>>>> unless you have made sure that there are no unprocessed Requests. If you
>>>>>> start the camera again, and you have left Requests unprocessed, you will
>>>>>> get those "old" Requests when you expect to get the new Requests.
>>>>>>
>>>>>> It may be good to call this even if your script exits after stopping
>>>>>> the cameras, as unprocessed Requests will keep the Cameras and related
>>>>>> objects alive, and thus they won't be freed. As your script is exiting
>>>>>> it's strictly speaking not an issue, but it does make tracking other
>>>>>> "real" memory leaks more difficult.
>>>
>>> Developers will forget to do so, so I think a better API would be nice.
>>
>> I don't know yet what that better API could be, but perhaps something we
>> can easily do is to add a check in camera.start(), which will warn if
>> there are events about Requests already queued.
> 
> That could indeed help debugging.
> 
>>>>>> Perhaps the camera.start() should go and discard any old Requests
>>>>>> related to that camera. For the exit issue I don't see any automatic
>>>>>> solution.
>>>>>
>>>>> At the end of camera.stop() we should validate and ensure that all
>>>>> Requests are released to the application. We should hold no internal
>>>>> requests when camera.stop() completes.
>>>>>
>>>>> I.e. ... if we have things to discard at camera.start() - that's a bug I
>>>>> believe.
>>>>>
>>>>> ahhh but here perhaps the issue is that the python code is the one that
>>>>> has to 'retrieve' those, while in C++ they are returned via a signal?
>>>>>
>>>>> Is there any harm in discarding the requests at the end of camera.stop()
>>>>> such that there is simply 'nothing left to process' after? Or does the
>>>>> python application have to end up owning the requests to correctly
>>>>> release them?
>>>
>>> Sounds like an idea to explore. What's the drawback of clearing in
>>> stop() ?
>>
>> Losing events that you expected to get, I think.
>>
>> I'm talking here about the event handling after adding the new event
>> handling, as I think it's more relevant than figuring out how to do
>> things with just the single event.
>>
>> We could dispatch the events (All events? Or just events related to
>> Requests for that camera?) automatically in stop(), but that would break
>> the backward compatibility.
> 
> I don't recall if I've mentioned it in the review of another patch in
> the series, but I've been thinking about dispatching events at stop
> time, yes. This is how libcamera operates for buffer and request
> completion events, doing the same in Python would make the behaviour
> consistent. Backward compatibility isn't a concern, the Python bindings
> are experimental, we shouldn't let that block the design of a good API.

With C++, it sounds fine as the signals are fired behind the scenes. In 
the Python bindings there's a specific call to dispatch the events. 
Implicitly dispatching the events elsewhere can be confusing.

  Tomi