[libcamera-devel,v5,2/2] android: CameraDevice: Fix Camera3RequestDescriptor leakage

11822 diff mbox series

CameraDevice creates Camera3RequestDescriptor in
processCaptureRequest() and disallocates in requestComplete().
Camera3RequestDescriptor can never be destroyed if
requestComplete() is never called. This avoid the memory
leakage by storing them in map CameraRequestDescriptor.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/camera_device.cpp | 83 +++++++++++++++++++++--------------
 src/android/camera_device.h   | 11 +++--
 src/android/camera_worker.cpp |  4 +-
 src/android/camera_worker.h   |  2 +-
 4 files changed, 60 insertions(+), 40 deletions(-)

Hi Hiro,

Thank you for the patch.

On Fri, Apr 02, 2021 at 11:12:38AM +0900, Hirokazu Honda wrote:
> CameraDevice creates Camera3RequestDescriptor in
> processCaptureRequest() and disallocates in requestComplete().
> Camera3RequestDescriptor can never be destroyed if
> requestComplete() is never called. This avoid the memory
> leakage by storing them in map CameraRequestDescriptor.
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/android/camera_device.cpp | 83 +++++++++++++++++++++--------------
>  src/android/camera_device.h   | 11 +++--
>  src/android/camera_worker.cpp |  4 +-
>  src/android/camera_worker.h   |  2 +-
>  4 files changed, 60 insertions(+), 40 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index b45d3a54..3d98e54c 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -9,9 +9,11 @@
>  #include "camera_ops.h"
>  #include "post_processor.h"
>  
> +#include <chrono>

Is this needed ?

>  #include <cmath>
>  #include <fstream>
>  #include <sys/mman.h>
> +#include <thread>

std::scoped_lock is provided by <mutex>, not <thread>. As you already
include <mutex> in camera_device.h, you can drop this.

>  #include <tuple>
>  #include <vector>
>  
> @@ -287,15 +289,11 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
>  
>  	/*
>  	 * Create the libcamera::Request unique_ptr<> to tie its lifetime
> -	 * to the descriptor's one. Set the descriptor's address as the
> -	 * request's cookie to retrieve it at completion time.
> +	 * to the descriptor's one.

This comment seems a bit outdated, as we're creating a CaptureRequest.
Let's fix it:

	/*
	 * Create the CaptureRequest, stored as a unique_ptr<> to tie its
	 * lifetime to the descriptor.
	 */

>  	 */
> -	request_ = std::make_unique<CaptureRequest>(camera,
> -						    reinterpret_cast<uint64_t>(this));
> +	request_ = std::make_unique<CaptureRequest>(camera);
>  }
>  
> -CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;
> -
>  /*
>   * \class CameraDevice
>   *
> @@ -672,6 +670,7 @@ void CameraDevice::stop()
>  	worker_.stop();
>  	camera_->stop();
>  
> +	descriptors_.clear();
>  	running_ = false;
>  }
>  
> @@ -1818,8 +1817,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	 * The descriptor and the associated memory reserved here are freed
>  	 * at request complete time.
>  	 */
> -	Camera3RequestDescriptor *descriptor =
> -		new Camera3RequestDescriptor(camera_.get(), camera3Request);
> +	Camera3RequestDescriptor descriptor(camera_.get(), camera3Request);
> +
>  	/*
>  	 * \todo The Android request model is incremental, settings passed in
>  	 * previous requests are to be effective until overridden explicitly in
> @@ -1829,12 +1828,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	if (camera3Request->settings)
>  		lastSettings_ = camera3Request->settings;
>  	else
> -		descriptor->settings_ = lastSettings_;
> +		descriptor.settings_ = lastSettings_;
>  
> -	LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie()
> -			<< " with " << descriptor->buffers_.size() << " streams";
> -	for (unsigned int i = 0; i < descriptor->buffers_.size(); ++i) {
> -		const camera3_stream_buffer_t &camera3Buffer = descriptor->buffers_[i];
> +	LOG(HAL, Debug) << "Queueing request " << descriptor.request_->cookie()
> +			<< " with " << descriptor.buffers_.size() << " streams";
> +	for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {
> +		const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];
>  		camera3_stream *camera3Stream = camera3Buffer.stream;
>  		CameraStream *cameraStream = static_cast<CameraStream *>(camera3Stream->priv);
>  
> @@ -1867,7 +1866,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  			 * lifetime management only.
>  			 */
>  			buffer = createFrameBuffer(*camera3Buffer.buffer);
> -			descriptor->frameBuffers_.emplace_back(buffer);
> +			descriptor.frameBuffers_.emplace_back(buffer);
>  			LOG(HAL, Debug) << ss.str() << " (direct)";
>  			break;
>  
> @@ -1886,11 +1885,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  
>  		if (!buffer) {
>  			LOG(HAL, Error) << "Failed to create buffer";
> -			delete descriptor;
>  			return -ENOMEM;
>  		}
>  
> -		descriptor->request_->addBuffer(cameraStream->stream(), buffer,
> +		descriptor.request_->addBuffer(cameraStream->stream(), buffer,
>  						camera3Buffer.acquire_fence);
>  	}
>  
> @@ -1898,11 +1896,16 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	 * Translate controls from Android to libcamera and queue the request
>  	 * to the CameraWorker thread.
>  	 */
> -	int ret = processControls(descriptor);
> +	int ret = processControls(&descriptor);
>  	if (ret)
>  		return ret;
>  
> -	worker_.queueRequest(descriptor->request_.get());
> +	worker_.queueRequest(descriptor.request_.get());
> +
> +	{
> +		std::scoped_lock<std::mutex> lock(mutex_);
> +		descriptors_[descriptor.request_->cookie()] = std::move(descriptor);
> +	}
>  
>  	return 0;
>  }
> @@ -1912,8 +1915,18 @@ void CameraDevice::requestComplete(Request *request)
>  	const Request::BufferMap &buffers = request->buffers();
>  	camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
>  	std::unique_ptr<CameraMetadata> resultMetadata;
> -	Camera3RequestDescriptor *descriptor =
> -		reinterpret_cast<Camera3RequestDescriptor *>(request->cookie());
> +
> +	Camera3RequestDescriptor descriptor;
> +	{
> +		std::scoped_lock<std::mutex> lock(mutex_);
> +		auto it = descriptors_.find(request->cookie());
> +		if (it == descriptors_.end()) {
> +			LOG(HAL, Error) << "Unknown request: " << request->cookie();

You mentioned in the review of v3 that this will be turned into Fatal,
is there a specific reason you have kept Error ?

> +			status = CAMERA3_BUFFER_STATUS_ERROR;
> +			return;
> +		}
> +		descriptor = std::move(it->second);
> +	}

