[libcamera-devel] android: camera_device: Return unique_ptr from createFrameBuffer
diff mbox series

Message ID 20210923115439.30863-1-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • [libcamera-devel] android: camera_device: Return unique_ptr from createFrameBuffer
Related show

Commit Message

Laurent Pinchart Sept. 23, 2021, 11:54 a.m. UTC 
Returning a non-managed pointer can cause leaks. Use a unique_ptr<>
instead to avoid possible future issues.

The std::move() for the planes argument to the FrameBuffer constructor
is dropped as it's misleading. FrameBuffer has no constructor that takes
an rvalue reference to planes, so the vector was copied despite the
move. This only clarifies the intent, no functional change is
introduced.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/android/camera_device.cpp | 18 ++++++++++--------
 src/android/camera_device.h   |  7 ++++---
 2 files changed, 14 insertions(+), 11 deletions(-)


base-commit: 2cc4303b172a76ac5b431c4fb4df8a083f7d3fcf

Comments

Jacopo Mondi Sept. 23, 2021, 12:08 p.m. UTC | #1 
Hi Laurent

On Thu, Sep 23, 2021 at 02:54:39PM +0300, Laurent Pinchart wrote:
> Returning a non-managed pointer can cause leaks. Use a unique_ptr<>
> instead to avoid possible future issues.
>
> The std::move() for the planes argument to the FrameBuffer constructor
> is dropped as it's misleading. FrameBuffer has no constructor that takes
> an rvalue reference to planes, so the vector was copied despite the
> move. This only clarifies the intent, no functional change is
> introduced.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Looks good!

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

> ---
>  src/android/camera_device.cpp | 18 ++++++++++--------
>  src/android/camera_device.h   |  7 ++++---
>  2 files changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index a693dcbe89f4..21844e5114a9 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -774,9 +774,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  	return 0;
>  }
>
> -FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer,
> -					     PixelFormat pixelFormat,
> -					     const Size &size)
> +std::unique_ptr<FrameBuffer>
> +CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer,
> +				PixelFormat pixelFormat, const Size &size)
>  {
>  	CameraBuffer buf(camera3buffer, pixelFormat, size, PROT_READ);
>  	if (!buf.isValid()) {
> @@ -797,7 +797,7 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
>  		planes[i].length = buf.size(i);
>  	}
>
> -	return new FrameBuffer(std::move(planes));
> +	return std::make_unique<FrameBuffer>(planes);
>  }
>
>  int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
> @@ -1002,10 +1002,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  			 * associate it with the Camera3RequestDescriptor for
>  			 * lifetime management only.
>  			 */
> -			buffer = createFrameBuffer(*camera3Buffer.buffer,
> -						   cameraStream->configuration().pixelFormat,
o
> -						   cameraStream->configuration().size);
> -			descriptor.frameBuffers_.emplace_back(buffer);
> +			descriptor.frameBuffers_.push_back(
> +				createFrameBuffer(*camera3Buffer.buffer,
> +						  cameraStream->configuration().pixelFormat,
> +						  cameraStream->configuration().size));
> +
> +			buffer = descriptor.frameBuffers_.back().get();
>  			LOG(HAL, Debug) << ss.str() << " (direct)";
>  			break;
>
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 296c2f185e4e..43eb5895ba64 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -94,9 +94,10 @@ private:
>
>  	void stop();
>
> -	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer,
> -						  libcamera::PixelFormat pixelFormat,
> -						  const libcamera::Size &size);
> +	std::unique_ptr<libcamera::FrameBuffer>
> +	createFrameBuffer(const buffer_handle_t camera3buffer,
> +			  libcamera::PixelFormat pixelFormat,
> +			  const libcamera::Size &size);
>  	void abortRequest(camera3_capture_request_t *request);
>  	bool isValidRequest(camera3_capture_request_t *request) const;
>  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
>
> base-commit: 2cc4303b172a76ac5b431c4fb4df8a083f7d3fcf
> --
> Regards,
>
> Laurent Pinchart
>
Hirokazu Honda Sept. 24, 2021, 3:06 a.m. UTC | #2 
Hi Laurent, thank you for the patch.

On Thu, Sep 23, 2021 at 9:08 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Laurent
>
> On Thu, Sep 23, 2021 at 02:54:39PM +0300, Laurent Pinchart wrote:
> > Returning a non-managed pointer can cause leaks. Use a unique_ptr<>
> > instead to avoid possible future issues.
> >
> > The std::move() for the planes argument to the FrameBuffer constructor
> > is dropped as it's misleading. FrameBuffer has no constructor that takes
> > an rvalue reference to planes, so the vector was copied despite the
> > move. This only clarifies the intent, no functional change is
> > introduced.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>

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

