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

[Laurent Pinchart]

Hi Tomi,

On Mon, Jun 27, 2022 at 03:54:11PM +0300, Tomi Valkeinen wrote:
> On 27/06/2022 15:25, Laurent Pinchart wrote:
> > 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.

If it's not trivial then maybe it means the core bindings are not good
enough :-) Implementing this purely in Python would be a good test.

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

It seems to be time to start discussing this. So far, I would prefer
having the core bindings matching the C++ API as closely as possible,
without additional features, and having convenience helpers developed on
top. We could develop our own convenience helpers shipped with libcamera
of course.

Anyone else wants to chime in ?

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

Maybe David could shine some light here, as our mean user of the core
bindings at this point ?

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

We could only dispatch buffer and request completion events in stop()
for the camera being stopped, that would be fine with me. I'm not sure
if we would gain much from such a selective dispatch though, but it
would certainly mimic the C++ API.

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

Maybe a cleanup function on the camera manager would do the job ? We're
going back to explicit start/stop then though :-)

-- 
Regards,

Laurent Pinchart