[libcamera-devel] [PATCH 4/8] android: CameraDevice: Take shared_ptr in constructor
Hirokazu Honda
hiroh at chromium.org
Tue Mar 23 03:56:33 CET 2021
Hi Laurent, thanks for reviewing.
On Tue, Mar 23, 2021 at 11:19 AM Laurent Pinchart
<laurent.pinchart at 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 at 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
More information about the libcamera-devel
mailing list