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

[Tomi Valkeinen]

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?

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

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