[libcamera-devel,v6] android: CameraDevice: Fix Camera3RequestDescriptor leakage
diff mbox series

Message ID 20210403135734.1600523-1-hiroh@chromium.org
State Accepted
Headers show
Series
  • [libcamera-devel,v6] android: CameraDevice: Fix Camera3RequestDescriptor leakage
Related show

Commit Message

Hirokazu Honda April 3, 2021, 1:57 p.m. UTC 
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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/android/camera_device.cpp | 80 ++++++++++++++++++++---------------
 src/android/camera_device.h   | 10 ++++-
 src/android/camera_worker.cpp |  4 +-
 src/android/camera_worker.h   |  2 +-
 4 files changed, 57 insertions(+), 39 deletions(-)

Comments

Laurent Pinchart April 4, 2021, 1:30 a.m. UTC | #1 
Hi Hiro,

Just a small comment.

On Sat, Apr 03, 2021 at 10:57:34PM +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>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 80 ++++++++++++++++++++---------------
>  src/android/camera_device.h   | 10 ++++-
>  src/android/camera_worker.cpp |  4 +-
>  src/android/camera_worker.h   |  2 +-
>  4 files changed, 57 insertions(+), 39 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index b45d3a54..9d6be1f9 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -286,16 +286,12 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
>  	settings_ = CameraMetadata(camera3Request->settings);
>  
>  	/*
> -	 * 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.
> +	 * Create the libcamera::CaptureRequest unique_ptr<> to tie its lifetime

This should be just CaptureRequest, not libcamera::CaptureRequest. I'll
fix when applying.

> +	 * to the descriptor's one.
>  	 */
> -	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 +668,7 @@ void CameraDevice::stop()
>  	worker_.stop();
>  	camera_->stop();
>  
> +	descriptors_.clear();
>  	running_ = false;
>  }
>  
> @@ -1818,8 +1815,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 +1826,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 +1864,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 +1883,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 +1894,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 +1913,21 @@ 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());
> +
> +	decltype(descriptors_)::node_type node;
> +	{
> +		std::scoped_lock<std::mutex> lock(mutex_);
> +		auto it = descriptors_.find(request->cookie());
> +		if (it == descriptors_.end()) {
> +			LOG(HAL, Fatal)
> +				<< "Unknown request: " << request->cookie();
> +			status = CAMERA3_BUFFER_STATUS_ERROR;
> +			return;
> +		}
> +
> +		node = descriptors_.extract(it);
> +	}
> +	Camera3RequestDescriptor &descriptor = node.mapped();
>  
>  	if (request->status() != Request::RequestComplete) {
>  		LOG(HAL, Error) << "Request not successfully completed: "
> @@ -1922,7 +1936,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 +1945,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 +1963,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 +1980,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 +2003,11 @@ 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::string CameraDevice::logPrefix() const
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 39cf95ad..c63e8e21 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>
>  
> @@ -69,11 +70,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 +127,9 @@ private:
>  	std::map<int, libcamera::PixelFormat> formatsMap_;
>  	std::vector<CameraStream> streams_;
>  
> +	std::mutex mutex_; /* Protect descriptors_ */
> +	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(); }
Jacopo Mondi April 6, 2021, 12:31 p.m. UTC | #2 
Hi Hiro,

On Sat, Apr 03, 2021 at 10:57:34PM +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>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 80 ++++++++++++++++++++---------------
>  src/android/camera_device.h   | 10 ++++-
>  src/android/camera_worker.cpp |  4 +-
>  src/android/camera_worker.h   |  2 +-
>  4 files changed, 57 insertions(+), 39 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index b45d3a54..9d6be1f9 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -286,16 +286,12 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
>  	settings_ = CameraMetadata(camera3Request->settings);
>
>  	/*
> -	 * 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.
> +	 * Create the libcamera::CaptureRequest unique_ptr<> to tie its lifetime
> +	 * to the descriptor's one.
>  	 */
> -	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 +668,7 @@ void CameraDevice::stop()

I have a bit lost track of where stop() is called, as this patch is
based on a pile of patches in review. My only concern is that
descriptor_ shall be cleared also at configureStreams() time.

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

Thanks
  j

