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

[Laurent Pinchart]

On Mon, Jun 27, 2022 at 01:44:22PM +0300, Tomi Valkeinen wrote:
> 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 agree it's a bit hard to follow :-) There's copy elision, and return
value optimization (RVO), and their behaviour change based on the C++
version. In practice, C++17 introduced mandatory RVO, you should have a
guarantee that the above code will use copy elision and avoid
unnecessary copies.

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

-- 
Regards,

Laurent Pinchart