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

[Laurent Pinchart]

Hello,

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 ?

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

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

> > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> > > ---
> > >  src/py/libcamera/py_camera_manager.cpp | 21 +++++++++++++++++++--
> > >  src/py/libcamera/py_camera_manager.h   |  4 +++-
> > >  src/py/libcamera/py_main.cpp           |  3 ++-
> > >  3 files changed, 24 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/py/libcamera/py_camera_manager.cpp b/src/py/libcamera/py_camera_manager.cpp
> > > index c9e5a99c..ba45f713 100644
> > > --- a/src/py/libcamera/py_camera_manager.cpp
> > > +++ b/src/py/libcamera/py_camera_manager.cpp
> > > @@ -5,6 +5,7 @@
> > >
> > >  #include "py_camera_manager.h"
> > >
> > > +#include <poll.h>
> > >  #include <sys/eventfd.h>
> > >  #include <unistd.h>
> > >
> > > @@ -55,9 +56,10 @@ py::list PyCameraManager::getCameras()
> > >         return l;
> > >  }
> > >
> > > -std::vector<py::object> PyCameraManager::getReadyRequests()
> > > +std::vector<py::object> PyCameraManager::getReadyRequests(bool nonBlocking)
> > >  {
> > > -       readFd();
> > > +       if (!nonBlocking || hasEvents())
> > > +               readFd();
> > >
> > >         std::vector<Request *> v;
> > >         getRequests(v);
> > > @@ -113,3 +115,18 @@ void PyCameraManager::getRequests(std::vector<Request *> &v)
> > >         std::lock_guard guard(reqlist_mutex_);
> > >         swap(v, reqList_);
> > >  }
> > > +
> > > +bool PyCameraManager::hasEvents()
> > > +{
> > > +       struct pollfd pfd = {
> > > +               .fd = eventFd_,
> > > +               .events = POLLIN,
> > > +               .revents = 0,
> > > +       };
> > > +
> > > +       int ret = poll(&pfd, 1, 0);
> > > +       if (ret == -1)
> > > +               throw std::system_error(errno, std::generic_category());
> > > +
> > > +       return pfd.revents & POLLIN;
> > > +}
> > > diff --git a/src/py/libcamera/py_camera_manager.h b/src/py/libcamera/py_camera_manager.h
> > > index b0b971ad..2396d236 100644
> > > --- a/src/py/libcamera/py_camera_manager.h
> > > +++ b/src/py/libcamera/py_camera_manager.h
> > > @@ -23,7 +23,7 @@ public:
> > >
> > >         int eventFd() const { return eventFd_; }
> > >
> > > -       std::vector<pybind11::object> getReadyRequests();
> > > +       std::vector<pybind11::object> getReadyRequests(bool nonBlocking = false);
> > >
> > >         void handleRequestCompleted(Request *req);
> > >
> > > @@ -36,4 +36,6 @@ private:
> > >         void readFd();
> > >         void pushRequest(Request *req);
> > >         void getRequests(std::vector<Request *> &v);
> > > +
> > > +       bool hasEvents();
> > >  };
> > > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> > > index 23018288..ee4ecb9b 100644
> > > --- a/src/py/libcamera/py_main.cpp
> > > +++ b/src/py/libcamera/py_main.cpp
> > > @@ -103,7 +103,8 @@ PYBIND11_MODULE(_libcamera, m)
> > >                 .def_property_readonly("cameras", &PyCameraManager::getCameras)
> > >
> > >                 .def_property_readonly("event_fd", &PyCameraManager::eventFd)
> > > -               .def("get_ready_requests", &PyCameraManager::getReadyRequests);
> > > +               .def("get_ready_requests", &PyCameraManager::getReadyRequests,
> > > +                    py::arg("nonblocking") = false);
> > >
> > >         pyCamera
> > >                 .def_property_readonly("id", &Camera::id)

-- 
Regards,

Laurent Pinchart