libcamera: Add static Object::Deleter function

21746 diff mbox series

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@chromium.org>
---
 include/libcamera/base/object.h |  2 ++
 src/libcamera/base/object.cpp   | 11 +++++++++++
 2 files changed, 13 insertions(+)

Hi


2024. október 23., szerda 18:58 keltezéssel, Harvey Yang <chenghaoyang@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@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?


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
>

Hi Barnabás,

On Thu, Oct 24, 2024 at 1:33 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Hi
>
>
> 2024. október 23., szerda 18:58 keltezéssel, Harvey Yang <chenghaoyang@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@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

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?

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

Hi


2024. október 24., csütörtök 11:11 keltezéssel, Cheng-Hao Yang <chenghaoyang@chromium.org> írta:

> Hi Barnabás,
> 
> On Thu, Oct 24, 2024 at 1:33 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> >
> > Hi
> >
> >
> > 2024. október 23., szerda 18:58 keltezéssel, Harvey Yang <chenghaoyang@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@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>(...));

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

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


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

Hi Barnabás,

On Thu, Oct 24, 2024 at 8:59 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Hi
>
>
> 2024. október 24., csütörtök 11:11 keltezéssel, Cheng-Hao Yang <chenghaoyang@chromium.org> írta:
>
> > Hi Barnabás,
> >
> > On Thu, Oct 24, 2024 at 1:33 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> > >
> > > Hi
> > >
> > >
> > > 2024. október 23., szerda 18:58 keltezéssel, Harvey Yang <chenghaoyang@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@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
> > > >