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

16343 diff mbox series

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.

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.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@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(-)

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

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

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

On 24/06/2022 13: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.
>>
>> 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?

Right. All events are queued by the bindings when they happen, and the 
Python code can then separately handle the events in the Python thread.

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

Discarding automatically at camera.stop() sounds bad to me. There could 
be normally completed requests in the list, not only cancelled requests. 
And you could well design an app so that the app expects each queued 
request to be "given back" via event handling.

We could make the camera.stop() call the event handlers, so that it 
would automatically dispatch the events. But that doesn't sound very 
good either, although perhaps better than discarding the events 
automatically.

  Tomi

On 24/06/2022 16:53, Laurent Pinchart wrote:
> 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 ?

I think we should have a blocking version too.

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

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

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.

  Tomi

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.

On Mon, Jun 27, 2022 at 12:37:10PM +0300, Tomi Valkeinen wrote:
> On 24/06/2022 13: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.
> >>
> >> 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?
> 
> Right. All events are queued by the bindings when they happen, and the 
> Python code can then separately handle the events in the Python thread.
> 
> > 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?
> 
> Discarding automatically at camera.stop() sounds bad to me. There could 
> be normally completed requests in the list, not only cancelled requests. 
> And you could well design an app so that the app expects each queued 
> request to be "given back" via event handling.

If requests are in flight when stop() is called, an application has no
way to know if any particular request will complete successfully or not,
so it shouldn't rely on that. Dropping all successful completion events
would then just hardcode one specific end result, which applications
must be ready to handle.

I however agree that an application can be designed to assume each
queued request will complete. The libcamera C++ API guarantees that, and
I think it makes sense to do the same in the Python bindings.

> We could make the camera.stop() call the event handlers, so that it 
> would automatically dispatch the events. But that doesn't sound very 
> good either, although perhaps better than discarding the events 
> automatically.

I like this option :-)

On 27/06/2022 13:16, Laurent Pinchart wrote:
> 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 ?

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.

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

  Tomi

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

On 27/06/2022 15:25, Laurent Pinchart wrote:
> 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.

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.

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.

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

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

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.

  Tomi

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

On 27/06/2022 23:42, Laurent Pinchart wrote:
> 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.

Oh, it's trivial in Python in the sense that it's a few lines of code. 
But it's not trivial in the sense that you could just write

while True:
	# This waits until there are requests
         reqs = cm.get_ready_requests()
         for req in reqs:
		do something

We can easily write a helper function on top of non-blocking API to 
achieve the above.

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

I can't make my mind on this. On one hand, what you suggest is a clean 
approach. On the other, if we have a core bindings module and a 
convenience module on top, it brings up some questions:

- Module naming. libcamera-core and libcamera? libcamera and 
libcamera-for-dummies?

- Is the convenience module just a sugar topping, i.e. helper 
functions/classes here and there, or is it something that fully wraps 
the core (somewhat like pilibcamera2)?

- If it's just some helpers, what is the benefit of using the core module?

- If it fully wraps the core, most likely it somewhat dummies down the 
libcamera features. If so, there are always users for the core module. 
Those users would most likely want to use simple helpers like blocking 
wait or mmapped fb.

- Perhaps a third module option would be something in between: a module 
that "pythonizes" the classes, while still exposing everything. But are 
the convenience features part of that, or yet another module on top? 
Such a module also brings up the annoyance that you would access many 
things via the higher level module (say, pylibcamera.Camera), but 
probably many things would be just exposed from libcamera module if we 
don't do a full wrap (say, camera.set_format(libcamera.formats.RGB123).

So... I think it's clear that a fully wrapping module is obviously a new 
module on top of the core bindings. But the simple helpers and possibly 
the pythonization could be part of the core bindings, or on top.

It's clear that features like mmapped fb class and blocking wait are 
features that are needed by the users, so I think it's not a question of 
should we have those, but where should they be.

Also, not that even if the helpers would be part of the core bindings, 
we could have internal split there: the bindings made in C++ would be 
the core bindings, and in the same module we could have convenience 
features written in Python (similar to the MappedFrameBuffer.py we have 
now). However, while that sounds nice, I don't know if there's any real 
logic to it. If something is easier/better written in the C++ bindings, 
I don't see a point in forcing that feature to be written in Python instead.

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

