[{"id":15902,"web_url":"https://patchwork.libcamera.org/comment/15902/","msgid":"<20210325160243.yo67ycwa6zaooxoy@uno.localdomain>","date":"2021-03-25T16:02:43","subject":"Re: [libcamera-devel] [PATCH 2/2] 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 Thu, Mar 25, 2021 at 08:13:57PM +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 CamerarRequestDescriptor.\n\nVery nice, I'm not particularly proud of this part of the\nimplementation as it is quite hard to follow, this makes it easier\nindeed\n\nOne question below\n\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera_device.cpp | 67 +++++++++++++++++++----------------\n> src/android/camera_device.h | 6 +++-\n> src/android/camera_worker.cpp | 4 +--\n> src/android/camera_worker.h | 2 +-\n> 4 files changed, 44 insertions(+), 35 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index ae693664..50b017a3 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -287,15 +287,17 @@ 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> \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> CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n>\n> +CameraDevice::Camera3RequestDescriptor&\n> +CameraDevice::Camera3RequestDescriptor::operator=(Camera3RequestDescriptor&&) = default;\n> +\n> /*\n> * \\class CameraDevice\n> *\n> @@ -1811,8 +1813,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> @@ -1822,12 +1824,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> @@ -1860,7 +1862,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> @@ -1879,11 +1881,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> @@ -1891,11 +1892,12 @@ 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> +\tdescriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n>\n> \treturn 0;\n> }\n> @@ -1905,8 +1907,13 @@ 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> +\tauto node = descriptors_.extract(request->cookie());\n\nSo once node goes out of scope the mapped descriptor gets deleted.\nVery nice.\n\nHowever, shouldn't descriptors_ be cleared at stop() time or when a\nnew stream configuration is requested ?\n\n> +\tif (node.empty()) {\n> +\t\tLOG(HAL, Error) << \"Unknown request: \" << request->cookie();\n> +\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t\treturn;\n> +\t}\n> +\tCamera3RequestDescriptor& descriptor = node.mapped();\n>\n> \tif (request->status() != Request::RequestComplete) {\n> \t\tLOG(HAL, Error) << \"Request not successfully completed: \"\n> @@ -1915,7 +1922,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> @@ -1924,10 +1931,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> @@ -1942,7 +1949,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> @@ -1959,17 +1966,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> @@ -1982,13 +1989,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 11bdfec8..54d7c319 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -69,9 +69,11 @@ private:\n> \tCameraDevice(unsigned int id, std::shared_ptr camera);\n>\n> \tstruct Camera3RequestDescriptor {\n> +\t\tCamera3RequestDescriptor();\n> +\t\t~Camera3RequestDescriptor();\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&&);\n>\n> \t\tuint32_t frameNumber_;\n> \t\tstd::vector buffers_;\n> @@ -122,6 +124,8 @@ private:\n> \tstd::map formatsMap_;\n> \tstd::vector streams_;\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(); }\n> --\n> 2.31.0.291.g576ba9dcdaf-goog\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 4FAF2C32E9\n\tfor ;\n\tThu, 25 Mar 2021 16:02:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7BFFB68D65;\n\tThu, 25 Mar 2021 17:02:13 +0100 (CET)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6A99F602D5\n\tfor ;\n\tThu, 25 Mar 2021 17:02:11 +0100 (CET)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id B6E07240008;\n\tThu, 25 Mar 2021 16:02:10 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Thu, 25 Mar 2021 17:02:43 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210325160243.yo67ycwa6zaooxoy@uno.localdomain>","References":"<20210325111357.3862847-1-hiroh@chromium.org>\n\t<20210325111357.3862847-2-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210325111357.3862847-2-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH 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\" "}},{"id":15905,"web_url":"https://patchwork.libcamera.org/comment/15905/","msgid":"","date":"2021-03-25T17:21:27","subject":"Re: [libcamera-devel] [PATCH 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, it's nice to see the HAL being cleaned up :-)\n\nOn Thu, Mar 25, 2021 at 05:02:43PM +0100, Jacopo Mondi wrote:\n> On Thu, Mar 25, 2021 at 08:13:57PM +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 CamerarRequestDescriptor.\n\ns/CamerarRequestDescriptor/CameraRequestDescriptor/\n\nWhen does this happen ? You've reported issues with the IPU3 pipeline\nhandler failing to queue a request due to fact that it has no internal\nqueue of requests waiting for hardware resources to be available. Is\nthat what you're addressing here, or is there something else ? The\nlibcamera core should guarantee that requestComplete() gets called for\nall requests successfully queued, so if there are other leakages, I'd\nlike to investigate them.\n\n> Very nice, I'm not particularly proud of this part of the\n> implementation as it is quite hard to follow, this makes it easier\n> indeed\n> \n> One question below\n> \n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/camera_device.cpp | 67 +++++++++++++++++++----------------\n> > src/android/camera_device.h | 6 +++-\n> > src/android/camera_worker.cpp | 4 +--\n> > src/android/camera_worker.h | 2 +-\n> > 4 files changed, 44 insertions(+), 35 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index ae693664..50b017a3 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -287,15 +287,17 @@ 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> > \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> > CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> >\n> > +CameraDevice::Camera3RequestDescriptor&\n> > +CameraDevice::Camera3RequestDescriptor::operator=(Camera3RequestDescriptor&&) = default;\n> > +\n> > /*\n> > * \\class CameraDevice\n> > *\n> > @@ -1811,8 +1813,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> > @@ -1822,12 +1824,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> > @@ -1860,7 +1862,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> > @@ -1879,11 +1881,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> > @@ -1891,11 +1892,12 @@ 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> > +\tdescriptors_[descriptor.request_->cookie()] = std::move(descriptor);\n> >\n> > \treturn 0;\n> > }\n> > @@ -1905,8 +1907,13 @@ 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> > +\tauto node = descriptors_.extract(request->cookie());\n> \n> So once node goes out of scope the mapped descriptor gets deleted.\n> Very nice.\n> \n> However, shouldn't descriptors_ be cleared at stop() time or when a\n> new stream configuration is requested ?\n\nI'm wondering if we shouldn't go one step forward. I'll assume here that\nthis patch is dealing with failures to queue requests when calling\nCamera::queueRequest(). If there are other issues, the comments below\nmay not apply to them.\n\nWhen queuing a request to libcamera, we call\nCameraWorker::queueRequest(). It's a void function that invokes\nWorker::processRequest() as a queued call, so it returns immediately,\nand we can't know if Worker::processRequest() will fail or not. We have\nthus sent the request onto a journey, hoping it will be successfully\nqueued. There can be multiple reasons that won't be the case:\n\n- CameraWorker::Worker::processRequest() failing to wait for a fence.\n Oops, shouldn't happen, but we still need to handle this gracefully.\n We're still in the HAL here, the request hasn't reached libcamera. The\n HAL should thus handle the failure. This is case number 1.\n\n- CameraWorker::Worker::processRequest() calls CaptureRequest::queue(),\n which calls Camera::queueRequest(). That function can return an error\n if some of the sanity checks fail. We've reached libcamera, but it has\n failed synchronously, so the HAL should handle the failure. Still case\n number 1.\n\n- At this point, the request is sent to PipelineHandler::queueRequest(),\n asynchronously again. That function doesn't perform any sanity check\n (but it could in the future), and queues the request to the pipeline\n handler with a call to PipelineHandler:queueRequestDevice(),\n synchronously. This call can fail. If the future sanity checks in\n PipelineHandler::queueRequest() or\n PipelineHandler:queueRequestDevice() fail, the request belongs to the\n libcamera core (the HAL has queued it successfully, and the pipeline\n handler hasn't accepted it). This is case number 2, and I believe this\n is the one that has triggered this patch.\n\n- The pipeline handler has accepted the request, but something fails\n later. That's case number 3.\n\nNow let's look at those three cases, in reverse order.\n\n3. The request belongs to the pipeline handler, which must complete it.\nWe have support in the PipelineHandler base class to accept completion\nof requests out of sequence, and reorder the completion events to the\nHAL to guarantee they will be issued in sequence. The pipeline handler\ncan thus complete the request with an error state as soon as it detects\nthe error, and the HAL will eventually receive a requestComplete() call.\nAs far as I can tell, this is working correctly.\n\n2. This is embarassing, we don't handle this at all.\nPipelineHandler::queueRequest() returns an error code, but it never\nreaches Camera::queueReuest() as the call to\nPipelineHandler::queueRequest() is asynchronous. We should make\nPipelineHandler::queueRequest() a void function to be less misleading,\nand PipelineHandler::queueRequest() needs to signal request completion\nin that case. I think we can reuse the same request completion ordering\nmechanism and immediately mark the request as complete (with an error)\nin PipelineHandler::queueRequest() in that case. It should be an easy\nfix (famous last words... :-)).\n\n1. This is the responsibility of the HAL, which should signal request\ncompletion to the camera service. Depending on whether the camera\nservice can support out-of-order completion or not, we can signal\ncompletion immediately, or need to keep the request in a queue and\ncomplete it at the appropriate time.\n\nAs far as I understand, this patch is meant to address the memory leak\nrelated to case 2, and also addresses as a consequence the leak related\nto case 1 (case 3 should be working correctly already, if I'm not\nmistaken). For case 2 I think the correct fix should be to ensure that\nlibcamera will complete the request, as explained above. For case 1, we\nneed a specific fix in the HAL, not only for the memory leak, but to\nalso complete the request towards the camera service.\n\nIs this analysis correct, or am I missing something ?\n\n> > +\tif (node.empty()) {\n> > +\t\tLOG(HAL, Error) << \"Unknown request: \" << request->cookie();\n> > +\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> > +\t\treturn;\n> > +\t}\n> > +\tCamera3RequestDescriptor& descriptor = node.mapped();\n> >\n> > \tif (request->status() != Request::RequestComplete) {\n> > \t\tLOG(HAL, Error) << \"Request not successfully completed: \"\n> > @@ -1915,7 +1922,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> > @@ -1924,10 +1931,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> > @@ -1942,7 +1949,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> > @@ -1959,17 +1966,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> > @@ -1982,13 +1989,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 11bdfec8..54d7c319 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -69,9 +69,11 @@ private:\n> > \tCameraDevice(unsigned int id, std::shared_ptr camera);\n> >\n> > \tstruct Camera3RequestDescriptor {\n> > +\t\tCamera3RequestDescriptor();\n> > +\t\t~Camera3RequestDescriptor();\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&&);\n> >\n> > \t\tuint32_t frameNumber_;\n> > \t\tstd::vector buffers_;\n> > @@ -122,6 +124,8 @@ private:\n> > \tstd::map formatsMap_;\n> > \tstd::vector streams_;\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 DCB76C32E9\n\tfor ;\n\tThu, 25 Mar 2021 17:22:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0DEE268D65;\n\tThu, 25 Mar 2021 18:22:13 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C0CA5602D5\n\tfor ;\n\tThu, 25 Mar 2021 18:22:11 +0100 (CET)","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 145048F0;\n\tThu, 25 Mar 2021 18:22:11 +0100 (CET)"],"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=\"p6upwXPm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616692931;\n\tbh=FAbZfZYU7S27yS7JwKb4XDvRNL/12mTfIKyu1zC1jrw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=p6upwXPmiHuF/ViimXxKFsxqowbk8xpWOAE6FyXDmZ5fvI2kG4j+xZhaEOdikkIL2\n\tSwxiLOol0ZeEjSBH+R6TJ1CBLIRbj8m4n7uTkjE5ZBdaTZuGwoRGT7G22nExxPPyzn\n\tLOydSK6zyXdhI6E3+zB5iYyUJYicKmP6sWZWyNvA=","Date":"Thu, 25 Mar 2021 19:21:27 +0200","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210325111357.3862847-1-hiroh@chromium.org>\n\t<20210325111357.3862847-2-hiroh@chromium.org>\n\t<20210325160243.yo67ycwa6zaooxoy@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210325160243.yo67ycwa6zaooxoy@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 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\" "}},{"id":15973,"web_url":"https://patchwork.libcamera.org/comment/15973/","msgid":"","date":"2021-03-28T22:10:27","subject":"Re: [libcamera-devel] [PATCH 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":"Hi Jacopo and Laurent, thanks for reviewing.\n\nOn Fri, Mar 26, 2021 at 2:22 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch, it's nice to see the HAL being cleaned up :-)\n>\n> On Thu, Mar 25, 2021 at 05:02:43PM +0100, Jacopo Mondi wrote:\n> > On Thu, Mar 25, 2021 at 08:13:57PM +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 CamerarRequestDescriptor.\n>\n> s/CamerarRequestDescriptor/CameraRequestDescriptor/\n>\n> When does this happen ? You've reported issues with the IPU3 pipeline\n> handler failing to queue a request due to fact that it has no internal\n> queue of requests waiting for hardware resources to be available. Is\n> that what you're addressing here, or is there something else ? The\n> libcamera core should guarantee that requestComplete() gets called for\n> all requests successfully queued, so if there are other leakages, I'd\n> like to investigate them.\n>\n\nSince a libcamera process is likely alive as long as a system runs, I\nwould like to avoid any potential leakage as much as possible.\nRegardless of the issue I've reported, I consider it is good to\nguarantee the descriptor is destroyed at the latest in CameraDevice\ndescriptor.\nAs you said, requestComplete() should be returned and I haven't found\nany other issues of not invoking it than the issue I reported, but I\nthink it isn't theoretically ensured.\n\n> > Very nice, I'm not particularly proud of this part of the\n> > implementation as it is quite hard to follow, this makes it easier\n> > indeed\n> >\n> > One question below\n> >\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > src/android/camera_device.cpp | 67 +++++++++++++++++++----------------\n> > > src/android/camera_device.h | 6 +++-\n> > > src/android/camera_worker.cpp | 4 +--\n> > > src/android/camera_worker.h | 2 +-\n> > > 4 files changed, 44 insertions(+), 35 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index ae693664..50b017a3 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -287,15 +287,17 @@ 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> > > CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> > >\n> > > +CameraDevice::Camera3RequestDescriptor&\n> > > +CameraDevice::Camera3RequestDescriptor::operator=(Camera3RequestDescriptor&&) = default;\n> > > +\n> > > /*\n> > > * \\class CameraDevice\n> > > *\n> > > @@ -1811,8 +1813,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> > > @@ -1822,12 +1824,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> > > @@ -1860,7 +1862,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> > > @@ -1879,11 +1881,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> > > @@ -1891,11 +1892,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> > > @@ -1905,8 +1907,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> >\n> > So once node goes out of scope the mapped descriptor gets deleted.\n> > Very nice.\n> >\n> > However, shouldn't descriptors_ be cleared at stop() time or when a\n> > new stream configuration is requested ?\n>\n\ndescriptors_ is cleared thanks to std::map destructor, isn't it?\nI think it is also true to ought to clear when a new stream\nconfiguration is requested from the following camera3 API statement,\n> Add signal_stream_flush() to camera3_device_ops_t for camera service to notify HAL an\n> upcoming configure_streams() call requires HAL to return buffers of certain streams.\nhttps://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera3.h#194\n\n> I'm wondering if we shouldn't go one step forward. I'll assume here that\n> this patch is dealing with failures to queue requests when calling\n> Camera::queueRequest(). If there are other issues, the comments below\n> may not apply to them.\n>\n> When queuing a request to libcamera, we call\n> CameraWorker::queueRequest(). It's a void function that invokes\n> Worker::processRequest() as a queued call, so it returns immediately,\n> and we can't know if Worker::processRequest() will fail or not. We have\n> thus sent the request onto a journey, hoping it will be successfully\n> queued. There can be multiple reasons that won't be the case:\n>\n> - CameraWorker::Worker::processRequest() failing to wait for a fence.\n> Oops, shouldn't happen, but we still need to handle this gracefully.\n> We're still in the HAL here, the request hasn't reached libcamera. The\n> HAL should thus handle the failure. This is case number 1.\n>\n> - CameraWorker::Worker::processRequest() calls CaptureRequest::queue(),\n> which calls Camera::queueRequest(). That function can return an error\n> if some of the sanity checks fail. We've reached libcamera, but it has\n> failed synchronously, so the HAL should handle the failure. Still case\n> number 1.\n>\n> - At this point, the request is sent to PipelineHandler::queueRequest(),\n> asynchronously again. That function doesn't perform any sanity check\n> (but it could in the future), and queues the request to the pipeline\n> handler with a call to PipelineHandler:queueRequestDevice(),\n> synchronously. This call can fail. If the future sanity checks in\n> PipelineHandler::queueRequest() or\n> PipelineHandler:queueRequestDevice() fail, the request belongs to the\n> libcamera core (the HAL has queued it successfully, and the pipeline\n> handler hasn't accepted it). This is case number 2, and I believe this\n> is the one that has triggered this patch.\n>\n> - The pipeline handler has accepted the request, but something fails\n> later. That's case number 3.\n>\n> Now let's look at those three cases, in reverse order.\n>\n> 3. The request belongs to the pipeline handler, which must complete it.\n> We have support in the PipelineHandler base class to accept completion\n> of requests out of sequence, and reorder the completion events to the\n> HAL to guarantee they will be issued in sequence. The pipeline handler\n> can thus complete the request with an error state as soon as it detects\n> the error, and the HAL will eventually receive a requestComplete() call.\n> As far as I can tell, this is working correctly.\n>\n> 2. This is embarassing, we don't handle this at all.\n> PipelineHandler::queueRequest() returns an error code, but it never\n> reaches Camera::queueReuest() as the call to\n> PipelineHandler::queueRequest() is asynchronous. We should make\n> PipelineHandler::queueRequest() a void function to be less misleading,\n> and PipelineHandler::queueRequest() needs to signal request completion\n> in that case. I think we can reuse the same request completion ordering\n> mechanism and immediately mark the request as complete (with an error)\n> in PipelineHandler::queueRequest() in that case. It should be an easy\n> fix (famous last words... :-)).\n>\n> 1. This is the responsibility of the HAL, which should signal request\n> completion to the camera service. Depending on whether the camera\n> service can support out-of-order completion or not, we can signal\n> completion immediately, or need to keep the request in a queue and\n> complete it at the appropriate time.\n>\n> As far as I understand, this patch is meant to address the memory leak\n> related to case 2, and also addresses as a consequence the leak related\n> to case 1 (case 3 should be working correctly already, if I'm not\n> mistaken). For case 2 I think the correct fix should be to ensure that\n> libcamera will complete the request, as explained above. For case 1, we\n> need a specific fix in the HAL, not only for the memory leak, but to\n> also complete the request towards the camera service.\n>\n> Is this analysis correct, or am I missing something ?\n>\n\nThanks for analyzing the details. Yes your analysis looks correct.\nWell, I expect this patch is to retain memory leakage in any cases.\nI mean that no memory is leaked due to any failures.\n\nI agree that the failure 1 should be handled properly.\nThe fix to failure 2 also sounds good to me.\n\nBest Regards,\n-Hiro\n\n> > > + if (node.empty()) {\n> > > + LOG(HAL, Error) << \"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> > > @@ -1915,7 +1922,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> > > @@ -1924,10 +1931,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> > > @@ -1942,7 +1949,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> > > @@ -1959,17 +1966,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> > > @@ -1982,13 +1989,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 11bdfec8..54d7c319 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -69,9 +69,11 @@ private:\n> > > CameraDevice(unsigned int id, std::shared_ptr camera);\n> > >\n> > > struct Camera3RequestDescriptor {\n> > > + Camera3RequestDescriptor();\n> > > + ~Camera3RequestDescriptor();\n> > > Camera3RequestDescriptor(libcamera::Camera *camera,\n> > > const camera3_capture_request_t *camera3Request);\n> > > - ~Camera3RequestDescriptor();\n> > > + Camera3RequestDescriptor& operator=(Camera3RequestDescriptor&&);\n> > >\n> > > uint32_t frameNumber_;\n> > > std::vector buffers_;\n> > > @@ -122,6 +124,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> --\n> Regards,\n>\n> Laurent Pinchart","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 754D9C32EA\n\tfor ;\n\tSun, 28 Mar 2021 22:10:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BBC9C6877F;\n\tMon, 29 Mar 2021 00:10:41 +0200 (CEST)","from mail-ej1-x633.google.com (mail-ej1-x633.google.com\n\t[IPv6:2a00:1450:4864:20::633])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5C2E6602D7\n\tfor ;\n\tMon, 29 Mar 2021 00:10:39 +0200 (CEST)","by mail-ej1-x633.google.com with SMTP id u5so16511483ejn.8\n\tfor ;\n\tSun, 28 Mar 2021 15:10:39 -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=\"bfcNLsts\"; 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=6L6mAFxKw7CJZjnT8On5Jc1T4W6W0bGdNXUNb8Nxte4=;\n\tb=bfcNLstsGDBfaIY/qPkx/0u2Ijms1f/C2UksnbXpWb+kdc+4Gdo+4lf+7H+tP/OFMp\n\tbo5TM3X2+0qs5scuN9svsPuTMh4X69WCq7vG6AN6jAOGWkLeschFrhdKtH3qSzmBGtM3\n\tsLNV9G4N+cxipRt22y6BMKtYRn4/LHL+EAcKc=","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=6L6mAFxKw7CJZjnT8On5Jc1T4W6W0bGdNXUNb8Nxte4=;\n\tb=Lk9WaQ4zNzYAA7hIij4VcTX+PmCe8oEF6ZQoalsnwd26MHZN3KeEftlCPi/sLGmCtl\n\tA3rCQ8vNzr1OAkdG+HPdlTRtIEeBm1q9OEh9q+vRTl+atcPPgU55K7NQgRQ7kZNOVvmH\n\tWZTwMWAVg4vnxnSTGh6Vos7EHaKW+ISM4YAbP1LXUzp9m5RIWaVC9GRLXvsJJjbZ43sR\n\tz79r/CFxnseNXTJV40L5tmgGJdks1EkF5HZxR0A3iqmh9Pfg9rN1DU1y0cv/iPlWpR2k\n\t1qRyxtbU3jG000eMU+5KG/eu7SHAUOrDTJ0GL2KN7yKFjD1uAO0jJdB5JxRYKQL+t4kx\n\t/Ebg==","X-Gm-Message-State":"AOAM532eqdZOGpgnWYOCVWNmIzqlZ8cqilhNjrnm4Hi23vaXuQ92HlZ+\n\tKf4rR7jEeTIQpad09CvpzLl5kMGsbn5N5iX3Guc7OQ==","X-Google-Smtp-Source":"ABdhPJyommg8o6AxtanT3hvZTyLloZSgPSnUkBlzcVjE8vGDVi+YEEVFFQxkjcJjz8gTUnKZX6fU9PDsuFPGedUkNIk=","X-Received":"by 2002:a17:907:36e:: with SMTP id\n\trs14mr25157756ejb.42.1616969438825; \n\tSun, 28 Mar 2021 15:10:38 -0700 (PDT)","MIME-Version":"1.0","References":"<20210325111357.3862847-1-hiroh@chromium.org>\n\t<20210325111357.3862847-2-hiroh@chromium.org>\n\t<20210325160243.yo67ycwa6zaooxoy@uno.localdomain>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 29 Mar 2021 07:10:27 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH 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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15974,"web_url":"https://patchwork.libcamera.org/comment/15974/","msgid":"","date":"2021-03-28T22:17:01","subject":"Re: [libcamera-devel] [PATCH 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\nOn Mon, Mar 29, 2021 at 07:10:27AM +0900, Hirokazu Honda wrote:\n> Hi Jacopo and Laurent, thanks for reviewing.\n> \n> On Fri, Mar 26, 2021 at 2:22 AM Laurent Pinchartwrote:\n> >\n> > Hi Hiro,\n> >\n> > Thank you for the patch, it's nice to see the HAL being cleaned up :-)\n> >\n> > On Thu, Mar 25, 2021 at 05:02:43PM +0100, Jacopo Mondi wrote:\n> > > On Thu, Mar 25, 2021 at 08:13:57PM +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 CamerarRequestDescriptor.\n> >\n> > s/CamerarRequestDescriptor/CameraRequestDescriptor/\n> >\n> > When does this happen ? You've reported issues with the IPU3 pipeline\n> > handler failing to queue a request due to fact that it has no internal\n> > queue of requests waiting for hardware resources to be available. Is\n> > that what you're addressing here, or is there something else ? The\n> > libcamera core should guarantee that requestComplete() gets called for\n> > all requests successfully queued, so if there are other leakages, I'd\n> > like to investigate them.\n> \n> Since a libcamera process is likely alive as long as a system runs, I\n> would like to avoid any potential leakage as much as possible.\n\nWe agree on this :-)\n\n> Regardless of the issue I've reported, I consider it is good to\n> guarantee the descriptor is destroyed at the latest in CameraDevice\n> descriptor.\n> As you said, requestComplete() should be returned and I haven't found\n> any other issues of not invoking it than the issue I reported, but I\n> think it isn't theoretically ensured.\n\nMy point was that, as this isn't supposed to happen in the first place,\nI want to make sure we investigate any occurrence of the issue you may\nhave been able to trigger. There's one particular problem you've\nreported that can lead this this leakage, and that will be addressed on\nits own, and I was wondering if there were any other. You're confirming\nthat you haven't found any other, so I'm happy :-)\n\n> > > Very nice, I'm not particularly proud of this part of the\n> > > implementation as it is quite hard to follow, this makes it easier\n> > > indeed\n> > >\n> > > One question below\n> > >\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > ---\n> > > > src/android/camera_device.cpp | 67 +++++++++++++++++++----------------\n> > > > src/android/camera_device.h | 6 +++-\n> > > > src/android/camera_worker.cpp | 4 +--\n> > > > src/android/camera_worker.h | 2 +-\n> > > > 4 files changed, 44 insertions(+), 35 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index ae693664..50b017a3 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -287,15 +287,17 @@ 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> > > > CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> > > >\n> > > > +CameraDevice::Camera3RequestDescriptor&\n> > > > +CameraDevice::Camera3RequestDescriptor::operator=(Camera3RequestDescriptor&&) = default;\n> > > > +\n> > > > /*\n> > > > * \\class CameraDevice\n> > > > *\n> > > > @@ -1811,8 +1813,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> > > > @@ -1822,12 +1824,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> > > > @@ -1860,7 +1862,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> > > > @@ -1879,11 +1881,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> > > > @@ -1891,11 +1892,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> > > > @@ -1905,8 +1907,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> > >\n> > > So once node goes out of scope the mapped descriptor gets deleted.\n> > > Very nice.\n> > >\n> > > However, shouldn't descriptors_ be cleared at stop() time or when a\n> > > new stream configuration is requested ?\n> \n> descriptors_ is cleared thanks to std::map destructor, isn't it?\n> I think it is also true to ought to clear when a new stream\n> configuration is requested from the following camera3 API statement,\n>\n> > Add signal_stream_flush() to camera3_device_ops_t for camera service to notify HAL an\n> > upcoming configure_streams() call requires HAL to return buffers of certain streams.\n> https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera3.h#194\n> \n> > I'm wondering if we shouldn't go one step forward. I'll assume here that\n> > this patch is dealing with failures to queue requests when calling\n> > Camera::queueRequest(). If there are other issues, the comments below\n> > may not apply to them.\n> >\n> > When queuing a request to libcamera, we call\n> > CameraWorker::queueRequest(). It's a void function that invokes\n> > Worker::processRequest() as a queued call, so it returns immediately,\n> > and we can't know if Worker::processRequest() will fail or not. We have\n> > thus sent the request onto a journey, hoping it will be successfully\n> > queued. There can be multiple reasons that won't be the case:\n> >\n> > - CameraWorker::Worker::processRequest() failing to wait for a fence.\n> > Oops, shouldn't happen, but we still need to handle this gracefully.\n> > We're still in the HAL here, the request hasn't reached libcamera. The\n> > HAL should thus handle the failure. This is case number 1.\n> >\n> > - CameraWorker::Worker::processRequest() calls CaptureRequest::queue(),\n> > which calls Camera::queueRequest(). That function can return an error\n> > if some of the sanity checks fail. We've reached libcamera, but it has\n> > failed synchronously, so the HAL should handle the failure. Still case\n> > number 1.\n> >\n> > - At this point, the request is sent to PipelineHandler::queueRequest(),\n> > asynchronously again. That function doesn't perform any sanity check\n> > (but it could in the future), and queues the request to the pipeline\n> > handler with a call to PipelineHandler:queueRequestDevice(),\n> > synchronously. This call can fail. If the future sanity checks in\n> > PipelineHandler::queueRequest() or\n> > PipelineHandler:queueRequestDevice() fail, the request belongs to the\n> > libcamera core (the HAL has queued it successfully, and the pipeline\n> > handler hasn't accepted it). This is case number 2, and I believe this\n> > is the one that has triggered this patch.\n> >\n> > - The pipeline handler has accepted the request, but something fails\n> > later. That's case number 3.\n> >\n> > Now let's look at those three cases, in reverse order.\n> >\n> > 3. The request belongs to the pipeline handler, which must complete it.\n> > We have support in the PipelineHandler base class to accept completion\n> > of requests out of sequence, and reorder the completion events to the\n> > HAL to guarantee they will be issued in sequence. The pipeline handler\n> > can thus complete the request with an error state as soon as it detects\n> > the error, and the HAL will eventually receive a requestComplete() call.\n> > As far as I can tell, this is working correctly.\n> >\n> > 2. This is embarassing, we don't handle this at all.\n> > PipelineHandler::queueRequest() returns an error code, but it never\n> > reaches Camera::queueReuest() as the call to\n> > PipelineHandler::queueRequest() is asynchronous. We should make\n> > PipelineHandler::queueRequest() a void function to be less misleading,\n> > and PipelineHandler::queueRequest() needs to signal request completion\n> > in that case. I think we can reuse the same request completion ordering\n> > mechanism and immediately mark the request as complete (with an error)\n> > in PipelineHandler::queueRequest() in that case. It should be an easy\n> > fix (famous last words... :-)).\n> >\n> > 1. This is the responsibility of the HAL, which should signal request\n> > completion to the camera service. Depending on whether the camera\n> > service can support out-of-order completion or not, we can signal\n> > completion immediately, or need to keep the request in a queue and\n> > complete it at the appropriate time.\n> >\n> > As far as I understand, this patch is meant to address the memory leak\n> > related to case 2, and also addresses as a consequence the leak related\n> > to case 1 (case 3 should be working correctly already, if I'm not\n> > mistaken). For case 2 I think the correct fix should be to ensure that\n> > libcamera will complete the request, as explained above. For case 1, we\n> > need a specific fix in the HAL, not only for the memory leak, but to\n> > also complete the request towards the camera service.\n> >\n> > Is this analysis correct, or am I missing something ?\n> \n> Thanks for analyzing the details. Yes your analysis looks correct.\n> Well, I expect this patch is to retain memory leakage in any cases.\n> I mean that no memory is leaked due to any failures.\n> \n> I agree that the failure 1 should be handled properly.\n> The fix to failure 2 also sounds good to me.\n\nThanks for confirming. Do you plan to look into addressing issues 1\nand/or 2 ?\n\n> > > > + if (node.empty()) {\n> > > > + LOG(HAL, Error) << \"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> > > > @@ -1915,7 +1922,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> > > > @@ -1924,10 +1931,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> > > > @@ -1942,7 +1949,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> > > > @@ -1959,17 +1966,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> > > > @@ -1982,13 +1989,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 11bdfec8..54d7c319 100644\n> > > > --- a/src/android/camera_device.h\n> > > > +++ b/src/android/camera_device.h\n> > > > @@ -69,9 +69,11 @@ private:\n> > > > CameraDevice(unsigned int id, std::shared_ptr camera);\n> > > >\n> > > > struct Camera3RequestDescriptor {\n> > > > + Camera3RequestDescriptor();\n> > > > + ~Camera3RequestDescriptor();\n> > > > Camera3RequestDescriptor(libcamera::Camera *camera,\n> > > > const camera3_capture_request_t *camera3Request);\n> > > > - ~Camera3RequestDescriptor();\n> > > > + Camera3RequestDescriptor& operator=(Camera3RequestDescriptor&&);\n> > > >\n> > > > uint32_t frameNumber_;\n> > > > std::vector buffers_;\n> > > > @@ -122,6 +124,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(); }","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 A551AC32ED\n\tfor ;\n\tSun, 28 Mar 2021 22:17:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2672F6877F;\n\tMon, 29 Mar 2021 00:17:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3BC9F602D7\n\tfor ;\n\tMon, 29 Mar 2021 00:17:46 +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 97DCA8C4;\n\tMon, 29 Mar 2021 00:17:45 +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=\"F1Vv7tGX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616969865;\n\tbh=swdA72W/x/seRlGCqB+oNzsnYU3X0WItKC7yCAUy8nk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=F1Vv7tGXXR4c9lLzlPh5RuuyUi+wkJ56L1U6hnhnHbltmS5M4Md771Wh89KxZdy/i\n\t4Yufi9DpXx7db7Hb08KYf2XzI3AOwoYmsGJCh3VxekstDyHHL8dV+x0Gdrvw8ibD+r\n\tQlfknYQBZ8jTS8QaV7OT33ZJOScpWem4LtiuTeCA=","Date":"Mon, 29 Mar 2021 01:17:01 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210325111357.3862847-1-hiroh@chromium.org>\n\t<20210325111357.3862847-2-hiroh@chromium.org>\n\t<20210325160243.yo67ycwa6zaooxoy@uno.localdomain>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15975,"web_url":"https://patchwork.libcamera.org/comment/15975/","msgid":"","date":"2021-03-28T22:24:02","subject":"Re: [libcamera-devel] [PATCH 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":"Hi Laurent,\n\nOn Mon, Mar 29, 2021 at 7:17 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Mon, Mar 29, 2021 at 07:10:27AM +0900, Hirokazu Honda wrote:\n> > Hi Jacopo and Laurent, thanks for reviewing.\n> >\n> > On Fri, Mar 26, 2021 at 2:22 AM Laurent Pinchartwrote:\n> > >\n> > > Hi Hiro,\n> > >\n> > > Thank you for the patch, it's nice to see the HAL being cleaned up :-)\n> > >\n> > > On Thu, Mar 25, 2021 at 05:02:43PM +0100, Jacopo Mondi wrote:\n> > > > On Thu, Mar 25, 2021 at 08:13:57PM +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 CamerarRequestDescriptor.\n> > >\n> > > s/CamerarRequestDescriptor/CameraRequestDescriptor/\n> > >\n> > > When does this happen ? You've reported issues with the IPU3 pipeline\n> > > handler failing to queue a request due to fact that it has no internal\n> > > queue of requests waiting for hardware resources to be available. Is\n> > > that what you're addressing here, or is there something else ? The\n> > > libcamera core should guarantee that requestComplete() gets called for\n> > > all requests successfully queued, so if there are other leakages, I'd\n> > > like to investigate them.\n> >\n> > Since a libcamera process is likely alive as long as a system runs, I\n> > would like to avoid any potential leakage as much as possible.\n>\n> We agree on this :-)\n>\n> > Regardless of the issue I've reported, I consider it is good to\n> > guarantee the descriptor is destroyed at the latest in CameraDevice\n> > descriptor.\n> > As you said, requestComplete() should be returned and I haven't found\n> > any other issues of not invoking it than the issue I reported, but I\n> > think it isn't theoretically ensured.\n>\n> My point was that, as this isn't supposed to happen in the first place,\n> I want to make sure we investigate any occurrence of the issue you may\n> have been able to trigger. There's one particular problem you've\n> reported that can lead this this leakage, and that will be addressed on\n> its own, and I was wondering if there were any other. You're confirming\n> that you haven't found any other, so I'm happy :-)\n>\n> > > > Very nice, I'm not particularly proud of this part of the\n> > > > implementation as it is quite hard to follow, this makes it easier\n> > > > indeed\n> > > >\n> > > > One question below\n> > > >\n> > > > >\n> > > > > Signed-off-by: Hirokazu Honda \n> > > > > ---\n> > > > > src/android/camera_device.cpp | 67 +++++++++++++++++++----------------\n> > > > > src/android/camera_device.h | 6 +++-\n> > > > > src/android/camera_worker.cpp | 4 +--\n> > > > > src/android/camera_worker.h | 2 +-\n> > > > > 4 files changed, 44 insertions(+), 35 deletions(-)\n> > > > >\n> > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > index ae693664..50b017a3 100644\n> > > > > --- a/src/android/camera_device.cpp\n> > > > > +++ b/src/android/camera_device.cpp\n> > > > > @@ -287,15 +287,17 @@ 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> > > > > CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> > > > >\n> > > > > +CameraDevice::Camera3RequestDescriptor&\n> > > > > +CameraDevice::Camera3RequestDescriptor::operator=(Camera3RequestDescriptor&&) = default;\n> > > > > +\n> > > > > /*\n> > > > > * \\class CameraDevice\n> > > > > *\n> > > > > @@ -1811,8 +1813,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> > > > > @@ -1822,12 +1824,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> > > > > @@ -1860,7 +1862,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> > > > > @@ -1879,11 +1881,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> > > > > @@ -1891,11 +1892,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> > > > > @@ -1905,8 +1907,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> > > >\n> > > > So once node goes out of scope the mapped descriptor gets deleted.\n> > > > Very nice.\n> > > >\n> > > > However, shouldn't descriptors_ be cleared at stop() time or when a\n> > > > new stream configuration is requested ?\n> >\n> > descriptors_ is cleared thanks to std::map destructor, isn't it?\n> > I think it is also true to ought to clear when a new stream\n> > configuration is requested from the following camera3 API statement,\n> >\n> > > Add signal_stream_flush() to camera3_device_ops_t for camera service to notify HAL an\n> > > upcoming configure_streams() call requires HAL to return buffers of certain streams.\n> > https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera3.h#194\n> >\n> > > I'm wondering if we shouldn't go one step forward. I'll assume here that\n> > > this patch is dealing with failures to queue requests when calling\n> > > Camera::queueRequest(). If there are other issues, the comments below\n> > > may not apply to them.\n> > >\n> > > When queuing a request to libcamera, we call\n> > > CameraWorker::queueRequest(). It's a void function that invokes\n> > > Worker::processRequest() as a queued call, so it returns immediately,\n> > > and we can't know if Worker::processRequest() will fail or not. We have\n> > > thus sent the request onto a journey, hoping it will be successfully\n> > > queued. There can be multiple reasons that won't be the case:\n> > >\n> > > - CameraWorker::Worker::processRequest() failing to wait for a fence.\n> > > Oops, shouldn't happen, but we still need to handle this gracefully.\n> > > We're still in the HAL here, the request hasn't reached libcamera. The\n> > > HAL should thus handle the failure. This is case number 1.\n> > >\n> > > - CameraWorker::Worker::processRequest() calls CaptureRequest::queue(),\n> > > which calls Camera::queueRequest(). That function can return an error\n> > > if some of the sanity checks fail. We've reached libcamera, but it has\n> > > failed synchronously, so the HAL should handle the failure. Still case\n> > > number 1.\n> > >\n> > > - At this point, the request is sent to PipelineHandler::queueRequest(),\n> > > asynchronously again. That function doesn't perform any sanity check\n> > > (but it could in the future), and queues the request to the pipeline\n> > > handler with a call to PipelineHandler:queueRequestDevice(),\n> > > synchronously. This call can fail. If the future sanity checks in\n> > > PipelineHandler::queueRequest() or\n> > > PipelineHandler:queueRequestDevice() fail, the request belongs to the\n> > > libcamera core (the HAL has queued it successfully, and the pipeline\n> > > handler hasn't accepted it). This is case number 2, and I believe this\n> > > is the one that has triggered this patch.\n> > >\n> > > - The pipeline handler has accepted the request, but something fails\n> > > later. That's case number 3.\n> > >\n> > > Now let's look at those three cases, in reverse order.\n> > >\n> > > 3. The request belongs to the pipeline handler, which must complete it.\n> > > We have support in the PipelineHandler base class to accept completion\n> > > of requests out of sequence, and reorder the completion events to the\n> > > HAL to guarantee they will be issued in sequence. The pipeline handler\n> > > can thus complete the request with an error state as soon as it detects\n> > > the error, and the HAL will eventually receive a requestComplete() call.\n> > > As far as I can tell, this is working correctly.\n> > >\n> > > 2. This is embarassing, we don't handle this at all.\n> > > PipelineHandler::queueRequest() returns an error code, but it never\n> > > reaches Camera::queueReuest() as the call to\n> > > PipelineHandler::queueRequest() is asynchronous. We should make\n> > > PipelineHandler::queueRequest() a void function to be less misleading,\n> > > and PipelineHandler::queueRequest() needs to signal request completion\n> > > in that case. I think we can reuse the same request completion ordering\n> > > mechanism and immediately mark the request as complete (with an error)\n> > > in PipelineHandler::queueRequest() in that case. It should be an easy\n> > > fix (famous last words... :-)).\n> > >\n> > > 1. This is the responsibility of the HAL, which should signal request\n> > > completion to the camera service. Depending on whether the camera\n> > > service can support out-of-order completion or not, we can signal\n> > > completion immediately, or need to keep the request in a queue and\n> > > complete it at the appropriate time.\n> > >\n> > > As far as I understand, this patch is meant to address the memory leak\n> > > related to case 2, and also addresses as a consequence the leak related\n> > > to case 1 (case 3 should be working correctly already, if I'm not\n> > > mistaken). For case 2 I think the correct fix should be to ensure that\n> > > libcamera will complete the request, as explained above. For case 1, we\n> > > need a specific fix in the HAL, not only for the memory leak, but to\n> > > also complete the request towards the camera service.\n> > >\n> > > Is this analysis correct, or am I missing something ?\n> >\n> > Thanks for analyzing the details. Yes your analysis looks correct.\n> > Well, I expect this patch is to retain memory leakage in any cases.\n> > I mean that no memory is leaked due to any failures.\n> >\n> > I agree that the failure 1 should be handled properly.\n> > The fix to failure 2 also sounds good to me.\n>\n> Thanks for confirming. Do you plan to look into addressing issues 1\n> and/or 2 ?\n>\n\nSure. I will do it. Can we review and check in this patch while I am doing?\n\n-Hiro\n\n> > > > > + if (node.empty()) {\n> > > > > + LOG(HAL, Error) << \"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> > > > > @@ -1915,7 +1922,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> > > > > @@ -1924,10 +1931,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> > > > > @@ -1942,7 +1949,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> > > > > @@ -1959,17 +1966,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> > > > > @@ -1982,13 +1989,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 11bdfec8..54d7c319 100644\n> > > > > --- a/src/android/camera_device.h\n> > > > > +++ b/src/android/camera_device.h\n> > > > > @@ -69,9 +69,11 @@ private:\n> > > > > CameraDevice(unsigned int id, std::shared_ptr camera);\n> > > > >\n> > > > > struct Camera3RequestDescriptor {\n> > > > > + Camera3RequestDescriptor();\n> > > > > + ~Camera3RequestDescriptor();\n> > > > > Camera3RequestDescriptor(libcamera::Camera *camera,\n> > > > > const camera3_capture_request_t *camera3Request);\n> > > > > - ~Camera3RequestDescriptor();\n> > > > > + Camera3RequestDescriptor& operator=(Camera3RequestDescriptor&&);\n> > > > >\n> > > > > uint32_t frameNumber_;\n> > > > > std::vector buffers_;\n> > > > > @@ -122,6 +124,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> --\n> Regards,\n>\n> Laurent Pinchart","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 03BF4C32EA\n\tfor ;\n\tSun, 28 Mar 2021 22:24:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 537DD6877F;\n\tMon, 29 Mar 2021 00:24:16 +0200 (CEST)","from mail-ej1-x631.google.com (mail-ej1-x631.google.com\n\t[IPv6:2a00:1450:4864:20::631])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 70883602D7\n\tfor ;\n\tMon, 29 Mar 2021 00:24:14 +0200 (CEST)","by mail-ej1-x631.google.com with SMTP id kt15so16494981ejb.12\n\tfor ;\n\tSun, 28 Mar 2021 15:24:14 -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=\"hkEEyM/O\"; 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=M1Hu9vX1KY8G+oXIxJtyKicpQNSBniUV/A8LRdhreJE=;\n\tb=hkEEyM/OLRsmDrB4gGwZ4GpkKtA7NLNrCBMbmK1JqsixR8/tmhVw5uLCUR4Hn2igAu\n\taIEfq/xXqpjHGRqzEO/9UnrgU4k2J1W52MOkM/0ok8q3LhEXVh/mtd7qSWdLSZdjVcaW\n\tv3iOq4hbFTWcVCO8Mz55dUMzg/lVVCICQXjmM=","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=M1Hu9vX1KY8G+oXIxJtyKicpQNSBniUV/A8LRdhreJE=;\n\tb=mX1suPN3gAkI1Q/dSPNQAkknY+Np1Rt5tPAi1SQxDaLkPlAY8MecaneNGpqXPBXmf3\n\tDywi+uN97mJk6gvIKUhwOPI9E8/sBhahhsZOudURaWVBcmmOyopWoR5lE3NSpz7KMeZ9\n\tUCjPPWczgP7I+R+SrjHmcV723RTlb+yyqG5YH7sV3jlZ8YXjHutj3rFk1yyMkosouzbp\n\tbxxPRG/wE0G8XLaNfpk0Pah4QWcp/h9naT3biMu8VRbU29imycjqAoY8O4LCjUzH43oa\n\tvvtXUAyRXwh/dZenBLRPUEdBnalkuX4mYBB5+yH0jTSZ5La4bdkZIEh4m4+8oK0yN0O5\n\t+TLw==","X-Gm-Message-State":"AOAM531Ec0h5beSmwCAfv6DSTksneRMkeYhW6s9fceq9VYbXfXeWDf2Z\n\tC7dxci28TLueBE5/fsd7I94ipcII5gvvxdm+OzJwqQmAEdw=","X-Google-Smtp-Source":"ABdhPJzCCd++rh3KC8ZH5/YOpUOh0tQewOFNLGBuW/5kj/DszQpZZRf4855WfzlEtOYkOfPUHJONIKEFupsSHiwhKiM=","X-Received":"by 2002:a17:906:701:: with SMTP id\n\ty1mr651420ejb.243.1616970253875; \n\tSun, 28 Mar 2021 15:24:13 -0700 (PDT)","MIME-Version":"1.0","References":"<20210325111357.3862847-1-hiroh@chromium.org>\n\t<20210325111357.3862847-2-hiroh@chromium.org>\n\t<20210325160243.yo67ycwa6zaooxoy@uno.localdomain>\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 29 Mar 2021 07:24:02 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH 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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15994,"web_url":"https://patchwork.libcamera.org/comment/15994/","msgid":"","date":"2021-03-29T05:35:21","subject":"Re: [libcamera-devel] [PATCH 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\nOn Mon, Mar 29, 2021 at 07:24:02AM +0900, Hirokazu Honda wrote:\n> On Mon, Mar 29, 2021 at 7:17 AM Laurent Pinchart wrote:\n> > On Mon, Mar 29, 2021 at 07:10:27AM +0900, Hirokazu Honda wrote:\n> > > On Fri, Mar 26, 2021 at 2:22 AM Laurent Pinchartwrote:\n> > > > On Thu, Mar 25, 2021 at 05:02:43PM +0100, Jacopo Mondi wrote:\n> > > > > On Thu, Mar 25, 2021 at 08:13:57PM +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 CamerarRequestDescriptor.\n> > > >\n> > > > s/CamerarRequestDescriptor/CameraRequestDescriptor/\n> > > >\n> > > > When does this happen ? You've reported issues with the IPU3 pipeline\n> > > > handler failing to queue a request due to fact that it has no internal\n> > > > queue of requests waiting for hardware resources to be available. Is\n> > > > that what you're addressing here, or is there something else ? The\n> > > > libcamera core should guarantee that requestComplete() gets called for\n> > > > all requests successfully queued, so if there are other leakages, I'd\n> > > > like to investigate them.\n> > >\n> > > Since a libcamera process is likely alive as long as a system runs, I\n> > > would like to avoid any potential leakage as much as possible.\n> >\n> > We agree on this :-)\n> >\n> > > Regardless of the issue I've reported, I consider it is good to\n> > > guarantee the descriptor is destroyed at the latest in CameraDevice\n> > > descriptor.\n> > > As you said, requestComplete() should be returned and I haven't found\n> > > any other issues of not invoking it than the issue I reported, but I\n> > > think it isn't theoretically ensured.\n> >\n> > My point was that, as this isn't supposed to happen in the first place,\n> > I want to make sure we investigate any occurrence of the issue you may\n> > have been able to trigger. There's one particular problem you've\n> > reported that can lead this this leakage, and that will be addressed on\n> > its own, and I was wondering if there were any other. You're confirming\n> > that you haven't found any other, so I'm happy :-)\n> >\n> > > > > Very nice, I'm not particularly proud of this part of the\n> > > > > implementation as it is quite hard to follow, this makes it easier\n> > > > > indeed\n> > > > >\n> > > > > One question below\n> > > > >\n> > > > > >\n> > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > ---\n> > > > > > src/android/camera_device.cpp | 67 +++++++++++++++++++----------------\n> > > > > > src/android/camera_device.h | 6 +++-\n> > > > > > src/android/camera_worker.cpp | 4 +--\n> > > > > > src/android/camera_worker.h | 2 +-\n> > > > > > 4 files changed, 44 insertions(+), 35 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > > index ae693664..50b017a3 100644\n> > > > > > --- a/src/android/camera_device.cpp\n> > > > > > +++ b/src/android/camera_device.cpp\n> > > > > > @@ -287,15 +287,17 @@ 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> > > > > > CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> > > > > >\n> > > > > > +CameraDevice::Camera3RequestDescriptor&\n> > > > > > +CameraDevice::Camera3RequestDescriptor::operator=(Camera3RequestDescriptor&&) = default;\n> > > > > > +\n> > > > > > /*\n> > > > > > * \\class CameraDevice\n> > > > > > *\n> > > > > > @@ -1811,8 +1813,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> > > > > > @@ -1822,12 +1824,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> > > > > > @@ -1860,7 +1862,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> > > > > > @@ -1879,11 +1881,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> > > > > > @@ -1891,11 +1892,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> > > > > > @@ -1905,8 +1907,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> > > > >\n> > > > > So once node goes out of scope the mapped descriptor gets deleted.\n> > > > > Very nice.\n> > > > >\n> > > > > However, shouldn't descriptors_ be cleared at stop() time or when a\n> > > > > new stream configuration is requested ?\n> > >\n> > > descriptors_ is cleared thanks to std::map destructor, isn't it?\n> > > I think it is also true to ought to clear when a new stream\n> > > configuration is requested from the following camera3 API statement,\n> > >\n> > > > Add signal_stream_flush() to camera3_device_ops_t for camera service to notify HAL an\n> > > > upcoming configure_streams() call requires HAL to return buffers of certain streams.\n> > > https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera3.h#194\n> > >\n> > > > I'm wondering if we shouldn't go one step forward. I'll assume here that\n> > > > this patch is dealing with failures to queue requests when calling\n> > > > Camera::queueRequest(). If there are other issues, the comments below\n> > > > may not apply to them.\n> > > >\n> > > > When queuing a request to libcamera, we call\n> > > > CameraWorker::queueRequest(). It's a void function that invokes\n> > > > Worker::processRequest() as a queued call, so it returns immediately,\n> > > > and we can't know if Worker::processRequest() will fail or not. We have\n> > > > thus sent the request onto a journey, hoping it will be successfully\n> > > > queued. There can be multiple reasons that won't be the case:\n> > > >\n> > > > - CameraWorker::Worker::processRequest() failing to wait for a fence.\n> > > > Oops, shouldn't happen, but we still need to handle this gracefully.\n> > > > We're still in the HAL here, the request hasn't reached libcamera. The\n> > > > HAL should thus handle the failure. This is case number 1.\n> > > >\n> > > > - CameraWorker::Worker::processRequest() calls CaptureRequest::queue(),\n> > > > which calls Camera::queueRequest(). That function can return an error\n> > > > if some of the sanity checks fail. We've reached libcamera, but it has\n> > > > failed synchronously, so the HAL should handle the failure. Still case\n> > > > number 1.\n> > > >\n> > > > - At this point, the request is sent to PipelineHandler::queueRequest(),\n> > > > asynchronously again. That function doesn't perform any sanity check\n> > > > (but it could in the future), and queues the request to the pipeline\n> > > > handler with a call to PipelineHandler:queueRequestDevice(),\n> > > > synchronously. This call can fail. If the future sanity checks in\n> > > > PipelineHandler::queueRequest() or\n> > > > PipelineHandler:queueRequestDevice() fail, the request belongs to the\n> > > > libcamera core (the HAL has queued it successfully, and the pipeline\n> > > > handler hasn't accepted it). This is case number 2, and I believe this\n> > > > is the one that has triggered this patch.\n> > > >\n> > > > - The pipeline handler has accepted the request, but something fails\n> > > > later. That's case number 3.\n> > > >\n> > > > Now let's look at those three cases, in reverse order.\n> > > >\n> > > > 3. The request belongs to the pipeline handler, which must complete it.\n> > > > We have support in the PipelineHandler base class to accept completion\n> > > > of requests out of sequence, and reorder the completion events to the\n> > > > HAL to guarantee they will be issued in sequence. The pipeline handler\n> > > > can thus complete the request with an error state as soon as it detects\n> > > > the error, and the HAL will eventually receive a requestComplete() call.\n> > > > As far as I can tell, this is working correctly.\n> > > >\n> > > > 2. This is embarassing, we don't handle this at all.\n> > > > PipelineHandler::queueRequest() returns an error code, but it never\n> > > > reaches Camera::queueReuest() as the call to\n> > > > PipelineHandler::queueRequest() is asynchronous. We should make\n> > > > PipelineHandler::queueRequest() a void function to be less misleading,\n> > > > and PipelineHandler::queueRequest() needs to signal request completion\n> > > > in that case. I think we can reuse the same request completion ordering\n> > > > mechanism and immediately mark the request as complete (with an error)\n> > > > in PipelineHandler::queueRequest() in that case. It should be an easy\n> > > > fix (famous last words... :-)).\n> > > >\n> > > > 1. This is the responsibility of the HAL, which should signal request\n> > > > completion to the camera service. Depending on whether the camera\n> > > > service can support out-of-order completion or not, we can signal\n> > > > completion immediately, or need to keep the request in a queue and\n> > > > complete it at the appropriate time.\n> > > >\n> > > > As far as I understand, this patch is meant to address the memory leak\n> > > > related to case 2, and also addresses as a consequence the leak related\n> > > > to case 1 (case 3 should be working correctly already, if I'm not\n> > > > mistaken). For case 2 I think the correct fix should be to ensure that\n> > > > libcamera will complete the request, as explained above. For case 1, we\n> > > > need a specific fix in the HAL, not only for the memory leak, but to\n> > > > also complete the request towards the camera service.\n> > > >\n> > > > Is this analysis correct, or am I missing something ?\n> > >\n> > > Thanks for analyzing the details. Yes your analysis looks correct.\n> > > Well, I expect this patch is to retain memory leakage in any cases.\n> > > I mean that no memory is leaked due to any failures.\n> > >\n> > > I agree that the failure 1 should be handled properly.\n> > > The fix to failure 2 also sounds good to me.\n> >\n> > Thanks for confirming. Do you plan to look into addressing issues 1\n> > and/or 2 ?\n> \n> Sure. I will do it. Can we review and check in this patch while I am doing?\n\nOverall I think the patch is OK, but could we clear the descriptors_ map\nwhen Camera::stop() is called in CameraDevice::configureStreams() ? I\nwould create a new private CameraDevice::stop() function:\n\nvoid CameraDevice::stop()\n{\n\tworker_.stop();\n\tcamera_->stop();\n\n\tdescriptors_.clear();\n\trunning_ = false;\n}\n\nand call it from both CameraDevice::close() and\nCameraDevice::configureStreams().\n\n> > > > > > + if (node.empty()) {\n> > > > > > + LOG(HAL, Error) << \"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> > > > > > @@ -1915,7 +1922,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> > > > > > @@ -1924,10 +1931,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> > > > > > @@ -1942,7 +1949,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> > > > > > @@ -1959,17 +1966,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> > > > > > @@ -1982,13 +1989,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 11bdfec8..54d7c319 100644\n> > > > > > --- a/src/android/camera_device.h\n> > > > > > +++ b/src/android/camera_device.h\n> > > > > > @@ -69,9 +69,11 @@ private:\n> > > > > > CameraDevice(unsigned int id, std::shared_ptr camera);\n> > > > > >\n> > > > > > struct Camera3RequestDescriptor {\n> > > > > > + Camera3RequestDescriptor();\n> > > > > > + ~Camera3RequestDescriptor();\n> > > > > > Camera3RequestDescriptor(libcamera::Camera *camera,\n> > > > > > const camera3_capture_request_t *camera3Request);\n> > > > > > - ~Camera3RequestDescriptor();\n> > > > > > + Camera3RequestDescriptor& operator=(Camera3RequestDescriptor&&);\n> > > > > >\n> > > > > > uint32_t frameNumber_;\n> > > > > > std::vector buffers_;\n> > > > > > @@ -122,6 +124,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(); }","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 903DBC32EA\n\tfor ;\n\tMon, 29 Mar 2021 05:36:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DF16268783;\n\tMon, 29 Mar 2021 07:36:06 +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 B9C9A602D2\n\tfor ;\n\tMon, 29 Mar 2021 07:36:05 +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 051CA31A;\n\tMon, 29 Mar 2021 07:36:04 +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=\"SSTDICA+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616996165;\n\tbh=AXYTM8CDBjIBfvoS0QdDEcyfbwsHLgwHpBvWZcTsDeA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SSTDICA+l5saZmVmzkE4jC8bTLrQV2JKg1ExQSBpo8bFlwxBBgEliq8NYDv8QKQXZ\n\t/amCTzVSVanbXT4kjk5SH/M8Yi9aHKNV5L1VsB/9I3nN+s+tpkex5EtGP3OWvynwEg\n\tEJMiwiAaLpePGNkFIlFC2fsd1QdUIUzHz3xtnYls=","Date":"Mon, 29 Mar 2021 08:35:21 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210325111357.3862847-1-hiroh@chromium.org>\n\t<20210325111357.3862847-2-hiroh@chromium.org>\n\t<20210325160243.yo67ycwa6zaooxoy@uno.localdomain>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15995,"web_url":"https://patchwork.libcamera.org/comment/15995/","msgid":"","date":"2021-03-29T06:12:02","subject":"Re: [libcamera-devel] [PATCH 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":"Hi Laurent,\n\nOn Mon, Mar 29, 2021 at 2:36 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Mon, Mar 29, 2021 at 07:24:02AM +0900, Hirokazu Honda wrote:\n> > On Mon, Mar 29, 2021 at 7:17 AM Laurent Pinchart wrote:\n> > > On Mon, Mar 29, 2021 at 07:10:27AM +0900, Hirokazu Honda wrote:\n> > > > On Fri, Mar 26, 2021 at 2:22 AM Laurent Pinchartwrote:\n> > > > > On Thu, Mar 25, 2021 at 05:02:43PM +0100, Jacopo Mondi wrote:\n> > > > > > On Thu, Mar 25, 2021 at 08:13:57PM +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 CamerarRequestDescriptor.\n> > > > >\n> > > > > s/CamerarRequestDescriptor/CameraRequestDescriptor/\n> > > > >\n> > > > > When does this happen ? You've reported issues with the IPU3 pipeline\n> > > > > handler failing to queue a request due to fact that it has no internal\n> > > > > queue of requests waiting for hardware resources to be available. Is\n> > > > > that what you're addressing here, or is there something else ? The\n> > > > > libcamera core should guarantee that requestComplete() gets called for\n> > > > > all requests successfully queued, so if there are other leakages, I'd\n> > > > > like to investigate them.\n> > > >\n> > > > Since a libcamera process is likely alive as long as a system runs, I\n> > > > would like to avoid any potential leakage as much as possible.\n> > >\n> > > We agree on this :-)\n> > >\n> > > > Regardless of the issue I've reported, I consider it is good to\n> > > > guarantee the descriptor is destroyed at the latest in CameraDevice\n> > > > descriptor.\n> > > > As you said, requestComplete() should be returned and I haven't found\n> > > > any other issues of not invoking it than the issue I reported, but I\n> > > > think it isn't theoretically ensured.\n> > >\n> > > My point was that, as this isn't supposed to happen in the first place,\n> > > I want to make sure we investigate any occurrence of the issue you may\n> > > have been able to trigger. There's one particular problem you've\n> > > reported that can lead this this leakage, and that will be addressed on\n> > > its own, and I was wondering if there were any other. You're confirming\n> > > that you haven't found any other, so I'm happy :-)\n> > >\n> > > > > > Very nice, I'm not particularly proud of this part of the\n> > > > > > implementation as it is quite hard to follow, this makes it easier\n> > > > > > indeed\n> > > > > >\n> > > > > > One question below\n> > > > > >\n> > > > > > >\n> > > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > > ---\n> > > > > > > src/android/camera_device.cpp | 67 +++++++++++++++++++----------------\n> > > > > > > src/android/camera_device.h | 6 +++-\n> > > > > > > src/android/camera_worker.cpp | 4 +--\n> > > > > > > src/android/camera_worker.h | 2 +-\n> > > > > > > 4 files changed, 44 insertions(+), 35 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > > > > index ae693664..50b017a3 100644\n> > > > > > > --- a/src/android/camera_device.cpp\n> > > > > > > +++ b/src/android/camera_device.cpp\n> > > > > > > @@ -287,15 +287,17 @@ 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> > > > > > > CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> > > > > > >\n> > > > > > > +CameraDevice::Camera3RequestDescriptor&\n> > > > > > > +CameraDevice::Camera3RequestDescriptor::operator=(Camera3RequestDescriptor&&) = default;\n> > > > > > > +\n> > > > > > > /*\n> > > > > > > * \\class CameraDevice\n> > > > > > > *\n> > > > > > > @@ -1811,8 +1813,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> > > > > > > @@ -1822,12 +1824,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> > > > > > > @@ -1860,7 +1862,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> > > > > > > @@ -1879,11 +1881,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> > > > > > > @@ -1891,11 +1892,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> > > > > > > @@ -1905,8 +1907,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> > > > > >\n> > > > > > So once node goes out of scope the mapped descriptor gets deleted.\n> > > > > > Very nice.\n> > > > > >\n> > > > > > However, shouldn't descriptors_ be cleared at stop() time or when a\n> > > > > > new stream configuration is requested ?\n> > > >\n> > > > descriptors_ is cleared thanks to std::map destructor, isn't it?\n> > > > I think it is also true to ought to clear when a new stream\n> > > > configuration is requested from the following camera3 API statement,\n> > > >\n> > > > > Add signal_stream_flush() to camera3_device_ops_t for camera service to notify HAL an\n> > > > > upcoming configure_streams() call requires HAL to return buffers of certain streams.\n> > > > https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera3.h#194\n> > > >\n> > > > > I'm wondering if we shouldn't go one step forward. I'll assume here that\n> > > > > this patch is dealing with failures to queue requests when calling\n> > > > > Camera::queueRequest(). If there are other issues, the comments below\n> > > > > may not apply to them.\n> > > > >\n> > > > > When queuing a request to libcamera, we call\n> > > > > CameraWorker::queueRequest(). It's a void function that invokes\n> > > > > Worker::processRequest() as a queued call, so it returns immediately,\n> > > > > and we can't know if Worker::processRequest() will fail or not. We have\n> > > > > thus sent the request onto a journey, hoping it will be successfully\n> > > > > queued. There can be multiple reasons that won't be the case:\n> > > > >\n> > > > > - CameraWorker::Worker::processRequest() failing to wait for a fence.\n> > > > > Oops, shouldn't happen, but we still need to handle this gracefully.\n> > > > > We're still in the HAL here, the request hasn't reached libcamera. The\n> > > > > HAL should thus handle the failure. This is case number 1.\n> > > > >\n> > > > > - CameraWorker::Worker::processRequest() calls CaptureRequest::queue(),\n> > > > > which calls Camera::queueRequest(). That function can return an error\n> > > > > if some of the sanity checks fail. We've reached libcamera, but it has\n> > > > > failed synchronously, so the HAL should handle the failure. Still case\n> > > > > number 1.\n> > > > >\n> > > > > - At this point, the request is sent to PipelineHandler::queueRequest(),\n> > > > > asynchronously again. That function doesn't perform any sanity check\n> > > > > (but it could in the future), and queues the request to the pipeline\n> > > > > handler with a call to PipelineHandler:queueRequestDevice(),\n> > > > > synchronously. This call can fail. If the future sanity checks in\n> > > > > PipelineHandler::queueRequest() or\n> > > > > PipelineHandler:queueRequestDevice() fail, the request belongs to the\n> > > > > libcamera core (the HAL has queued it successfully, and the pipeline\n> > > > > handler hasn't accepted it). This is case number 2, and I believe this\n> > > > > is the one that has triggered this patch.\n> > > > >\n> > > > > - The pipeline handler has accepted the request, but something fails\n> > > > > later. That's case number 3.\n> > > > >\n> > > > > Now let's look at those three cases, in reverse order.\n> > > > >\n> > > > > 3. The request belongs to the pipeline handler, which must complete it.\n> > > > > We have support in the PipelineHandler base class to accept completion\n> > > > > of requests out of sequence, and reorder the completion events to the\n> > > > > HAL to guarantee they will be issued in sequence. The pipeline handler\n> > > > > can thus complete the request with an error state as soon as it detects\n> > > > > the error, and the HAL will eventually receive a requestComplete() call.\n> > > > > As far as I can tell, this is working correctly.\n> > > > >\n> > > > > 2. This is embarassing, we don't handle this at all.\n> > > > > PipelineHandler::queueRequest() returns an error code, but it never\n> > > > > reaches Camera::queueReuest() as the call to\n> > > > > PipelineHandler::queueRequest() is asynchronous. We should make\n> > > > > PipelineHandler::queueRequest() a void function to be less misleading,\n> > > > > and PipelineHandler::queueRequest() needs to signal request completion\n> > > > > in that case. I think we can reuse the same request completion ordering\n> > > > > mechanism and immediately mark the request as complete (with an error)\n> > > > > in PipelineHandler::queueRequest() in that case. It should be an easy\n> > > > > fix (famous last words... :-)).\n> > > > >\n> > > > > 1. This is the responsibility of the HAL, which should signal request\n> > > > > completion to the camera service. Depending on whether the camera\n> > > > > service can support out-of-order completion or not, we can signal\n> > > > > completion immediately, or need to keep the request in a queue and\n> > > > > complete it at the appropriate time.\n> > > > >\n> > > > > As far as I understand, this patch is meant to address the memory leak\n> > > > > related to case 2, and also addresses as a consequence the leak related\n> > > > > to case 1 (case 3 should be working correctly already, if I'm not\n> > > > > mistaken). For case 2 I think the correct fix should be to ensure that\n> > > > > libcamera will complete the request, as explained above. For case 1, we\n> > > > > need a specific fix in the HAL, not only for the memory leak, but to\n> > > > > also complete the request towards the camera service.\n> > > > >\n> > > > > Is this analysis correct, or am I missing something ?\n> > > >\n> > > > Thanks for analyzing the details. Yes your analysis looks correct.\n> > > > Well, I expect this patch is to retain memory leakage in any cases.\n> > > > I mean that no memory is leaked due to any failures.\n> > > >\n> > > > I agree that the failure 1 should be handled properly.\n> > > > The fix to failure 2 also sounds good to me.\n> > >\n> > > Thanks for confirming. Do you plan to look into addressing issues 1\n> > > and/or 2 ?\n> >\n> > Sure. I will do it. Can we review and check in this patch while I am doing?\n>\n> Overall I think the patch is OK, but could we clear the descriptors_ map\n> when Camera::stop() is called in CameraDevice::configureStreams() ? I\n> would create a new private CameraDevice::stop() function:\n>\n> void CameraDevice::stop()\n> {\n> worker_.stop();\n> camera_->stop();\n>\n> descriptors_.clear();\n> running_ = false;\n> }\n>\n> and call it from both CameraDevice::close() and\n> CameraDevice::configureStreams().\n>\n\nSounds good to me,\n-Hiro\n> > > > > > > + if (node.empty()) {\n> > > > > > > + LOG(HAL, Error) << \"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> > > > > > > @@ -1915,7 +1922,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> > > > > > > @@ -1924,10 +1931,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> > > > > > > @@ -1942,7 +1949,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> > > > > > > @@ -1959,17 +1966,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> > > > > > > @@ -1982,13 +1989,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 11bdfec8..54d7c319 100644\n> > > > > > > --- a/src/android/camera_device.h\n> > > > > > > +++ b/src/android/camera_device.h\n> > > > > > > @@ -69,9 +69,11 @@ private:\n> > > > > > > CameraDevice(unsigned int id, std::shared_ptr camera);\n> > > > > > >\n> > > > > > > struct Camera3RequestDescriptor {\n> > > > > > > + Camera3RequestDescriptor();\n> > > > > > > + ~Camera3RequestDescriptor();\n> > > > > > > Camera3RequestDescriptor(libcamera::Camera *camera,\n> > > > > > > const camera3_capture_request_t *camera3Request);\n> > > > > > > - ~Camera3RequestDescriptor();\n> > > > > > > + Camera3RequestDescriptor& operator=(Camera3RequestDescriptor&&);\n> > > > > > >\n> > > > > > > uint32_t frameNumber_;\n> > > > > > > std::vector buffers_;\n> > > > > > > @@ -122,6 +124,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> --\n> Regards,\n>\n> Laurent Pinchart","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 1D94FC32EF\n\tfor ;\n\tMon, 29 Mar 2021 06:12:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CD9A168783;\n\tMon, 29 Mar 2021 08:12:14 +0200 (CEST)","from mail-ej1-x634.google.com (mail-ej1-x634.google.com\n\t[IPv6:2a00:1450:4864:20::634])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 095AE602D2\n\tfor ;\n\tMon, 29 Mar 2021 08:12:14 +0200 (CEST)","by mail-ej1-x634.google.com with SMTP id w3so17592879ejc.4\n\tfor ;\n\tSun, 28 Mar 2021 23:12:14 -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=\"eUiKbArY\"; 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=F788FC8PPidaI/GLd5Ua6peVRLwzHl0rMH7DLU9Pcqg=;\n\tb=eUiKbArY4XjwXjVdUulRgYPmnMlvzkwWNmdNSOwlSeiUPhrq+w1LRUtid9ptxe9Mj0\n\tkSsMC6c9qwPZ5KHtDLZB70sQqFqX4TIZwmi8ClEb2MoHp1jEy6Y4coiLWHRXgnpqWFpW\n\tie+9Gi55yLvBtY8sCCb3Us8yJN7B60/ChZnfM=","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=F788FC8PPidaI/GLd5Ua6peVRLwzHl0rMH7DLU9Pcqg=;\n\tb=aFUrqILtXvWR7G1UdKamgEfiepxNfEkFmF/cP11ft/J6Cn2T0zViez01JXhWb7b2Ca\n\tmoEefLSN/qqpxrNopytw/iSbAOFf2xSbgMMDq5iOfr4BsiiVHwVEIsrYwO8pSKyp6KQj\n\ttM9H/53dF6hWEmlSwvQnSYq9iadQPMbywT2I4R+Y0bo4+Ok+07BJ6rg92B27B/FVmXgW\n\tijRCC7+SXIxxpKePkP/y5WrAFgR3GbMgJIF4U5lLaLgHknjEfi2t7yQuZy8k4+VeWjmg\n\tfIhtCfnfHLiHGKe+55mrZjAy0toJTc5MVUkGSLL2oiidAuZ6MEAQDtPhpVCgDw896w/G\n\tn2XQ==","X-Gm-Message-State":"AOAM530UxVtVULUCIP6IfvEpNsaYHhxD2WbN98N6jUF6SVTkR8NrHs9e\n\tebLYM48kPWm/o0UZT/42P8ww0+wSarKSJti14czTIw==","X-Google-Smtp-Source":"ABdhPJx/+2bk4sJTb2GtrTY2oknz9MZdY/9ZjwC+OrcjBL83ZjJpugv5BhkrtixpPGgJ9GgxmEgTL9k3DtO97UfjQNU=","X-Received":"by 2002:a17:906:4801:: with SMTP id\n\tw1mr27087218ejq.475.1616998333447; \n\tSun, 28 Mar 2021 23:12:13 -0700 (PDT)","MIME-Version":"1.0","References":"<20210325111357.3862847-1-hiroh@chromium.org>\n\t<20210325111357.3862847-2-hiroh@chromium.org>\n\t<20210325160243.yo67ycwa6zaooxoy@uno.localdomain>\n\t\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 29 Mar 2021 15:12:02 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH 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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]