[libcamera-devel] [RFC v3 1/5] HACK: Camera public destructor

[Tomi Valkeinen]

On 09/12/2021 18:46, Laurent Pinchart wrote:
> On Thu, Dec 09, 2021 at 12:54:07PM +0200, Tomi Valkeinen wrote:
>> On 09/12/2021 12:34, Kieran Bingham wrote:
>>> Quoting Tomi Valkeinen (2021-12-09 10:22:16)
>>>> On 09/12/2021 12:05, Kieran Bingham wrote:
>>>>> Quoting Tomi Valkeinen (2021-12-09 09:29:02)
>>>>>> pybind11 needs a public destructor for Camera to be able to manage the
>>>>>> shared_ptr. It's not clear why this is needed, and can it be fixed in
>>>>>> pybind11.
>>>>>>
>>>>>
>>>>> :-( The 'HACK' here seems to be the only thing that would count as a
>>>>> blocker to integration.
>>>>>
>>>>> I wonder if this is really a HACK or if we just need to explain that the
>>>>> destructor needs to be public to support the python bindings, but I
>>>>> myself don't understand the reasoning itself.
>>>>>
>>>>> Is the python code the only instance where we create a shared_ptr for
>>>>> the Camera? If that's true - then perhaps this patch should actually be
>>>>> squashed with the one that returns the Camera as a shared_ptr...
>>>>
>>>> This is not related to the patch 2.
>>>>
>>>> pybind11 needs a holder type that is used to manage the references to
>>>> the C++ instances. By default it's unique_ptr, but can be defined. We
>>>> use shared_ptr for many classes
>>>> (https://pybind11.readthedocs.io/en/stable/advanced/smart_ptrs.html?highlight=shared_ptr#std-shared-ptr).
>>>>
>>>> I think normally it makes sense that the destructor must be public: if
>>>> pybind11 wrapper code can create the instances, it can also destruct
>>>> them. Here we don't create Cameras, nor destruct them, but the related
>>>> code is still created, causing the build error.
>>>>
>>>> I believe it should be possible to fix this in pybind11, by making the
>>>> template code to detect these kind of classes and skip the destructor
>>>> code. Maybe. Somehow. I have no idea how =).
>>>
>>> But how could a shared_ptr skip calling the destructor? Isn't the
>>> purpose of a shared_ptr to keep the object 'alive' until the last user
>>> is removed? Therefore the shared_ptr has to be able to destruct the
>>> object?
>>>
>>> Is there some magic that means the destructor can still be private for a
>>> shared_ptr in that instance?
>>>
>>> A quick google on shared_ptr private destructor brings up:
>>> 	https://stackoverflow.com/a/8202667
>>>
>>> Saying it can be solved by making a custom deleter for the shared
>>> pointer. Is that what we're missing perhaps?
>>
>> That is what is done in Camera.cpp, Camera::create(). Afaik that is the
>> only place where cameras are instantiated, and it, of course, has access
>> to the destructor.
>>
>> The pybind11 template code generates code to manage the wrapped class,
>> and I think that code includes constructing and destructing of the
>> instance. Or maybe only destructing, as otherwise we should get a
>> similar error for the constructor if that is required.
> 
> If std::shared_ptr<> causes issue, we could manually implement a
> PyCamera (name to be bikeshedded, could be just Camera in a py::
> namespace) C++ class that would wrap std::shared_ptr<Camera>.

Looks like there's a proposed fix:

https://github.com/jagerman/pybind11/commit/53be81931f35313f70affc9826bdfed9820cce2c

but the pull-req hasn't been approved:

https://github.com/pybind/pybind11/pull/2067

There also seems to be a fork (?) or pybind11 which implements something related,
which would possibly help here (or not):

https://github.com/pybind/pybind11/blob/49f8f60ec4254e0f55db3c095c945210bcb43ad2/README_smart_holder.rst

I'm a bit reluctant going to the wrapper class just to avoid this issue,
as I fear it will create a bit of a mess in the code.

  Tomi