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

[Hirokazu Honda]

Hi Laurent,

On Tue, Mar 23, 2021 at 11:28 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Tue, Mar 23, 2021 at 11:33:40AM +0900, Hirokazu Honda wrote:
> > On Tue, Mar 23, 2021 at 11:04 AM Laurent Pinchart wrote:
> > > 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?
>
> If it was a std::unique_ptr I would certainly agree. With a
> std::shared_ptr it's a bit different, as both the caller and the callee
> can store their own difference. As we still have a use case for using
> the cam object after the call to CameraDevice::create(), I think that
> not transfering ownership will make the code simpler here.
>

I see. I will upload the patch to not std::move().
-Hiro

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