The discard_events() is essentially a cleanup function.

Perhaps we can do some clever weak-ref tricks there, though... I think 
the events should not keep anything alive. Oh, but we depend on the 
event keeping the Request alive, so that doesn't work.

  Tomi

Hi everyone

Sorry for not taking part in this discussion rather more, and thanks
to those who have! I think I was asked further back if I had an
opinion, so let me try and explain how I see things.

Actually I don't have any particularly strong opinions, other than
that I want things to work and to be easy to use. And if they stopped
changing, that would be nice too!!

I was actually OK with the previous version where get_ready_requests
was non-blocking, it's only the more recent version that causes me
some trouble because I can't simply flush out any lurking requests
after stopping the camera - because there might not be any and I will
simply lock up the system.

It's perhaps worth noting that, like our C++ apps, I recycle requests
back to libcamera just as soon as is humanly possible, as that's the
best way to avoid frame drops. The number of request objects that I
make is typically from 1 to about 8 (inclusive).

I'm happy to go back to the non-blocking version, or I'm happy to have
a blocking version where I'm guaranteed not to get any more requests
out once the camera has stopped. Once the camera is stopped I just
discard all my requests and don't want to see them again! I suppose if
they came out later but were marked as "cancelled" that would be
workable, though it feels less tidy.

I would dislike the approach where I have to keep track of which
requests I've sent and which ones haven't yet come back, so that I
know whether there are lurking requests that I have to wait for. Not
that it's difficult, it's just annoying, and I expect it would be a
nuisance to very many Python users. It might also be slow if there are
quite a few requests still to be fulfilled (depending on whether they
can be "cancelled" quickly).

I guess there are other solutions where maybe I could test if the file
descriptor is readable and so on, but again, why would we make it
complicated like this?

I did notice the comment earlier that the Python bindings are far from
stable. I certainly agree that where things are unfinished or untidy,
those things need to be sorted out, but in other areas we basically
have what we need for all our functionality, and so would be happy to
see less churn. I don't know if that's possible in the short term...

So maybe I did have some opinions after all!

Thanks everyone
David

On Tue, 28 Jun 2022 at 08:08, Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> On 27/06/2022 23:42, Laurent Pinchart wrote:
> > 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.
>
> Oh, it's trivial in Python in the sense that it's a few lines of code.
> But it's not trivial in the sense that you could just write
>
> while True:
>         # This waits until there are requests
>          reqs = cm.get_ready_requests()
>          for req in reqs:
>                 do something
>
> We can easily write a helper function on top of non-blocking API to
> achieve the above.
>
> >> 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.
>
> I can't make my mind on this. On one hand, what you suggest is a clean
> approach. On the other, if we have a core bindings module and a
> convenience module on top, it brings up some questions:
>
> - Module naming. libcamera-core and libcamera? libcamera and
> libcamera-for-dummies?
>
> - Is the convenience module just a sugar topping, i.e. helper
> functions/classes here and there, or is it something that fully wraps
> the core (somewhat like pilibcamera2)?
>
> - If it's just some helpers, what is the benefit of using the core module?
>
> - If it fully wraps the core, most likely it somewhat dummies down the
> libcamera features. If so, there are always users for the core module.
> Those users would most likely want to use simple helpers like blocking
> wait or mmapped fb.
>
> - Perhaps a third module option would be something in between: a module
> that "pythonizes" the classes, while still exposing everything. But are
> the convenience features part of that, or yet another module on top?
> Such a module also brings up the annoyance that you would access many
> things via the higher level module (say, pylibcamera.Camera), but
> probably many things would be just exposed from libcamera module if we
> don't do a full wrap (say, camera.set_format(libcamera.formats.RGB123).
>
> So... I think it's clear that a fully wrapping module is obviously a new
> module on top of the core bindings. But the simple helpers and possibly
> the pythonization could be part of the core bindings, or on top.
>
> It's clear that features like mmapped fb class and blocking wait are
> features that are needed by the users, so I think it's not a question of
> should we have those, but where should they be.
>
> Also, not that even if the helpers would be part of the core bindings,
> we could have internal split there: the bindings made in C++ would be
> the core bindings, and in the same module we could have convenience
> features written in Python (similar to the MappedFrameBuffer.py we have
> now). However, while that sounds nice, I don't know if there's any real
> logic to it. If something is easier/better written in the C++ bindings,
> I don't see a point in forcing that feature to be written in Python instead.
>
> > 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 :-)
>
> The discard_events() is essentially a cleanup function.
>
> Perhaps we can do some clever weak-ref tricks there, though... I think
> the events should not keep anything alive. Oh, but we depend on the
> event keeping the Request alive, so that doesn't work.
>
>   Tomi

