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

[David Plowman]

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

Thanks
David

On Fri, 24 Jun 2022 at 11:13, Kieran Bingham
<kieran.bingham at ideasonboard.com> 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.
> >
> > 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?
>
>
> > 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)
> > --
> > 2.34.1
> >