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

[Umang Jain]

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


Reviewed-by: Umang Jain <umang.jain 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