> Looks good!
>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
>    j
>
> > ---
> >  src/android/camera_device.cpp | 18 ++++++++++--------
> >  src/android/camera_device.h   |  7 ++++---
> >  2 files changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index a693dcbe89f4..21844e5114a9 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -774,9 +774,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >       return 0;
> >  }
> >
> > -FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer,
> > -                                          PixelFormat pixelFormat,
> > -                                          const Size &size)
> > +std::unique_ptr<FrameBuffer>
> > +CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer,
> > +                             PixelFormat pixelFormat, const Size &size)
> >  {
> >       CameraBuffer buf(camera3buffer, pixelFormat, size, PROT_READ);
> >       if (!buf.isValid()) {
> > @@ -797,7 +797,7 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
> >               planes[i].length = buf.size(i);
> >       }
> >
> > -     return new FrameBuffer(std::move(planes));
> > +     return std::make_unique<FrameBuffer>(planes);
> >  }
> >
> >  int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
> > @@ -1002,10 +1002,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >                        * associate it with the Camera3RequestDescriptor for
> >                        * lifetime management only.
> >                        */
> > -                     buffer = createFrameBuffer(*camera3Buffer.buffer,
> > -                                                cameraStream->configuration().pixelFormat,
> o
> > -                                                cameraStream->configuration().size);
> > -                     descriptor.frameBuffers_.emplace_back(buffer);
> > +                     descriptor.frameBuffers_.push_back(
> > +                             createFrameBuffer(*camera3Buffer.buffer,
> > +                                               cameraStream->configuration().pixelFormat,
> > +                                               cameraStream->configuration().size));
> > +
> > +                     buffer = descriptor.frameBuffers_.back().get();
> >                       LOG(HAL, Debug) << ss.str() << " (direct)";
> >                       break;
> >
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 296c2f185e4e..43eb5895ba64 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -94,9 +94,10 @@ private:
> >
> >       void stop();
> >
> > -     libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer,
> > -                                               libcamera::PixelFormat pixelFormat,
> > -                                               const libcamera::Size &size);
> > +     std::unique_ptr<libcamera::FrameBuffer>
> > +     createFrameBuffer(const buffer_handle_t camera3buffer,
> > +                       libcamera::PixelFormat pixelFormat,
> > +                       const libcamera::Size &size);
> >       void abortRequest(camera3_capture_request_t *request);
> >       bool isValidRequest(camera3_capture_request_t *request) const;
> >       void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> >
> > base-commit: 2cc4303b172a76ac5b431c4fb4df8a083f7d3fcf
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
Umang Jain Sept. 24, 2021, 4:20 a.m. UTC | #3 
Hi Laurent,

On 9/23/21 5:24 PM, Laurent Pinchart wrote:
> Returning a non-managed pointer can cause leaks. Use a unique_ptr<>
> instead to avoid possible future issues.
>
> The std::move() for the planes argument to the FrameBuffer constructor
> is dropped as it's misleading. FrameBuffer has no constructor that takes
> an rvalue reference to planes, so the vector was copied despite the
> move. This only clarifies the intent, no functional change is
> introduced.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>


Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> ---
>   src/android/camera_device.cpp | 18 ++++++++++--------
>   src/android/camera_device.h   |  7 ++++---
>   2 files changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index a693dcbe89f4..21844e5114a9 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -774,9 +774,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>   	return 0;
>   }
>   
> -FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer,
> -					     PixelFormat pixelFormat,
> -					     const Size &size)
> +std::unique_ptr<FrameBuffer>
> +CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer,
> +				PixelFormat pixelFormat, const Size &size)
>   {
>   	CameraBuffer buf(camera3buffer, pixelFormat, size, PROT_READ);
>   	if (!buf.isValid()) {
> @@ -797,7 +797,7 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
>   		planes[i].length = buf.size(i);
>   	}
>   
> -	return new FrameBuffer(std::move(planes));
> +	return std::make_unique<FrameBuffer>(planes);
>   }
>   
>   int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
> @@ -1002,10 +1002,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>   			 * associate it with the Camera3RequestDescriptor for
>   			 * lifetime management only.
>   			 */
> -			buffer = createFrameBuffer(*camera3Buffer.buffer,
> -						   cameraStream->configuration().pixelFormat,
> -						   cameraStream->configuration().size);
> -			descriptor.frameBuffers_.emplace_back(buffer);
> +			descriptor.frameBuffers_.push_back(
> +				createFrameBuffer(*camera3Buffer.buffer,
> +						  cameraStream->configuration().pixelFormat,
> +						  cameraStream->configuration().size));
> +
> +			buffer = descriptor.frameBuffers_.back().get();
>   			LOG(HAL, Debug) << ss.str() << " (direct)";
>   			break;
>   
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 296c2f185e4e..43eb5895ba64 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -94,9 +94,10 @@ private:
>   
>   	void stop();
>   
> -	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer,
> -						  libcamera::PixelFormat pixelFormat,
> -						  const libcamera::Size &size);
> +	std::unique_ptr<libcamera::FrameBuffer>
> +	createFrameBuffer(const buffer_handle_t camera3buffer,
> +			  libcamera::PixelFormat pixelFormat,
> +			  const libcamera::Size &size);
>   	void abortRequest(camera3_capture_request_t *request);
>   	bool isValidRequest(camera3_capture_request_t *request) const;
>   	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
>
> base-commit: 2cc4303b172a76ac5b431c4fb4df8a083f7d3fcf
Paul Elder Sept. 24, 2021, 5:37 a.m. UTC | #4 
Hi Laurent,

