[{"id":34981,"web_url":"https://patchwork.libcamera.org/comment/34981/","msgid":"<175312160641.50296.13088832792560201008@ping.linuxembedded.co.uk>","date":"2025-07-21T18:13:26","subject":"Re: [RFC PATCH v2] libcamera: pipeline: Avoid unnecessary\n\tindirection in frame info map","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-01-21 19:05:50)\n> There is no reason to allocate the frame info objects dynamically,\n> and then store raw pointers in the `std::map` in the rkisp1\n> and ipu3 pipeline handler.\n> \n> Instead, store the objects directly in the map. This removes\n> the need for manully calling new/delete, simplifies the code,\n> and eliminates one memory allocation per frame.\n\nThis sounds like a good thing. Is it still 'RFC'? Maybe we should try it\non some RKISP1 devices and test...\n\nIs patch this still relevant? It was posted in January ... I suspect it\nis.\n\nI'm trying to dig into some issues where I think we get out of sync with\nimage and stats/param buffers in error paths - so I'll try to test this\nalong with my investigations there.\n\n\n> \n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/libcamera/pipeline/ipu3/frames.cpp | 38 ++++++------\n> src/libcamera/pipeline/ipu3/frames.h | 2 +-\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 75 +++++++++++-------------\n> 3 files changed, 53 insertions(+), 62 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp\n> index bc0526a77..f3ea78797 100644\n> --- a/src/libcamera/pipeline/ipu3/frames.cpp\n> +++ b/src/libcamera/pipeline/ipu3/frames.cpp\n> @@ -64,20 +64,20 @@ IPU3Frames::Info *IPU3Frames::create(Request *request)\n> availableParamBuffers_.pop();\n> availableStatBuffers_.pop();\n> \n> - /* \\todo Remove the dynamic allocation of Info */\n> - std::unique_ptr info = std::make_unique();\n> + auto [it, inserted] = frameInfo_.try_emplace(id);\n> + ASSERT(inserted);\n> \n> - info->id = id;\n> - info->request = request;\n> - info->rawBuffer = nullptr;\n> - info->paramBuffer = paramBuffer;\n> - info->statBuffer = statBuffer;\n> - info->paramDequeued = false;\n> - info->metadataProcessed = false;\n> + auto &info = it->second;\n> \n> - frameInfo_[id] = std::move(info);\n> + info.id = id;\n> + info.request = request;\n> + info.rawBuffer = nullptr;\n> + info.paramBuffer = paramBuffer;\n> + info.statBuffer = statBuffer;\n> + info.paramDequeued = false;\n> + info.metadataProcessed = false;\n> \n> - return frameInfo_[id].get();\n> + return &info;\n> }\n> \n> void IPU3Frames::remove(IPU3Frames::Info *info)\n> @@ -115,7 +115,7 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id)\n> const auto &itInfo = frameInfo_.find(id);\n> \n> if (itInfo != frameInfo_.end())\n> - return itInfo->second.get();\n> + return &itInfo->second;\n> \n> LOG(IPU3, Fatal) << \"Can't find tracking information for frame \" << id;\n> \n> @@ -124,16 +124,14 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id)\n> \n> IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer)\n> {\n> - for (auto const &itInfo : frameInfo_) {\n> - Info *info = itInfo.second.get();\n> -\n> - for (auto const itBuffers : info->request->buffers())\n> + for (auto &[id, info] : frameInfo_) {\n> + for (auto const itBuffers : info.request->buffers())\n> if (itBuffers.second == buffer)\n> - return info;\n> + return &info;\n> \n> - if (info->rawBuffer == buffer || info->paramBuffer == buffer ||\n> - info->statBuffer == buffer)\n> - return info;\n> + if (info.rawBuffer == buffer || info.paramBuffer == buffer ||\n> + info.statBuffer == buffer)\n> + return &info;\n> }\n> \n> LOG(IPU3, Fatal) << \"Can't find tracking information from buffer\";\n> diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h\n> index a347b66f3..1cabd0e64 100644\n> --- a/src/libcamera/pipeline/ipu3/frames.h\n> +++ b/src/libcamera/pipeline/ipu3/frames.h\n> @@ -61,7 +61,7 @@ private:\n> std::queue availableParamBuffers_;\n> std::queue availableStatBuffers_;\n> \n> - std::map> frameInfo_;\n> + std::map frameInfo_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index ca8bbaaf1..953138305 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -84,7 +84,7 @@ public:\n> \n> private:\n> PipelineHandlerRkISP1 *pipe_;\n> - std::map frameInfo_;\n> + std::map frameInfo_;\n> };\n> \n> class RkISP1CameraData : public Camera::Private\n> @@ -269,49 +269,46 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n> \n> - RkISP1FrameInfo *info = new RkISP1FrameInfo;\n> + auto [it, inserted] = frameInfo_.try_emplace(frame);\n> + ASSERT(inserted);\n> \n> - info->frame = frame;\n> - info->request = request;\n> - info->paramBuffer = paramBuffer;\n> - info->mainPathBuffer = mainPathBuffer;\n> - info->selfPathBuffer = selfPathBuffer;\n> - info->statBuffer = statBuffer;\n> - info->paramDequeued = false;\n> - info->metadataProcessed = false;\n> + auto &info = it->second;\n> \n> - frameInfo_[frame] = info;\n> + info.frame = frame;\n> + info.request = request;\n> + info.paramBuffer = paramBuffer;\n> + info.mainPathBuffer = mainPathBuffer;\n> + info.selfPathBuffer = selfPathBuffer;\n> + info.statBuffer = statBuffer;\n> + info.paramDequeued = false;\n> + info.metadataProcessed = false;\n> \n> - return info;\n> + return &info;\n> }\n> \n> int RkISP1Frames::destroy(unsigned int frame)\n> {\n> - RkISP1FrameInfo *info = find(frame);\n> - if (!info)\n> + auto it = frameInfo_.find(frame);\n> + if (it == frameInfo_.end())\n> return -ENOENT;\n> \n> - pipe_->availableParamBuffers_.push(info->paramBuffer);\n> - pipe_->availableStatBuffers_.push(info->statBuffer);\n> - pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n> + auto &info = it->second;\n> \n> - frameInfo_.erase(info->frame);\n> + pipe_->availableParamBuffers_.push(info.paramBuffer);\n> + pipe_->availableStatBuffers_.push(info.statBuffer);\n> + pipe_->availableMainPathBuffers_.push(info.mainPathBuffer);\n> \n> - delete info;\n> + frameInfo_.erase(it);\n> \n> return 0;\n> }\n> \n> void RkISP1Frames::clear()\n> {\n> - for (const auto &entry : frameInfo_) {\n> - RkISP1FrameInfo *info = entry.second;\n> -\n> - pipe_->availableParamBuffers_.push(info->paramBuffer);\n> - pipe_->availableStatBuffers_.push(info->statBuffer);\n> - pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n> -\n> - delete info;\n> + for (const auto &[frame, info] : frameInfo_) {\n> + pipe_->availableParamBuffers_.push(info.paramBuffer);\n> + pipe_->availableStatBuffers_.push(info.statBuffer);\n> + pipe_->availableMainPathBuffers_.push(info.mainPathBuffer);\n> }\n> \n> frameInfo_.clear();\n> @@ -322,7 +319,7 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n> auto itInfo = frameInfo_.find(frame);\n> \n> if (itInfo != frameInfo_.end())\n> - return itInfo->second;\n> + return &itInfo->second;\n> \n> LOG(RkISP1, Fatal) << \"Can't locate info from frame\";\n> \n> @@ -331,14 +328,12 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n> \n> RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n> {\n> - for (auto &itInfo : frameInfo_) {\n> - RkISP1FrameInfo *info = itInfo.second;\n> -\n> - if (info->paramBuffer == buffer ||\n> - info->statBuffer == buffer ||\n> - info->mainPathBuffer == buffer ||\n> - info->selfPathBuffer == buffer)\n> - return info;\n> + for (auto &[frame, info] : frameInfo_) {\n> + if (info.paramBuffer == buffer ||\n> + info.statBuffer == buffer ||\n> + info.mainPathBuffer == buffer ||\n> + info.selfPathBuffer == buffer)\n> + return &info;\n> }\n> \n> LOG(RkISP1, Fatal) << \"Can't locate info from buffer\";\n> @@ -348,11 +343,9 @@ RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n> \n> RkISP1FrameInfo *RkISP1Frames::find(Request *request)\n> {\n> - for (auto &itInfo : frameInfo_) {\n> - RkISP1FrameInfo *info = itInfo.second;\n> -\n> - if (info->request == request)\n> - return info;\n> + for (auto &[frame, info] : frameInfo_) {\n> + if (info.request == request)\n> + return &info;\n> }\n> \n> LOG(RkISP1, Fatal) << \"Can't locate info from request\";\n> -- \n> 2.48.1\n> \n>","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 59326C3237\n\tfor ;\n\tMon, 21 Jul 2025 18:13:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 970C968FB1;\n\tMon, 21 Jul 2025 20:13:31 +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 66C7F68FB1\n\tfor ;\n\tMon, 21 Jul 2025 20:13:29 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4662322D2;\n\tMon, 21 Jul 2025 20:12:52 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"P4YFE+Kn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753121572;\n\tbh=71A5/Sp7H1L9liwHw6+iz3LMEfEK6i0VoE+z5FtZMLU=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=P4YFE+KnlKAuNkFN5jKLtdjicndZJox6y4/pJynuAz4hNXzBGuK3k093ajGpMz2cK\n\tNRlFbheLTvquZ77LV2sZuJasnw2ZPxohurRood5/Mj/Q3DrU51sZnD2mcD2OBeXum7\n\tZ95AhFHB2BA2PBf0wZCSIyTO5mzZcyP4F4CgvEyI=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250121190547.304107-1-pobrn@protonmail.com>","References":"<20250121190547.304107-1-pobrn@protonmail.com>","Subject":"Re: [RFC PATCH v2] libcamera: pipeline: Avoid unnecessary\n\tindirection in frame info map","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 21 Jul 2025 19:13:26 +0100","Message-ID":"<175312160641.50296.13088832792560201008@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34982,"web_url":"https://patchwork.libcamera.org/comment/34982/","msgid":"<20beb51a-bcb1-4abf-b3db-aa9d27fcd00b@ideasonboard.com>","date":"2025-07-21T18:17:40","subject":"Re: [RFC PATCH v2] libcamera: pipeline: Avoid unnecessary\n\tindirection in frame info map","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 07. 21. 20:13 keltezéssel, Kieran Bingham írta:\n> Quoting Barnabás Pőcze (2025-01-21 19:05:50)\n>> There is no reason to allocate the frame info objects dynamically,\n>> and then store raw pointers in the `std::map` in the rkisp1\n>> and ipu3 pipeline handler.\n>>\n>> Instead, store the objects directly in the map. This removes\n>> the need for manully calling new/delete, simplifies the code,\n>> and eliminates one memory allocation per frame.\n> \n> This sounds like a good thing. Is it still 'RFC'? Maybe we should try it\n> on some RKISP1 devices and test...\n> \n> Is patch this still relevant? It was posted in January ... I suspect it\n> is.\n\nYes it is. I have been running it ever since.\n\n\n> \n> I'm trying to dig into some issues where I think we get out of sync with\n> image and stats/param buffers in error paths - so I'll try to test this\n> along with my investigations there.\n> \n> \n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> src/libcamera/pipeline/ipu3/frames.cpp | 38 ++++++------\n>> src/libcamera/pipeline/ipu3/frames.h | 2 +-\n>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 75 +++++++++++-------------\n>> 3 files changed, 53 insertions(+), 62 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp\n>> index bc0526a77..f3ea78797 100644\n>> --- a/src/libcamera/pipeline/ipu3/frames.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/frames.cpp\n>> @@ -64,20 +64,20 @@ IPU3Frames::Info *IPU3Frames::create(Request *request)\n>> availableParamBuffers_.pop();\n>> availableStatBuffers_.pop();\n>> \n>> - /* \\todo Remove the dynamic allocation of Info */\n>> - std::unique_ptr info = std::make_unique();\n>> + auto [it, inserted] = frameInfo_.try_emplace(id);\n>> + ASSERT(inserted);\n>> \n>> - info->id = id;\n>> - info->request = request;\n>> - info->rawBuffer = nullptr;\n>> - info->paramBuffer = paramBuffer;\n>> - info->statBuffer = statBuffer;\n>> - info->paramDequeued = false;\n>> - info->metadataProcessed = false;\n>> + auto &info = it->second;\n>> \n>> - frameInfo_[id] = std::move(info);\n>> + info.id = id;\n>> + info.request = request;\n>> + info.rawBuffer = nullptr;\n>> + info.paramBuffer = paramBuffer;\n>> + info.statBuffer = statBuffer;\n>> + info.paramDequeued = false;\n>> + info.metadataProcessed = false;\n>> \n>> - return frameInfo_[id].get();\n>> + return &info;\n>> }\n>> \n>> void IPU3Frames::remove(IPU3Frames::Info *info)\n>> @@ -115,7 +115,7 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id)\n>> const auto &itInfo = frameInfo_.find(id);\n>> \n>> if (itInfo != frameInfo_.end())\n>> - return itInfo->second.get();\n>> + return &itInfo->second;\n>> \n>> LOG(IPU3, Fatal) << \"Can't find tracking information for frame \" << id;\n>> \n>> @@ -124,16 +124,14 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id)\n>> \n>> IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer)\n>> {\n>> - for (auto const &itInfo : frameInfo_) {\n>> - Info *info = itInfo.second.get();\n>> -\n>> - for (auto const itBuffers : info->request->buffers())\n>> + for (auto &[id, info] : frameInfo_) {\n>> + for (auto const itBuffers : info.request->buffers())\n>> if (itBuffers.second == buffer)\n>> - return info;\n>> + return &info;\n>> \n>> - if (info->rawBuffer == buffer || info->paramBuffer == buffer ||\n>> - info->statBuffer == buffer)\n>> - return info;\n>> + if (info.rawBuffer == buffer || info.paramBuffer == buffer ||\n>> + info.statBuffer == buffer)\n>> + return &info;\n>> }\n>> \n>> LOG(IPU3, Fatal) << \"Can't find tracking information from buffer\";\n>> diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h\n>> index a347b66f3..1cabd0e64 100644\n>> --- a/src/libcamera/pipeline/ipu3/frames.h\n>> +++ b/src/libcamera/pipeline/ipu3/frames.h\n>> @@ -61,7 +61,7 @@ private:\n>> std::queue availableParamBuffers_;\n>> std::queue availableStatBuffers_;\n>> \n>> - std::map> frameInfo_;\n>> + std::map frameInfo_;\n>> };\n>> \n>> } /* namespace libcamera */\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> index ca8bbaaf1..953138305 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> @@ -84,7 +84,7 @@ public:\n>> \n>> private:\n>> PipelineHandlerRkISP1 *pipe_;\n>> - std::map frameInfo_;\n>> + std::map frameInfo_;\n>> };\n>> \n>> class RkISP1CameraData : public Camera::Private\n>> @@ -269,49 +269,46 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n>> mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n>> selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n>> \n>> - RkISP1FrameInfo *info = new RkISP1FrameInfo;\n>> + auto [it, inserted] = frameInfo_.try_emplace(frame);\n>> + ASSERT(inserted);\n>> \n>> - info->frame = frame;\n>> - info->request = request;\n>> - info->paramBuffer = paramBuffer;\n>> - info->mainPathBuffer = mainPathBuffer;\n>> - info->selfPathBuffer = selfPathBuffer;\n>> - info->statBuffer = statBuffer;\n>> - info->paramDequeued = false;\n>> - info->metadataProcessed = false;\n>> + auto &info = it->second;\n>> \n>> - frameInfo_[frame] = info;\n>> + info.frame = frame;\n>> + info.request = request;\n>> + info.paramBuffer = paramBuffer;\n>> + info.mainPathBuffer = mainPathBuffer;\n>> + info.selfPathBuffer = selfPathBuffer;\n>> + info.statBuffer = statBuffer;\n>> + info.paramDequeued = false;\n>> + info.metadataProcessed = false;\n>> \n>> - return info;\n>> + return &info;\n>> }\n>> \n>> int RkISP1Frames::destroy(unsigned int frame)\n>> {\n>> - RkISP1FrameInfo *info = find(frame);\n>> - if (!info)\n>> + auto it = frameInfo_.find(frame);\n>> + if (it == frameInfo_.end())\n>> return -ENOENT;\n>> \n>> - pipe_->availableParamBuffers_.push(info->paramBuffer);\n>> - pipe_->availableStatBuffers_.push(info->statBuffer);\n>> - pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n>> + auto &info = it->second;\n>> \n>> - frameInfo_.erase(info->frame);\n>> + pipe_->availableParamBuffers_.push(info.paramBuffer);\n>> + pipe_->availableStatBuffers_.push(info.statBuffer);\n>> + pipe_->availableMainPathBuffers_.push(info.mainPathBuffer);\n>> \n>> - delete info;\n>> + frameInfo_.erase(it);\n>> \n>> return 0;\n>> }\n>> \n>> void RkISP1Frames::clear()\n>> {\n>> - for (const auto &entry : frameInfo_) {\n>> - RkISP1FrameInfo *info = entry.second;\n>> -\n>> - pipe_->availableParamBuffers_.push(info->paramBuffer);\n>> - pipe_->availableStatBuffers_.push(info->statBuffer);\n>> - pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n>> -\n>> - delete info;\n>> + for (const auto &[frame, info] : frameInfo_) {\n>> + pipe_->availableParamBuffers_.push(info.paramBuffer);\n>> + pipe_->availableStatBuffers_.push(info.statBuffer);\n>> + pipe_->availableMainPathBuffers_.push(info.mainPathBuffer);\n>> }\n>> \n>> frameInfo_.clear();\n>> @@ -322,7 +319,7 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n>> auto itInfo = frameInfo_.find(frame);\n>> \n>> if (itInfo != frameInfo_.end())\n>> - return itInfo->second;\n>> + return &itInfo->second;\n>> \n>> LOG(RkISP1, Fatal) << \"Can't locate info from frame\";\n>> \n>> @@ -331,14 +328,12 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n>> \n>> RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n>> {\n>> - for (auto &itInfo : frameInfo_) {\n>> - RkISP1FrameInfo *info = itInfo.second;\n>> -\n>> - if (info->paramBuffer == buffer ||\n>> - info->statBuffer == buffer ||\n>> - info->mainPathBuffer == buffer ||\n>> - info->selfPathBuffer == buffer)\n>> - return info;\n>> + for (auto &[frame, info] : frameInfo_) {\n>> + if (info.paramBuffer == buffer ||\n>> + info.statBuffer == buffer ||\n>> + info.mainPathBuffer == buffer ||\n>> + info.selfPathBuffer == buffer)\n>> + return &info;\n>> }\n>> \n>> LOG(RkISP1, Fatal) << \"Can't locate info from buffer\";\n>> @@ -348,11 +343,9 @@ RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n>> \n>> RkISP1FrameInfo *RkISP1Frames::find(Request *request)\n>> {\n>> - for (auto &itInfo : frameInfo_) {\n>> - RkISP1FrameInfo *info = itInfo.second;\n>> -\n>> - if (info->request == request)\n>> - return info;\n>> + for (auto &[frame, info] : frameInfo_) {\n>> + if (info.request == request)\n>> + return &info;\n>> }\n>> \n>> LOG(RkISP1, Fatal) << \"Can't locate info from request\";\n>> -- \n>> 2.48.1\n>>\n>>","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 0EA6ABDCC1\n\tfor ;\n\tMon, 21 Jul 2025 18:17:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6F9D268FFF;\n\tMon, 21 Jul 2025 20:17:46 +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 9341368FB1\n\tfor ;\n\tMon, 21 Jul 2025 20:17:44 +0200 (CEST)","from [192.168.33.18] (185.221.140.39.nat.pool.zt.hu\n\t[185.221.140.39])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 59869243C;\n\tMon, 21 Jul 2025 20:17:07 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Icwpinzc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753121827;\n\tbh=JAvnAwpMO7dkJeFS76Gqmx5Dh9v/6MUn42XeDNA6gOE=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=IcwpinzcHByo0yevj3Tx/RLamBAHpDA+fJ0pZVrprm4DjJtK1/5XHkLhbpclCA1I6\n\tOz9TdUk13zhMKnzsQtrgBFGuR6lalrMkh/dOFmjZqV44ZClRIpjPh7bSShDu9GcuIa\n\t0s/GtVLPe4/hWsSZRCx/csVwPTG3i9trr1KGP8a8=","Message-ID":"<20beb51a-bcb1-4abf-b3db-aa9d27fcd00b@ideasonboard.com>","Date":"Mon, 21 Jul 2025 20:17:40 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v2] libcamera: pipeline: Avoid unnecessary\n\tindirection in frame info map","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250121190547.304107-1-pobrn@protonmail.com>\n\t<175312160641.50296.13088832792560201008@ping.linuxembedded.co.uk>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<175312160641.50296.13088832792560201008@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35068,"web_url":"https://patchwork.libcamera.org/comment/35068/","msgid":"<175334029324.774292.14899966407348401327@neptunite.rasen.tech>","date":"2025-07-24T06:58:13","subject":"Re: [RFC PATCH v2] libcamera: pipeline: Avoid unnecessary\n\tindirection in frame info map","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-01-22 04:05:50)\n> There is no reason to allocate the frame info objects dynamically,\n> and then store raw pointers in the `std::map` in the rkisp1\n> and ipu3 pipeline handler.\n> \n> Instead, store the objects directly in the map. This removes\n> the need for manully calling new/delete, simplifies the code,\n> and eliminates one memory allocation per frame.\n> \n> Signed-off-by: Barnabás Pőcze \n\nKieran replying pushed it into my inbox.\n\nLooks good to me.\n\nReviewed-by: Paul Elder \n\n> ---\n> src/libcamera/pipeline/ipu3/frames.cpp | 38 ++++++------\n> src/libcamera/pipeline/ipu3/frames.h | 2 +-\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 75 +++++++++++-------------\n> 3 files changed, 53 insertions(+), 62 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp\n> index bc0526a77..f3ea78797 100644\n> --- a/src/libcamera/pipeline/ipu3/frames.cpp\n> +++ b/src/libcamera/pipeline/ipu3/frames.cpp\n> @@ -64,20 +64,20 @@ IPU3Frames::Info *IPU3Frames::create(Request *request)\n> availableParamBuffers_.pop();\n> availableStatBuffers_.pop();\n> \n> - /* \\todo Remove the dynamic allocation of Info */\n> - std::unique_ptr info = std::make_unique();\n> + auto [it, inserted] = frameInfo_.try_emplace(id);\n> + ASSERT(inserted);\n> \n> - info->id = id;\n> - info->request = request;\n> - info->rawBuffer = nullptr;\n> - info->paramBuffer = paramBuffer;\n> - info->statBuffer = statBuffer;\n> - info->paramDequeued = false;\n> - info->metadataProcessed = false;\n> + auto &info = it->second;\n> \n> - frameInfo_[id] = std::move(info);\n> + info.id = id;\n> + info.request = request;\n> + info.rawBuffer = nullptr;\n> + info.paramBuffer = paramBuffer;\n> + info.statBuffer = statBuffer;\n> + info.paramDequeued = false;\n> + info.metadataProcessed = false;\n> \n> - return frameInfo_[id].get();\n> + return &info;\n> }\n> \n> void IPU3Frames::remove(IPU3Frames::Info *info)\n> @@ -115,7 +115,7 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id)\n> const auto &itInfo = frameInfo_.find(id);\n> \n> if (itInfo != frameInfo_.end())\n> - return itInfo->second.get();\n> + return &itInfo->second;\n> \n> LOG(IPU3, Fatal) << \"Can't find tracking information for frame \" << id;\n> \n> @@ -124,16 +124,14 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id)\n> \n> IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer)\n> {\n> - for (auto const &itInfo : frameInfo_) {\n> - Info *info = itInfo.second.get();\n> -\n> - for (auto const itBuffers : info->request->buffers())\n> + for (auto &[id, info] : frameInfo_) {\n> + for (auto const itBuffers : info.request->buffers())\n> if (itBuffers.second == buffer)\n> - return info;\n> + return &info;\n> \n> - if (info->rawBuffer == buffer || info->paramBuffer == buffer ||\n> - info->statBuffer == buffer)\n> - return info;\n> + if (info.rawBuffer == buffer || info.paramBuffer == buffer ||\n> + info.statBuffer == buffer)\n> + return &info;\n> }\n> \n> LOG(IPU3, Fatal) << \"Can't find tracking information from buffer\";\n> diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h\n> index a347b66f3..1cabd0e64 100644\n> --- a/src/libcamera/pipeline/ipu3/frames.h\n> +++ b/src/libcamera/pipeline/ipu3/frames.h\n> @@ -61,7 +61,7 @@ private:\n> std::queue availableParamBuffers_;\n> std::queue availableStatBuffers_;\n> \n> - std::map> frameInfo_;\n> + std::map frameInfo_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index ca8bbaaf1..953138305 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -84,7 +84,7 @@ public:\n> \n> private:\n> PipelineHandlerRkISP1 *pipe_;\n> - std::map frameInfo_;\n> + std::map frameInfo_;\n> };\n> \n> class RkISP1CameraData : public Camera::Private\n> @@ -269,49 +269,46 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n> \n> - RkISP1FrameInfo *info = new RkISP1FrameInfo;\n> + auto [it, inserted] = frameInfo_.try_emplace(frame);\n> + ASSERT(inserted);\n> \n> - info->frame = frame;\n> - info->request = request;\n> - info->paramBuffer = paramBuffer;\n> - info->mainPathBuffer = mainPathBuffer;\n> - info->selfPathBuffer = selfPathBuffer;\n> - info->statBuffer = statBuffer;\n> - info->paramDequeued = false;\n> - info->metadataProcessed = false;\n> + auto &info = it->second;\n> \n> - frameInfo_[frame] = info;\n> + info.frame = frame;\n> + info.request = request;\n> + info.paramBuffer = paramBuffer;\n> + info.mainPathBuffer = mainPathBuffer;\n> + info.selfPathBuffer = selfPathBuffer;\n> + info.statBuffer = statBuffer;\n> + info.paramDequeued = false;\n> + info.metadataProcessed = false;\n> \n> - return info;\n> + return &info;\n> }\n> \n> int RkISP1Frames::destroy(unsigned int frame)\n> {\n> - RkISP1FrameInfo *info = find(frame);\n> - if (!info)\n> + auto it = frameInfo_.find(frame);\n> + if (it == frameInfo_.end())\n> return -ENOENT;\n> \n> - pipe_->availableParamBuffers_.push(info->paramBuffer);\n> - pipe_->availableStatBuffers_.push(info->statBuffer);\n> - pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n> + auto &info = it->second;\n> \n> - frameInfo_.erase(info->frame);\n> + pipe_->availableParamBuffers_.push(info.paramBuffer);\n> + pipe_->availableStatBuffers_.push(info.statBuffer);\n> + pipe_->availableMainPathBuffers_.push(info.mainPathBuffer);\n> \n> - delete info;\n> + frameInfo_.erase(it);\n> \n> return 0;\n> }\n> \n> void RkISP1Frames::clear()\n> {\n> - for (const auto &entry : frameInfo_) {\n> - RkISP1FrameInfo *info = entry.second;\n> -\n> - pipe_->availableParamBuffers_.push(info->paramBuffer);\n> - pipe_->availableStatBuffers_.push(info->statBuffer);\n> - pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n> -\n> - delete info;\n> + for (const auto &[frame, info] : frameInfo_) {\n> + pipe_->availableParamBuffers_.push(info.paramBuffer);\n> + pipe_->availableStatBuffers_.push(info.statBuffer);\n> + pipe_->availableMainPathBuffers_.push(info.mainPathBuffer);\n> }\n> \n> frameInfo_.clear();\n> @@ -322,7 +319,7 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n> auto itInfo = frameInfo_.find(frame);\n> \n> if (itInfo != frameInfo_.end())\n> - return itInfo->second;\n> + return &itInfo->second;\n> \n> LOG(RkISP1, Fatal) << \"Can't locate info from frame\";\n> \n> @@ -331,14 +328,12 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n> \n> RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n> {\n> - for (auto &itInfo : frameInfo_) {\n> - RkISP1FrameInfo *info = itInfo.second;\n> -\n> - if (info->paramBuffer == buffer ||\n> - info->statBuffer == buffer ||\n> - info->mainPathBuffer == buffer ||\n> - info->selfPathBuffer == buffer)\n> - return info;\n> + for (auto &[frame, info] : frameInfo_) {\n> + if (info.paramBuffer == buffer ||\n> + info.statBuffer == buffer ||\n> + info.mainPathBuffer == buffer ||\n> + info.selfPathBuffer == buffer)\n> + return &info;\n> }\n> \n> LOG(RkISP1, Fatal) << \"Can't locate info from buffer\";\n> @@ -348,11 +343,9 @@ RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n> \n> RkISP1FrameInfo *RkISP1Frames::find(Request *request)\n> {\n> - for (auto &itInfo : frameInfo_) {\n> - RkISP1FrameInfo *info = itInfo.second;\n> -\n> - if (info->request == request)\n> - return info;\n> + for (auto &[frame, info] : frameInfo_) {\n> + if (info.request == request)\n> + return &info;\n> }\n> \n> LOG(RkISP1, Fatal) << \"Can't locate info from request\";\n> -- \n> 2.48.1\n> \n>","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 71E05C3237\n\tfor ;\n\tThu, 24 Jul 2025 06:58:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AA357690A0;\n\tThu, 24 Jul 2025 08:58:23 +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 AFD076909C\n\tfor ;\n\tThu, 24 Jul 2025 08:58:21 +0200 (CEST)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:be7f:7fca:78b4:1343])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 5558BC0B;\n\tThu, 24 Jul 2025 08:57:42 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dNxDNh0/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753340262;\n\tbh=Q/ZLwy0raFc5dDD+BLPEa7cScEpf1p0sVv1F25ZO0tA=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=dNxDNh0/+MwGIfotdKHOzLpJaxKZnDTy2CHRqo4/IpeH/sAASiEyFdAJPwr8ZPDYL\n\tuxX2ZkDMZ+a8WiDBt/N6IaxL5lR1ZLC3zsbMEX5PkS0Cdx2RrIpw+usG1SgISnPseL\n\tInf2yVb7tchDcXZbISp+kHpkx+3Cp24e1VUTWiOs=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250121190547.304107-1-pobrn@protonmail.com>","References":"<20250121190547.304107-1-pobrn@protonmail.com>","Subject":"Re: [RFC PATCH v2] libcamera: pipeline: Avoid unnecessary\n\tindirection in frame info map","From":"Paul Elder ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 24 Jul 2025 15:58:13 +0900","Message-ID":"<175334029324.774292.14899966407348401327@neptunite.rasen.tech>","User-Agent":"alot/0.0.0","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35109,"web_url":"https://patchwork.libcamera.org/comment/35109/","msgid":"<20250724183227.GS11202@pendragon.ideasonboard.com>","date":"2025-07-24T18:32:27","subject":"Re: [RFC PATCH v2] libcamera: pipeline: Avoid unnecessary\n\tindirection in frame info map","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nOn Tue, Jan 21, 2025 at 07:05:50PM +0000, Barnabás Pőcze wrote:\n> There is no reason to allocate the frame info objects dynamically,\n> and then store raw pointers in the `std::map` in the rkisp1\n> and ipu3 pipeline handler.\n> \n> Instead, store the objects directly in the map. This removes\n> the need for manully calling new/delete, simplifies the code,\n> and eliminates one memory allocation per frame.\n\nWe share the common goal of reducing memory allocations at runtime, and\nthis patch helps by turning two allocations into one. However, I think\nit could make it more difficult to get rid of the second allocation.\n\nIdeally, we should preallocate all the frame info at start time and put\nthem in a pool. Have you considered how this could be done ? Would it be\nas simple as creating the map with a custom allocator ?\n\n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/libcamera/pipeline/ipu3/frames.cpp | 38 ++++++------\n> src/libcamera/pipeline/ipu3/frames.h | 2 +-\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 75 +++++++++++-------------\n> 3 files changed, 53 insertions(+), 62 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp\n> index bc0526a77..f3ea78797 100644\n> --- a/src/libcamera/pipeline/ipu3/frames.cpp\n> +++ b/src/libcamera/pipeline/ipu3/frames.cpp\n> @@ -64,20 +64,20 @@ IPU3Frames::Info *IPU3Frames::create(Request *request)\n> \tavailableParamBuffers_.pop();\n> \tavailableStatBuffers_.pop();\n> \n> -\t/* \\todo Remove the dynamic allocation of Info */\n> -\tstd::unique_ptr info = std::make_unique();\n> +\tauto [it, inserted] = frameInfo_.try_emplace(id);\n> +\tASSERT(inserted);\n> \n> -\tinfo->id = id;\n> -\tinfo->request = request;\n> -\tinfo->rawBuffer = nullptr;\n> -\tinfo->paramBuffer = paramBuffer;\n> -\tinfo->statBuffer = statBuffer;\n> -\tinfo->paramDequeued = false;\n> -\tinfo->metadataProcessed = false;\n> +\tauto &info = it->second;\n> \n> -\tframeInfo_[id] = std::move(info);\n> +\tinfo.id = id;\n> +\tinfo.request = request;\n> +\tinfo.rawBuffer = nullptr;\n> +\tinfo.paramBuffer = paramBuffer;\n> +\tinfo.statBuffer = statBuffer;\n> +\tinfo.paramDequeued = false;\n> +\tinfo.metadataProcessed = false;\n> \n> -\treturn frameInfo_[id].get();\n> +\treturn &info;\n> }\n> \n> void IPU3Frames::remove(IPU3Frames::Info *info)\n> @@ -115,7 +115,7 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id)\n> \tconst auto &itInfo = frameInfo_.find(id);\n> \n> \tif (itInfo != frameInfo_.end())\n> -\t\treturn itInfo->second.get();\n> +\t\treturn &itInfo->second;\n> \n> \tLOG(IPU3, Fatal) << \"Can't find tracking information for frame \" << id;\n> \n> @@ -124,16 +124,14 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id)\n> \n> IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer)\n> {\n> -\tfor (auto const &itInfo : frameInfo_) {\n> -\t\tInfo *info = itInfo.second.get();\n> -\n> -\t\tfor (auto const itBuffers : info->request->buffers())\n> +\tfor (auto &[id, info] : frameInfo_) {\n> +\t\tfor (auto const itBuffers : info.request->buffers())\n> \t\t\tif (itBuffers.second == buffer)\n> -\t\t\t\treturn info;\n> +\t\t\t\treturn &info;\n> \n> -\t\tif (info->rawBuffer == buffer || info->paramBuffer == buffer ||\n> -\t\t info->statBuffer == buffer)\n> -\t\t\treturn info;\n> +\t\tif (info.rawBuffer == buffer || info.paramBuffer == buffer ||\n> +\t\t info.statBuffer == buffer)\n> +\t\t\treturn &info;\n> \t}\n> \n> \tLOG(IPU3, Fatal) << \"Can't find tracking information from buffer\";\n> diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h\n> index a347b66f3..1cabd0e64 100644\n> --- a/src/libcamera/pipeline/ipu3/frames.h\n> +++ b/src/libcamera/pipeline/ipu3/frames.h\n> @@ -61,7 +61,7 @@ private:\n> \tstd::queue availableParamBuffers_;\n> \tstd::queue availableStatBuffers_;\n> \n> -\tstd::map> frameInfo_;\n> +\tstd::map frameInfo_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index ca8bbaaf1..953138305 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -84,7 +84,7 @@ public:\n> \n> private:\n> \tPipelineHandlerRkISP1 *pipe_;\n> -\tstd::map frameInfo_;\n> +\tstd::map frameInfo_;\n> };\n> \n> class RkISP1CameraData : public Camera::Private\n> @@ -269,49 +269,46 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> \t\tmainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> \tselfPathBuffer = request->findBuffer(&data->selfPathStream_);\n> \n> -\tRkISP1FrameInfo *info = new RkISP1FrameInfo;\n> +\tauto [it, inserted] = frameInfo_.try_emplace(frame);\n> +\tASSERT(inserted);\n> \n> -\tinfo->frame = frame;\n> -\tinfo->request = request;\n> -\tinfo->paramBuffer = paramBuffer;\n> -\tinfo->mainPathBuffer = mainPathBuffer;\n> -\tinfo->selfPathBuffer = selfPathBuffer;\n> -\tinfo->statBuffer = statBuffer;\n> -\tinfo->paramDequeued = false;\n> -\tinfo->metadataProcessed = false;\n> +\tauto &info = it->second;\n> \n> -\tframeInfo_[frame] = info;\n> +\tinfo.frame = frame;\n> +\tinfo.request = request;\n> +\tinfo.paramBuffer = paramBuffer;\n> +\tinfo.mainPathBuffer = mainPathBuffer;\n> +\tinfo.selfPathBuffer = selfPathBuffer;\n> +\tinfo.statBuffer = statBuffer;\n> +\tinfo.paramDequeued = false;\n> +\tinfo.metadataProcessed = false;\n> \n> -\treturn info;\n> +\treturn &info;\n> }\n> \n> int RkISP1Frames::destroy(unsigned int frame)\n> {\n> -\tRkISP1FrameInfo *info = find(frame);\n> -\tif (!info)\n> +\tauto it = frameInfo_.find(frame);\n> +\tif (it == frameInfo_.end())\n> \t\treturn -ENOENT;\n> \n> -\tpipe_->availableParamBuffers_.push(info->paramBuffer);\n> -\tpipe_->availableStatBuffers_.push(info->statBuffer);\n> -\tpipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n> +\tauto &info = it->second;\n> \n> -\tframeInfo_.erase(info->frame);\n> +\tpipe_->availableParamBuffers_.push(info.paramBuffer);\n> +\tpipe_->availableStatBuffers_.push(info.statBuffer);\n> +\tpipe_->availableMainPathBuffers_.push(info.mainPathBuffer);\n> \n> -\tdelete info;\n> +\tframeInfo_.erase(it);\n> \n> \treturn 0;\n> }\n> \n> void RkISP1Frames::clear()\n> {\n> -\tfor (const auto &entry : frameInfo_) {\n> -\t\tRkISP1FrameInfo *info = entry.second;\n> -\n> -\t\tpipe_->availableParamBuffers_.push(info->paramBuffer);\n> -\t\tpipe_->availableStatBuffers_.push(info->statBuffer);\n> -\t\tpipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n> -\n> -\t\tdelete info;\n> +\tfor (const auto &[frame, info] : frameInfo_) {\n> +\t\tpipe_->availableParamBuffers_.push(info.paramBuffer);\n> +\t\tpipe_->availableStatBuffers_.push(info.statBuffer);\n> +\t\tpipe_->availableMainPathBuffers_.push(info.mainPathBuffer);\n> \t}\n> \n> \tframeInfo_.clear();\n> @@ -322,7 +319,7 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n> \tauto itInfo = frameInfo_.find(frame);\n> \n> \tif (itInfo != frameInfo_.end())\n> -\t\treturn itInfo->second;\n> +\t\treturn &itInfo->second;\n> \n> \tLOG(RkISP1, Fatal) << \"Can't locate info from frame\";\n> \n> @@ -331,14 +328,12 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n> \n> RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n> {\n> -\tfor (auto &itInfo : frameInfo_) {\n> -\t\tRkISP1FrameInfo *info = itInfo.second;\n> -\n> -\t\tif (info->paramBuffer == buffer ||\n> -\t\t info->statBuffer == buffer ||\n> -\t\t info->mainPathBuffer == buffer ||\n> -\t\t info->selfPathBuffer == buffer)\n> -\t\t\treturn info;\n> +\tfor (auto &[frame, info] : frameInfo_) {\n> +\t\tif (info.paramBuffer == buffer ||\n> +\t\t info.statBuffer == buffer ||\n> +\t\t info.mainPathBuffer == buffer ||\n> +\t\t info.selfPathBuffer == buffer)\n> +\t\t\treturn &info;\n> \t}\n> \n> \tLOG(RkISP1, Fatal) << \"Can't locate info from buffer\";\n> @@ -348,11 +343,9 @@ RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n> \n> RkISP1FrameInfo *RkISP1Frames::find(Request *request)\n> {\n> -\tfor (auto &itInfo : frameInfo_) {\n> -\t\tRkISP1FrameInfo *info = itInfo.second;\n> -\n> -\t\tif (info->request == request)\n> -\t\t\treturn info;\n> +\tfor (auto &[frame, info] : frameInfo_) {\n> +\t\tif (info.request == request)\n> +\t\t\treturn &info;\n> \t}\n> \n> \tLOG(RkISP1, Fatal) << \"Can't locate info from request\";","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 C2084BDCC1\n\tfor ;\n\tThu, 24 Jul 2025 18:32:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8031E6904C;\n\tThu, 24 Jul 2025 20:32:33 +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 8DB04614D4\n\tfor ;\n\tThu, 24 Jul 2025 20:32:31 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 291EBC74;\n\tThu, 24 Jul 2025 20:31:52 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nCMK1yHc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753381912;\n\tbh=F3fI/ZvVhdqJQ172B8AW8ZO4dZIGXeO84ll/tUP5mRM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nCMK1yHcIpVYF1jY0qzm/Iv4ps1H0ZDC6sMVI4KsUINQLhSRu2JaLkvHlGDxbNiVj\n\t6ZPr3HEuJajNXahGu0V+ttbNDVRlQjLqBHuM5riTCqAzDM+/uo9TASshKjKSbZ9HeA\n\t3Tit0u9ynPr/OBakvllvputoqWnz+kz+MYz+bb20=","Date":"Thu, 24 Jul 2025 21:32:27 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2] libcamera: pipeline: Avoid unnecessary\n\tindirection in frame info map","Message-ID":"<20250724183227.GS11202@pendragon.ideasonboard.com>","References":"<20250121190547.304107-1-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250121190547.304107-1-pobrn@protonmail.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35112,"web_url":"https://patchwork.libcamera.org/comment/35112/","msgid":"<17969e8c-8ec7-46dc-a8b6-f58b91383251@ideasonboard.com>","date":"2025-07-25T07:17:59","subject":"Re: [RFC PATCH v2] libcamera: pipeline: Avoid unnecessary\n\tindirection in frame info map","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 07. 24. 20:32 keltezéssel, Laurent Pinchart írta:\n> Hi Barnabás,\n> \n> Thank you for the patch.\n> \n> On Tue, Jan 21, 2025 at 07:05:50PM +0000, Barnabás Pőcze wrote:\n>> There is no reason to allocate the frame info objects dynamically,\n>> and then store raw pointers in the `std::map` in the rkisp1\n>> and ipu3 pipeline handler.\n>>\n>> Instead, store the objects directly in the map. This removes\n>> the need for manully calling new/delete, simplifies the code,\n>> and eliminates one memory allocation per frame.\n> \n> We share the common goal of reducing memory allocations at runtime, and\n> this patch helps by turning two allocations into one. However, I think\n> it could make it more difficult to get rid of the second allocation.\n> \n> Ideally, we should preallocate all the frame info at start time and put\n> them in a pool. Have you considered how this could be done ? Would it be\n> as simple as creating the map with a custom allocator ?\n\nThe map will always allocate nodes, this cannot be avoided. So if you want to\nimprove still, the data structure should be replaced (e.g. to something intrusive).\nWith `std::map`, I think the choice is between a single pool of nodes (which embed\nthe useful data directly), or two pools of nodes (one for nodes, one for the useful data).\n\nSomething like `std::pmr::unsynchronized_pool_resource` seems like the best choice\navailable in the STL, unfortunately that introduces virtual dispatch for the allocator.\nAnything better than that would need a custom allocator wrapper I think. There are\nparticular requirements for an stl allocators[0] that memory resources do not satisfy,\neven if they had the right interface.\n\nSo in summary, no I have not considered preallocating, and I still think it is a step\nin the right direction.\n\n[0]: https://en.cppreference.com/w/cpp/named_req/Allocator.html\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> src/libcamera/pipeline/ipu3/frames.cpp | 38 ++++++------\n>> src/libcamera/pipeline/ipu3/frames.h | 2 +-\n>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 75 +++++++++++-------------\n>> 3 files changed, 53 insertions(+), 62 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp\n>> index bc0526a77..f3ea78797 100644\n>> --- a/src/libcamera/pipeline/ipu3/frames.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/frames.cpp\n>> @@ -64,20 +64,20 @@ IPU3Frames::Info *IPU3Frames::create(Request *request)\n>> \tavailableParamBuffers_.pop();\n>> \tavailableStatBuffers_.pop();\n>> \n>> -\t/* \\todo Remove the dynamic allocation of Info */\n>> -\tstd::unique_ptr info = std::make_unique();\n>> +\tauto [it, inserted] = frameInfo_.try_emplace(id);\n>> +\tASSERT(inserted);\n>> \n>> -\tinfo->id = id;\n>> -\tinfo->request = request;\n>> -\tinfo->rawBuffer = nullptr;\n>> -\tinfo->paramBuffer = paramBuffer;\n>> -\tinfo->statBuffer = statBuffer;\n>> -\tinfo->paramDequeued = false;\n>> -\tinfo->metadataProcessed = false;\n>> +\tauto &info = it->second;\n>> \n>> -\tframeInfo_[id] = std::move(info);\n>> +\tinfo.id = id;\n>> +\tinfo.request = request;\n>> +\tinfo.rawBuffer = nullptr;\n>> +\tinfo.paramBuffer = paramBuffer;\n>> +\tinfo.statBuffer = statBuffer;\n>> +\tinfo.paramDequeued = false;\n>> +\tinfo.metadataProcessed = false;\n>> \n>> -\treturn frameInfo_[id].get();\n>> +\treturn &info;\n>> }\n>> \n>> void IPU3Frames::remove(IPU3Frames::Info *info)\n>> @@ -115,7 +115,7 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id)\n>> \tconst auto &itInfo = frameInfo_.find(id);\n>> \n>> \tif (itInfo != frameInfo_.end())\n>> -\t\treturn itInfo->second.get();\n>> +\t\treturn &itInfo->second;\n>> \n>> \tLOG(IPU3, Fatal) << \"Can't find tracking information for frame \" << id;\n>> \n>> @@ -124,16 +124,14 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id)\n>> \n>> IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer)\n>> {\n>> -\tfor (auto const &itInfo : frameInfo_) {\n>> -\t\tInfo *info = itInfo.second.get();\n>> -\n>> -\t\tfor (auto const itBuffers : info->request->buffers())\n>> +\tfor (auto &[id, info] : frameInfo_) {\n>> +\t\tfor (auto const itBuffers : info.request->buffers())\n>> \t\t\tif (itBuffers.second == buffer)\n>> -\t\t\t\treturn info;\n>> +\t\t\t\treturn &info;\n>> \n>> -\t\tif (info->rawBuffer == buffer || info->paramBuffer == buffer ||\n>> -\t\t info->statBuffer == buffer)\n>> -\t\t\treturn info;\n>> +\t\tif (info.rawBuffer == buffer || info.paramBuffer == buffer ||\n>> +\t\t info.statBuffer == buffer)\n>> +\t\t\treturn &info;\n>> \t}\n>> \n>> \tLOG(IPU3, Fatal) << \"Can't find tracking information from buffer\";\n>> diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h\n>> index a347b66f3..1cabd0e64 100644\n>> --- a/src/libcamera/pipeline/ipu3/frames.h\n>> +++ b/src/libcamera/pipeline/ipu3/frames.h\n>> @@ -61,7 +61,7 @@ private:\n>> \tstd::queue availableParamBuffers_;\n>> \tstd::queue availableStatBuffers_;\n>> \n>> -\tstd::map> frameInfo_;\n>> +\tstd::map frameInfo_;\n>> };\n>> \n>> } /* namespace libcamera */\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> index ca8bbaaf1..953138305 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> @@ -84,7 +84,7 @@ public:\n>> \n>> private:\n>> \tPipelineHandlerRkISP1 *pipe_;\n>> -\tstd::map frameInfo_;\n>> +\tstd::map frameInfo_;\n>> };\n>> \n>> class RkISP1CameraData : public Camera::Private\n>> @@ -269,49 +269,46 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n>> \t\tmainPathBuffer = request->findBuffer(&data->mainPathStream_);\n>> \tselfPathBuffer = request->findBuffer(&data->selfPathStream_);\n>> \n>> -\tRkISP1FrameInfo *info = new RkISP1FrameInfo;\n>> +\tauto [it, inserted] = frameInfo_.try_emplace(frame);\n>> +\tASSERT(inserted);\n>> \n>> -\tinfo->frame = frame;\n>> -\tinfo->request = request;\n>> -\tinfo->paramBuffer = paramBuffer;\n>> -\tinfo->mainPathBuffer = mainPathBuffer;\n>> -\tinfo->selfPathBuffer = selfPathBuffer;\n>> -\tinfo->statBuffer = statBuffer;\n>> -\tinfo->paramDequeued = false;\n>> -\tinfo->metadataProcessed = false;\n>> +\tauto &info = it->second;\n>> \n>> -\tframeInfo_[frame] = info;\n>> +\tinfo.frame = frame;\n>> +\tinfo.request = request;\n>> +\tinfo.paramBuffer = paramBuffer;\n>> +\tinfo.mainPathBuffer = mainPathBuffer;\n>> +\tinfo.selfPathBuffer = selfPathBuffer;\n>> +\tinfo.statBuffer = statBuffer;\n>> +\tinfo.paramDequeued = false;\n>> +\tinfo.metadataProcessed = false;\n>> \n>> -\treturn info;\n>> +\treturn &info;\n>> }\n>> \n>> int RkISP1Frames::destroy(unsigned int frame)\n>> {\n>> -\tRkISP1FrameInfo *info = find(frame);\n>> -\tif (!info)\n>> +\tauto it = frameInfo_.find(frame);\n>> +\tif (it == frameInfo_.end())\n>> \t\treturn -ENOENT;\n>> \n>> -\tpipe_->availableParamBuffers_.push(info->paramBuffer);\n>> -\tpipe_->availableStatBuffers_.push(info->statBuffer);\n>> -\tpipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n>> +\tauto &info = it->second;\n>> \n>> -\tframeInfo_.erase(info->frame);\n>> +\tpipe_->availableParamBuffers_.push(info.paramBuffer);\n>> +\tpipe_->availableStatBuffers_.push(info.statBuffer);\n>> +\tpipe_->availableMainPathBuffers_.push(info.mainPathBuffer);\n>> \n>> -\tdelete info;\n>> +\tframeInfo_.erase(it);\n>> \n>> \treturn 0;\n>> }\n>> \n>> void RkISP1Frames::clear()\n>> {\n>> -\tfor (const auto &entry : frameInfo_) {\n>> -\t\tRkISP1FrameInfo *info = entry.second;\n>> -\n>> -\t\tpipe_->availableParamBuffers_.push(info->paramBuffer);\n>> -\t\tpipe_->availableStatBuffers_.push(info->statBuffer);\n>> -\t\tpipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n>> -\n>> -\t\tdelete info;\n>> +\tfor (const auto &[frame, info] : frameInfo_) {\n>> +\t\tpipe_->availableParamBuffers_.push(info.paramBuffer);\n>> +\t\tpipe_->availableStatBuffers_.push(info.statBuffer);\n>> +\t\tpipe_->availableMainPathBuffers_.push(info.mainPathBuffer);\n>> \t}\n>> \n>> \tframeInfo_.clear();\n>> @@ -322,7 +319,7 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n>> \tauto itInfo = frameInfo_.find(frame);\n>> \n>> \tif (itInfo != frameInfo_.end())\n>> -\t\treturn itInfo->second;\n>> +\t\treturn &itInfo->second;\n>> \n>> \tLOG(RkISP1, Fatal) << \"Can't locate info from frame\";\n>> \n>> @@ -331,14 +328,12 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n>> \n>> RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n>> {\n>> -\tfor (auto &itInfo : frameInfo_) {\n>> -\t\tRkISP1FrameInfo *info = itInfo.second;\n>> -\n>> -\t\tif (info->paramBuffer == buffer ||\n>> -\t\t info->statBuffer == buffer ||\n>> -\t\t info->mainPathBuffer == buffer ||\n>> -\t\t info->selfPathBuffer == buffer)\n>> -\t\t\treturn info;\n>> +\tfor (auto &[frame, info] : frameInfo_) {\n>> +\t\tif (info.paramBuffer == buffer ||\n>> +\t\t info.statBuffer == buffer ||\n>> +\t\t info.mainPathBuffer == buffer ||\n>> +\t\t info.selfPathBuffer == buffer)\n>> +\t\t\treturn &info;\n>> \t}\n>> \n>> \tLOG(RkISP1, Fatal) << \"Can't locate info from buffer\";\n>> @@ -348,11 +343,9 @@ RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n>> \n>> RkISP1FrameInfo *RkISP1Frames::find(Request *request)\n>> {\n>> -\tfor (auto &itInfo : frameInfo_) {\n>> -\t\tRkISP1FrameInfo *info = itInfo.second;\n>> -\n>> -\t\tif (info->request == request)\n>> -\t\t\treturn info;\n>> +\tfor (auto &[frame, info] : frameInfo_) {\n>> +\t\tif (info.request == request)\n>> +\t\t\treturn &info;\n>> \t}\n>> \n>> \tLOG(RkISP1, Fatal) << \"Can't locate info from request\";\n>","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 7A7E4BDCC1\n\tfor ;\n\tFri, 25 Jul 2025 07:18:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 278C569083;\n\tFri, 25 Jul 2025 09:18:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5A3FF69025\n\tfor ;\n\tFri, 25 Jul 2025 09:18:03 +0200 (CEST)","from [192.168.33.15] (185.221.140.39.nat.pool.zt.hu\n\t[185.221.140.39])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A535EBA;\n\tFri, 25 Jul 2025 09:17:23 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"IaSvYrZ6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753427843;\n\tbh=JwtXAVZt8hhf6b+QETpFPl9CRgq6xTrRxTuK8dT+vGU=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=IaSvYrZ6/a/HVExR69wCakK7TUuoQMpmtsMv2WHXjfvR75hco66lkSamWsVhWghsk\n\txJOjByqa8tEnSmLKhx5+9hyJ6SCQz4xc42qR7s8pixVKloBX27suwJc6QWip3t0p5G\n\t5idMW+P4/ud1klcj3T4NfjBuXuIZ9IjE8QrPCj/k=","Message-ID":"<17969e8c-8ec7-46dc-a8b6-f58b91383251@ideasonboard.com>","Date":"Fri, 25 Jul 2025 09:17:59 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v2] libcamera: pipeline: Avoid unnecessary\n\tindirection in frame info map","To":"libcamera-devel@lists.libcamera.org,\n\tLaurent Pinchart ","References":"<20250121190547.304107-1-pobrn@protonmail.com>\n\t<20250724183227.GS11202@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250724183227.GS11202@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35143,"web_url":"https://patchwork.libcamera.org/comment/35143/","msgid":"<20250725151628.GB30386@pendragon.ideasonboard.com>","date":"2025-07-25T15:16:28","subject":"Re: [RFC PATCH v2] libcamera: pipeline: Avoid unnecessary\n\tindirection in frame info map","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Jul 25, 2025 at 09:17:59AM +0200, Barnabás Pőcze wrote:\n> Hi\n> \n> 2025. 07. 24. 20:32 keltezéssel, Laurent Pinchart írta:\n> > Hi Barnabás,\n> > \n> > Thank you for the patch.\n> > \n> > On Tue, Jan 21, 2025 at 07:05:50PM +0000, Barnabás Pőcze wrote:\n> >> There is no reason to allocate the frame info objects dynamically,\n> >> and then store raw pointers in the `std::map` in the rkisp1\n> >> and ipu3 pipeline handler.\n> >>\n> >> Instead, store the objects directly in the map. This removes\n> >> the need for manully calling new/delete, simplifies the code,\n> >> and eliminates one memory allocation per frame.\n> > \n> > We share the common goal of reducing memory allocations at runtime, and\n> > this patch helps by turning two allocations into one. However, I think\n> > it could make it more difficult to get rid of the second allocation.\n> > \n> > Ideally, we should preallocate all the frame info at start time and put\n> > them in a pool. Have you considered how this could be done ? Would it be\n> > as simple as creating the map with a custom allocator ?\n> \n> The map will always allocate nodes, this cannot be avoided. So if you want to\n> improve still, the data structure should be replaced (e.g. to something intrusive).\n> With `std::map`, I think the choice is between a single pool of nodes (which embed\n> the useful data directly), or two pools of nodes (one for nodes, one for the useful data).\n> \n> Something like `std::pmr::unsynchronized_pool_resource` seems like the best choice\n> available in the STL, unfortunately that introduces virtual dispatch for the allocator.\n> Anything better than that would need a custom allocator wrapper I think. There are\n> particular requirements for an stl allocators[0] that memory resources do not satisfy,\n> even if they had the right interface.\n> \n> So in summary, no I have not considered preallocating, and I still think it is a step\n> in the right direction.\n\nOK.\n\nAcked-by: Laurent Pinchart \n\n> [0]: https://en.cppreference.com/w/cpp/named_req/Allocator.html\n> \n> >> Signed-off-by: Barnabás Pőcze \n> >> ---\n> >> src/libcamera/pipeline/ipu3/frames.cpp | 38 ++++++------\n> >> src/libcamera/pipeline/ipu3/frames.h | 2 +-\n> >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 75 +++++++++++-------------\n> >> 3 files changed, 53 insertions(+), 62 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp\n> >> index bc0526a77..f3ea78797 100644\n> >> --- a/src/libcamera/pipeline/ipu3/frames.cpp\n> >> +++ b/src/libcamera/pipeline/ipu3/frames.cpp\n> >> @@ -64,20 +64,20 @@ IPU3Frames::Info *IPU3Frames::create(Request *request)\n> >> \tavailableParamBuffers_.pop();\n> >> \tavailableStatBuffers_.pop();\n> >> \n> >> -\t/* \\todo Remove the dynamic allocation of Info */\n> >> -\tstd::unique_ptr info = std::make_unique();\n> >> +\tauto [it, inserted] = frameInfo_.try_emplace(id);\n> >> +\tASSERT(inserted);\n> >> \n> >> -\tinfo->id = id;\n> >> -\tinfo->request = request;\n> >> -\tinfo->rawBuffer = nullptr;\n> >> -\tinfo->paramBuffer = paramBuffer;\n> >> -\tinfo->statBuffer = statBuffer;\n> >> -\tinfo->paramDequeued = false;\n> >> -\tinfo->metadataProcessed = false;\n> >> +\tauto &info = it->second;\n> >> \n> >> -\tframeInfo_[id] = std::move(info);\n> >> +\tinfo.id = id;\n> >> +\tinfo.request = request;\n> >> +\tinfo.rawBuffer = nullptr;\n> >> +\tinfo.paramBuffer = paramBuffer;\n> >> +\tinfo.statBuffer = statBuffer;\n> >> +\tinfo.paramDequeued = false;\n> >> +\tinfo.metadataProcessed = false;\n> >> \n> >> -\treturn frameInfo_[id].get();\n> >> +\treturn &info;\n> >> }\n> >> \n> >> void IPU3Frames::remove(IPU3Frames::Info *info)\n> >> @@ -115,7 +115,7 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id)\n> >> \tconst auto &itInfo = frameInfo_.find(id);\n> >> \n> >> \tif (itInfo != frameInfo_.end())\n> >> -\t\treturn itInfo->second.get();\n> >> +\t\treturn &itInfo->second;\n> >> \n> >> \tLOG(IPU3, Fatal) << \"Can't find tracking information for frame \" << id;\n> >> \n> >> @@ -124,16 +124,14 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id)\n> >> \n> >> IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer)\n> >> {\n> >> -\tfor (auto const &itInfo : frameInfo_) {\n> >> -\t\tInfo *info = itInfo.second.get();\n> >> -\n> >> -\t\tfor (auto const itBuffers : info->request->buffers())\n> >> +\tfor (auto &[id, info] : frameInfo_) {\n> >> +\t\tfor (auto const itBuffers : info.request->buffers())\n> >> \t\t\tif (itBuffers.second == buffer)\n> >> -\t\t\t\treturn info;\n> >> +\t\t\t\treturn &info;\n> >> \n> >> -\t\tif (info->rawBuffer == buffer || info->paramBuffer == buffer ||\n> >> -\t\t info->statBuffer == buffer)\n> >> -\t\t\treturn info;\n> >> +\t\tif (info.rawBuffer == buffer || info.paramBuffer == buffer ||\n> >> +\t\t info.statBuffer == buffer)\n> >> +\t\t\treturn &info;\n> >> \t}\n> >> \n> >> \tLOG(IPU3, Fatal) << \"Can't find tracking information from buffer\";\n> >> diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h\n> >> index a347b66f3..1cabd0e64 100644\n> >> --- a/src/libcamera/pipeline/ipu3/frames.h\n> >> +++ b/src/libcamera/pipeline/ipu3/frames.h\n> >> @@ -61,7 +61,7 @@ private:\n> >> \tstd::queue availableParamBuffers_;\n> >> \tstd::queue availableStatBuffers_;\n> >> \n> >> -\tstd::map> frameInfo_;\n> >> +\tstd::map frameInfo_;\n> >> };\n> >> \n> >> } /* namespace libcamera */\n> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> index ca8bbaaf1..953138305 100644\n> >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> @@ -84,7 +84,7 @@ public:\n> >> \n> >> private:\n> >> \tPipelineHandlerRkISP1 *pipe_;\n> >> -\tstd::map frameInfo_;\n> >> +\tstd::map frameInfo_;\n> >> };\n> >> \n> >> class RkISP1CameraData : public Camera::Private\n> >> @@ -269,49 +269,46 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> >> \t\tmainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> >> \tselfPathBuffer = request->findBuffer(&data->selfPathStream_);\n> >> \n> >> -\tRkISP1FrameInfo *info = new RkISP1FrameInfo;\n> >> +\tauto [it, inserted] = frameInfo_.try_emplace(frame);\n> >> +\tASSERT(inserted);\n> >> \n> >> -\tinfo->frame = frame;\n> >> -\tinfo->request = request;\n> >> -\tinfo->paramBuffer = paramBuffer;\n> >> -\tinfo->mainPathBuffer = mainPathBuffer;\n> >> -\tinfo->selfPathBuffer = selfPathBuffer;\n> >> -\tinfo->statBuffer = statBuffer;\n> >> -\tinfo->paramDequeued = false;\n> >> -\tinfo->metadataProcessed = false;\n> >> +\tauto &info = it->second;\n> >> \n> >> -\tframeInfo_[frame] = info;\n> >> +\tinfo.frame = frame;\n> >> +\tinfo.request = request;\n> >> +\tinfo.paramBuffer = paramBuffer;\n> >> +\tinfo.mainPathBuffer = mainPathBuffer;\n> >> +\tinfo.selfPathBuffer = selfPathBuffer;\n> >> +\tinfo.statBuffer = statBuffer;\n> >> +\tinfo.paramDequeued = false;\n> >> +\tinfo.metadataProcessed = false;\n> >> \n> >> -\treturn info;\n> >> +\treturn &info;\n> >> }\n> >> \n> >> int RkISP1Frames::destroy(unsigned int frame)\n> >> {\n> >> -\tRkISP1FrameInfo *info = find(frame);\n> >> -\tif (!info)\n> >> +\tauto it = frameInfo_.find(frame);\n> >> +\tif (it == frameInfo_.end())\n> >> \t\treturn -ENOENT;\n> >> \n> >> -\tpipe_->availableParamBuffers_.push(info->paramBuffer);\n> >> -\tpipe_->availableStatBuffers_.push(info->statBuffer);\n> >> -\tpipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n> >> +\tauto &info = it->second;\n> >> \n> >> -\tframeInfo_.erase(info->frame);\n> >> +\tpipe_->availableParamBuffers_.push(info.paramBuffer);\n> >> +\tpipe_->availableStatBuffers_.push(info.statBuffer);\n> >> +\tpipe_->availableMainPathBuffers_.push(info.mainPathBuffer);\n> >> \n> >> -\tdelete info;\n> >> +\tframeInfo_.erase(it);\n> >> \n> >> \treturn 0;\n> >> }\n> >> \n> >> void RkISP1Frames::clear()\n> >> {\n> >> -\tfor (const auto &entry : frameInfo_) {\n> >> -\t\tRkISP1FrameInfo *info = entry.second;\n> >> -\n> >> -\t\tpipe_->availableParamBuffers_.push(info->paramBuffer);\n> >> -\t\tpipe_->availableStatBuffers_.push(info->statBuffer);\n> >> -\t\tpipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n> >> -\n> >> -\t\tdelete info;\n> >> +\tfor (const auto &[frame, info] : frameInfo_) {\n> >> +\t\tpipe_->availableParamBuffers_.push(info.paramBuffer);\n> >> +\t\tpipe_->availableStatBuffers_.push(info.statBuffer);\n> >> +\t\tpipe_->availableMainPathBuffers_.push(info.mainPathBuffer);\n> >> \t}\n> >> \n> >> \tframeInfo_.clear();\n> >> @@ -322,7 +319,7 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n> >> \tauto itInfo = frameInfo_.find(frame);\n> >> \n> >> \tif (itInfo != frameInfo_.end())\n> >> -\t\treturn itInfo->second;\n> >> +\t\treturn &itInfo->second;\n> >> \n> >> \tLOG(RkISP1, Fatal) << \"Can't locate info from frame\";\n> >> \n> >> @@ -331,14 +328,12 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n> >> \n> >> RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n> >> {\n> >> -\tfor (auto &itInfo : frameInfo_) {\n> >> -\t\tRkISP1FrameInfo *info = itInfo.second;\n> >> -\n> >> -\t\tif (info->paramBuffer == buffer ||\n> >> -\t\t info->statBuffer == buffer ||\n> >> -\t\t info->mainPathBuffer == buffer ||\n> >> -\t\t info->selfPathBuffer == buffer)\n> >> -\t\t\treturn info;\n> >> +\tfor (auto &[frame, info] : frameInfo_) {\n> >> +\t\tif (info.paramBuffer == buffer ||\n> >> +\t\t info.statBuffer == buffer ||\n> >> +\t\t info.mainPathBuffer == buffer ||\n> >> +\t\t info.selfPathBuffer == buffer)\n> >> +\t\t\treturn &info;\n> >> \t}\n> >> \n> >> \tLOG(RkISP1, Fatal) << \"Can't locate info from buffer\";\n> >> @@ -348,11 +343,9 @@ RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n> >> \n> >> RkISP1FrameInfo *RkISP1Frames::find(Request *request)\n> >> {\n> >> -\tfor (auto &itInfo : frameInfo_) {\n> >> -\t\tRkISP1FrameInfo *info = itInfo.second;\n> >> -\n> >> -\t\tif (info->request == request)\n> >> -\t\t\treturn info;\n> >> +\tfor (auto &[frame, info] : frameInfo_) {\n> >> +\t\tif (info.request == request)\n> >> +\t\t\treturn &info;\n> >> \t}\n> >> \n> >> \tLOG(RkISP1, Fatal) << \"Can't locate info from request\";","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 5C594C3237\n\tfor ;\n\tFri, 25 Jul 2025 15:16:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 010F0690F8;\n\tFri, 25 Jul 2025 17:16:34 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1BA98690A6\n\tfor ;\n\tFri, 25 Jul 2025 17:16:33 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 296E4C66;\n\tFri, 25 Jul 2025 17:15:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"MJsaQNXK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753456553;\n\tbh=5PoKs1ZSUdD+HYxBapVMq9hDUHqw8F9vkWb3RMySmX4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MJsaQNXK+QjjfIpTb22+jvKQbC0n6PG4zlPYdxTyghFumeWtChWQn4+CGVt1IO5KW\n\tzaDJHo5040RmZoai5nZfkw1XlsHGaAKtuQ/ygPnUV2/JobmD/l9f6/A/sImQwCsgRA\n\tiOoFr14nrUvoWORvB93gh6D69dkPQH2TArcPFlgY=","Date":"Fri, 25 Jul 2025 18:16:28 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2] libcamera: pipeline: Avoid unnecessary\n\tindirection in frame info map","Message-ID":"<20250725151628.GB30386@pendragon.ideasonboard.com>","References":"<20250121190547.304107-1-pobrn@protonmail.com>\n\t<20250724183227.GS11202@pendragon.ideasonboard.com>\n\t<17969e8c-8ec7-46dc-a8b6-f58b91383251@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<17969e8c-8ec7-46dc-a8b6-f58b91383251@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35386,"web_url":"https://patchwork.libcamera.org/comment/35386/","msgid":"<2b5a9711-c279-40c9-88e3-49521d67289e@ideasonboard.com>","date":"2025-08-13T13:22:05","subject":"Re: [RFC PATCH v2] libcamera: pipeline: Avoid unnecessary\n\tindirection in frame info map","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 07. 21. 20:13 keltezéssel, Kieran Bingham írta:\n> Quoting Barnabás Pőcze (2025-01-21 19:05:50)\n>> There is no reason to allocate the frame info objects dynamically,\n>> and then store raw pointers in the `std::map` in the rkisp1\n>> and ipu3 pipeline handler.\n>>\n>> Instead, store the objects directly in the map. This removes\n>> the need for manully calling new/delete, simplifies the code,\n>> and eliminates one memory allocation per frame.\n> \n> This sounds like a good thing. Is it still 'RFC'? Maybe we should try it\n> on some RKISP1 devices and test...\n> \n> Is patch this still relevant? It was posted in January ... I suspect it\n> is.\n> \n> I'm trying to dig into some issues where I think we get out of sync with\n> image and stats/param buffers in error paths - so I'll try to test this\n> along with my investigations there.\n\nI would like to go ahead with this change. Or should I wait?\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> \n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> src/libcamera/pipeline/ipu3/frames.cpp | 38 ++++++------\n>> src/libcamera/pipeline/ipu3/frames.h | 2 +-\n>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 75 +++++++++++-------------\n>> 3 files changed, 53 insertions(+), 62 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp\n>> index bc0526a77..f3ea78797 100644\n>> --- a/src/libcamera/pipeline/ipu3/frames.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/frames.cpp\n>> @@ -64,20 +64,20 @@ IPU3Frames::Info *IPU3Frames::create(Request *request)\n>> availableParamBuffers_.pop();\n>> availableStatBuffers_.pop();\n>> \n>> - /* \\todo Remove the dynamic allocation of Info */\n>> - std::unique_ptr info = std::make_unique();\n>> + auto [it, inserted] = frameInfo_.try_emplace(id);\n>> + ASSERT(inserted);\n>> \n>> - info->id = id;\n>> - info->request = request;\n>> - info->rawBuffer = nullptr;\n>> - info->paramBuffer = paramBuffer;\n>> - info->statBuffer = statBuffer;\n>> - info->paramDequeued = false;\n>> - info->metadataProcessed = false;\n>> + auto &info = it->second;\n>> \n>> - frameInfo_[id] = std::move(info);\n>> + info.id = id;\n>> + info.request = request;\n>> + info.rawBuffer = nullptr;\n>> + info.paramBuffer = paramBuffer;\n>> + info.statBuffer = statBuffer;\n>> + info.paramDequeued = false;\n>> + info.metadataProcessed = false;\n>> \n>> - return frameInfo_[id].get();\n>> + return &info;\n>> }\n>> \n>> void IPU3Frames::remove(IPU3Frames::Info *info)\n>> @@ -115,7 +115,7 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id)\n>> const auto &itInfo = frameInfo_.find(id);\n>> \n>> if (itInfo != frameInfo_.end())\n>> - return itInfo->second.get();\n>> + return &itInfo->second;\n>> \n>> LOG(IPU3, Fatal) << \"Can't find tracking information for frame \" << id;\n>> \n>> @@ -124,16 +124,14 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id)\n>> \n>> IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer)\n>> {\n>> - for (auto const &itInfo : frameInfo_) {\n>> - Info *info = itInfo.second.get();\n>> -\n>> - for (auto const itBuffers : info->request->buffers())\n>> + for (auto &[id, info] : frameInfo_) {\n>> + for (auto const itBuffers : info.request->buffers())\n>> if (itBuffers.second == buffer)\n>> - return info;\n>> + return &info;\n>> \n>> - if (info->rawBuffer == buffer || info->paramBuffer == buffer ||\n>> - info->statBuffer == buffer)\n>> - return info;\n>> + if (info.rawBuffer == buffer || info.paramBuffer == buffer ||\n>> + info.statBuffer == buffer)\n>> + return &info;\n>> }\n>> \n>> LOG(IPU3, Fatal) << \"Can't find tracking information from buffer\";\n>> diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h\n>> index a347b66f3..1cabd0e64 100644\n>> --- a/src/libcamera/pipeline/ipu3/frames.h\n>> +++ b/src/libcamera/pipeline/ipu3/frames.h\n>> @@ -61,7 +61,7 @@ private:\n>> std::queue availableParamBuffers_;\n>> std::queue availableStatBuffers_;\n>> \n>> - std::map> frameInfo_;\n>> + std::map frameInfo_;\n>> };\n>> \n>> } /* namespace libcamera */\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> index ca8bbaaf1..953138305 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> @@ -84,7 +84,7 @@ public:\n>> \n>> private:\n>> PipelineHandlerRkISP1 *pipe_;\n>> - std::map frameInfo_;\n>> + std::map frameInfo_;\n>> };\n>> \n>> class RkISP1CameraData : public Camera::Private\n>> @@ -269,49 +269,46 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n>> mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n>> selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n>> \n>> - RkISP1FrameInfo *info = new RkISP1FrameInfo;\n>> + auto [it, inserted] = frameInfo_.try_emplace(frame);\n>> + ASSERT(inserted);\n>> \n>> - info->frame = frame;\n>> - info->request = request;\n>> - info->paramBuffer = paramBuffer;\n>> - info->mainPathBuffer = mainPathBuffer;\n>> - info->selfPathBuffer = selfPathBuffer;\n>> - info->statBuffer = statBuffer;\n>> - info->paramDequeued = false;\n>> - info->metadataProcessed = false;\n>> + auto &info = it->second;\n>> \n>> - frameInfo_[frame] = info;\n>> + info.frame = frame;\n>> + info.request = request;\n>> + info.paramBuffer = paramBuffer;\n>> + info.mainPathBuffer = mainPathBuffer;\n>> + info.selfPathBuffer = selfPathBuffer;\n>> + info.statBuffer = statBuffer;\n>> + info.paramDequeued = false;\n>> + info.metadataProcessed = false;\n>> \n>> - return info;\n>> + return &info;\n>> }\n>> \n>> int RkISP1Frames::destroy(unsigned int frame)\n>> {\n>> - RkISP1FrameInfo *info = find(frame);\n>> - if (!info)\n>> + auto it = frameInfo_.find(frame);\n>> + if (it == frameInfo_.end())\n>> return -ENOENT;\n>> \n>> - pipe_->availableParamBuffers_.push(info->paramBuffer);\n>> - pipe_->availableStatBuffers_.push(info->statBuffer);\n>> - pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n>> + auto &info = it->second;\n>> \n>> - frameInfo_.erase(info->frame);\n>> + pipe_->availableParamBuffers_.push(info.paramBuffer);\n>> + pipe_->availableStatBuffers_.push(info.statBuffer);\n>> + pipe_->availableMainPathBuffers_.push(info.mainPathBuffer);\n>> \n>> - delete info;\n>> + frameInfo_.erase(it);\n>> \n>> return 0;\n>> }\n>> \n>> void RkISP1Frames::clear()\n>> {\n>> - for (const auto &entry : frameInfo_) {\n>> - RkISP1FrameInfo *info = entry.second;\n>> -\n>> - pipe_->availableParamBuffers_.push(info->paramBuffer);\n>> - pipe_->availableStatBuffers_.push(info->statBuffer);\n>> - pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n>> -\n>> - delete info;\n>> + for (const auto &[frame, info] : frameInfo_) {\n>> + pipe_->availableParamBuffers_.push(info.paramBuffer);\n>> + pipe_->availableStatBuffers_.push(info.statBuffer);\n>> + pipe_->availableMainPathBuffers_.push(info.mainPathBuffer);\n>> }\n>> \n>> frameInfo_.clear();\n>> @@ -322,7 +319,7 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n>> auto itInfo = frameInfo_.find(frame);\n>> \n>> if (itInfo != frameInfo_.end())\n>> - return itInfo->second;\n>> + return &itInfo->second;\n>> \n>> LOG(RkISP1, Fatal) << \"Can't locate info from frame\";\n>> \n>> @@ -331,14 +328,12 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n>> \n>> RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n>> {\n>> - for (auto &itInfo : frameInfo_) {\n>> - RkISP1FrameInfo *info = itInfo.second;\n>> -\n>> - if (info->paramBuffer == buffer ||\n>> - info->statBuffer == buffer ||\n>> - info->mainPathBuffer == buffer ||\n>> - info->selfPathBuffer == buffer)\n>> - return info;\n>> + for (auto &[frame, info] : frameInfo_) {\n>> + if (info.paramBuffer == buffer ||\n>> + info.statBuffer == buffer ||\n>> + info.mainPathBuffer == buffer ||\n>> + info.selfPathBuffer == buffer)\n>> + return &info;\n>> }\n>> \n>> LOG(RkISP1, Fatal) << \"Can't locate info from buffer\";\n>> @@ -348,11 +343,9 @@ RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n>> \n>> RkISP1FrameInfo *RkISP1Frames::find(Request *request)\n>> {\n>> - for (auto &itInfo : frameInfo_) {\n>> - RkISP1FrameInfo *info = itInfo.second;\n>> -\n>> - if (info->request == request)\n>> - return info;\n>> + for (auto &[frame, info] : frameInfo_) {\n>> + if (info.request == request)\n>> + return &info;\n>> }\n>> \n>> LOG(RkISP1, Fatal) << \"Can't locate info from request\";\n>> -- \n>> 2.48.1\n>>\n>>","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 E6D5ABDCC1\n\tfor ;\n\tWed, 13 Aug 2025 13:22:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BAE6F69249;\n\tWed, 13 Aug 2025 15:22:09 +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 9AA8E61444\n\tfor ;\n\tWed, 13 Aug 2025 15:22:08 +0200 (CEST)","from [192.168.33.21] (185.221.141.188.nat.pool.zt.hu\n\t[185.221.141.188])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EA19B3A4;\n\tWed, 13 Aug 2025 15:21:14 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"J8+TYHsg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755091275;\n\tbh=2FWZp/f69z/JabdKrKvnFk6suZZA4MUBfENa00SrTZM=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=J8+TYHsglpu/06FamBtnowu1l5osClLXu7ynlHFe35tmObRMULslzDKFYEO85pLav\n\tWyNJ7ZDdAhlKyFcuu2lwO0ZaQdaIrEX1/XLk8sU64n2k2gcYpugS6giKG54R37EWxE\n\tI9a7E9n2JMvDsxQvEOieag0KKpbUTwH0NbDuyXKU=","Message-ID":"<2b5a9711-c279-40c9-88e3-49521d67289e@ideasonboard.com>","Date":"Wed, 13 Aug 2025 15:22:05 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v2] libcamera: pipeline: Avoid unnecessary\n\tindirection in frame info map","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250121190547.304107-1-pobrn@protonmail.com>\n\t<175312160641.50296.13088832792560201008@ping.linuxembedded.co.uk>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<175312160641.50296.13088832792560201008@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35387,"web_url":"https://patchwork.libcamera.org/comment/35387/","msgid":"<175509222320.560048.5902175810745634714@ping.linuxembedded.co.uk>","date":"2025-08-13T13:37:03","subject":"Re: [RFC PATCH v2] libcamera: pipeline: Avoid unnecessary\n\tindirection in frame info map","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-08-13 14:22:05)\n> Hi\n> \n> \n> 2025. 07. 21. 20:13 keltezéssel, Kieran Bingham írta:\n> > Quoting Barnabás Pőcze (2025-01-21 19:05:50)\n> >> There is no reason to allocate the frame info objects dynamically,\n> >> and then store raw pointers in the `std::map` in the rkisp1\n> >> and ipu3 pipeline handler.\n> >>\n> >> Instead, store the objects directly in the map. This removes\n> >> the need for manully calling new/delete, simplifies the code,\n> >> and eliminates one memory allocation per frame.\n> > \n> > This sounds like a good thing. Is it still 'RFC'? Maybe we should try it\n> > on some RKISP1 devices and test...\n> > \n> > Is patch this still relevant? It was posted in January ... I suspect it\n> > is.\n> > \n> > I'm trying to dig into some issues where I think we get out of sync with\n> > image and stats/param buffers in error paths - so I'll try to test this\n> > along with my investigations there.\n> \n> I would like to go ahead with this change. Or should I wait?\n\nThis patch is successfully running on my local i.MX8MP board with FPGA\nand hotplug cameras - so I think that's a pass.\n\nTested-by: Kieran Bingham \nReviewed-by: Kieran Bingham \n\n> Regards,\n> Barnabás Pőcze\n> \n> \n> > \n> > \n> >>\n> >> Signed-off-by: Barnabás Pőcze \n> >> ---\n> >> src/libcamera/pipeline/ipu3/frames.cpp | 38 ++++++------\n> >> src/libcamera/pipeline/ipu3/frames.h | 2 +-\n> >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 75 +++++++++++-------------\n> >> 3 files changed, 53 insertions(+), 62 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp\n> >> index bc0526a77..f3ea78797 100644\n> >> --- a/src/libcamera/pipeline/ipu3/frames.cpp\n> >> +++ b/src/libcamera/pipeline/ipu3/frames.cpp\n> >> @@ -64,20 +64,20 @@ IPU3Frames::Info *IPU3Frames::create(Request *request)\n> >> availableParamBuffers_.pop();\n> >> availableStatBuffers_.pop();\n> >> \n> >> - /* \\todo Remove the dynamic allocation of Info */\n> >> - std::unique_ptr info = std::make_unique();\n> >> + auto [it, inserted] = frameInfo_.try_emplace(id);\n> >> + ASSERT(inserted);\n> >> \n> >> - info->id = id;\n> >> - info->request = request;\n> >> - info->rawBuffer = nullptr;\n> >> - info->paramBuffer = paramBuffer;\n> >> - info->statBuffer = statBuffer;\n> >> - info->paramDequeued = false;\n> >> - info->metadataProcessed = false;\n> >> + auto &info = it->second;\n> >> \n> >> - frameInfo_[id] = std::move(info);\n> >> + info.id = id;\n> >> + info.request = request;\n> >> + info.rawBuffer = nullptr;\n> >> + info.paramBuffer = paramBuffer;\n> >> + info.statBuffer = statBuffer;\n> >> + info.paramDequeued = false;\n> >> + info.metadataProcessed = false;\n> >> \n> >> - return frameInfo_[id].get();\n> >> + return &info;\n> >> }\n> >> \n> >> void IPU3Frames::remove(IPU3Frames::Info *info)\n> >> @@ -115,7 +115,7 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id)\n> >> const auto &itInfo = frameInfo_.find(id);\n> >> \n> >> if (itInfo != frameInfo_.end())\n> >> - return itInfo->second.get();\n> >> + return &itInfo->second;\n> >> \n> >> LOG(IPU3, Fatal) << \"Can't find tracking information for frame \" << id;\n> >> \n> >> @@ -124,16 +124,14 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id)\n> >> \n> >> IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer)\n> >> {\n> >> - for (auto const &itInfo : frameInfo_) {\n> >> - Info *info = itInfo.second.get();\n> >> -\n> >> - for (auto const itBuffers : info->request->buffers())\n> >> + for (auto &[id, info] : frameInfo_) {\n> >> + for (auto const itBuffers : info.request->buffers())\n> >> if (itBuffers.second == buffer)\n> >> - return info;\n> >> + return &info;\n> >> \n> >> - if (info->rawBuffer == buffer || info->paramBuffer == buffer ||\n> >> - info->statBuffer == buffer)\n> >> - return info;\n> >> + if (info.rawBuffer == buffer || info.paramBuffer == buffer ||\n> >> + info.statBuffer == buffer)\n> >> + return &info;\n> >> }\n> >> \n> >> LOG(IPU3, Fatal) << \"Can't find tracking information from buffer\";\n> >> diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h\n> >> index a347b66f3..1cabd0e64 100644\n> >> --- a/src/libcamera/pipeline/ipu3/frames.h\n> >> +++ b/src/libcamera/pipeline/ipu3/frames.h\n> >> @@ -61,7 +61,7 @@ private:\n> >> std::queue availableParamBuffers_;\n> >> std::queue availableStatBuffers_;\n> >> \n> >> - std::map> frameInfo_;\n> >> + std::map frameInfo_;\n> >> };\n> >> \n> >> } /* namespace libcamera */\n> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> index ca8bbaaf1..953138305 100644\n> >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> @@ -84,7 +84,7 @@ public:\n> >> \n> >> private:\n> >> PipelineHandlerRkISP1 *pipe_;\n> >> - std::map frameInfo_;\n> >> + std::map frameInfo_;\n> >> };\n> >> \n> >> class RkISP1CameraData : public Camera::Private\n> >> @@ -269,49 +269,46 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> >> mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> >> selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n> >> \n> >> - RkISP1FrameInfo *info = new RkISP1FrameInfo;\n> >> + auto [it, inserted] = frameInfo_.try_emplace(frame);\n> >> + ASSERT(inserted);\n> >> \n> >> - info->frame = frame;\n> >> - info->request = request;\n> >> - info->paramBuffer = paramBuffer;\n> >> - info->mainPathBuffer = mainPathBuffer;\n> >> - info->selfPathBuffer = selfPathBuffer;\n> >> - info->statBuffer = statBuffer;\n> >> - info->paramDequeued = false;\n> >> - info->metadataProcessed = false;\n> >> + auto &info = it->second;\n> >> \n> >> - frameInfo_[frame] = info;\n> >> + info.frame = frame;\n> >> + info.request = request;\n> >> + info.paramBuffer = paramBuffer;\n> >> + info.mainPathBuffer = mainPathBuffer;\n> >> + info.selfPathBuffer = selfPathBuffer;\n> >> + info.statBuffer = statBuffer;\n> >> + info.paramDequeued = false;\n> >> + info.metadataProcessed = false;\n> >> \n> >> - return info;\n> >> + return &info;\n> >> }\n> >> \n> >> int RkISP1Frames::destroy(unsigned int frame)\n> >> {\n> >> - RkISP1FrameInfo *info = find(frame);\n> >> - if (!info)\n> >> + auto it = frameInfo_.find(frame);\n> >> + if (it == frameInfo_.end())\n> >> return -ENOENT;\n> >> \n> >> - pipe_->availableParamBuffers_.push(info->paramBuffer);\n> >> - pipe_->availableStatBuffers_.push(info->statBuffer);\n> >> - pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n> >> + auto &info = it->second;\n> >> \n> >> - frameInfo_.erase(info->frame);\n> >> + pipe_->availableParamBuffers_.push(info.paramBuffer);\n> >> + pipe_->availableStatBuffers_.push(info.statBuffer);\n> >> + pipe_->availableMainPathBuffers_.push(info.mainPathBuffer);\n> >> \n> >> - delete info;\n> >> + frameInfo_.erase(it);\n> >> \n> >> return 0;\n> >> }\n> >> \n> >> void RkISP1Frames::clear()\n> >> {\n> >> - for (const auto &entry : frameInfo_) {\n> >> - RkISP1FrameInfo *info = entry.second;\n> >> -\n> >> - pipe_->availableParamBuffers_.push(info->paramBuffer);\n> >> - pipe_->availableStatBuffers_.push(info->statBuffer);\n> >> - pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n> >> -\n> >> - delete info;\n> >> + for (const auto &[frame, info] : frameInfo_) {\n> >> + pipe_->availableParamBuffers_.push(info.paramBuffer);\n> >> + pipe_->availableStatBuffers_.push(info.statBuffer);\n> >> + pipe_->availableMainPathBuffers_.push(info.mainPathBuffer);\n> >> }\n> >> \n> >> frameInfo_.clear();\n> >> @@ -322,7 +319,7 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n> >> auto itInfo = frameInfo_.find(frame);\n> >> \n> >> if (itInfo != frameInfo_.end())\n> >> - return itInfo->second;\n> >> + return &itInfo->second;\n> >> \n> >> LOG(RkISP1, Fatal) << \"Can't locate info from frame\";\n> >> \n> >> @@ -331,14 +328,12 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n> >> \n> >> RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n> >> {\n> >> - for (auto &itInfo : frameInfo_) {\n> >> - RkISP1FrameInfo *info = itInfo.second;\n> >> -\n> >> - if (info->paramBuffer == buffer ||\n> >> - info->statBuffer == buffer ||\n> >> - info->mainPathBuffer == buffer ||\n> >> - info->selfPathBuffer == buffer)\n> >> - return info;\n> >> + for (auto &[frame, info] : frameInfo_) {\n> >> + if (info.paramBuffer == buffer ||\n> >> + info.statBuffer == buffer ||\n> >> + info.mainPathBuffer == buffer ||\n> >> + info.selfPathBuffer == buffer)\n> >> + return &info;\n> >> }\n> >> \n> >> LOG(RkISP1, Fatal) << \"Can't locate info from buffer\";\n> >> @@ -348,11 +343,9 @@ RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n> >> \n> >> RkISP1FrameInfo *RkISP1Frames::find(Request *request)\n> >> {\n> >> - for (auto &itInfo : frameInfo_) {\n> >> - RkISP1FrameInfo *info = itInfo.second;\n> >> -\n> >> - if (info->request == request)\n> >> - return info;\n> >> + for (auto &[frame, info] : frameInfo_) {\n> >> + if (info.request == request)\n> >> + return &info;\n> >> }\n> >> \n> >> LOG(RkISP1, Fatal) << \"Can't locate info from request\";\n> >> -- \n> >> 2.48.1\n> >>\n> >>\n>","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 A2DBCBEFBE\n\tfor ;\n\tWed, 13 Aug 2025 13:37:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B630D69249;\n\tWed, 13 Aug 2025 15:37:07 +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 3A9D961444\n\tfor ;\n\tWed, 13 Aug 2025 15:37:06 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8EE443A4;\n\tWed, 13 Aug 2025 15:36:12 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"J/zp02Bh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755092172;\n\tbh=Ex3mcPXkBmgSA7nWCDSGhd9rzefZVjJ+EjmNon98QjQ=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=J/zp02BhLSPOjz1X7UfxREFwAwlWz5bBWnH4ppOVGagIESjrkI4g0hj7cL1hQbha9\n\tTnQeeQ+Ev40iIOejC+7Yab9izooaj0heEbC6E487GR07JFos2NSCN5xcl5F070/KPQ\n\t30WLvlICPwz6rWsArd68WnHNVy4HvjQWzqc7b2MM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<2b5a9711-c279-40c9-88e3-49521d67289e@ideasonboard.com>","References":"<20250121190547.304107-1-pobrn@protonmail.com>\n\t<175312160641.50296.13088832792560201008@ping.linuxembedded.co.uk>\n\t<2b5a9711-c279-40c9-88e3-49521d67289e@ideasonboard.com>","Subject":"Re: [RFC PATCH v2] libcamera: pipeline: Avoid unnecessary\n\tindirection in frame info map","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 13 Aug 2025 14:37:03 +0100","Message-ID":"<175509222320.560048.5902175810745634714@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35388,"web_url":"https://patchwork.libcamera.org/comment/35388/","msgid":"","date":"2025-08-13T13:42:42","subject":"Re: [RFC PATCH v2] libcamera: pipeline: Avoid unnecessary\n\tindirection in frame info map","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 08. 13. 15:37 keltezéssel, Kieran Bingham írta:\n> Quoting Barnabás Pőcze (2025-08-13 14:22:05)\n>> Hi\n>>\n>>\n>> 2025. 07. 21. 20:13 keltezéssel, Kieran Bingham írta:\n>>> Quoting Barnabás Pőcze (2025-01-21 19:05:50)\n>>>> There is no reason to allocate the frame info objects dynamically,\n>>>> and then store raw pointers in the `std::map` in the rkisp1\n>>>> and ipu3 pipeline handler.\n>>>>\n>>>> Instead, store the objects directly in the map. This removes\n>>>> the need for manully calling new/delete, simplifies the code,\n>>>> and eliminates one memory allocation per frame.\n>>>\n>>> This sounds like a good thing. Is it still 'RFC'? Maybe we should try it\n>>> on some RKISP1 devices and test...\n>>>\n>>> Is patch this still relevant? It was posted in January ... I suspect it\n>>> is.\n>>>\n>>> I'm trying to dig into some issues where I think we get out of sync with\n>>> image and stats/param buffers in error paths - so I'll try to test this\n>>> along with my investigations there.\n>>\n>> I would like to go ahead with this change. Or should I wait?\n> \n> This patch is successfully running on my local i.MX8MP board with FPGA\n> and hotplug cameras - so I think that's a pass.\n> \n> Tested-by: Kieran Bingham \n> Reviewed-by: Kieran Bingham \n\nOkay, thanks. Let's hope ipu3 works as just as well.\n\n\n> \n>> Regards,\n>> Barnabás Pőcze\n>>\n>>\n>>>\n>>>\n>>>>\n>>>> Signed-off-by: Barnabás Pőcze \n>>>> ---\n>>>> src/libcamera/pipeline/ipu3/frames.cpp | 38 ++++++------\n>>>> src/libcamera/pipeline/ipu3/frames.h | 2 +-\n>>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 75 +++++++++++-------------\n>>>> 3 files changed, 53 insertions(+), 62 deletions(-)\n>>>>\n>>>> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp\n>>>> index bc0526a77..f3ea78797 100644\n>>>> --- a/src/libcamera/pipeline/ipu3/frames.cpp\n>>>> +++ b/src/libcamera/pipeline/ipu3/frames.cpp\n>>>> @@ -64,20 +64,20 @@ IPU3Frames::Info *IPU3Frames::create(Request *request)\n>>>> availableParamBuffers_.pop();\n>>>> availableStatBuffers_.pop();\n>>>> \n>>>> - /* \\todo Remove the dynamic allocation of Info */\n>>>> - std::unique_ptr info = std::make_unique();\n>>>> + auto [it, inserted] = frameInfo_.try_emplace(id);\n>>>> + ASSERT(inserted);\n>>>> \n>>>> - info->id = id;\n>>>> - info->request = request;\n>>>> - info->rawBuffer = nullptr;\n>>>> - info->paramBuffer = paramBuffer;\n>>>> - info->statBuffer = statBuffer;\n>>>> - info->paramDequeued = false;\n>>>> - info->metadataProcessed = false;\n>>>> + auto &info = it->second;\n>>>> \n>>>> - frameInfo_[id] = std::move(info);\n>>>> + info.id = id;\n>>>> + info.request = request;\n>>>> + info.rawBuffer = nullptr;\n>>>> + info.paramBuffer = paramBuffer;\n>>>> + info.statBuffer = statBuffer;\n>>>> + info.paramDequeued = false;\n>>>> + info.metadataProcessed = false;\n>>>> \n>>>> - return frameInfo_[id].get();\n>>>> + return &info;\n>>>> }\n>>>> \n>>>> void IPU3Frames::remove(IPU3Frames::Info *info)\n>>>> @@ -115,7 +115,7 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id)\n>>>> const auto &itInfo = frameInfo_.find(id);\n>>>> \n>>>> if (itInfo != frameInfo_.end())\n>>>> - return itInfo->second.get();\n>>>> + return &itInfo->second;\n>>>> \n>>>> LOG(IPU3, Fatal) << \"Can't find tracking information for frame \" << id;\n>>>> \n>>>> @@ -124,16 +124,14 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id)\n>>>> \n>>>> IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer)\n>>>> {\n>>>> - for (auto const &itInfo : frameInfo_) {\n>>>> - Info *info = itInfo.second.get();\n>>>> -\n>>>> - for (auto const itBuffers : info->request->buffers())\n>>>> + for (auto &[id, info] : frameInfo_) {\n>>>> + for (auto const itBuffers : info.request->buffers())\n>>>> if (itBuffers.second == buffer)\n>>>> - return info;\n>>>> + return &info;\n>>>> \n>>>> - if (info->rawBuffer == buffer || info->paramBuffer == buffer ||\n>>>> - info->statBuffer == buffer)\n>>>> - return info;\n>>>> + if (info.rawBuffer == buffer || info.paramBuffer == buffer ||\n>>>> + info.statBuffer == buffer)\n>>>> + return &info;\n>>>> }\n>>>> \n>>>> LOG(IPU3, Fatal) << \"Can't find tracking information from buffer\";\n>>>> diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h\n>>>> index a347b66f3..1cabd0e64 100644\n>>>> --- a/src/libcamera/pipeline/ipu3/frames.h\n>>>> +++ b/src/libcamera/pipeline/ipu3/frames.h\n>>>> @@ -61,7 +61,7 @@ private:\n>>>> std::queue availableParamBuffers_;\n>>>> std::queue availableStatBuffers_;\n>>>> \n>>>> - std::map> frameInfo_;\n>>>> + std::map frameInfo_;\n>>>> };\n>>>> \n>>>> } /* namespace libcamera */\n>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>> index ca8bbaaf1..953138305 100644\n>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>> @@ -84,7 +84,7 @@ public:\n>>>> \n>>>> private:\n>>>> PipelineHandlerRkISP1 *pipe_;\n>>>> - std::map frameInfo_;\n>>>> + std::map frameInfo_;\n>>>> };\n>>>> \n>>>> class RkISP1CameraData : public Camera::Private\n>>>> @@ -269,49 +269,46 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n>>>> mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n>>>> selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n>>>> \n>>>> - RkISP1FrameInfo *info = new RkISP1FrameInfo;\n>>>> + auto [it, inserted] = frameInfo_.try_emplace(frame);\n>>>> + ASSERT(inserted);\n>>>> \n>>>> - info->frame = frame;\n>>>> - info->request = request;\n>>>> - info->paramBuffer = paramBuffer;\n>>>> - info->mainPathBuffer = mainPathBuffer;\n>>>> - info->selfPathBuffer = selfPathBuffer;\n>>>> - info->statBuffer = statBuffer;\n>>>> - info->paramDequeued = false;\n>>>> - info->metadataProcessed = false;\n>>>> + auto &info = it->second;\n>>>> \n>>>> - frameInfo_[frame] = info;\n>>>> + info.frame = frame;\n>>>> + info.request = request;\n>>>> + info.paramBuffer = paramBuffer;\n>>>> + info.mainPathBuffer = mainPathBuffer;\n>>>> + info.selfPathBuffer = selfPathBuffer;\n>>>> + info.statBuffer = statBuffer;\n>>>> + info.paramDequeued = false;\n>>>> + info.metadataProcessed = false;\n>>>> \n>>>> - return info;\n>>>> + return &info;\n>>>> }\n>>>> \n>>>> int RkISP1Frames::destroy(unsigned int frame)\n>>>> {\n>>>> - RkISP1FrameInfo *info = find(frame);\n>>>> - if (!info)\n>>>> + auto it = frameInfo_.find(frame);\n>>>> + if (it == frameInfo_.end())\n>>>> return -ENOENT;\n>>>> \n>>>> - pipe_->availableParamBuffers_.push(info->paramBuffer);\n>>>> - pipe_->availableStatBuffers_.push(info->statBuffer);\n>>>> - pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n>>>> + auto &info = it->second;\n>>>> \n>>>> - frameInfo_.erase(info->frame);\n>>>> + pipe_->availableParamBuffers_.push(info.paramBuffer);\n>>>> + pipe_->availableStatBuffers_.push(info.statBuffer);\n>>>> + pipe_->availableMainPathBuffers_.push(info.mainPathBuffer);\n>>>> \n>>>> - delete info;\n>>>> + frameInfo_.erase(it);\n>>>> \n>>>> return 0;\n>>>> }\n>>>> \n>>>> void RkISP1Frames::clear()\n>>>> {\n>>>> - for (const auto &entry : frameInfo_) {\n>>>> - RkISP1FrameInfo *info = entry.second;\n>>>> -\n>>>> - pipe_->availableParamBuffers_.push(info->paramBuffer);\n>>>> - pipe_->availableStatBuffers_.push(info->statBuffer);\n>>>> - pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);\n>>>> -\n>>>> - delete info;\n>>>> + for (const auto &[frame, info] : frameInfo_) {\n>>>> + pipe_->availableParamBuffers_.push(info.paramBuffer);\n>>>> + pipe_->availableStatBuffers_.push(info.statBuffer);\n>>>> + pipe_->availableMainPathBuffers_.push(info.mainPathBuffer);\n>>>> }\n>>>> \n>>>> frameInfo_.clear();\n>>>> @@ -322,7 +319,7 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n>>>> auto itInfo = frameInfo_.find(frame);\n>>>> \n>>>> if (itInfo != frameInfo_.end())\n>>>> - return itInfo->second;\n>>>> + return &itInfo->second;\n>>>> \n>>>> LOG(RkISP1, Fatal) << \"Can't locate info from frame\";\n>>>> \n>>>> @@ -331,14 +328,12 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n>>>> \n>>>> RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n>>>> {\n>>>> - for (auto &itInfo : frameInfo_) {\n>>>> - RkISP1FrameInfo *info = itInfo.second;\n>>>> -\n>>>> - if (info->paramBuffer == buffer ||\n>>>> - info->statBuffer == buffer ||\n>>>> - info->mainPathBuffer == buffer ||\n>>>> - info->selfPathBuffer == buffer)\n>>>> - return info;\n>>>> + for (auto &[frame, info] : frameInfo_) {\n>>>> + if (info.paramBuffer == buffer ||\n>>>> + info.statBuffer == buffer ||\n>>>> + info.mainPathBuffer == buffer ||\n>>>> + info.selfPathBuffer == buffer)\n>>>> + return &info;\n>>>> }\n>>>> \n>>>> LOG(RkISP1, Fatal) << \"Can't locate info from buffer\";\n>>>> @@ -348,11 +343,9 @@ RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n>>>> \n>>>> RkISP1FrameInfo *RkISP1Frames::find(Request *request)\n>>>> {\n>>>> - for (auto &itInfo : frameInfo_) {\n>>>> - RkISP1FrameInfo *info = itInfo.second;\n>>>> -\n>>>> - if (info->request == request)\n>>>> - return info;\n>>>> + for (auto &[frame, info] : frameInfo_) {\n>>>> + if (info.request == request)\n>>>> + return &info;\n>>>> }\n>>>> \n>>>> LOG(RkISP1, Fatal) << \"Can't locate info from request\";\n>>>> -- \n>>>> 2.48.1\n>>>>\n>>>>\n>>","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 8BF28BDCC1\n\tfor ;\n\tWed, 13 Aug 2025 13:42:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AFCA76922C;\n\tWed, 13 Aug 2025 15:42: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 1AE9461444\n\tfor ;\n\tWed, 13 Aug 2025 15:42:46 +0200 (CEST)","from [192.168.33.21] (185.221.141.188.nat.pool.zt.hu\n\t[185.221.141.188])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4320B3A4;\n\tWed, 13 Aug 2025 15:41:52 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ON8eqcVD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755092512;\n\tbh=EOfzWvlMbCg7J9/7SPEoNc5H4y5bz/6fRY5nOI2WaLQ=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=ON8eqcVDVGqBN15i/+WjveLX8DY1nqnyfKLWkuuEO4YNUBXmrAat38IJ/7RIgu2Jp\n\tiD/EGA/rbfqfD2kUa7AEDYUGPyOQ8a3taCYwtUIDr1y/iSB44UNn8ZFDP16kPFNA99\n\t+t5TTbxn+eg5+Ar0rGtoPopGkcG/XOj+pmL2yPNY=","Message-ID":"","Date":"Wed, 13 Aug 2025 15:42:42 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v2] libcamera: pipeline: Avoid unnecessary\n\tindirection in frame info map","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250121190547.304107-1-pobrn@protonmail.com>\n\t<175312160641.50296.13088832792560201008@ping.linuxembedded.co.uk>\n\t<2b5a9711-c279-40c9-88e3-49521d67289e@ideasonboard.com>\n\t<175509222320.560048.5902175810745634714@ping.linuxembedded.co.uk>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<175509222320.560048.5902175810745634714@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]