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

[Laurent Pinchart]

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 ?

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

> If we drop the backward compatibility, automatically dispatching the 
> events in camera.stop() still feels a bit wrong to me. Instead of 
> dispatching to functions, we could expose some kind of event objects to 
> Python, and return a list of the events. The list would be returned 
> normally with cm.get_events() or such, but camera.stop() could then also 
> return the events (related to that camera?).
> 
> Handling or returning just some events is of course a bit more work, as 
> we need to lock the events list, then choose and pick, and remove the 
> picked ones. But as that would only be done on camera.stop(), it's not 
> really an issue.

-- 
Regards,

Laurent Pinchart