[libcamera-devel,3/8] android: CameraHalManager: Fix a function call of a moved Camera
diff mbox series

Message ID 20210323014226.3211412-4-hiroh@chromium.org
State Superseded
Headers show
Series
  • Improve pointer types in android HAL adaptation layer
Related show

Commit Message

Hirokazu Honda March 23, 2021, 1:42 a.m. UTC 
This fixes a bug of calling Camera::id() of a moved Camera.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>

---
 src/android/camera_hal_manager.cpp | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

--
2.31.0.rc2.261.g7f71774620-goog

Comments

Laurent Pinchart March 23, 2021, 2:03 a.m. UTC | #1 
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 ?

-	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@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_++;
Hirokazu Honda March 23, 2021, 2:33 a.m. UTC | #2 
On Tue, Mar 23, 2021 at 11:04 AM Laurent Pinchart
<laurent.pinchart@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@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
Laurent Pinchart March 23, 2021, 2:27 p.m. UTC | #3 
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@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_++;
Hirokazu Honda March 24, 2021, 6:46 a.m. UTC | #4 
Hi Laurent,

On Tue, Mar 23, 2021 at 11:28 PM Laurent Pinchart
<laurent.pinchart@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@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

Patch
diff mbox series 

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_++;