On Thu, Sep 23, 2021 at 02:54:39PM +0300, Laurent Pinchart wrote:
> Returning a non-managed pointer can cause leaks. Use a unique_ptr<>
> instead to avoid possible future issues.
> 
> The std::move() for the planes argument to the FrameBuffer constructor
> is dropped as it's misleading. FrameBuffer has no constructor that takes
> an rvalue reference to planes, so the vector was copied despite the
> move. This only clarifies the intent, no functional change is
> introduced.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/android/camera_device.cpp | 18 ++++++++++--------
>  src/android/camera_device.h   |  7 ++++---
>  2 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index a693dcbe89f4..21844e5114a9 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -774,9 +774,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  	return 0;
>  }
>  
> -FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer,
> -					     PixelFormat pixelFormat,
> -					     const Size &size)
> +std::unique_ptr<FrameBuffer>
> +CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer,
> +				PixelFormat pixelFormat, const Size &size)
>  {
>  	CameraBuffer buf(camera3buffer, pixelFormat, size, PROT_READ);
>  	if (!buf.isValid()) {
> @@ -797,7 +797,7 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
>  		planes[i].length = buf.size(i);
>  	}
>  
> -	return new FrameBuffer(std::move(planes));
> +	return std::make_unique<FrameBuffer>(planes);
>  }
>  
>  int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
> @@ -1002,10 +1002,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  			 * associate it with the Camera3RequestDescriptor for
>  			 * lifetime management only.
>  			 */
> -			buffer = createFrameBuffer(*camera3Buffer.buffer,
> -						   cameraStream->configuration().pixelFormat,
> -						   cameraStream->configuration().size);
> -			descriptor.frameBuffers_.emplace_back(buffer);
> +			descriptor.frameBuffers_.push_back(
> +				createFrameBuffer(*camera3Buffer.buffer,
> +						  cameraStream->configuration().pixelFormat,
> +						  cameraStream->configuration().size));
> +
> +			buffer = descriptor.frameBuffers_.back().get();
>  			LOG(HAL, Debug) << ss.str() << " (direct)";
>  			break;
>  
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 296c2f185e4e..43eb5895ba64 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -94,9 +94,10 @@ private:
>  
>  	void stop();
>  
> -	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer,
> -						  libcamera::PixelFormat pixelFormat,
> -						  const libcamera::Size &size);
> +	std::unique_ptr<libcamera::FrameBuffer>
> +	createFrameBuffer(const buffer_handle_t camera3buffer,
> +			  libcamera::PixelFormat pixelFormat,
> +			  const libcamera::Size &size);
>  	void abortRequest(camera3_capture_request_t *request);
>  	bool isValidRequest(camera3_capture_request_t *request) const;
>  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> 
> base-commit: 2cc4303b172a76ac5b431c4fb4df8a083f7d3fcf
> -- 
> Regards,
> 
> Laurent Pinchart
>

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index a693dcbe89f4..21844e5114a9 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -774,9 +774,9 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 	return 0;
 }
 
-FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer,
-					     PixelFormat pixelFormat,
-					     const Size &size)
+std::unique_ptr<FrameBuffer>
+CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer,
+				PixelFormat pixelFormat, const Size &size)
 {
 	CameraBuffer buf(camera3buffer, pixelFormat, size, PROT_READ);
 	if (!buf.isValid()) {
@@ -797,7 +797,7 @@  FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
 		planes[i].length = buf.size(i);
 	}
 
-	return new FrameBuffer(std::move(planes));
+	return std::make_unique<FrameBuffer>(planes);
 }
 
 int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
@@ -1002,10 +1002,12 @@  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 			 * associate it with the Camera3RequestDescriptor for
 			 * lifetime management only.
 			 */
-			buffer = createFrameBuffer(*camera3Buffer.buffer,
-						   cameraStream->configuration().pixelFormat,
-						   cameraStream->configuration().size);
-			descriptor.frameBuffers_.emplace_back(buffer);
+			descriptor.frameBuffers_.push_back(
+				createFrameBuffer(*camera3Buffer.buffer,
+						  cameraStream->configuration().pixelFormat,
+						  cameraStream->configuration().size));
+
+			buffer = descriptor.frameBuffers_.back().get();
 			LOG(HAL, Debug) << ss.str() << " (direct)";
 			break;
 
diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index 296c2f185e4e..43eb5895ba64 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -94,9 +94,10 @@  private:
 
 	void stop();
 
-	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer,
-						  libcamera::PixelFormat pixelFormat,
-						  const libcamera::Size &size);
+	std::unique_ptr<libcamera::FrameBuffer>
+	createFrameBuffer(const buffer_handle_t camera3buffer,
+			  libcamera::PixelFormat pixelFormat,
+			  const libcamera::Size &size);
 	void abortRequest(camera3_capture_request_t *request);
 	bool isValidRequest(camera3_capture_request_t *request) const;
 	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);