[{"id":16009,"web_url":"https://patchwork.libcamera.org/comment/16009/","msgid":"<20210329091642.2jao7zx6jb3txylr@uno.localdomain>","date":"2021-03-29T09:16:42","subject":"Re: [libcamera-devel] [PATCH v3 1/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 Mon, Mar 29, 2021 at 03:11:18PM +0900, Hirokazu Honda wrote:\n> CameraDevice creates Camera3RequestDescriptor in\n> processCaptureRequest() and disallocates in requestComplete().\n> Camera3RequestDescriptor can never be destroyed if\n> requestComplete() is never called. This avoid the memory\n> leakage by storing them in map CameraRequestDescriptor.\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera_device.cpp | 67 +++++++++++++++++++----------------\n> src/android/camera_device.h | 8 +++--\n> src/android/camera_worker.cpp | 4 +--\n> src/android/camera_worker.h | 2 +-\n> 4 files changed, 45 insertions(+), 36 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index ae693664..02d3bfb2 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\nNit: I would initialize these with default in the header.\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> +\tif (node.empty()) {\n> +\t\tLOG(HAL, Error) << \"Unknown request: \" << request->cookie();\n\nIs this worth a 'Fatal' error as this condition should only be\ntriggered by a development error, or do you think it could happen at\nrun-time ?\n\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..5a2d22d6 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -69,11 +69,13 @@ 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\tuint32_t frameNumber_ = 0;\n\nUnrelated. Or do you need this ? Isn't frameNumber initialized with\nthe value in camera3_capture_request_t ?\n\n> \t\tstd::vector buffers_;\n> \t\tstd::vector> frameBuffers_;\n> \t\tCameraMetadata settings_;\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 B3C62C32EA\n\tfor ;\n\tMon, 29 Mar 2021 09:16:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7BD2C68780;\n\tMon, 29 Mar 2021 11:16:11 +0200 (CEST)","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 6387268787\n\tfor ;\n\tMon, 29 Mar 2021 11:16:08 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id CDCDE240002;\n\tMon, 29 Mar 2021 09:16:07 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 29 Mar 2021 11:16:42 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210329091642.2jao7zx6jb3txylr@uno.localdomain>","References":"<20210329061119.135513-1-hiroh@chromium.org>\n\t<20210329061119.135513-2-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210329061119.135513-2-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v3 1/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":16051,"web_url":"https://patchwork.libcamera.org/comment/16051/","msgid":"","date":"2021-03-30T05:30:41","subject":"Re: [libcamera-devel] [PATCH v3 1/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, thanks for reviewing.\n\nOn Mon, Mar 29, 2021 at 6:16 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Mon, Mar 29, 2021 at 03:11:18PM +0900, Hirokazu Honda wrote:\n> > CameraDevice creates Camera3RequestDescriptor in\n> > processCaptureRequest() and disallocates in requestComplete().\n> > Camera3RequestDescriptor can never be destroyed if\n> > requestComplete() is never called. This avoid the memory\n> > leakage by storing them in map CameraRequestDescriptor.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/camera_device.cpp | 67 +++++++++++++++++++----------------\n> > src/android/camera_device.h | 8 +++--\n> > src/android/camera_worker.cpp | 4 +--\n> > src/android/camera_worker.h | 2 +-\n> > 4 files changed, 45 insertions(+), 36 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index ae693664..02d3bfb2 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> Nit: I would initialize these with default in the header.\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> > + if (node.empty()) {\n> > + LOG(HAL, Error) << \"Unknown request: \" << request->cookie();\n>\n> Is this worth a 'Fatal' error as this condition should only be\n> triggered by a development error, or do you think it could happen at\n> run-time ?\n>\n\nRight, this should not happen unless a development error.\nChanged to Fatal.\n\n\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..5a2d22d6 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -69,11 +69,13 @@ private:\n> > CameraDevice(unsigned int id, std::shared_ptr camera);\n> >\n> > struct Camera3RequestDescriptor {\n> > + Camera3RequestDescriptor();\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> > + uint32_t frameNumber_ = 0;\n>\n> Unrelated. Or do you need this ? Isn't frameNumber initialized with\n> the value in camera3_capture_request_t ?\n>\n\nSince I have to introduce a default constructor sadly due to std::map,\nI would like to initialize this to 0 for default constructor, although\nit shall never be called by a user.\n\n-Hiro\n\n> > std::vector buffers_;\n> > std::vector> frameBuffers_;\n> > CameraMetadata settings_;\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> > 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 A33FAC32EA\n\tfor ;\n\tTue, 30 Mar 2021 05:30:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 14E9468782;\n\tTue, 30 Mar 2021 07:30:56 +0200 (CEST)","from mail-ed1-x532.google.com (mail-ed1-x532.google.com\n\t[IPv6:2a00:1450:4864:20::532])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B7B76602D7\n\tfor ;\n\tTue, 30 Mar 2021 07:30:54 +0200 (CEST)","by mail-ed1-x532.google.com with SMTP id h10so16663722edt.13\n\tfor ;\n\tMon, 29 Mar 2021 22:30:54 -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=\"HTwVVDUw\"; 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=nuCTuEQvbB2WAsWr0jd8FnstdaEDQoG/9dNHznes+lE=;\n\tb=HTwVVDUw6Fj9mVftwlFlTBq4tA0RI3uHfIm5vkR33gmReZ8SeBI9E7oaDC3a6oSQEQ\n\ty3ruuKGvGUhyIJjSMDw8/Y4ELdOvcpf4kdBUG/PP6QwHEViUauwZLnO0vnNSoMgwtIll\n\tPSBOFVD2fFgFsWX6PCmK4H3ylG3VaNDSn3Gbo=","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=nuCTuEQvbB2WAsWr0jd8FnstdaEDQoG/9dNHznes+lE=;\n\tb=q1liP3lg7iUKUCm80VY/6d98jr7dyFo9E/l9TN/ji1cQ4GE3Qq5YnWVXipnYOJCj0j\n\t7JNFkOdYsV93oPcG9O4Xtfg/o1STI01TrkTIz4MOGDUx/t5b5kXA5qw9AbwSW08C1PVu\n\tlMVpQ6VH+RLzX+9vVOju/ZAA1xFV9XRLORTegrDTPd4qQvjoODVFOZaRaLCG32xhZUXw\n\tB2FfzvMy3O4u+jVdf/bnTxN+iq1JlZz5Sj5PicKQ4T7XviLbLOyZGVQNufMc1xxdEaGo\n\tFGY8ubbjMCRq91sj8VC76myxWQ/DsDZSl3h8CZY4hKM3U0wu9jm9+1iXSZkeIjXnbdjf\n\t1umg==","X-Gm-Message-State":"AOAM530bpDAIyHiB9EoFjk3ZXgot5BwH7HPMKHzWiUy4qXakV6f01+Z5\n\t3qsht5JLA0Ey/Cak5P+EkI1R4QUDnIcUBA6GJNlnX94p5I8=","X-Google-Smtp-Source":"ABdhPJw0v9FUf5Wzq/7Ts0ysvQAQo8e24B4VnsLh/pyQuyA/mtII3TiNeD0Z0zjnJOo8tdlBAmQAFxYbZ4cUXxCq4WQ=","X-Received":"by 2002:aa7:c4cc:: with SMTP id\n\tp12mr31217646edr.325.1617082254219; \n\tMon, 29 Mar 2021 22:30:54 -0700 (PDT)","MIME-Version":"1.0","References":"<20210329061119.135513-1-hiroh@chromium.org>\n\t<20210329061119.135513-2-hiroh@chromium.org>\n\t<20210329091642.2jao7zx6jb3txylr@uno.localdomain>","In-Reply-To":"<20210329091642.2jao7zx6jb3txylr@uno.localdomain>","From":"Hirokazu Honda ","Date":"Tue, 30 Mar 2021 14:30:41 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v3 1/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\" "}}]