[PATCH] libcamera: Add static Object::Deleter function

[Cheng-Hao Yang]

Hi Barnabás,

On Thu, Oct 24, 2024 at 8:59 PM Barnabás Pőcze <pobrn at protonmail.com> wrote:
>
> Hi
>
>
> 2024. október 24., csütörtök 11:11 keltezéssel, Cheng-Hao Yang <chenghaoyang at chromium.org> írta:
>
> > Hi Barnabás,
> >
> > On Thu, Oct 24, 2024 at 1:33 AM Barnabás Pőcze <pobrn at protonmail.com> wrote:
> > >
> > > Hi
> > >
> > >
> > > 2024. október 23., szerda 18:58 keltezéssel, Harvey Yang <chenghaoyang at chromium.org> írta:
> > >
> > > > This patch allows a smart pointer of Object to be destructed in other
> > > > threads.
> > > >
> > > > There will be multiple usages of smart pointers of Object. This patch
> > > > adds the deleter function to avoid duplicated code.
> > > >
> > > > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > > > ---
> > > >  include/libcamera/base/object.h |  2 ++
> > > >  src/libcamera/base/object.cpp   | 11 +++++++++++
> > > >  2 files changed, 13 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/base/object.h b/include/libcamera/base/object.h
> > > > index 508773cd0..c4522d480 100644
> > > > --- a/include/libcamera/base/object.h
> > > > +++ b/include/libcamera/base/object.h
> > > > @@ -24,6 +24,8 @@ class Thread;
> > > >  class Object
> > > >  {
> > > >  public:
> > > > +     static void Deleter(Object *obj);
> > >
> > > I think a functor would be preferable, see camera.cpp:Camera::create():
> > >
> > >   struct Deleter {
> > >     void operator()(Object *obj) const
> > >     {
> > >       if (Thread::current() == obj->thread())
> > >         delete obj;
> > >       else
> > >         obj->deleteLater();
> > >     }
> > >   };
> > >
> > > Or is there a reason why this wouldn't work?
> >
> > True, thanks for the notice, and it works.
> >
> > I also just found that it's stated in object.cpp:
> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/base/object.cpp#n143
>
> Ahh you're right, I didn't notice that.
>
>
> >
> > Do you think I should add this functor to avoid duplication,
> > or the developers should copy and paste the above
> > functor each time for a derived class?
>
> Well, I think having just the functor here is a bit suboptimal because generally
> it makes sense to keep the deleter and the smart pointer construction close
> to each other.
>
> So now I am thinking about something like this:
>
>   template<typename T, typename... Args>
>   static std::unique_ptr<T, Deleter> create(Args&&... args)
>   {
>     return std::unique_ptr<T, Deleter>(new T(std::forward<Args>(args)...));
>   }
>
> And if you want a shared_ptr, you can simply do
>
>   std::shared_ptr<T>(Object::create<T>(...));

I encountered some undefined symbols with this template function,
which I normally
fixed by putting the definition in the header file instead of the source file.
However, it seems that including `base/thread.h` in `base/object.h` causes other
issues (cyclic dependencies...?).

>
> Or I guess you could add two functions: `make_unique` and `make_shared` instead of `create`.

I think defining make_unique of Object would overwrite the default one, and thus
cause some issues in other Object derived classes' usages. Let's avoid
this as well.

>
> I don't know what your use cases are, but I believe this would likely allow
> you to make simplifications.

My use case currently is for std::unique_ptr [1], FYI.

As there doesn't seem to be a clean way to do this, we can also just
drop this patch.
WDYT?

BR,
Harvey

[1]:https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/ipa/mtkisp7/mtkisp7.cpp;l=43

>
>
> Regards,
> Barnabás Pőcze
>
> >
> > BR,
> > Harvey
> >
> > >
> > >
> > > Regards,
> > > Barnabás Pőcze
> > >
> > > > +
> > > >       Object(Object *parent = nullptr);
> > > >       virtual ~Object();
> > > >
> > > > diff --git a/src/libcamera/base/object.cpp b/src/libcamera/base/object.cpp
> > > > index 745d2565a..2c04b99a5 100644
> > > > --- a/src/libcamera/base/object.cpp
> > > > +++ b/src/libcamera/base/object.cpp
> > > > @@ -59,6 +59,17 @@ LOG_DEFINE_CATEGORY(Object)
> > > >   * \sa Message, Signal, Thread
> > > >   */
> > > >
> > > > +/**
> > > > + * \brief A deleter function that calls Object::deleteLater
> > > > + * \param[in] obj The object itself
> > > > + *
> > > > + * The static deleter function that's used in smart pointers.
> > > > + */
> > > > +void Object::Deleter(Object *obj)
> > > > +{
> > > > +     obj->deleteLater();
> > > > +}
> > > > +
> > > >  /**
> > > >   * \brief Construct an Object instance
> > > >   * \param[in] parent The object parent
> > > > --
> > > > 2.47.0.105.g07ac214952-goog
> > > >