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

[Laurent Pinchart]

On Thu, Aug 18, 2022 at 05:26:20PM +0200, Jacopo Mondi wrote:
> 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.

Note that LOG_DECLARE_CATEGORY is similar to declaring a function
prototype or external variable. Having it in a header file makes sense
to me, but I would also not be against doing it manually in the .cpp
files that need it, probably mostly because we do that already in a few
places.

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

-- 
Regards,

Laurent Pinchart