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

[Tomi Valkeinen]

On 27/06/2022 15:25, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Mon, Jun 27, 2022 at 01:51:36PM +0300, Tomi Valkeinen wrote:
>> On 27/06/2022 13:16, Laurent Pinchart wrote:
>>> 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.
> 
> I'd prefer, if possible, to offer higher-level features on top of the
> core bindings. By excluding them from the core bindings, I think it will
> leave room for people to experiment with higher-level camera APIs, be it
> in the Raspberry Pi camera Python code, or in other implementations.

That is a valid point, but I think mine is too =). Especially with a 
feature like this, which doesn't hide or take anything away. I don't see 
what a blocking version would take away from a higher level API.

I'll do some experimenting, perhaps it's so trivial to use a 
non-blocking version with Selector or something else that we can just 
drop the blocking version.

But I do think it's important to easily provide that feature, even with 
the core bindings. Unless we want to target the core bindings only as a 
base for higher level libraries, which I don't think is a good idea.

>>>>>>>> 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.
> 
> The explicit dispatching call is needed to work around a Python
> limitation related to threads. I'm fine with that, but I'd like to keep
> it as much as an internal detail as possible, thus minimizing its
> explicit usage from applications. I get your point about this being
> possibly confusing for users. I'd be interested in feedback from said
> users :-)

Me too, but this is a rather difficult question to answer, so I don't 
really expect to get help here =).

I do like the idea of somehow forcibly making the events handled at 
camera.stop() time, though.

Although there may be other events queued anyway. Say, camera_added. 
Those will stay in the event queue, holding references to the relevant 
objects, until the user calls dispatch_events().

Then again those don't cause similar problems as getting "old" Request 
events after restarting a camera, so maybe they are fine.

If the user wants a clean exit, he needs to either dispatch or discard 
those events, otherwise the cameras and camera managers will be kept 
alive at the app exit time.

  Tomi