[RFC PATCH v1] pipeline: rkisp1: Avoid unnecessary indirection in `std::map`

[Kieran Bingham]

Quoting Barnabás Pőcze (2025-01-11 11:47:41)
> There is no reason to allocate the `RkISP1FrameInfo` objects
> dynamically, and then store raw pointers in the `std::map`.
> 
> Instead, store the objects directly in the map. This removes
> the need for manully calling new/delete, simplifies the code,
> and eliminates one memory allocation per frame.

This sounds awesome. But I think this pattern has been replicated in
other pipeline handlers, could you check IPU3Frames::Info as well
please? And any others that might have the same constructs.


> Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 76 +++++++++++-------------
>  1 file changed, 34 insertions(+), 42 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 35c793da9..111acc35b 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -84,7 +84,7 @@ public:
>  
>  private:
>         PipelineHandlerRkISP1 *pipe_;
> -       std::map<unsigned int, RkISP1FrameInfo *> frameInfo_;
> +       std::map<unsigned int, RkISP1FrameInfo> frameInfo_;
>  };
>  
>  class RkISP1CameraData : public Camera::Private
> @@ -269,49 +269,45 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req
>                 mainPathBuffer = request->findBuffer(&data->mainPathStream_);
>         selfPathBuffer = request->findBuffer(&data->selfPathStream_);
>  
> -       RkISP1FrameInfo *info = new RkISP1FrameInfo;
> +       auto [it, inserted] = frameInfo_.try_emplace(frame);
> +       ASSERT(inserted);
>  
> -       info->frame = frame;
> -       info->request = request;
> -       info->paramBuffer = paramBuffer;
> -       info->mainPathBuffer = mainPathBuffer;
> -       info->selfPathBuffer = selfPathBuffer;
> -       info->statBuffer = statBuffer;
> -       info->paramDequeued = false;
> -       info->metadataProcessed = false;
> +       auto &info = it->second;
>  
> -       frameInfo_[frame] = info;
> +       info.frame = frame;
> +       info.request = request;
> +       info.paramBuffer = paramBuffer;
> +       info.mainPathBuffer = mainPathBuffer;
> +       info.selfPathBuffer = selfPathBuffer;
> +       info.statBuffer = statBuffer;
> +       info.paramDequeued = false;
> +       info.metadataProcessed = false;
>  
> -       return info;
> +       return &info;
>  }
>  
>  int RkISP1Frames::destroy(unsigned int frame)
>  {
> -       RkISP1FrameInfo *info = find(frame);
> -       if (!info)
> +       auto it = frameInfo_.find(frame);
> +       if (it == frameInfo_.end())
>                 return -ENOENT;
>  
> -       pipe_->availableParamBuffers_.push(info->paramBuffer);
> -       pipe_->availableStatBuffers_.push(info->statBuffer);
> -       pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);
> -
> -       frameInfo_.erase(info->frame);
> +       auto &info = it->second;
> +       pipe_->availableParamBuffers_.push(info.paramBuffer);
> +       pipe_->availableStatBuffers_.push(info.statBuffer);
> +       pipe_->availableMainPathBuffers_.push(info.mainPathBuffer);
>  
> -       delete info;
> +       frameInfo_.erase(it);
>  
>         return 0;
>  }
>  
>  void RkISP1Frames::clear()
>  {
> -       for (const auto &entry : frameInfo_) {
> -               RkISP1FrameInfo *info = entry.second;
> -
> -               pipe_->availableParamBuffers_.push(info->paramBuffer);
> -               pipe_->availableStatBuffers_.push(info->statBuffer);
> -               pipe_->availableMainPathBuffers_.push(info->mainPathBuffer);
> -
> -               delete info;
> +       for (const auto &[frame, info] : frameInfo_) {
> +               pipe_->availableParamBuffers_.push(info.paramBuffer);
> +               pipe_->availableStatBuffers_.push(info.statBuffer);
> +               pipe_->availableMainPathBuffers_.push(info.mainPathBuffer);
>         }
>  
>         frameInfo_.clear();
> @@ -322,7 +318,7 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)
>         auto itInfo = frameInfo_.find(frame);
>  
>         if (itInfo != frameInfo_.end())
> -               return itInfo->second;
> +               return &itInfo->second;
>  
>         LOG(RkISP1, Fatal) << "Can't locate info from frame";
>  
> @@ -331,14 +327,12 @@ RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)
>  
>  RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)
>  {
> -       for (auto &itInfo : frameInfo_) {
> -               RkISP1FrameInfo *info = itInfo.second;
> -
> -               if (info->paramBuffer == buffer ||
> -                   info->statBuffer == buffer ||
> -                   info->mainPathBuffer == buffer ||
> -                   info->selfPathBuffer == buffer)
> -                       return info;
> +       for (auto &[frame, info] : frameInfo_) {
> +               if (info.paramBuffer == buffer ||
> +                   info.statBuffer == buffer ||
> +                   info.mainPathBuffer == buffer ||
> +                   info.selfPathBuffer == buffer)
> +                       return &info;
>         }
>  
>         LOG(RkISP1, Fatal) << "Can't locate info from buffer";
> @@ -348,11 +342,9 @@ RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)
>  
>  RkISP1FrameInfo *RkISP1Frames::find(Request *request)
>  {
> -       for (auto &itInfo : frameInfo_) {
> -               RkISP1FrameInfo *info = itInfo.second;
> -
> -               if (info->request == request)
> -                       return info;
> +       for (auto &[frame, info] : frameInfo_) {
> +               if (info.request == request)
> +                       return &info;
>         }
>  
>         LOG(RkISP1, Fatal) << "Can't locate info from request";
> -- 
> 2.47.1
> 
>