[{"id":33028,"web_url":"https://patchwork.libcamera.org/comment/33028/","msgid":"<173661715932.1939311.8983880284717100822@ping.linuxembedded.co.uk>","date":"2025-01-11T17:39:19","subject":"Re: [RFC PATCH v1] pipeline: rkisp1: Avoid unnecessary indirection\n\tin `std::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-11 11:47:41)\n> There is no reason to allocate the `RkISP1FrameInfo` objects\n> dynamically, and then store raw pointers in the `std::map`.\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 awesome. But I think this pattern has been replicated in\nother pipeline handlers, could you check IPU3Frames::Info as well\nplease? And any others that might have the same constructs.\n\n\n> Signed-off-by: Barnabás Pőcze \n\nReviewed-by: Kieran Bingham \n\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 76 +++++++++++-------------\n> 1 file changed, 34 insertions(+), 42 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 35c793da9..111acc35b 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,45 @@ 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> -\n> - frameInfo_.erase(info->frame);\n> + auto &info = it->second;\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 +318,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 +327,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 +342,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.47.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 6F3E5C326C\n\tfor ;\n\tSat, 11 Jan 2025 17:39:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C313684EA;\n\tSat, 11 Jan 2025 18:39:23 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B04FC684D0\n\tfor ;\n\tSat, 11 Jan 2025 18:39:22 +0100 (CET)","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 3CF78844;\n\tSat, 11 Jan 2025 18:38:27 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"kgD7/9fw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736617107;\n\tbh=WQydd6aYW1nXLjz2liDittBwK/WIH0E53GlFETKjvjA=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=kgD7/9fwKR1KjN62ufWOdm4b+cq2tJ5wBMFJgC8r0L+0un0W9dcJlO9ovaMfltF3G\n\tulEL/EkTPSTzJT2ZZFbf653VY8u7R1j4IGMfOJq3zKm/et/xlfGhZ4iAWHT+FmE0c6\n\taR3YrzXr5Cfcj7sY+bnZiJm9mC7P6KJacICLE4/M=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250111114739.170891-1-pobrn@protonmail.com>","References":"<20250111114739.170891-1-pobrn@protonmail.com>","Subject":"Re: [RFC PATCH v1] pipeline: rkisp1: Avoid unnecessary indirection\n\tin `std::map`","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Sat, 11 Jan 2025 17:39:19 +0000","Message-ID":"<173661715932.1939311.8983880284717100822@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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\" "}}]