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

[Laurent Pinchart]

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.

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

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 :-)

-- 
Regards,

Laurent Pinchart