[libcamera-devel] [PATCH] android: camera_device: Return unique_ptr from createFrameBuffer

[paul.elder at ideasonboard.com]

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 at ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder at 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
>