[{"id":16093,"web_url":"https://patchwork.libcamera.org/comment/16093/","msgid":"","date":"2021-04-02T23:13:38","subject":"Re: [libcamera-devel] [PATCH v5 2/2] android: CameraDevice: Fix\n\tCamera3RequestDescriptor leakage","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Fri, Apr 02, 2021 at 11:12:38AM +0900, Hirokazu Honda wrote:\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 | 83 +++++++++++++++++++++--------------\n> src/android/camera_device.h | 11 +++--\n> src/android/camera_worker.cpp | 4 +-\n> src/android/camera_worker.h | 2 +-\n> 4 files changed, 60 insertions(+), 40 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index b45d3a54..3d98e54c 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -9,9 +9,11 @@\n> #include \"camera_ops.h\"\n> #include \"post_processor.h\"\n> \n> +#include \n\nIs this needed ?\n\n> #include \n> #include \n> #include \n> +#include \n\nstd::scoped_lock is provided by , not . As you already\ninclude in camera_device.h, you can drop this.\n\n> #include \n> #include \n> \n> @@ -287,15 +289,11 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(\n> \n> \t/*\n> \t * Create the libcamera::Request unique_ptr<> to tie its lifetime\n> -\t * to the descriptor's one. Set the descriptor's address as the\n> -\t * request's cookie to retrieve it at completion time.\n> +\t * to the descriptor's one.\n\nThis comment seems a bit outdated, as we're creating a CaptureRequest.\nLet's fix it:\n\n\t/*\n\t * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n\t * lifetime to the descriptor.\n\t */\n\n> \t */\n> -\trequest_ = std::make_unique(camera,\n> -\t\t\t\t\t\t reinterpret_cast(this));\n> +\trequest_ = std::make_unique(camera);\n> }\n> \n> -CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> -\n> /*\n> * \\class CameraDevice\n> *\n> @@ -672,6 +670,7 @@ void CameraDevice::stop()\n> \tworker_.stop();\n> \tcamera_->stop();\n> \n> +\tdescriptors_.clear();\n> \trunning_ = false;\n> }\n> \n> @@ -1818,8 +1817,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t * The descriptor and the associated memory reserved here are freed\n> \t * at request complete time.\n> \t */\n> -\tCamera3RequestDescriptor *descriptor =\n> -\t\tnew Camera3RequestDescriptor(camera_.get(), camera3Request);\n> +\tCamera3RequestDescriptor descriptor(camera_.get(), camera3Request);\n> +\n> \t/*\n> \t * \\todo The Android request model is incremental, settings passed in\n> \t * previous requests are to be effective until overridden explicitly in\n> @@ -1829,12 +1828,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \tif (camera3Request->settings)\n> \t\tlastSettings_ = camera3Request->settings;\n> \telse\n> -\t\tdescriptor->settings_ = lastSettings_;\n> +\t\tdescriptor.settings_ = lastSettings_;\n> \n> -\tLOG(HAL, Debug) << \"Queueing request \" << descriptor->request_->cookie()\n> -\t\t\t<< \" with \" << descriptor->buffers_.size() << \" streams\";\n> -\tfor (unsigned int i = 0; i < descriptor->buffers_.size(); ++i) {\n> -\t\tconst camera3_stream_buffer_t &camera3Buffer = descriptor->buffers_[i];\n> +\tLOG(HAL, Debug) << \"Queueing request \" << descriptor.request_->cookie()\n> +\t\t\t<< \" with \" << descriptor.buffers_.size() << \" streams\";\n> +\tfor (unsigned int i = 0; i < descriptor.buffers_.size(); ++i) {\n> +\t\tconst camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\n> \t\tcamera3_stream *camera3Stream = camera3Buffer.stream;\n> \t\tCameraStream *cameraStream = static_cast(camera3Stream->priv);\n> \n> @@ -1867,7 +1866,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t\t * lifetime management only.\n> \t\t\t */\n> \t\t\tbuffer = createFrameBuffer(*camera3Buffer.buffer);\n> -\t\t\tdescriptor->frameBuffers_.emplace_back(buffer);\n> +\t\t\tdescriptor.frameBuffers_.emplace_back(buffer);\n> \t\t\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n> \t\t\tbreak;\n> \n> @@ -1886,11 +1885,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \n> \t\tif (!buffer) {\n> \t\t\tLOG(HAL, Error) << \"Failed to create buffer\";\n> -\t\t\tdelete descriptor;\n> \t\t\treturn -ENOMEM;\n> \t\t}\n> \n> -\t\tdescriptor->request_->addBuffer(cameraStream->stream(), buffer,\n> +\t\tdescriptor.request_->addBuffer(cameraStream->stream(), buffer,\n> \t\t\t\t\t\tcamera3Buffer.acquire_fence);\n> \t}\n> \n> @@ -1898,11 +1896,16 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t * Translate controls from Android to libcamera and queue the request\n> \t * to the CameraWorker thread.\n> \t */\n> -\tint ret = processControls(descriptor);\n> +\tint ret = processControls(&descriptor);\n> \tif (ret)\n> \t\treturn ret;\n> \n> -\tworker_.queueRequest(descriptor->request_.get());\n> +\tworker_.queueRequest(descriptor.request_.get());\n> +\n> +\t{\n> +\t\tstd::scoped_lock lock(mutex_);\n> +\t\tdescriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> +\t}\n> \n> \treturn 0;\n> }\n> @@ -1912,8 +1915,18 @@ void CameraDevice::requestComplete(Request *request)\n> \tconst Request::BufferMap &buffers = request->buffers();\n> \tcamera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> \tstd::unique_ptr resultMetadata;\n> -\tCamera3RequestDescriptor *descriptor =\n> -\t\treinterpret_cast(request->cookie());\n> +\n> +\tCamera3RequestDescriptor descriptor;\n> +\t{\n> +\t\tstd::scoped_lock lock(mutex_);\n> +\t\tauto it = descriptors_.find(request->cookie());\n> +\t\tif (it == descriptors_.end()) {\n> +\t\t\tLOG(HAL, Error) << \"Unknown request: \" << request->cookie();\n\nYou mentioned in the review of v3 that this will be turned into Fatal,\nis there a specific reason you have kept Error ?\n\n> +\t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t\t\treturn;\n> +\t\t}\n> +\t\tdescriptor = std::move(it->second);\n> +\t}\n\nAt the end of this function, you call descriptors_.erase() with the\nmutex lock. To minimize lock contention, you could turn the above code\ninto\n\n\tdecltype(descriptors_)::node_type node;\n\t{\n\t\tstd::scoped_lock lock(mutex_);\n\t\tauto it = descriptors_.find(request->cookie());\n\t\tif (it == descriptors_.end()) {\n\t\t\tLOG(HAL, Error) << \"Unknown request: \" << request->cookie();\n\t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n\t\t\treturn;\n\t\t}\n\n\t\tnode = desciptors_.extract(it);\n\t}\n\n\tCamera3RequestDescriptor &descriptor = node.mapped();\n\nand drop the erase() below.\n\n> \tif (request->status() != Request::RequestComplete) {\n> \t\tLOG(HAL, Error) << \"Request not successfully completed: \"\n> @@ -1922,7 +1935,7 @@ void CameraDevice::requestComplete(Request *request)\n> \t}\n> \n> \tLOG(HAL, Debug) << \"Request \" << request->cookie() << \" completed with \"\n> -\t\t\t<< descriptor->buffers_.size() << \" streams\";\n> +\t\t\t<< descriptor.buffers_.size() << \" streams\";\n> \n> \t/*\n> \t * \\todo The timestamp used for the metadata is currently always taken\n> @@ -1931,10 +1944,10 @@ void CameraDevice::requestComplete(Request *request)\n> \t * pipeline handlers) timestamp in the Request itself.\n> \t */\n> \tuint64_t timestamp = buffers.begin()->second->metadata().timestamp;\n> -\tresultMetadata = getResultMetadata(*descriptor, timestamp);\n> +\tresultMetadata = getResultMetadata(descriptor, timestamp);\n> \n> \t/* Handle any JPEG compression. */\n> -\tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> +\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> \t\tCameraStream *cameraStream =\n> \t\t\tstatic_cast(buffer.stream->priv);\n> \n> @@ -1949,7 +1962,7 @@ void CameraDevice::requestComplete(Request *request)\n> \n> \t\tint ret = cameraStream->process(*src,\n> \t\t\t\t\t\t*buffer.buffer,\n> -\t\t\t\t\t\tdescriptor->settings_,\n> +\t\t\t\t\t\tdescriptor.settings_,\n> \t\t\t\t\t\tresultMetadata.get());\n> \t\tif (ret) {\n> \t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> @@ -1966,17 +1979,17 @@ void CameraDevice::requestComplete(Request *request)\n> \n> \t/* Prepare to call back the Android camera stack. */\n> \tcamera3_capture_result_t captureResult = {};\n> -\tcaptureResult.frame_number = descriptor->frameNumber_;\n> -\tcaptureResult.num_output_buffers = descriptor->buffers_.size();\n> -\tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> +\tcaptureResult.frame_number = descriptor.frameNumber_;\n> +\tcaptureResult.num_output_buffers = descriptor.buffers_.size();\n> +\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> \t\tbuffer.acquire_fence = -1;\n> \t\tbuffer.release_fence = -1;\n> \t\tbuffer.status = status;\n> \t}\n> -\tcaptureResult.output_buffers = descriptor->buffers_.data();\n> +\tcaptureResult.output_buffers = descriptor.buffers_.data();\n> \n> \tif (status == CAMERA3_BUFFER_STATUS_OK) {\n> -\t\tnotifyShutter(descriptor->frameNumber_, timestamp);\n> +\t\tnotifyShutter(descriptor.frameNumber_, timestamp);\n> \n> \t\tcaptureResult.partial_result = 1;\n> \t\tcaptureResult.result = resultMetadata->get();\n> @@ -1989,13 +2002,15 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t * is here signalled. Make sure the error path plays well with\n> \t\t * the camera stack state machine.\n> \t\t */\n> -\t\tnotifyError(descriptor->frameNumber_,\n> -\t\t\t descriptor->buffers_[0].stream);\n> +\t\tnotifyError(descriptor.frameNumber_,\n> +\t\t\t descriptor.buffers_[0].stream);\n> \t}\n> \n> -\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> -\n> -\tdelete descriptor;\n> +\t{\n> +\t\tstd::scoped_lock lock(mutex_);\n> +\t\tdescriptors_.erase(request->cookie());\n> +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n\nI don't think you need to call process_capture_result() with the mutex_\nheld.\n\n> +\t}\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..e4ee0cb0 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -9,6 +9,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -61,7 +62,6 @@ public:\n> \tint configureStreams(camera3_stream_configuration_t *stream_list);\n> \tint processCaptureRequest(camera3_capture_request_t *request);\n> \tvoid requestComplete(libcamera::Request *request);\n> -\n\nLet's keep the blank line for readability.\n\n> protected:\n> \tstd::string logPrefix() const override;\n> \n> @@ -69,11 +69,13 @@ private:\n> \tCameraDevice(unsigned int id, std::shared_ptr camera);\n> \n> \tstruct Camera3RequestDescriptor {\n> +\t\tCamera3RequestDescriptor() = default;\n> +\t\t~Camera3RequestDescriptor() = default;\n> \t\tCamera3RequestDescriptor(libcamera::Camera *camera,\n> \t\t\t\t\t const camera3_capture_request_t *camera3Request);\n> -\t\t~Camera3RequestDescriptor();\n> +\t\tCamera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;\n> \n> -\t\tuint32_t frameNumber_;\n> +\t\tuint32_t frameNumber_ = 0;\n> \t\tstd::vector buffers_;\n> \t\tstd::vector> frameBuffers_;\n> \t\tCameraMetadata settings_;\n> @@ -124,6 +126,9 @@ private:\n> \tstd::map formatsMap_;\n> \tstd::vector streams_;\n> \n> +\tstd::mutex mutex_;\n\nLocking can be complicated, it's thus important to clearly document what\nlocks protect.\n\n\tstd::mutex mutex_;\t/* Protects descriptors_ */\n\nWith those small issues addressed,\n\nReviewed-by: Laurent Pinchart \n\n> +\tstd::map descriptors_;\n> +\n> \tstd::string maker_;\n> \tstd::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> \t: camera_(camera)\n> {\n> -\trequest_ = camera_->createRequest(cookie);\n> +\trequest_ = 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> -\tCaptureRequest(libcamera::Camera *camera, uint64_t cookie);\n> +\tCaptureRequest(libcamera::Camera *camera);\n> \n> \tconst std::vector &fences() const { return acquireFences_; }\n> \tlibcamera::ControlList &controls() { return request_->controls(); }","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 51583C0DA3\n\tfor ;\n\tFri, 2 Apr 2021 23:14:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BC583602D7;\n\tSat, 3 Apr 2021 01:14:25 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 24EC4602CF\n\tfor ;\n\tSat, 3 Apr 2021 01:14:24 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8F68C3D7;\n\tSat, 3 Apr 2021 01:14:23 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"taWs/a+d\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617405263;\n\tbh=hxwOyxDZqd3/5zER0ZiPsrHerPw6RFv3AF3gK36TuqI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=taWs/a+d/FZcVuz1NZbITgmL02Kk55zyxkDH08bx6v8Y7NOk9RUJY8SVBSc4tnOQV\n\thZw8n+NnYaKOLA+WsvRlSRS6ZHrdQA3bebNg+1iHS+8k+XsL7dM1rguBfUCnDj0s3e\n\tRljmJh6gQXZEicCWDJnBML8pAuOx/XHoa8OBfEEA=","Date":"Sat, 3 Apr 2021 02:13:38 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210402021238.1297591-1-hiroh@chromium.org>\n\t<20210402021238.1297591-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210402021238.1297591-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v5 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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]