[{"id":16085,"web_url":"https://patchwork.libcamera.org/comment/16085/","msgid":"","date":"2021-04-01T15:47:41","subject":"Re: [libcamera-devel] [PATCH v4 2/2] android: CameraDevice: Fix\n\tCamera3RequestDescriptor leakage","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"I would introduce std::mutex to protect descriptors_ in the next patch\nbecause requestComplete() and processCaptureRequest() run on different\nthreads.\n\nOn Tue, Mar 30, 2021 at 2:46 PM Hirokazu Honda wrote:\n>\n> CameraDevice creates Camera3RequestDescriptor in\n> processCaptureRequest() and disallocates in requestComplete().\n> Camera3RequestDescriptor can never be destroyed if\n> requestComplete() is never called. This avoid the memory\n> leakage by storing them in map CameraRequestDescriptor.\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera_device.cpp | 66 +++++++++++++++++------------------\n> src/android/camera_device.h | 8 +++--\n> src/android/camera_worker.cpp | 4 +--\n> src/android/camera_worker.h | 2 +-\n> 4 files changed, 42 insertions(+), 38 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 14299429..100aacbc 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -287,15 +287,11 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(\n>\n> /*\n> * Create the libcamera::Request unique_ptr<> to tie its lifetime\n> - * to the descriptor's one. Set the descriptor's address as the\n> - * request's cookie to retrieve it at completion time.\n> + * to the descriptor's one.\n> */\n> - request_ = std::make_unique(camera,\n> - reinterpret_cast(this));\n> + request_ = std::make_unique(camera);\n> }\n>\n> -CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> -\n> /*\n> * \\class CameraDevice\n> *\n> @@ -670,6 +666,7 @@ void CameraDevice::stop()\n> worker_.stop();\n> camera_->stop();\n>\n> + descriptors_.clear();\n> running_ = false;\n> }\n>\n> @@ -1814,8 +1811,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> * The descriptor and the associated memory reserved here are freed\n> * at request complete time.\n> */\n> - Camera3RequestDescriptor *descriptor =\n> - new Camera3RequestDescriptor(camera_.get(), camera3Request);\n> + Camera3RequestDescriptor descriptor(camera_.get(), camera3Request);\n> +\n> /*\n> * \\todo The Android request model is incremental, settings passed in\n> * previous requests are to be effective until overridden explicitly in\n> @@ -1825,12 +1822,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> if (camera3Request->settings)\n> lastSettings_ = camera3Request->settings;\n> else\n> - descriptor->settings_ = lastSettings_;\n> + descriptor.settings_ = lastSettings_;\n>\n> - LOG(HAL, Debug) << \"Queueing request \" << descriptor->request_->cookie()\n> - << \" with \" << descriptor->buffers_.size() << \" streams\";\n> - for (unsigned int i = 0; i < descriptor->buffers_.size(); ++i) {\n> - const camera3_stream_buffer_t &camera3Buffer = descriptor->buffers_[i];\n> + LOG(HAL, Debug) << \"Queueing request \" << descriptor.request_->cookie()\n> + << \" with \" << descriptor.buffers_.size() << \" streams\";\n> + for (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {\n> + const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\n> camera3_stream *camera3Stream = camera3Buffer.stream;\n> CameraStream *cameraStream = static_cast(camera3Stream->priv);\n>\n> @@ -1863,7 +1860,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> * lifetime management only.\n> */\n> buffer = createFrameBuffer(*camera3Buffer.buffer);\n> - descriptor->frameBuffers_.emplace_back(buffer);\n> + descriptor.frameBuffers_.emplace_back(buffer);\n> LOG(HAL, Debug) << ss.str() << \" (direct)\";\n> break;\n>\n> @@ -1882,11 +1879,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>\n> if (!buffer) {\n> LOG(HAL, Error) << \"Failed to create buffer\";\n> - delete descriptor;\n> return -ENOMEM;\n> }\n>\n> - descriptor->request_->addBuffer(cameraStream->stream(), buffer,\n> + descriptor.request_->addBuffer(cameraStream->stream(), buffer,\n> camera3Buffer.acquire_fence);\n> }\n>\n> @@ -1894,11 +1890,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> * Translate controls from Android to libcamera and queue the request\n> * to the CameraWorker thread.\n> */\n> - int ret = processControls(descriptor);\n> + int ret = processControls(&descriptor);\n> if (ret)\n> return ret;\n>\n> - worker_.queueRequest(descriptor->request_.get());\n> + worker_.queueRequest(descriptor.request_.get());\n> + descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n>\n> return 0;\n> }\n> @@ -1908,8 +1905,13 @@ void CameraDevice::requestComplete(Request *request)\n> const Request::BufferMap &buffers = request->buffers();\n> camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> std::unique_ptr resultMetadata;\n> - Camera3RequestDescriptor *descriptor =\n> - reinterpret_cast(request->cookie());\n> + auto node = descriptors_.extract(request->cookie());\n> + if (node.empty()) {\n> + LOG(HAL, Fatal) << \"Unknown request: \" << request->cookie();\n> + status = CAMERA3_BUFFER_STATUS_ERROR;\n> + return;\n> + }\n> + Camera3RequestDescriptor &descriptor = node.mapped();\n>\n> if (request->status() != Request::RequestComplete) {\n> LOG(HAL, Error) << \"Request not successfully completed: \"\n> @@ -1918,7 +1920,7 @@ void CameraDevice::requestComplete(Request *request)\n> }\n>\n> LOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed with \"\n> - << descriptor->buffers_.size() << \" streams\";\n> + << descriptor.buffers_.size() << \" streams\";\n>\n> /*\n> * \\todo The timestamp used for the metadata is currently always taken\n> @@ -1927,10 +1929,10 @@ void CameraDevice::requestComplete(Request *request)\n> * pipeline handlers) timestamp in the Request itself.\n> */\n> uint64_t timestamp = buffers.begin()->second->metadata().timestamp;\n> - resultMetadata = getResultMetadata(*descriptor, timestamp);\n> + resultMetadata = getResultMetadata(descriptor, timestamp);\n>\n> /* Handle any JPEG compression. */\n> - for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> CameraStream *cameraStream =\n> static_cast(buffer.stream->priv);\n>\n> @@ -1945,7 +1947,7 @@ void CameraDevice::requestComplete(Request *request)\n>\n> int ret = cameraStream->process(*src,\n> *buffer.buffer,\n> - descriptor->settings_,\n> + descriptor.settings_,\n> resultMetadata.get());\n> if (ret) {\n> status = CAMERA3_BUFFER_STATUS_ERROR;\n> @@ -1962,17 +1964,17 @@ void CameraDevice::requestComplete(Request *request)\n>\n> /* Prepare to call back the Android camera stack. */\n> camera3_capture_result_t captureResult = {};\n> - captureResult.frame_number = descriptor->frameNumber_;\n> - captureResult.num_output_buffers = descriptor->buffers_.size();\n> - for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> + captureResult.frame_number = descriptor.frameNumber_;\n> + captureResult.num_output_buffers = descriptor.buffers_.size();\n> + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> buffer.acquire_fence = -1;\n> buffer.release_fence = -1;\n> buffer.status = status;\n> }\n> - captureResult.output_buffers = descriptor->buffers_.data();\n> + captureResult.output_buffers = descriptor.buffers_.data();\n>\n> if (status == CAMERA3_BUFFER_STATUS_OK) {\n> - notifyShutter(descriptor->frameNumber_, timestamp);\n> + notifyShutter(descriptor.frameNumber_, timestamp);\n>\n> captureResult.partial_result = 1;\n> captureResult.result = resultMetadata->get();\n> @@ -1985,13 +1987,11 @@ void CameraDevice::requestComplete(Request *request)\n> * is here signalled. Make sure the error path plays well with\n> * the camera stack state machine.\n> */\n> - notifyError(descriptor->frameNumber_,\n> - descriptor->buffers_[0].stream);\n> + notifyError(descriptor.frameNumber_,\n> + descriptor.buffers_[0].stream);\n> }\n>\n> callbacks_->process_capture_result(callbacks_, &captureResult);\n> -\n> - delete descriptor;\n> }\n>\n> std::string CameraDevice::logPrefix() const\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 39cf95ad..6ca66c2d 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -69,11 +69,13 @@ private:\n> CameraDevice(unsigned int id, std::shared_ptr camera);\n>\n> struct Camera3RequestDescriptor {\n> + Camera3RequestDescriptor() = default;\n> + ~Camera3RequestDescriptor() = default;\n> Camera3RequestDescriptor(libcamera::Camera *camera,\n> const camera3_capture_request_t *camera3Request);\n> - ~Camera3RequestDescriptor();\n> + Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;\n>\n> - uint32_t frameNumber_;\n> + uint32_t frameNumber_ = 0;\n> std::vector buffers_;\n> std::vector> frameBuffers_;\n> CameraMetadata settings_;\n> @@ -124,6 +126,8 @@ private:\n> std::map formatsMap_;\n> std::vector streams_;\n>\n> + std::map descriptors_;\n> +\n> std::string maker_;\n> std::string model_;\n>\n> diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\n> index df436460..300ddde0 100644\n> --- a/src/android/camera_worker.cpp\n> +++ b/src/android/camera_worker.cpp\n> @@ -26,10 +26,10 @@ LOG_DECLARE_CATEGORY(HAL)\n> * by the CameraWorker which queues it to the libcamera::Camera after handling\n> * fences.\n> */\n> -CaptureRequest::CaptureRequest(libcamera::Camera *camera, uint64_t cookie)\n> +CaptureRequest::CaptureRequest(libcamera::Camera *camera)\n> : camera_(camera)\n> {\n> - request_ = camera_->createRequest(cookie);\n> + request_ = camera_->createRequest(reinterpret_cast(this));\n> }\n>\n> void CaptureRequest::addBuffer(Stream *stream, FrameBuffer *buffer, int fence)\n> diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h\n> index d7060576..64b1658b 100644\n> --- a/src/android/camera_worker.h\n> +++ b/src/android/camera_worker.h\n> @@ -22,7 +22,7 @@ class CameraDevice;\n> class CaptureRequest\n> {\n> public:\n> - CaptureRequest(libcamera::Camera *camera, uint64_t cookie);\n> + CaptureRequest(libcamera::Camera *camera);\n>\n> const std::vector &fences() const { return acquireFences_; }\n> libcamera::ControlList &controls() { return request_->controls(); }\n> --\n> 2.31.0.291.g576ba9dcdaf-goog","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8227CC0DA4\n\tfor ;\n\tThu, 1 Apr 2021 15:47:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E727968785;\n\tThu, 1 Apr 2021 17:47:54 +0200 (CEST)","from mail-yb1-xb31.google.com (mail-yb1-xb31.google.com\n\t[IPv6:2607:f8b0:4864:20::b31])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A0BAF68781\n\tfor ;\n\tThu, 1 Apr 2021 17:47:53 +0200 (CEST)","by mail-yb1-xb31.google.com with SMTP id g38so2219520ybi.12\n\tfor ;\n\tThu, 01 Apr 2021 08:47:53 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"l3ZcIwOe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to;\n\tbh=GE5gxJ8ZXrHU+XEfDzSlTYljwjswJrwWXb6YhGqLv0w=;\n\tb=l3ZcIwOex/xeZhcUdUxi0E6KIcCWeHxjPHJNEuvHLF1AriemtBLWQV6pjtqKhdk8W0\n\tcP6bPirn+YDewoK4d1UPiryD8IoyISAZxwKCA3kgVl8oh9sZcKQLIN/7feNqxRLzCamA\n\tLJGi7QIfEPM8wc9zg6yq+qTKHrdi+8NU1vrss=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to;\n\tbh=GE5gxJ8ZXrHU+XEfDzSlTYljwjswJrwWXb6YhGqLv0w=;\n\tb=ocZI+d3CYb+UeehX9n51eUPUMUyysq2vsAtBMX1NQufB6MaCbax56P/+dlMwQNy42+\n\tEmDQ1JliPga0iDrgSMuGCPY8oJs+pUQCC0dWX+jaSmcu7E87KE5jFRilgaPIqTzCTsaq\n\tJInBa52s0vU1Cl4xcqmeZSscz9ufc7sds66PMGEvLYFojOax9eA8r+N2zcTWmIIW/BwA\n\taZqIi9r8bbTg2q1ZyPeNENJZfnhJDXcAR9vcuFItkQAf1BH30bSybJEAYNWimoWcuz20\n\tClughP/mMjORw+U/+DIRnF5Xvag5SmvyJ/S5ffAChZVwRMgLvhX0exlBfS7KSejlK2c7\n\t8Q5Q==","X-Gm-Message-State":"AOAM530TAhtHkcszplkMRBiT4feRkItWEDgYhzp1qNwgxYtZm9LxqTsf\n\tnqYJAAuXkrLVv2j1Zxos/ukJ5vkIfUHaTldogx/7HTYUXq1T+w==","X-Google-Smtp-Source":"ABdhPJyeLxFJwzT7pfP1nqLf6FheUMA7y5CvhyIV91FQ/2Zor1v7fq1F8RQUbYzWU+RniuUiIIoqf5eLU/t/LnF+d+Y=","X-Received":"by 2002:a25:e803:: with SMTP id\n\tk3mr12295163ybd.268.1617292071920; \n\tThu, 01 Apr 2021 08:47:51 -0700 (PDT)","MIME-Version":"1.0","References":"<20210330054642.387562-1-hiroh@chromium.org>\n\t<20210330054642.387562-2-hiroh@chromium.org>","In-Reply-To":"<20210330054642.387562-2-hiroh@chromium.org>","From":"Hirokazu Honda ","Date":"Fri, 2 Apr 2021 00:47:41 +0900","Message-ID":"","To":"libcamera devel ","Subject":"Re: [libcamera-devel] [PATCH v4 2/2] android: CameraDevice: Fix\n\tCamera3RequestDescriptor leakage","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]