[libcamera-devel] [PATCH v5 2/2] android: CameraDevice: Fix Camera3RequestDescriptor leakage
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Apr 3 01:13:38 CEST 2021
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 at 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 at 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(); }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list