[{"id":16119,"web_url":"https://patchwork.libcamera.org/comment/16119/","msgid":"","date":"2021-04-04T01:30:51","subject":"Re: [libcamera-devel] [PATCH v6] 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\nJust a small comment.\n\nOn Sat, Apr 03, 2021 at 10:57:34PM +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> Reviewed-by: Laurent Pinchart \n> ---\n> src/android/camera_device.cpp | 80 ++++++++++++++++++++---------------\n> src/android/camera_device.h | 10 ++++-\n> src/android/camera_worker.cpp | 4 +-\n> src/android/camera_worker.h | 2 +-\n> 4 files changed, 57 insertions(+), 39 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index b45d3a54..9d6be1f9 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -286,16 +286,12 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(\n> \tsettings_ = CameraMetadata(camera3Request->settings);\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 * Create the libcamera::CaptureRequest unique_ptr<> to tie its lifetime\n\nThis should be just CaptureRequest, not libcamera::CaptureRequest. I'll\nfix when applying.\n\n> +\t * to the descriptor's one.\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 +668,7 @@ void CameraDevice::stop()\n> \tworker_.stop();\n> \tcamera_->stop();\n> \n> +\tdescriptors_.clear();\n> \trunning_ = false;\n> }\n> \n> @@ -1818,8 +1815,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 +1826,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 +1864,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 +1883,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 +1894,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 +1913,21 @@ 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> +\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, Fatal)\n> +\t\t\t\t<< \"Unknown request: \" << request->cookie();\n> +\t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t\t\treturn;\n> +\t\t}\n> +\n> +\t\tnode = descriptors_.extract(it);\n> +\t}\n> +\tCamera3RequestDescriptor &descriptor = node.mapped();\n> \n> \tif (request->status() != Request::RequestComplete) {\n> \t\tLOG(HAL, Error) << \"Request not successfully completed: \"\n> @@ -1922,7 +1936,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 +1945,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 +1963,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 +1980,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 +2003,11 @@ 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> }\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..c63e8e21 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> @@ -69,11 +70,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 +127,9 @@ private:\n> \tstd::map formatsMap_;\n> \tstd::vector streams_;\n> \n> +\tstd::mutex mutex_; /* Protect descriptors_ */\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 6C096C0DA3\n\tfor ;\n\tSun, 4 Apr 2021 01:31:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ACD1260518;\n\tSun, 4 Apr 2021 03:31:39 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F18D6602CE\n\tfor ;\n\tSun, 4 Apr 2021 03:31:37 +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 27C5780E;\n\tSun, 4 Apr 2021 03:31:37 +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=\"KL5Z9LeB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617499897;\n\tbh=Unf6nQtdhTMbBNBcQ9yqnGFUMkN917wtrc2UANVftPM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KL5Z9LeBRIRXqvmEB+jPTrRFYrDPtpvAaZqviGArvG4K0ymzzHFdha9FV65JeJ44X\n\tS82l1TdSWQqrM7Hpc5EV1kV2zac15z86/3Vb8bao33kezRr3GPzyjEmUSS0Qx2aphe\n\tihUM/ThK6gSoTO6e0QiXiBVPydsf0oZy7jFeLF8Q=","Date":"Sun, 4 Apr 2021 04:30:51 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210403135734.1600523-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210403135734.1600523-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v6] 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\" "}},{"id":16122,"web_url":"https://patchwork.libcamera.org/comment/16122/","msgid":"<20210406123132.qrb2ch7hs5nyd6zk@uno.localdomain>","date":"2021-04-06T12:31:32","subject":"Re: [libcamera-devel] [PATCH v6] android: CameraDevice: Fix\n\tCamera3RequestDescriptor leakage","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Sat, Apr 03, 2021 at 10:57:34PM +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> Reviewed-by: Laurent Pinchart \n> ---\n> src/android/camera_device.cpp | 80 ++++++++++++++++++++---------------\n> src/android/camera_device.h | 10 ++++-\n> src/android/camera_worker.cpp | 4 +-\n> src/android/camera_worker.h | 2 +-\n> 4 files changed, 57 insertions(+), 39 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index b45d3a54..9d6be1f9 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -286,16 +286,12 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(\n> \tsettings_ = CameraMetadata(camera3Request->settings);\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 * Create the libcamera::CaptureRequest unique_ptr<> to tie its lifetime\n> +\t * to the descriptor's one.\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 +668,7 @@ void CameraDevice::stop()\n\nI have a bit lost track of where stop() is called, as this patch is\nbased on a pile of patches in review. My only concern is that\ndescriptor_ shall be cleared also at configureStreams() time.\n\nWith that confirmed\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> \tworker_.stop();\n> \tcamera_->stop();\n>\n> +\tdescriptors_.clear();\n> \trunning_ = false;\n> }\n>\n> @@ -1818,8 +1815,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 +1826,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 +1864,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 +1883,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 +1894,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 +1913,21 @@ 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> +\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, Fatal)\n> +\t\t\t\t<< \"Unknown request: \" << request->cookie();\n> +\t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t\t\treturn;\n> +\t\t}\n> +\n> +\t\tnode = descriptors_.extract(it);\n> +\t}\n> +\tCamera3RequestDescriptor &descriptor = node.mapped();\n>\n> \tif (request->status() != Request::RequestComplete) {\n> \t\tLOG(HAL, Error) << \"Request not successfully completed: \"\n> @@ -1922,7 +1936,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 +1945,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 +1963,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 +1980,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 +2003,11 @@ 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> }\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..c63e8e21 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> @@ -69,11 +70,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 +127,9 @@ private:\n> \tstd::map formatsMap_;\n> \tstd::vector streams_;\n>\n> +\tstd::mutex mutex_; /* Protect descriptors_ */\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(); }\n> --\n> 2.31.0.208.g409f899ff0-goog\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 AA2D7BD695\n\tfor ;\n\tTue, 6 Apr 2021 12:30:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 298F268786;\n\tTue, 6 Apr 2021 14:30:58 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0991760518\n\tfor ;\n\tTue, 6 Apr 2021 14:30:56 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 7360660004;\n\tTue, 6 Apr 2021 12:30:55 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 6 Apr 2021 14:31:32 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210406123132.qrb2ch7hs5nyd6zk@uno.localdomain>","References":"<20210403135734.1600523-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210403135734.1600523-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v6] 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\" "}},{"id":16130,"web_url":"https://patchwork.libcamera.org/comment/16130/","msgid":"","date":"2021-04-07T02:48:05","subject":"Re: [libcamera-devel] [PATCH v6] 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":"Laurent, can you push this and stop() patch to tree?\n\nOn Tue, Apr 6, 2021 at 9:30 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Sat, Apr 03, 2021 at 10:57:34PM +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> > Reviewed-by: Laurent Pinchart \n> > ---\n> > src/android/camera_device.cpp | 80 ++++++++++++++++++++---------------\n> > src/android/camera_device.h | 10 ++++-\n> > src/android/camera_worker.cpp | 4 +-\n> > src/android/camera_worker.h | 2 +-\n> > 4 files changed, 57 insertions(+), 39 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index b45d3a54..9d6be1f9 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -286,16 +286,12 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(\n> > settings_ = CameraMetadata(camera3Request->settings);\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> > + * Create the libcamera::CaptureRequest unique_ptr<> to tie its lifetime\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> > @@ -672,6 +668,7 @@ void CameraDevice::stop()\n>\n> I have a bit lost track of where stop() is called, as this patch is\n> based on a pile of patches in review. My only concern is that\n> descriptor_ shall be cleared also at configureStreams() time.\n>\n> With that confirmed\n> Reviewed-by: Jacopo Mondi \n>\n> Thanks\n> j\n>\n> > worker_.stop();\n> > camera_->stop();\n> >\n> > + descriptors_.clear();\n> > running_ = false;\n> > }\n> >\n> > @@ -1818,8 +1815,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> > @@ -1829,12 +1826,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> > @@ -1867,7 +1864,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> > @@ -1886,11 +1883,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> > @@ -1898,11 +1894,16 @@ 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> > +\n> > + {\n> > + std::scoped_lock lock(mutex_);\n> > + descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> > + }\n> >\n> > return 0;\n> > }\n> > @@ -1912,8 +1913,21 @@ 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> > +\n> > + decltype(descriptors_)::node_type node;\n> > + {\n> > + std::scoped_lock lock(mutex_);\n> > + auto it = descriptors_.find(request->cookie());\n> > + if (it == descriptors_.end()) {\n> > + LOG(HAL, Fatal)\n> > + << \"Unknown request: \" << request->cookie();\n> > + status = CAMERA3_BUFFER_STATUS_ERROR;\n> > + return;\n> > + }\n> > +\n> > + node = descriptors_.extract(it);\n> > + }\n> > + Camera3RequestDescriptor &descriptor = node.mapped();\n> >\n> > if (request->status() != Request::RequestComplete) {\n> > LOG(HAL, Error) << \"Request not successfully completed: \"\n> > @@ -1922,7 +1936,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> > @@ -1931,10 +1945,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> > @@ -1949,7 +1963,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> > @@ -1966,17 +1980,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> > @@ -1989,13 +2003,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..c63e8e21 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> > @@ -69,11 +70,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 +127,9 @@ private:\n> > std::map formatsMap_;\n> > std::vector streams_;\n> >\n> > + std::mutex mutex_; /* Protect descriptors_ */\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.208.g409f899ff0-goog\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 7DD70BD695\n\tfor ;\n\tWed, 7 Apr 2021 02:48:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EAB4F687D6;\n\tWed, 7 Apr 2021 04:48:16 +0200 (CEST)","from mail-ed1-x536.google.com (mail-ed1-x536.google.com\n\t[IPv6:2a00:1450:4864:20::536])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9ED60602CD\n\tfor ;\n\tWed, 7 Apr 2021 04:48:15 +0200 (CEST)","by mail-ed1-x536.google.com with SMTP id k8so11523919edn.6\n\tfor ;\n\tTue, 06 Apr 2021 19:48:15 -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=\"Yx6fsQs3\"; 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\t:cc; bh=KLnDvDrmiUmF0e08bZhzy9rew2rm04dwbYEBozXYkUA=;\n\tb=Yx6fsQs3Acdl+P1zjlC9VGRfV0GYp7FSnEznOVgXNUzh4UkkpnyvljB2/wKWj7JXF5\n\t+yQqd2PJgq4ePU3AOb1OLzy/zmrugzrjYl8h3bjiJLd63wv5VCYof5jFElib73Vb1z3U\n\tBknYngcLm6OyezBqDAI/CF7BC4inU11V2ZsRM=","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:cc;\n\tbh=KLnDvDrmiUmF0e08bZhzy9rew2rm04dwbYEBozXYkUA=;\n\tb=KrJNbdkATk93wqQvd5YHmxHzP2Y06E+OzWkbHVsKRdkrohW4rnAeOjM4bom9njZR1N\n\t2Q7E9AmDrWL7hf0gW5if20A6EVrNsR9QfPMADNJlwsxeT64bK0Sb7XzNYkQ4x3LZIWiv\n\tsqhhsOhCnU8Yd96LNc6Z5oFSX+5lDzz7wm3eiAR9WPaVDmdATyfqlLdZP/qbCZdl/ED4\n\t07CjL3JeX6oueYW4g6IIxVvWij1wMDcrZkh+QdW0YlhHps51Aw1EitDAKtsoqBiWgzVZ\n\tcfLcNt8s7K+N5T5x/d6aS7P5kUfI4XcWBDN7WDw/8mYdon1J5Eq73AO3h7RzUSGuKW9a\n\tqhDA==","X-Gm-Message-State":"AOAM532eT/8KPRQotOQCy3WysB1rDP9cpSnkWOjx+wtLPeAUjRMgtRwW\n\tFIguq1XigkBpC4IBFRZ3a4oOSbnbgBgTF4zS3WUIxA==","X-Google-Smtp-Source":"ABdhPJzujloPPPLCfrWZsoi97rN4kevJp0oowQBDpFxZ9hgI0T/IdKBzTD7qzM+uIyroob3ZPwwsOG5X2TFnyr3q1H0=","X-Received":"by 2002:a05:6402:51cd:: with SMTP id\n\tr13mr1710967edd.116.1617763695191; \n\tTue, 06 Apr 2021 19:48:15 -0700 (PDT)","MIME-Version":"1.0","References":"<20210403135734.1600523-1-hiroh@chromium.org>\n\t<20210406123132.qrb2ch7hs5nyd6zk@uno.localdomain>","In-Reply-To":"<20210406123132.qrb2ch7hs5nyd6zk@uno.localdomain>","From":"Hirokazu Honda ","Date":"Wed, 7 Apr 2021 11:48:05 +0900","Message-ID":"","To":"Jacopo Mondi ,\n\tLaurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v6] 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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16380,"web_url":"https://patchwork.libcamera.org/comment/16380/","msgid":"","date":"2021-04-20T03:11:21","subject":"Re: [libcamera-devel] [PATCH v6] 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\nOn Wed, Apr 07, 2021 at 11:48:05AM +0900, Hirokazu Honda wrote:\n> Laurent, can you push this and stop() patch to tree?\n\nI'm sorry, those two patches have fallen through the crack (they were\nstill visible in patchwork though, so I suppose they haven't fallen\ncompletely and would have resurfaced, I'm happy Kieran kept pushing to\nhave a patchwork instance and got it up and running).\n\nAnyway, patches pushed. Thanks for reminding me.\n\n> On Tue, Apr 6, 2021 at 9:30 PM Jacopo Mondi wrote:\n> > On Sat, Apr 03, 2021 at 10:57:34PM +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> > > Reviewed-by: Laurent Pinchart \n> > > ---\n> > > src/android/camera_device.cpp | 80 ++++++++++++++++++++---------------\n> > > src/android/camera_device.h | 10 ++++-\n> > > src/android/camera_worker.cpp | 4 +-\n> > > src/android/camera_worker.h | 2 +-\n> > > 4 files changed, 57 insertions(+), 39 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index b45d3a54..9d6be1f9 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -286,16 +286,12 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(\n> > > settings_ = CameraMetadata(camera3Request->settings);\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> > > + * Create the libcamera::CaptureRequest unique_ptr<> to tie its lifetime\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> > > @@ -672,6 +668,7 @@ void CameraDevice::stop()\n> >\n> > I have a bit lost track of where stop() is called, as this patch is\n> > based on a pile of patches in review. My only concern is that\n> > descriptor_ shall be cleared also at configureStreams() time.\n> >\n> > With that confirmed\n> > Reviewed-by: Jacopo Mondi \n> >\n> > > worker_.stop();\n> > > camera_->stop();\n> > >\n> > > + descriptors_.clear();\n> > > running_ = false;\n> > > }\n> > >\n> > > @@ -1818,8 +1815,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> > > @@ -1829,12 +1826,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> > > @@ -1867,7 +1864,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> > > @@ -1886,11 +1883,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> > > @@ -1898,11 +1894,16 @@ 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> > > +\n> > > + {\n> > > + std::scoped_lock lock(mutex_);\n> > > + descriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> > > + }\n> > >\n> > > return 0;\n> > > }\n> > > @@ -1912,8 +1913,21 @@ 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> > > +\n> > > + decltype(descriptors_)::node_type node;\n> > > + {\n> > > + std::scoped_lock lock(mutex_);\n> > > + auto it = descriptors_.find(request->cookie());\n> > > + if (it == descriptors_.end()) {\n> > > + LOG(HAL, Fatal)\n> > > + << \"Unknown request: \" << request->cookie();\n> > > + status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > + return;\n> > > + }\n> > > +\n> > > + node = descriptors_.extract(it);\n> > > + }\n> > > + Camera3RequestDescriptor &descriptor = node.mapped();\n> > >\n> > > if (request->status() != Request::RequestComplete) {\n> > > LOG(HAL, Error) << \"Request not successfully completed: \"\n> > > @@ -1922,7 +1936,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> > > @@ -1931,10 +1945,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> > > @@ -1949,7 +1963,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> > > @@ -1966,17 +1980,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> > > @@ -1989,13 +2003,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..c63e8e21 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> > > @@ -69,11 +70,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 +127,9 @@ private:\n> > > std::map formatsMap_;\n> > > std::vector streams_;\n> > >\n> > > + std::mutex mutex_; /* Protect descriptors_ */\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(); }","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 80134BD816\n\tfor ;\n\tTue, 20 Apr 2021 03:11:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E13816883C;\n\tTue, 20 Apr 2021 05:11:27 +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 9BE32602CA\n\tfor ;\n\tTue, 20 Apr 2021 05:11:25 +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 033BF470;\n\tTue, 20 Apr 2021 05:11:24 +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=\"Z294M/SL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618888285;\n\tbh=pS07+8KfYoWuhd06fJtiq+WBKcPaxurpT3HLDhXywvY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Z294M/SLLnnA1TI2apRqUAP+L6IXKUt+Gc4CBmjHsiF9Uqiarozf/k2lgYhfGu1OY\n\tA0iQNrEh87/6gE6kXSuwiEA3J14MmiOjB38I59WRKFkoGaoWKWTOprUpo+vEkQT4+/\n\tgnZg6+qJDjAerey37oLg8l78CSzOtb7U4gmn2bYQ=","Date":"Tue, 20 Apr 2021 06:11:21 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210403135734.1600523-1-hiroh@chromium.org>\n\t<20210406123132.qrb2ch7hs5nyd6zk@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6] 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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]