[libcamera-devel] [PATCH v3 05/17] py: Create PyCameraManager

[Jacopo Mondi]

Hi Tomi,

On Thu, Aug 18, 2022 at 06:14:41PM +0300, Tomi Valkeinen wrote:
> On 18/08/2022 18:00, Jacopo Mondi wrote:
>
> > > > > +PyCameraManager::PyCameraManager()
> > > > > +{
> > > > > +	LOG(Python, Debug) << "PyCameraManager()";
> > > >
> > > > Don't you need to LOG_DECLARE_CATEGORY() ?
> > >
> > > It's in py_main.h.
> > >
> >
> > Ah, I overlooked the first patches as they were reviewed already.
> > Well, that header only serves for that purpose.
> >
> > I would have DEFINE_CATEGORY in py_main.cpp and DECLARE_CATEGORY()
> > and spare that header.
>
> Hmm, sorry can you clarify what you mean?
>

If I'm not mistaken py_main.h will only contain LOG_DEFINE_CATEGORY().

I would have DEFINE_CATEGORY() in py_main.cpp and use
LOG_DECLARE_CATEGORY() (which expands to an 'extern' declaration) in
the other compilation units that use the log symbol (which if I'm not
mistaken is py_camera_manager.cpp only). In this way you can get rid
of py_main.h completely.


> > > > What is the advantage of using a weak_ptr<> here ? Can't this be kept
> > > > as a shared_ptr ?
> > >
> > > shared_ptr keeps it alive. We don't want to keep it alive. It's the job of
> >
> > Oh, I thought we wanted to keep it alive and share ownership with
> > apps.
> >
> > I don't really understand how lifetime is handled in pybind, so I
> > trust your judjment here
>
> Well, in short, the _bindings_ do not want to keep anything alive. It's the
> python side that keeps things alive (by having objects in variables/fields).
> But sometimes the bindings need to be able to access things in places where
> the thing is not trivially available (i.e. passed as a parameter from the
> python side). So we have to store the camera manager, but only as weak_ptr.
>

Thanks for the summary :)

> > > the application to keep it alive (as long as it uses it).
> > >
> > > > >    			}
> > > > >
> > > > > -			std::vector<py::object> ret;
> > > > > -
> > > > > -			for (Request *req : v) {
> > > > > -				py::object o = py::cast(req);
> > > > > -				/* Decrease the ref increased in Camera.queue_request() */
> > > > > -				o.dec_ref();
> > > > > -				ret.push_back(o);
> > > > > -			}
> > > > > -
> > > > > -			return ret;
> > > > > +			return cm;
> > > > >    		})
> > > > >
> > > > > -		.def("get", py::overload_cast<const std::string &>(&CameraManager::get), py::keep_alive<0, 1>())
> > > > > -
> > > > > -		/* Create a list of Cameras, where each camera has a keep-alive to CameraManager */
> > > > > -		.def_property_readonly("cameras", [](CameraManager &self) {
> > > > > -			py::list l;
> > > > > -
> > > > > -			for (auto &c : self.cameras()) {
> > > > > -				py::object py_cm = py::cast(self);
> > > > > -				py::object py_cam = py::cast(c);
> > > > > -				py::detail::keep_alive_impl(py_cam, py_cm);
> > > > > -				l.append(py_cam);
> > > > > -			}
> > > > > +		.def_property_readonly("version", &PyCameraManager::version)
> > > > > +		.def("get", &PyCameraManager::get, py::keep_alive<0, 1>())
> > > > > +		.def_property_readonly("cameras", &PyCameraManager::cameras)
> > > > >
> > > > > -			return l;
> > > > > -		});
> > > > > +		.def_property_readonly("event_fd", &PyCameraManager::eventFd)
> > > >
> > > > Is access to eventfs useful for applications ?
> > >
> > > Yes, that's how they wait for events.
> > >
> >
> > Don't we maks them behind the getReadyRequests() and
> > handleRequestCompleted() ?
>
> There are two things, 1) using the fd for waiting for events, 2) reading and
> writing to the fd. The 1) has to be done by the app, the 2) is done with the
> helper functions here.
>
>  Tomi