[libcamera-devel] [RFC v1 4/7] py: Create PyCameraManager

[Tomi Valkeinen]

On 27/06/2022 13:25, Laurent Pinchart wrote:
>>>>> +       getRequests(v);
>>>
>>> You can make getRequests() return the vector instead of passing it as a
>>> reference, and then even do
>>>
>>> 	for (Request *req : getRequests()) {
>>
>> Ok. I usually keep away from those as I don't seem to ever exactly know
>> when copies are made and when not...
> 
> With copy elision the vector returned by getRequest() will not be copied.

Right. But when is copy elision done? Reading 
https://en.cppreference.com/w/cpp/language/copy_elision doesn't make it 
clear either, as it's a lot of rules and changes in each C++ version.

I'm sure the answer is there, my point is just that when I write code, 
it's often not obvious when a copy will be made, or will possibly be 
made, or will never be made. So I go with the safe way.

>>>>> +class PyCameraManager : public CameraManager
>>>
>>> CameraManager isn't meant to be inherited from (I'm surprised it's not
>>> marked as final). You can keep this for now, but we should soon switch
>>> to encapsulating the CameraManager instead.
>>
>> Hmm that may be problematic. At the moment if there's a CameraManager
>> pointer, the pybind11 bindings can easily convert that instance to a
>> Python instance. But if we encapsulate it, that's no longer the case, as
>> you need something extra to associate the CameraManager pointer to the
>> encapsulating class.
>>
>> I tried to do that with Cameras at some point (trying to sort out the
>> destructor issue), but I didn't get it working. Maybe I'll try again
>> with CameraManager this time.
> 
> Given that both the CameraManager and the PyCameraManager can only be
> instantiated once, the Python code could unconditionally use the
> PyCameraManager, without ever using the CameraManager pointer directly.
> Do we even have any event that passes the CameraManager as an argument ?
> For Camera instances we do, so that's indeed more problematic.

Afaik it should be possible with Cameras too, but, as I mentioned, I 
never got it working. I'll try again, CameraManager should be easier as 
you say.

>>>>> +                       self.requestCompleted.connect(&self, [cm=std::move(cm)](Request *req) {
>>>>> +
>>>
>>> Extra blank line.
>>
>> Yep.
>>
>>>>> +                               cm->handleRequestCompleted(req);
>>>>> +                       });
>>>
>>> So the following didn't work ?
>>>
>>> 			self.requestCompleted.connect(cm.get(), &PyCameraManager::handleRequestCompleted);
>>>
>>> ? I can investigate.
>>
>> I didn't try that. I don't think what you have above is the same. Note
>> that in my version I store a shared_ptr<CameraManager> to make sure it
>> stays alive.
> 
> Is there a way we could avoid that ?

Probably. The life-time questions are difficult, especially with Python 
bindings. But I think we can never get requestCompleted signal if the 
camera is not alive, and if the camera is alive, the camera manager must 
be alive, so we should be able to just use the CameraManager pointer.

>> The issue I had (which I asked about in the chat) was in the "New event
>> handling", when connecting the cameraAdded signal.
> 
> I recall you had trouble receiving the cameraAdded signal even when
> testing with cam, due to udev support being disabled. Have you retested
> after fixing that ?

Yes. And as I said, the signal works fine when I use a plain function. 
But, for whatever reason, I was not able to get connecting with a lambda 
or an object method working.

  Tomi