On 28/06/2022 11:16, David Plowman wrote:
> Hi everyone
> 
> Sorry for not taking part in this discussion rather more, and thanks
> to those who have! I think I was asked further back if I had an
> opinion, so let me try and explain how I see things.
> 
> Actually I don't have any particularly strong opinions, other than
> that I want things to work and to be easy to use. And if they stopped
> changing, that would be nice too!!

Valid requests, but I think I can promise you only the first one =). The 
easy-to-use would be nice, but as we've discussed, perhaps the core 
bindings should be, well, core, which doesn't always equal easy to use.

And the third one... I'll try to keep backward compatibility when 
possible (at least for a period of time), but that's just not always the 
case.

> I was actually OK with the previous version where get_ready_requests
> was non-blocking, it's only the more recent version that causes me
> some trouble because I can't simply flush out any lurking requests
> after stopping the camera - because there might not be any and I will
> simply lock up the system.

The v2 series goes back to that, although the code behind is a bit 
different.

  Tomi

On 28/06/2022 11:16, David Plowman wrote:
> Hi everyone
> 
> Sorry for not taking part in this discussion rather more, and thanks
> to those who have! I think I was asked further back if I had an
> opinion, so let me try and explain how I see things.
> 
> Actually I don't have any particularly strong opinions, other than
> that I want things to work and to be easy to use. And if they stopped
> changing, that would be nice too!!
> 
> I was actually OK with the previous version where get_ready_requests
> was non-blocking, it's only the more recent version that causes me
> some trouble because I can't simply flush out any lurking requests
> after stopping the camera - because there might not be any and I will
> simply lock up the system.

Btw, while we're discussing and fixing this issue, you can also do a 
check in Python:

         import selectors
         sel = selectors.DefaultSelector()
         sel.register(cm.event_fd, selectors.EVENT_READ)
         events = sel.select(0)
         if events:
             print("get_ready_requests doesn't block")
         else:
             print("get_ready_requests may block")

This could be used to avoid calling get_ready_requests when it may block.

  Tomi

Ah, I was just thinking of finding out how to do that, I'll give it a
try. Thanks!

David

On Thu, 30 Jun 2022 at 07:13, Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> On 28/06/2022 11:16, David Plowman wrote:
> > Hi everyone
> >
> > Sorry for not taking part in this discussion rather more, and thanks
> > to those who have! I think I was asked further back if I had an
> > opinion, so let me try and explain how I see things.
> >
> > Actually I don't have any particularly strong opinions, other than
> > that I want things to work and to be easy to use. And if they stopped
> > changing, that would be nice too!!
> >
> > I was actually OK with the previous version where get_ready_requests
> > was non-blocking, it's only the more recent version that causes me
> > some trouble because I can't simply flush out any lurking requests
> > after stopping the camera - because there might not be any and I will
> > simply lock up the system.
>
> Btw, while we're discussing and fixing this issue, you can also do a
> check in Python:
>
>          import selectors
>          sel = selectors.DefaultSelector()
>          sel.register(cm.event_fd, selectors.EVENT_READ)
>          events = sel.select(0)
>          if events:
>              print("get_ready_requests doesn't block")
>          else:
>              print("get_ready_requests may block")
>
> This could be used to avoid calling get_ready_requests when it may block.
>
>   Tomi