>  	worker_.stop();
>  	camera_->stop();
>
> +	descriptors_.clear();
>  	running_ = false;
>  }
>
> @@ -1818,8 +1815,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 +1826,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 +1864,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 +1883,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 +1894,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 +1913,21 @@ 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());
> +
> +	decltype(descriptors_)::node_type node;
> +	{
> +		std::scoped_lock<std::mutex> lock(mutex_);
> +		auto it = descriptors_.find(request->cookie());
> +		if (it == descriptors_.end()) {
> +			LOG(HAL, Fatal)
> +				<< "Unknown request: " << request->cookie();
> +			status = CAMERA3_BUFFER_STATUS_ERROR;
> +			return;
> +		}
> +
> +		node = descriptors_.extract(it);
> +	}
> +	Camera3RequestDescriptor &descriptor = node.mapped();
>
>  	if (request->status() != Request::RequestComplete) {
>  		LOG(HAL, Error) << "Request not successfully completed: "
> @@ -1922,7 +1936,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 +1945,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 +1963,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 +1980,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 +2003,11 @@ 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::string CameraDevice::logPrefix() const
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 39cf95ad..c63e8e21 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>
>
> @@ -69,11 +70,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 +127,9 @@ private:
>  	std::map<int, libcamera::PixelFormat> formatsMap_;
>  	std::vector<CameraStream> streams_;
>
> +	std::mutex mutex_; /* Protect descriptors_ */
> +	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(); }
> --
> 2.31.0.208.g409f899ff0-goog
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Hirokazu Honda April 7, 2021, 2:48 a.m. UTC | #3 
Laurent, can you push this and stop() patch to tree?

On Tue, Apr 6, 2021 at 9:30 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Hiro,
>
> On Sat, Apr 03, 2021 at 10:57:34PM +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>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/android/camera_device.cpp | 80 ++++++++++++++++++++---------------
> >  src/android/camera_device.h   | 10 ++++-
> >  src/android/camera_worker.cpp |  4 +-
> >  src/android/camera_worker.h   |  2 +-
> >  4 files changed, 57 insertions(+), 39 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index b45d3a54..9d6be1f9 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -286,16 +286,12 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
> >       settings_ = CameraMetadata(camera3Request->settings);
> >
> >       /*
> > -      * 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.
> > +      * Create the libcamera::CaptureRequest unique_ptr<> to tie its lifetime
> > +      * to the descriptor's one.
> >        */
> > -     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 +668,7 @@ void CameraDevice::stop()
>
> I have a bit lost track of where stop() is called, as this patch is
> based on a pile of patches in review. My only concern is that
> descriptor_ shall be cleared also at configureStreams() time.
>
> With that confirmed
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
>   j
>
> >       worker_.stop();
> >       camera_->stop();
> >
> > +     descriptors_.clear();
> >       running_ = false;
> >  }
> >
> > @@ -1818,8 +1815,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 +1826,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 +1864,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 +1883,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 +1894,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 +1913,21 @@ 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());
> > +
> > +     decltype(descriptors_)::node_type node;
> > +     {
> > +             std::scoped_lock<std::mutex> lock(mutex_);
> > +             auto it = descriptors_.find(request->cookie());
> > +             if (it == descriptors_.end()) {
> > +                     LOG(HAL, Fatal)
> > +                             << "Unknown request: " << request->cookie();
> > +                     status = CAMERA3_BUFFER_STATUS_ERROR;
> > +                     return;
> > +             }
> > +
> > +             node = descriptors_.extract(it);
> > +     }
> > +     Camera3RequestDescriptor &descriptor = node.mapped();
> >
> >       if (request->status() != Request::RequestComplete) {
> >               LOG(HAL, Error) << "Request not successfully completed: "
> > @@ -1922,7 +1936,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 +1945,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 +1963,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 +1980,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 +2003,11 @@ 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::string CameraDevice::logPrefix() const
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 39cf95ad..c63e8e21 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>
> >
> > @@ -69,11 +70,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 +127,9 @@ private:
> >       std::map<int, libcamera::PixelFormat> formatsMap_;
> >       std::vector<CameraStream> streams_;
> >
> > +     std::mutex mutex_; /* Protect descriptors_ */
> > +     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(); }
> > --
> > 2.31.0.208.g409f899ff0-goog
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart April 20, 2021, 3:11 a.m. UTC | #4 
Hi Hiro,

On Wed, Apr 07, 2021 at 11:48:05AM +0900, Hirokazu Honda wrote:
> Laurent, can you push this and stop() patch to tree?

I'm sorry, those two patches have fallen through the crack (they were
still visible in patchwork though, so I suppose they haven't fallen
completely and would have resurfaced, I'm happy Kieran kept pushing to
have a patchwork instance and got it up and running).