At the end of this function, you call descriptors_.erase() with the
mutex lock. To minimize lock contention, you could turn the above code
into

	decltype(descriptors_)::node_type node;
	{
		std::scoped_lock<std::mutex> lock(mutex_);
		auto it = descriptors_.find(request->cookie());
		if (it == descriptors_.end()) {
			LOG(HAL, Error) << "Unknown request: " << request->cookie();
			status = CAMERA3_BUFFER_STATUS_ERROR;
			return;
		}

		node = desciptors_.extract(it);
	}

	Camera3RequestDescriptor &descriptor = node.mapped();

and drop the erase() below.

>  	if (request->status() != Request::RequestComplete) {
>  		LOG(HAL, Error) << "Request not successfully completed: "
> @@ -1922,7 +1935,7 @@ void CameraDevice::requestComplete(Request *request)
>  	}
>  
>  	LOG(HAL, Debug) << "Request " << request->cookie() << " completed with "
> -			<< descriptor->buffers_.size() << " streams";
> +			<< descriptor.buffers_.size() << " streams";
>  
>  	/*
>  	 * \todo The timestamp used for the metadata is currently always taken
> @@ -1931,10 +1944,10 @@ void CameraDevice::requestComplete(Request *request)
>  	 * pipeline handlers) timestamp in the Request itself.
>  	 */
>  	uint64_t timestamp = buffers.begin()->second->metadata().timestamp;
> -	resultMetadata = getResultMetadata(*descriptor, timestamp);
> +	resultMetadata = getResultMetadata(descriptor, timestamp);
>  
>  	/* Handle any JPEG compression. */
> -	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> +	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
>  		CameraStream *cameraStream =
>  			static_cast<CameraStream *>(buffer.stream->priv);
>  
> @@ -1949,7 +1962,7 @@ void CameraDevice::requestComplete(Request *request)
>  
>  		int ret = cameraStream->process(*src,
>  						*buffer.buffer,
> -						descriptor->settings_,
> +						descriptor.settings_,
>  						resultMetadata.get());
>  		if (ret) {
>  			status = CAMERA3_BUFFER_STATUS_ERROR;
> @@ -1966,17 +1979,17 @@ void CameraDevice::requestComplete(Request *request)
>  
>  	/* Prepare to call back the Android camera stack. */
>  	camera3_capture_result_t captureResult = {};
> -	captureResult.frame_number = descriptor->frameNumber_;
> -	captureResult.num_output_buffers = descriptor->buffers_.size();
> -	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> +	captureResult.frame_number = descriptor.frameNumber_;
> +	captureResult.num_output_buffers = descriptor.buffers_.size();
> +	for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
>  		buffer.acquire_fence = -1;
>  		buffer.release_fence = -1;
>  		buffer.status = status;
>  	}
> -	captureResult.output_buffers = descriptor->buffers_.data();
> +	captureResult.output_buffers = descriptor.buffers_.data();
>  
>  	if (status == CAMERA3_BUFFER_STATUS_OK) {
> -		notifyShutter(descriptor->frameNumber_, timestamp);
> +		notifyShutter(descriptor.frameNumber_, timestamp);
>  
>  		captureResult.partial_result = 1;
>  		captureResult.result = resultMetadata->get();
> @@ -1989,13 +2002,15 @@ void CameraDevice::requestComplete(Request *request)
>  		 * is here signalled. Make sure the error path plays well with
>  		 * the camera stack state machine.
>  		 */
> -		notifyError(descriptor->frameNumber_,
> -			    descriptor->buffers_[0].stream);
> +		notifyError(descriptor.frameNumber_,
> +			    descriptor.buffers_[0].stream);
>  	}
>  
> -	callbacks_->process_capture_result(callbacks_, &captureResult);
> -
> -	delete descriptor;
> +	{
> +		std::scoped_lock<std::mutex> lock(mutex_);
> +		descriptors_.erase(request->cookie());
> +		callbacks_->process_capture_result(callbacks_, &captureResult);

I don't think you need to call process_capture_result() with the mutex_
held.

> +	}
>  }
>  
>  std::string CameraDevice::logPrefix() const
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 39cf95ad..e4ee0cb0 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -9,6 +9,7 @@
>  
>  #include <map>
>  #include <memory>
> +#include <mutex>
>  #include <tuple>
>  #include <vector>
>  
> @@ -61,7 +62,6 @@ public:
>  	int configureStreams(camera3_stream_configuration_t *stream_list);
>  	int processCaptureRequest(camera3_capture_request_t *request);
>  	void requestComplete(libcamera::Request *request);
> -

