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

[Laurent Pinchart]

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.

> 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