Anyway, patches pushed. Thanks for reminding me.

> On Tue, Apr 6, 2021 at 9:30 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> > On Sat, Apr 03, 2021 at 10:57:34PM +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>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  src/android/camera_device.cpp | 80 ++++++++++++++++++++---------------
> > >  src/android/camera_device.h   | 10 ++++-
> > >  src/android/camera_worker.cpp |  4 +-
> > >  src/android/camera_worker.h   |  2 +-
> > >  4 files changed, 57 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index b45d3a54..9d6be1f9 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -286,16 +286,12 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
> > >       settings_ = CameraMetadata(camera3Request->settings);
> > >
> > >       /*
> > > -      * 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.
> > > +      * Create the libcamera::CaptureRequest unique_ptr<> to tie its lifetime
> > > +      * to the descriptor's one.
> > >        */
> > > -     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 +668,7 @@ void CameraDevice::stop()
> >
> > I have a bit lost track of where stop() is called, as this patch is
> > based on a pile of patches in review. My only concern is that
> > descriptor_ shall be cleared also at configureStreams() time.
> >
> > With that confirmed
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > >       worker_.stop();
> > >       camera_->stop();
> > >
> > > +     descriptors_.clear();
> > >       running_ = false;
> > >  }
> > >
> > > @@ -1818,8 +1815,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 +1826,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 +1864,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 +1883,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 +1894,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 +1913,21 @@ 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());
> > > +
> > > +     decltype(descriptors_)::node_type node;
> > > +     {
> > > +             std::scoped_lock<std::mutex> lock(mutex_);
> > > +             auto it = descriptors_.find(request->cookie());
> > > +             if (it == descriptors_.end()) {
> > > +                     LOG(HAL, Fatal)
> > > +                             << "Unknown request: " << request->cookie();
> > > +                     status = CAMERA3_BUFFER_STATUS_ERROR;
> > > +                     return;
> > > +             }
> > > +
> > > +             node = descriptors_.extract(it);
> > > +     }
> > > +     Camera3RequestDescriptor &descriptor = node.mapped();
> > >
> > >       if (request->status() != Request::RequestComplete) {
> > >               LOG(HAL, Error) << "Request not successfully completed: "
> > > @@ -1922,7 +1936,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 +1945,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 +1963,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 +1980,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 +2003,11 @@ 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::string CameraDevice::logPrefix() const
> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > index 39cf95ad..c63e8e21 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>
> > >
> > > @@ -69,11 +70,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 +127,9 @@ private:
> > >       std::map<int, libcamera::PixelFormat> formatsMap_;
> > >       std::vector<CameraStream> streams_;
> > >
> > > +     std::mutex mutex_; /* Protect descriptors_ */
> > > +     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(); }

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index b45d3a54..9d6be1f9 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -286,16 +286,12 @@  CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
 	settings_ = CameraMetadata(camera3Request->settings);
 
 	/*
-	 * 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.
+	 * Create the libcamera::CaptureRequest unique_ptr<> to tie its lifetime
+	 * to the descriptor's one.
 	 */
-	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 +668,7 @@  void CameraDevice::stop()
 	worker_.stop();
 	camera_->stop();
 
+	descriptors_.clear();
 	running_ = false;
 }
 
@@ -1818,8 +1815,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 +1826,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 +1864,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 +1883,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 +1894,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 +1913,21 @@  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());
+
+	decltype(descriptors_)::node_type node;
+	{
+		std::scoped_lock<std::mutex> lock(mutex_);
+		auto it = descriptors_.find(request->cookie());
+		if (it == descriptors_.end()) {
+			LOG(HAL, Fatal)
+				<< "Unknown request: " << request->cookie();
+			status = CAMERA3_BUFFER_STATUS_ERROR;
+			return;
+		}
+
+		node = descriptors_.extract(it);
+	}
+	Camera3RequestDescriptor &descriptor = node.mapped();
 
 	if (request->status() != Request::RequestComplete) {
 		LOG(HAL, Error) << "Request not successfully completed: "
@@ -1922,7 +1936,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 +1945,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 +1963,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 +1980,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 +2003,11 @@  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::string CameraDevice::logPrefix() const
diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index 39cf95ad..c63e8e21 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>
 
@@ -69,11 +70,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 +127,9 @@  private:
 	std::map<int, libcamera::PixelFormat> formatsMap_;
 	std::vector<CameraStream> streams_;
 
+	std::mutex mutex_; /* Protect descriptors_ */
+	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(); }