[libcamera-devel,4/8] android: CameraDevice: Take shared_ptr in constructor
diff mbox series

Message ID 20210323014226.3211412-5-hiroh@chromium.org
State Superseded
Headers show
Series
  • Improve pointer types in android HAL adaptation layer
Related show

Commit Message

Hirokazu Honda March 23, 2021, 1:42 a.m. UTC 
CameraDevice takes the ownership of Camera. Therefore,
shared_ptr would rather be used than const shared_ptr&.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>

---
 src/android/camera_device.cpp | 12 +++++++-----
 src/android/camera_device.h   |  4 ++--
 2 files changed, 9 insertions(+), 7 deletions(-)

--
2.31.0.rc2.261.g7f71774620-goog

Comments

Laurent Pinchart March 23, 2021, 2:18 a.m. UTC | #1 
Hi Hiro,

Thank you for the patch.

On Tue, Mar 23, 2021 at 10:42:22AM +0900, Hirokazu Honda wrote:
> CameraDevice takes the ownership of Camera. Therefore,
> shared_ptr would rather be used than const shared_ptr&.

It's a shared pointer, so the concept of ownership is not as clear-cut
as with unique_ptr. I wonder if this really makes the code better, it
creates additional copies, but doesn't change much else.

> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> 
> ---
>  src/android/camera_device.cpp | 12 +++++++-----
>  src/android/camera_device.h   |  4 ++--
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index d0955de7..c0630e53 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -312,9 +312,10 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()
>   * back to the framework using the designated callbacks.
>   */
> 
> -CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camera)
> -	: id_(id), running_(false), camera_(camera), staticMetadata_(nullptr),
> -	  facing_(CAMERA_FACING_FRONT), orientation_(0)
> +CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)
> +	: id_(id), running_(false), camera_(std::move(camera)),
> +	  staticMetadata_(nullptr), facing_(CAMERA_FACING_FRONT),
> +	  orientation_(0)
>  {
>  	camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
> 
> @@ -351,9 +352,10 @@ CameraDevice::~CameraDevice()
>  }
> 
>  std::unique_ptr<CameraDevice> CameraDevice::create(unsigned int id,
> -						   const std::shared_ptr<Camera> &cam)
> +						   std::shared_ptr<Camera> cam)
>  {
> -	return std::unique_ptr<CameraDevice>(new CameraDevice(id, cam));
> +	return std::unique_ptr<CameraDevice>(
> +		new CameraDevice(id, std::move(cam)));
>  }
> 
>  /*
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 8be7f305..555b33e7 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -33,7 +33,7 @@ class CameraDevice : protected libcamera::Loggable
>  {
>  public:
>  	static std::unique_ptr<CameraDevice> create(unsigned int id,
> -						    const std::shared_ptr<libcamera::Camera> &cam);
> +						    std::shared_ptr<libcamera::Camera> cam);
>  	~CameraDevice();
> 
>  	int initialize();
> @@ -66,7 +66,7 @@ protected:
>  	std::string logPrefix() const override;
> 
>  private:
> -	CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera);
> +	CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);
> 
>  	struct Camera3RequestDescriptor {
>  		Camera3RequestDescriptor(libcamera::Camera *camera,
Hirokazu Honda March 23, 2021, 2:56 a.m. UTC | #2 
Hi Laurent, thanks for reviewing.

On Tue, Mar 23, 2021 at 11:19 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Tue, Mar 23, 2021 at 10:42:22AM +0900, Hirokazu Honda wrote:
> > CameraDevice takes the ownership of Camera. Therefore,
> > shared_ptr would rather be used than const shared_ptr&.
>
> It's a shared pointer, so the concept of ownership is not as clear-cut
> as with unique_ptr. I wonder if this really makes the code better, it
> creates additional copies, but doesn't change much else.
>

I am based on chromium c++ style guide.
https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++.md#object-ownership-and-calling-conventions
> If the function (at least sometimes) takes a ref on a refcounted object, declare the param as scoped_refptr<T>. The caller can decide whether it wishes to transfer ownership (by calling std::move(t) when passing t) or retain its ref (by simply passing t directly).

Let me compare the number of copies.
struct Foo {
 Foo (shared_ptr<T> a) : a_(std::move(a)) {}   <- [A]
 Foo (const shared_ptr<T> &a) : a_(a)) {}  <- [B]
  shared_ptr<T> a_;
};

1. A caller executes Foo(std::move(a)).
With [A], two std::move().
With [B], one copy.

2. A caller executes Foo(a).
With [A], one copy + one std::move(),
With [B], one copy.

The cost seems to be almost compatible.

Best Regards,
-Hiro
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> >
> > ---
> >  src/android/camera_device.cpp | 12 +++++++-----
> >  src/android/camera_device.h   |  4 ++--
> >  2 files changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index d0955de7..c0630e53 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -312,9 +312,10 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()
> >   * back to the framework using the designated callbacks.
> >   */
> >
> > -CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camera)
> > -     : id_(id), running_(false), camera_(camera), staticMetadata_(nullptr),
> > -       facing_(CAMERA_FACING_FRONT), orientation_(0)
> > +CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)
> > +     : id_(id), running_(false), camera_(std::move(camera)),
> > +       staticMetadata_(nullptr), facing_(CAMERA_FACING_FRONT),
> > +       orientation_(0)
> >  {
> >       camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
> >
> > @@ -351,9 +352,10 @@ CameraDevice::~CameraDevice()
> >  }
> >
> >  std::unique_ptr<CameraDevice> CameraDevice::create(unsigned int id,
> > -                                                const std::shared_ptr<Camera> &cam)
> > +                                                std::shared_ptr<Camera> cam)
> >  {
> > -     return std::unique_ptr<CameraDevice>(new CameraDevice(id, cam));
> > +     return std::unique_ptr<CameraDevice>(
> > +             new CameraDevice(id, std::move(cam)));
> >  }
> >
> >  /*
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 8be7f305..555b33e7 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -33,7 +33,7 @@ class CameraDevice : protected libcamera::Loggable
> >  {
> >  public:
> >       static std::unique_ptr<CameraDevice> create(unsigned int id,
> > -                                                 const std::shared_ptr<libcamera::Camera> &cam);
> > +                                                 std::shared_ptr<libcamera::Camera> cam);
> >       ~CameraDevice();
> >
> >       int initialize();
> > @@ -66,7 +66,7 @@ protected:
> >       std::string logPrefix() const override;
> >
> >  private:
> > -     CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera);
> > +     CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);
> >
> >       struct Camera3RequestDescriptor {
> >               Camera3RequestDescriptor(libcamera::Camera *camera,
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart March 23, 2021, 2:30 p.m. UTC | #3 
Hi Hiro,

Thank you for the patch.

On Tue, Mar 23, 2021 at 11:56:33AM +0900, Hirokazu Honda wrote:
> On Tue, Mar 23, 2021 at 11:19 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Hiro,
> >
> > Thank you for the patch.
> >
> > On Tue, Mar 23, 2021 at 10:42:22AM +0900, Hirokazu Honda wrote:
> > > CameraDevice takes the ownership of Camera. Therefore,
> > > shared_ptr would rather be used than const shared_ptr&.
> >
> > It's a shared pointer, so the concept of ownership is not as clear-cut
> > as with unique_ptr. I wonder if this really makes the code better, it
> > creates additional copies, but doesn't change much else.
> 
> I am based on chromium c++ style guide.
> https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++.md#object-ownership-and-calling-conventions
>
> > If the function (at least sometimes) takes a ref on a refcounted
> > object, declare the param as scoped_refptr<T>. The caller can decide
> > whether it wishes to transfer ownership (by calling std::move(t)
> > when passing t) or retain its ref (by simply passing t directly).
> 
> Let me compare the number of copies.
> struct Foo {
>  Foo (shared_ptr<T> a) : a_(std::move(a)) {}   <- [A]
>  Foo (const shared_ptr<T> &a) : a_(a)) {}  <- [B]
>   shared_ptr<T> a_;
> };
> 
> 1. A caller executes Foo(std::move(a)).
> With [A], two std::move().
> With [B], one copy.
> 
> 2. A caller executes Foo(a).
> With [A], one copy + one std::move(),
> With [B], one copy.
> 
> The cost seems to be almost compatible.

We're not in a hot path anyway, so I agree with you, it's best to follow
clear ownership rules here to make the code base more consistent
(especially given that the copy here is about copying a shared_ptr, not
copying the pointed object).

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > >
> > > ---
> > >  src/android/camera_device.cpp | 12 +++++++-----
> > >  src/android/camera_device.h   |  4 ++--
> > >  2 files changed, 9 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index d0955de7..c0630e53 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -312,9 +312,10 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()
> > >   * back to the framework using the designated callbacks.
> > >   */
> > >
> > > -CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camera)
> > > -     : id_(id), running_(false), camera_(camera), staticMetadata_(nullptr),
> > > -       facing_(CAMERA_FACING_FRONT), orientation_(0)
> > > +CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)
> > > +     : id_(id), running_(false), camera_(std::move(camera)),
> > > +       staticMetadata_(nullptr), facing_(CAMERA_FACING_FRONT),
> > > +       orientation_(0)
> > >  {
> > >       camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
> > >
> > > @@ -351,9 +352,10 @@ CameraDevice::~CameraDevice()
> > >  }
> > >
> > >  std::unique_ptr<CameraDevice> CameraDevice::create(unsigned int id,
> > > -                                                const std::shared_ptr<Camera> &cam)
> > > +                                                std::shared_ptr<Camera> cam)
> > >  {
> > > -     return std::unique_ptr<CameraDevice>(new CameraDevice(id, cam));
> > > +     return std::unique_ptr<CameraDevice>(
> > > +             new CameraDevice(id, std::move(cam)));
> > >  }
> > >
> > >  /*
> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > index 8be7f305..555b33e7 100644
> > > --- a/src/android/camera_device.h
> > > +++ b/src/android/camera_device.h
> > > @@ -33,7 +33,7 @@ class CameraDevice : protected libcamera::Loggable
> > >  {
> > >  public:
> > >       static std::unique_ptr<CameraDevice> create(unsigned int id,
> > > -                                                 const std::shared_ptr<libcamera::Camera> &cam);
> > > +                                                 std::shared_ptr<libcamera::Camera> cam);
> > >       ~CameraDevice();
> > >
> > >       int initialize();
> > > @@ -66,7 +66,7 @@ protected:
> > >       std::string logPrefix() const override;
> > >
> > >  private:
> > > -     CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera);
> > > +     CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);
> > >
> > >       struct Camera3RequestDescriptor {
> > >               Camera3RequestDescriptor(libcamera::Camera *camera,

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index d0955de7..c0630e53 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -312,9 +312,10 @@  CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()
  * back to the framework using the designated callbacks.
  */

-CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camera)
-	: id_(id), running_(false), camera_(camera), staticMetadata_(nullptr),
-	  facing_(CAMERA_FACING_FRONT), orientation_(0)
+CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)
+	: id_(id), running_(false), camera_(std::move(camera)),
+	  staticMetadata_(nullptr), facing_(CAMERA_FACING_FRONT),
+	  orientation_(0)
 {
 	camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);

@@ -351,9 +352,10 @@  CameraDevice::~CameraDevice()
 }

 std::unique_ptr<CameraDevice> CameraDevice::create(unsigned int id,
-						   const std::shared_ptr<Camera> &cam)
+						   std::shared_ptr<Camera> cam)
 {
-	return std::unique_ptr<CameraDevice>(new CameraDevice(id, cam));
+	return std::unique_ptr<CameraDevice>(
+		new CameraDevice(id, std::move(cam)));
 }

 /*
diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index 8be7f305..555b33e7 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -33,7 +33,7 @@  class CameraDevice : protected libcamera::Loggable
 {
 public:
 	static std::unique_ptr<CameraDevice> create(unsigned int id,
-						    const std::shared_ptr<libcamera::Camera> &cam);
+						    std::shared_ptr<libcamera::Camera> cam);
 	~CameraDevice();

 	int initialize();
@@ -66,7 +66,7 @@  protected:
 	std::string logPrefix() const override;

 private:
-	CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera);
+	CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);

 	struct Camera3RequestDescriptor {
 		Camera3RequestDescriptor(libcamera::Camera *camera,