[libcamera-devel] [PATCH 3/8] android: CameraHalManager: Fix a function call of a moved Camera

[Hirokazu Honda]

On Tue, Mar 23, 2021 at 11:04 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Tue, Mar 23, 2021 at 10:42:21AM +0900, Hirokazu Honda wrote:
> > This fixes a bug of calling Camera::id() of a moved Camera.
>
> Indeed, good catch. I however wonder if it wouldn't be simpler to just
> drop the move, as cam is a shared pointer ?
>

This might be related to the next patch.
I would pass either by copy or std::move().
I pass by copy only if a caller still needs the object, otherwise pass
by std::move().
In this case, we don't need the object's resource, but only the string.
So storing it before it enables us to pass cam by std::move().
I would rather do this because of this reason.
How do you think?

This shared_ptr strategy is come from chromium c++ style guide, where
scoped_refptr is almost equivalent to shared_ptr.
https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++.md#object-ownership-and-calling-conventions

Best Regards,
-Hiro

> -       std::shared_ptr<CameraDevice> camera = CameraDevice::create(id, std::move(cam));
> +       std::shared_ptr<CameraDevice> camera = CameraDevice::create(id, cam);
>
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> >
> > ---
> >  src/android/camera_hal_manager.cpp | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> > index fa398fea..df3f1cd2 100644
> > --- a/src/android/camera_hal_manager.cpp
> > +++ b/src/android/camera_hal_manager.cpp
> > @@ -95,7 +95,8 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
> >        * IDs starts from '0' for internal cameras and '1000' for external
> >        * cameras.
> >        */
> > -     auto iter = cameraIdsMap_.find(cam->id());
> > +     std::string cameraId = cam->id();
> > +     auto iter = cameraIdsMap_.find(cameraId);
> >       if (iter != cameraIdsMap_.end()) {
> >               id = iter->second;
> >       } else {
> > @@ -117,12 +118,12 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
> >       std::unique_ptr<CameraDevice> camera = CameraDevice::create(id, std::move(cam));
> >       int ret = camera->initialize();
> >       if (ret) {
> > -             LOG(HAL, Error) << "Failed to initialize camera: " << cam->id();
> > +             LOG(HAL, Error) << "Failed to initialize camera: " << cameraId;
> >               return;
> >       }
> >
> >       if (isCameraNew) {
> > -             cameraIdsMap_.emplace(cam->id(), id);
> > +             cameraIdsMap_.emplace(cameraId, id);
> >
> >               if (isCameraExternal)
> >                       nextExternalCameraId_++;
>
> --
> Regards,
>
> Laurent Pinchart