From patchwork Sat Apr 3 13:57:34 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hirokazu Honda X-Patchwork-Id: 11832 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 357D1C0DA3 for ; Sat, 3 Apr 2021 13:57:45 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 9BF40602E6; Sat, 3 Apr 2021 15:57:44 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.b="DhktLhhk"; dkim-atps=neutral Received: from mail-pg1-x52b.google.com (mail-pg1-x52b.google.com [IPv6:2607:f8b0:4864:20::52b]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 694A4602CF for ; Sat, 3 Apr 2021 15:57:42 +0200 (CEST) Received: by mail-pg1-x52b.google.com with SMTP id b17so1595408pgh.7 for ; Sat, 03 Apr 2021 06:57:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=2/qjJA9Ix6MT2C8XKK6yNXlpYziaJtkJr+QhxOpygeY=; b=DhktLhhk2M4m+wWDPMNJCjdbVz0aTnxEQ3w8/8EOe/InhUkI3ujxNqaxSwugADV8IR nSFXPQKusRRsgUgwXCgYaKTGHQcHHzaZ7mD4Xj3ZrpGo6zQkmJHcnCRvHwSUpA+9B6kn wD2RszUydFzb772VP1NdEb4KyPMhN9FSwIpLY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=2/qjJA9Ix6MT2C8XKK6yNXlpYziaJtkJr+QhxOpygeY=; b=gjkxAPuxJTAnyzEVfWr1LEPw3/UravBsiywVX/7nXHFweZKGihFcZ62z0qfSenoMRz O7sKgapQmzPwDeN8MaJ2EEaGMmJjeESMhJxt2FK6hcpaaiZ14KGvXyfxfTWmPRB6/DdD Hze1F4FDlzUYHTO+C8MgR1wGSiQTtInvZ+UGZJD1QSjwQh2DuAry7hlmZl20n160aeRd NxxWMhJOxgT5SC5ulbutGhobWgWhefmRmcOG7GDPnchKfu/3uJ+VJyM0ixCkSAiNcLSY /EmhavjU1HhQaHTSA/G19V1oGv80yCgUPFkjrXkPcSjE1ik5EYd1UgUeAPSoQL3q3okJ b92A== X-Gm-Message-State: AOAM530qI4Kp00JCgTqhXIXGDuiKQuvjQk0P2Y4HxjMwDRzeqcjN6zAu 7eYKEk2I+K46PKoUQzqsRsUEaUSugYDBXw== X-Google-Smtp-Source: ABdhPJy3F79EB0zDIXfne2F0/TDw7YuwvHyGTECLPNSha0cj+pv9Vk6IffR4e/MNKV/CElQf2qtv4Q== X-Received: by 2002:a63:f914:: with SMTP id h20mr15930303pgi.114.1617458260658; Sat, 03 Apr 2021 06:57:40 -0700 (PDT) Received: from hiroh2.tok.corp.google.com ([2401:fa00:8f:2:d1f5:1b96:f027:f646]) by smtp.gmail.com with ESMTPSA id e1sm11345988pfi.175.2021.04.03.06.57.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 03 Apr 2021 06:57:40 -0700 (PDT) From: Hirokazu Honda To: libcamera-devel@lists.libcamera.org Date: Sat, 3 Apr 2021 22:57:34 +0900 Message-Id: <20210403135734.1600523-1-hiroh@chromium.org> X-Mailer: git-send-email 2.31.0.208.g409f899ff0-goog MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v6] android: CameraDevice: Fix Camera3RequestDescriptor leakage X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" 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 Reviewed-by: Laurent Pinchart Reviewed-by: Jacopo Mondi --- 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(camera, - reinterpret_cast(this)); + request_ = std::make_unique(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(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 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 resultMetadata; - Camera3RequestDescriptor *descriptor = - reinterpret_cast(request->cookie()); + + decltype(descriptors_)::node_type node; + { + std::scoped_lock 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(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 #include +#include #include #include @@ -69,11 +70,13 @@ private: CameraDevice(unsigned int id, std::shared_ptr 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 buffers_; std::vector> frameBuffers_; CameraMetadata settings_; @@ -124,6 +127,9 @@ private: std::map formatsMap_; std::vector streams_; + std::mutex mutex_; /* Protect descriptors_ */ + std::map 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(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 &fences() const { return acquireFences_; } libcamera::ControlList &controls() { return request_->controls(); }