Let's keep the blank line for readability.

>  protected:
>  	std::string logPrefix() const override;
>  
> @@ -69,11 +69,13 @@ private:
>  	CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);
>  
>  	struct Camera3RequestDescriptor {
> +		Camera3RequestDescriptor() = default;
> +		~Camera3RequestDescriptor() = default;
>  		Camera3RequestDescriptor(libcamera::Camera *camera,
>  					 const camera3_capture_request_t *camera3Request);
> -		~Camera3RequestDescriptor();
> +		Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;
>  
> -		uint32_t frameNumber_;
> +		uint32_t frameNumber_ = 0;
>  		std::vector<camera3_stream_buffer_t> buffers_;
>  		std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
>  		CameraMetadata settings_;
> @@ -124,6 +126,9 @@ private:
>  	std::map<int, libcamera::PixelFormat> formatsMap_;
>  	std::vector<CameraStream> streams_;
>  
> +	std::mutex mutex_;

Locking can be complicated, it's thus important to clearly document what
locks protect.

	std::mutex mutex_;	/* Protects descriptors_ */

With those small issues addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	std::map<uint64_t, Camera3RequestDescriptor> descriptors_;
> +
>  	std::string maker_;
>  	std::string model_;
>  
> diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp
> index df436460..300ddde0 100644
> --- a/src/android/camera_worker.cpp
> +++ b/src/android/camera_worker.cpp
> @@ -26,10 +26,10 @@ LOG_DECLARE_CATEGORY(HAL)
>   * by the CameraWorker which queues it to the libcamera::Camera after handling
>   * fences.
>   */
> -CaptureRequest::CaptureRequest(libcamera::Camera *camera, uint64_t cookie)
> +CaptureRequest::CaptureRequest(libcamera::Camera *camera)
>  	: camera_(camera)
>  {
> -	request_ = camera_->createRequest(cookie);
> +	request_ = camera_->createRequest(reinterpret_cast<uint64_t>(this));
>  }
>  
>  void CaptureRequest::addBuffer(Stream *stream, FrameBuffer *buffer, int fence)
> diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h
> index d7060576..64b1658b 100644
> --- a/src/android/camera_worker.h
> +++ b/src/android/camera_worker.h
> @@ -22,7 +22,7 @@ class CameraDevice;
>  class CaptureRequest
>  {
>  public:
> -	CaptureRequest(libcamera::Camera *camera, uint64_t cookie);
> +	CaptureRequest(libcamera::Camera *camera);
>  
>  	const std::vector<int> &fences() const { return acquireFences_; }
>  	libcamera::ControlList &controls() { return request_->controls(); }