| Message ID | 20210324070757.3530377-6-hiroh@chromium.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. On Wed, Mar 24, 2021 at 04:07:54PM +0900, Hirokazu Honda wrote: > CameraDevice::camera() originally returns shared_ptr. It is > mandatory to make a copy by calling camera() in this way. There > is no need of copying if a caller needs the reference of the > camera like const shared_ptr<Camera> cam = camera(). That is, it It should still be '&cam' :-) I'll fix it when applying. > is a caller that copying is required. This changes the return > type of camera() to const shared_ptr&, so that we are able to > reduce one redundant copy in the above case. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/android/camera_device.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index 555b33e7..14e26b4d 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -43,7 +43,7 @@ public: > > unsigned int id() const { return id_; } > camera3_device_t *camera3Device() { return &camera3Device_; } > - std::shared_ptr<libcamera::Camera> camera() const { return camera_; } > + const std::shared_ptr<libcamera::Camera> &camera() const { return camera_; } > libcamera::CameraConfiguration *cameraConfiguration() const > { > return config_.get();
Hi Hiro, On Wed, Mar 24, 2021 at 04:07:54PM +0900, Hirokazu Honda wrote: > CameraDevice::camera() originally returns shared_ptr. It is > mandatory to make a copy by calling camera() in this way. There > is no need of copying if a caller needs the reference of the > camera like const shared_ptr<Camera> cam = camera(). That is, it > is a caller that copying is required. This changes the return > type of camera() to const shared_ptr&, so that we are able to > reduce one redundant copy in the above case. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> As I see CameraDevice::camera() being only used in src/android/camera_hal_manager.cpp: return cam == camera->camera(); This makes indeed sense. It might not be the greatest API as it's easy to assume it gets a shared_ptr<> to share ownership.. Probably we should just return a Camera* ? This is an improvement anyway, so Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > src/android/camera_device.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > index 555b33e7..14e26b4d 100644 > --- a/src/android/camera_device.h > +++ b/src/android/camera_device.h > @@ -43,7 +43,7 @@ public: > > unsigned int id() const { return id_; } > camera3_device_t *camera3Device() { return &camera3Device_; } > - std::shared_ptr<libcamera::Camera> camera() const { return camera_; } > + const std::shared_ptr<libcamera::Camera> &camera() const { return camera_; } > libcamera::CameraConfiguration *cameraConfiguration() const > { > return config_.get(); > -- > 2.31.0.291.g576ba9dcdaf-goog > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On Thu, Mar 25, 2021 at 5:43 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Hiro, > > On Wed, Mar 24, 2021 at 04:07:54PM +0900, Hirokazu Honda wrote: > > CameraDevice::camera() originally returns shared_ptr. It is > > mandatory to make a copy by calling camera() in this way. There > > is no need of copying if a caller needs the reference of the > > camera like const shared_ptr<Camera> cam = camera(). That is, it > > is a caller that copying is required. This changes the return > > type of camera() to const shared_ptr&, so that we are able to > > reduce one redundant copy in the above case. > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > As I see CameraDevice::camera() being only used in > src/android/camera_hal_manager.cpp: return cam == camera->camera(); > > This makes indeed sense. It might not be the greatest API as it's easy > to assume it gets a shared_ptr<> to share ownership.. Probably we > should just return a Camera* ? > There is one more call. FrameBufferAllocator takes shared_ptr. allocator_ = std::make_unique<FrameBufferAllocator>(cameraDevice_->camera()); So this should not be std::shared_ptr<>&. -Hiro > This is an improvement anyway, so > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Thanks > j > > > --- > > src/android/camera_device.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h > > index 555b33e7..14e26b4d 100644 > > --- a/src/android/camera_device.h > > +++ b/src/android/camera_device.h > > @@ -43,7 +43,7 @@ public: > > > > unsigned int id() const { return id_; } > > camera3_device_t *camera3Device() { return &camera3Device_; } > > - std::shared_ptr<libcamera::Camera> camera() const { return camera_; } > > + const std::shared_ptr<libcamera::Camera> &camera() const { return camera_; } > > libcamera::CameraConfiguration *cameraConfiguration() const > > { > > return config_.get(); > > -- > > 2.31.0.291.g576ba9dcdaf-goog > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/android/camera_device.h b/src/android/camera_device.h index 555b33e7..14e26b4d 100644 --- a/src/android/camera_device.h +++ b/src/android/camera_device.h @@ -43,7 +43,7 @@ public: unsigned int id() const { return id_; } camera3_device_t *camera3Device() { return &camera3Device_; } - std::shared_ptr<libcamera::Camera> camera() const { return camera_; } + const std::shared_ptr<libcamera::Camera> &camera() const { return camera_; } libcamera::CameraConfiguration *cameraConfiguration() const { return config_.get();