| Message ID | 20250121190547.304107-1-pobrn@protonmail.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Barnabás Pőcze (2025-01-21 19:05:50) > There is no reason to allocate the frame info objects dynamically, > and then store raw pointers in the `std::map` in the rkisp1 > and ipu3 pipeline handler. > > 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 like a good thing. Is it still 'RFC'? Maybe we should try it on some RKISP1 devices and test... Is patch this still relevant? It was posted in January ... I suspect it is. I'm trying to dig into some issues where I think we get out of sync with image and stats/param buffers in error paths - so I'll try to test this along with my investigations there. > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > --- > src/libcamera/pipeline/ipu3/frames.cpp | 38 ++++++------ > src/libcamera/pipeline/ipu3/frames.h | 2 +- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 75 +++++++++++------------- > 3 files changed, 53 insertions(+), 62 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp > index bc0526a77..f3ea78797 100644 > --- a/src/libcamera/pipeline/ipu3/frames.cpp > +++ b/src/libcamera/pipeline/ipu3/frames.cpp > @@ -64,20 +64,20 @@ IPU3Frames::Info *IPU3Frames::create(Request *request) > availableParamBuffers_.pop(); > availableStatBuffers_.pop(); > > - /* \todo Remove the dynamic allocation of Info */ > - std::unique_ptr<Info> info = std::make_unique<Info>(); > + auto [it, inserted] = frameInfo_.try_emplace(id); > + ASSERT(inserted); > > - info->id = id; > - info->request = request; > - info->rawBuffer = nullptr; > - info->paramBuffer = paramBuffer; > - info->statBuffer = statBuffer; > - info->paramDequeued = false; > - info->metadataProcessed = false; > + auto &info = it->second; > > - frameInfo_[id] = std::move(info); > + info.id = id; > + info.request = request; > + info.rawBuffer = nullptr; > + info.paramBuffer = paramBuffer; > + info.statBuffer = statBuffer; > + info.paramDequeued = false; > + info.metadataProcessed = false; > > - return frameInfo_[id].get(); > + return &info; > } > > void IPU3Frames::remove(IPU3Frames::Info *info) > @@ -115,7 +115,7 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id) > const auto &itInfo = frameInfo_.find(id); > > if (itInfo != frameInfo_.end()) > - return itInfo->second.get(); > + return &itInfo->second; > > LOG(IPU3, Fatal) << "Can't find tracking information for frame " << id; > > @@ -124,16 +124,14 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id) > > IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer) > { > - for (auto const &itInfo : frameInfo_) { > - Info *info = itInfo.second.get(); > - > - for (auto const itBuffers : info->request->buffers()) > + for (auto &[id, info] : frameInfo_) { > + for (auto const itBuffers : info.request->buffers()) > if (itBuffers.second == buffer) > - return info; > + return &info; > > - if (info->rawBuffer == buffer || info->paramBuffer == buffer || > - info->statBuffer == buffer) > - return info; > + if (info.rawBuffer == buffer || info.paramBuffer == buffer || > + info.statBuffer == buffer) > + return &info; > } > > LOG(IPU3, Fatal) << "Can't find tracking information from buffer"; > diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h > index a347b66f3..1cabd0e64 100644 > --- a/src/libcamera/pipeline/ipu3/frames.h > +++ b/src/libcamera/pipeline/ipu3/frames.h > @@ -61,7 +61,7 @@ private: > std::queue<FrameBuffer *> availableParamBuffers_; > std::queue<FrameBuffer *> availableStatBuffers_; > > - std::map<unsigned int, std::unique_ptr<Info>> frameInfo_; > + std::map<unsigned int, Info> frameInfo_; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index ca8bbaaf1..953138305 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,46 @@ 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); > + auto &info = it->second; > > - frameInfo_.erase(info->frame); > + 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 +319,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 +328,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 +343,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.48.1 > >
Hi 2025. 07. 21. 20:13 keltezéssel, Kieran Bingham írta: > Quoting Barnabás Pőcze (2025-01-21 19:05:50) >> There is no reason to allocate the frame info objects dynamically, >> and then store raw pointers in the `std::map` in the rkisp1 >> and ipu3 pipeline handler. >> >> 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 like a good thing. Is it still 'RFC'? Maybe we should try it > on some RKISP1 devices and test... > > Is patch this still relevant? It was posted in January ... I suspect it > is. Yes it is. I have been running it ever since. > > I'm trying to dig into some issues where I think we get out of sync with > image and stats/param buffers in error paths - so I'll try to test this > along with my investigations there. > > >> >> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> >> --- >> src/libcamera/pipeline/ipu3/frames.cpp | 38 ++++++------ >> src/libcamera/pipeline/ipu3/frames.h | 2 +- >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 75 +++++++++++------------- >> 3 files changed, 53 insertions(+), 62 deletions(-) >> >> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp >> index bc0526a77..f3ea78797 100644 >> --- a/src/libcamera/pipeline/ipu3/frames.cpp >> +++ b/src/libcamera/pipeline/ipu3/frames.cpp >> @@ -64,20 +64,20 @@ IPU3Frames::Info *IPU3Frames::create(Request *request) >> availableParamBuffers_.pop(); >> availableStatBuffers_.pop(); >> >> - /* \todo Remove the dynamic allocation of Info */ >> - std::unique_ptr<Info> info = std::make_unique<Info>(); >> + auto [it, inserted] = frameInfo_.try_emplace(id); >> + ASSERT(inserted); >> >> - info->id = id; >> - info->request = request; >> - info->rawBuffer = nullptr; >> - info->paramBuffer = paramBuffer; >> - info->statBuffer = statBuffer; >> - info->paramDequeued = false; >> - info->metadataProcessed = false; >> + auto &info = it->second; >> >> - frameInfo_[id] = std::move(info); >> + info.id = id; >> + info.request = request; >> + info.rawBuffer = nullptr; >> + info.paramBuffer = paramBuffer; >> + info.statBuffer = statBuffer; >> + info.paramDequeued = false; >> + info.metadataProcessed = false; >> >> - return frameInfo_[id].get(); >> + return &info; >> } >> >> void IPU3Frames::remove(IPU3Frames::Info *info) >> @@ -115,7 +115,7 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id) >> const auto &itInfo = frameInfo_.find(id); >> >> if (itInfo != frameInfo_.end()) >> - return itInfo->second.get(); >> + return &itInfo->second; >> >> LOG(IPU3, Fatal) << "Can't find tracking information for frame " << id; >> >> @@ -124,16 +124,14 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id) >> >> IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer) >> { >> - for (auto const &itInfo : frameInfo_) { >> - Info *info = itInfo.second.get(); >> - >> - for (auto const itBuffers : info->request->buffers()) >> + for (auto &[id, info] : frameInfo_) { >> + for (auto const itBuffers : info.request->buffers()) >> if (itBuffers.second == buffer) >> - return info; >> + return &info; >> >> - if (info->rawBuffer == buffer || info->paramBuffer == buffer || >> - info->statBuffer == buffer) >> - return info; >> + if (info.rawBuffer == buffer || info.paramBuffer == buffer || >> + info.statBuffer == buffer) >> + return &info; >> } >> >> LOG(IPU3, Fatal) << "Can't find tracking information from buffer"; >> diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h >> index a347b66f3..1cabd0e64 100644 >> --- a/src/libcamera/pipeline/ipu3/frames.h >> +++ b/src/libcamera/pipeline/ipu3/frames.h >> @@ -61,7 +61,7 @@ private: >> std::queue<FrameBuffer *> availableParamBuffers_; >> std::queue<FrameBuffer *> availableStatBuffers_; >> >> - std::map<unsigned int, std::unique_ptr<Info>> frameInfo_; >> + std::map<unsigned int, Info> frameInfo_; >> }; >> >> } /* namespace libcamera */ >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> index ca8bbaaf1..953138305 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,46 @@ 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); >> + auto &info = it->second; >> >> - frameInfo_.erase(info->frame); >> + 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 +319,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 +328,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 +343,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.48.1 >> >>
Quoting Barnabás Pőcze (2025-01-22 04:05:50) > There is no reason to allocate the frame info objects dynamically, > and then store raw pointers in the `std::map` in the rkisp1 > and ipu3 pipeline handler. > > 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. > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> Kieran replying pushed it into my inbox. Looks good to me. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/frames.cpp | 38 ++++++------ > src/libcamera/pipeline/ipu3/frames.h | 2 +- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 75 +++++++++++------------- > 3 files changed, 53 insertions(+), 62 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp > index bc0526a77..f3ea78797 100644 > --- a/src/libcamera/pipeline/ipu3/frames.cpp > +++ b/src/libcamera/pipeline/ipu3/frames.cpp > @@ -64,20 +64,20 @@ IPU3Frames::Info *IPU3Frames::create(Request *request) > availableParamBuffers_.pop(); > availableStatBuffers_.pop(); > > - /* \todo Remove the dynamic allocation of Info */ > - std::unique_ptr<Info> info = std::make_unique<Info>(); > + auto [it, inserted] = frameInfo_.try_emplace(id); > + ASSERT(inserted); > > - info->id = id; > - info->request = request; > - info->rawBuffer = nullptr; > - info->paramBuffer = paramBuffer; > - info->statBuffer = statBuffer; > - info->paramDequeued = false; > - info->metadataProcessed = false; > + auto &info = it->second; > > - frameInfo_[id] = std::move(info); > + info.id = id; > + info.request = request; > + info.rawBuffer = nullptr; > + info.paramBuffer = paramBuffer; > + info.statBuffer = statBuffer; > + info.paramDequeued = false; > + info.metadataProcessed = false; > > - return frameInfo_[id].get(); > + return &info; > } > > void IPU3Frames::remove(IPU3Frames::Info *info) > @@ -115,7 +115,7 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id) > const auto &itInfo = frameInfo_.find(id); > > if (itInfo != frameInfo_.end()) > - return itInfo->second.get(); > + return &itInfo->second; > > LOG(IPU3, Fatal) << "Can't find tracking information for frame " << id; > > @@ -124,16 +124,14 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id) > > IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer) > { > - for (auto const &itInfo : frameInfo_) { > - Info *info = itInfo.second.get(); > - > - for (auto const itBuffers : info->request->buffers()) > + for (auto &[id, info] : frameInfo_) { > + for (auto const itBuffers : info.request->buffers()) > if (itBuffers.second == buffer) > - return info; > + return &info; > > - if (info->rawBuffer == buffer || info->paramBuffer == buffer || > - info->statBuffer == buffer) > - return info; > + if (info.rawBuffer == buffer || info.paramBuffer == buffer || > + info.statBuffer == buffer) > + return &info; > } > > LOG(IPU3, Fatal) << "Can't find tracking information from buffer"; > diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h > index a347b66f3..1cabd0e64 100644 > --- a/src/libcamera/pipeline/ipu3/frames.h > +++ b/src/libcamera/pipeline/ipu3/frames.h > @@ -61,7 +61,7 @@ private: > std::queue<FrameBuffer *> availableParamBuffers_; > std::queue<FrameBuffer *> availableStatBuffers_; > > - std::map<unsigned int, std::unique_ptr<Info>> frameInfo_; > + std::map<unsigned int, Info> frameInfo_; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index ca8bbaaf1..953138305 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,46 @@ 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); > + auto &info = it->second; > > - frameInfo_.erase(info->frame); > + 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 +319,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 +328,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 +343,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.48.1 > >
Hi Barnabás, Thank you for the patch. On Tue, Jan 21, 2025 at 07:05:50PM +0000, Barnabás Pőcze wrote: > There is no reason to allocate the frame info objects dynamically, > and then store raw pointers in the `std::map` in the rkisp1 > and ipu3 pipeline handler. > > 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. We share the common goal of reducing memory allocations at runtime, and this patch helps by turning two allocations into one. However, I think it could make it more difficult to get rid of the second allocation. Ideally, we should preallocate all the frame info at start time and put them in a pool. Have you considered how this could be done ? Would it be as simple as creating the map with a custom allocator ? > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > --- > src/libcamera/pipeline/ipu3/frames.cpp | 38 ++++++------ > src/libcamera/pipeline/ipu3/frames.h | 2 +- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 75 +++++++++++------------- > 3 files changed, 53 insertions(+), 62 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp > index bc0526a77..f3ea78797 100644 > --- a/src/libcamera/pipeline/ipu3/frames.cpp > +++ b/src/libcamera/pipeline/ipu3/frames.cpp > @@ -64,20 +64,20 @@ IPU3Frames::Info *IPU3Frames::create(Request *request) > availableParamBuffers_.pop(); > availableStatBuffers_.pop(); > > - /* \todo Remove the dynamic allocation of Info */ > - std::unique_ptr<Info> info = std::make_unique<Info>(); > + auto [it, inserted] = frameInfo_.try_emplace(id); > + ASSERT(inserted); > > - info->id = id; > - info->request = request; > - info->rawBuffer = nullptr; > - info->paramBuffer = paramBuffer; > - info->statBuffer = statBuffer; > - info->paramDequeued = false; > - info->metadataProcessed = false; > + auto &info = it->second; > > - frameInfo_[id] = std::move(info); > + info.id = id; > + info.request = request; > + info.rawBuffer = nullptr; > + info.paramBuffer = paramBuffer; > + info.statBuffer = statBuffer; > + info.paramDequeued = false; > + info.metadataProcessed = false; > > - return frameInfo_[id].get(); > + return &info; > } > > void IPU3Frames::remove(IPU3Frames::Info *info) > @@ -115,7 +115,7 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id) > const auto &itInfo = frameInfo_.find(id); > > if (itInfo != frameInfo_.end()) > - return itInfo->second.get(); > + return &itInfo->second; > > LOG(IPU3, Fatal) << "Can't find tracking information for frame " << id; > > @@ -124,16 +124,14 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id) > > IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer) > { > - for (auto const &itInfo : frameInfo_) { > - Info *info = itInfo.second.get(); > - > - for (auto const itBuffers : info->request->buffers()) > + for (auto &[id, info] : frameInfo_) { > + for (auto const itBuffers : info.request->buffers()) > if (itBuffers.second == buffer) > - return info; > + return &info; > > - if (info->rawBuffer == buffer || info->paramBuffer == buffer || > - info->statBuffer == buffer) > - return info; > + if (info.rawBuffer == buffer || info.paramBuffer == buffer || > + info.statBuffer == buffer) > + return &info; > } > > LOG(IPU3, Fatal) << "Can't find tracking information from buffer"; > diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h > index a347b66f3..1cabd0e64 100644 > --- a/src/libcamera/pipeline/ipu3/frames.h > +++ b/src/libcamera/pipeline/ipu3/frames.h > @@ -61,7 +61,7 @@ private: > std::queue<FrameBuffer *> availableParamBuffers_; > std::queue<FrameBuffer *> availableStatBuffers_; > > - std::map<unsigned int, std::unique_ptr<Info>> frameInfo_; > + std::map<unsigned int, Info> frameInfo_; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index ca8bbaaf1..953138305 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,46 @@ 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); > + auto &info = it->second; > > - frameInfo_.erase(info->frame); > + 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 +319,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 +328,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 +343,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";
Hi 2025. 07. 24. 20:32 keltezéssel, Laurent Pinchart írta: > Hi Barnabás, > > Thank you for the patch. > > On Tue, Jan 21, 2025 at 07:05:50PM +0000, Barnabás Pőcze wrote: >> There is no reason to allocate the frame info objects dynamically, >> and then store raw pointers in the `std::map` in the rkisp1 >> and ipu3 pipeline handler. >> >> 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. > > We share the common goal of reducing memory allocations at runtime, and > this patch helps by turning two allocations into one. However, I think > it could make it more difficult to get rid of the second allocation. > > Ideally, we should preallocate all the frame info at start time and put > them in a pool. Have you considered how this could be done ? Would it be > as simple as creating the map with a custom allocator ? The map will always allocate nodes, this cannot be avoided. So if you want to improve still, the data structure should be replaced (e.g. to something intrusive). With `std::map`, I think the choice is between a single pool of nodes (which embed the useful data directly), or two pools of nodes (one for nodes, one for the useful data). Something like `std::pmr::unsynchronized_pool_resource` seems like the best choice available in the STL, unfortunately that introduces virtual dispatch for the allocator. Anything better than that would need a custom allocator wrapper I think. There are particular requirements for an stl allocators[0] that memory resources do not satisfy, even if they had the right interface. So in summary, no I have not considered preallocating, and I still think it is a step in the right direction. [0]: https://en.cppreference.com/w/cpp/named_req/Allocator.html Regards, Barnabás Pőcze > >> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> >> --- >> src/libcamera/pipeline/ipu3/frames.cpp | 38 ++++++------ >> src/libcamera/pipeline/ipu3/frames.h | 2 +- >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 75 +++++++++++------------- >> 3 files changed, 53 insertions(+), 62 deletions(-) >> >> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp >> index bc0526a77..f3ea78797 100644 >> --- a/src/libcamera/pipeline/ipu3/frames.cpp >> +++ b/src/libcamera/pipeline/ipu3/frames.cpp >> @@ -64,20 +64,20 @@ IPU3Frames::Info *IPU3Frames::create(Request *request) >> availableParamBuffers_.pop(); >> availableStatBuffers_.pop(); >> >> - /* \todo Remove the dynamic allocation of Info */ >> - std::unique_ptr<Info> info = std::make_unique<Info>(); >> + auto [it, inserted] = frameInfo_.try_emplace(id); >> + ASSERT(inserted); >> >> - info->id = id; >> - info->request = request; >> - info->rawBuffer = nullptr; >> - info->paramBuffer = paramBuffer; >> - info->statBuffer = statBuffer; >> - info->paramDequeued = false; >> - info->metadataProcessed = false; >> + auto &info = it->second; >> >> - frameInfo_[id] = std::move(info); >> + info.id = id; >> + info.request = request; >> + info.rawBuffer = nullptr; >> + info.paramBuffer = paramBuffer; >> + info.statBuffer = statBuffer; >> + info.paramDequeued = false; >> + info.metadataProcessed = false; >> >> - return frameInfo_[id].get(); >> + return &info; >> } >> >> void IPU3Frames::remove(IPU3Frames::Info *info) >> @@ -115,7 +115,7 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id) >> const auto &itInfo = frameInfo_.find(id); >> >> if (itInfo != frameInfo_.end()) >> - return itInfo->second.get(); >> + return &itInfo->second; >> >> LOG(IPU3, Fatal) << "Can't find tracking information for frame " << id; >> >> @@ -124,16 +124,14 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id) >> >> IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer) >> { >> - for (auto const &itInfo : frameInfo_) { >> - Info *info = itInfo.second.get(); >> - >> - for (auto const itBuffers : info->request->buffers()) >> + for (auto &[id, info] : frameInfo_) { >> + for (auto const itBuffers : info.request->buffers()) >> if (itBuffers.second == buffer) >> - return info; >> + return &info; >> >> - if (info->rawBuffer == buffer || info->paramBuffer == buffer || >> - info->statBuffer == buffer) >> - return info; >> + if (info.rawBuffer == buffer || info.paramBuffer == buffer || >> + info.statBuffer == buffer) >> + return &info; >> } >> >> LOG(IPU3, Fatal) << "Can't find tracking information from buffer"; >> diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h >> index a347b66f3..1cabd0e64 100644 >> --- a/src/libcamera/pipeline/ipu3/frames.h >> +++ b/src/libcamera/pipeline/ipu3/frames.h >> @@ -61,7 +61,7 @@ private: >> std::queue<FrameBuffer *> availableParamBuffers_; >> std::queue<FrameBuffer *> availableStatBuffers_; >> >> - std::map<unsigned int, std::unique_ptr<Info>> frameInfo_; >> + std::map<unsigned int, Info> frameInfo_; >> }; >> >> } /* namespace libcamera */ >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> index ca8bbaaf1..953138305 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,46 @@ 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); >> + auto &info = it->second; >> >> - frameInfo_.erase(info->frame); >> + 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 +319,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 +328,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 +343,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"; >
On Fri, Jul 25, 2025 at 09:17:59AM +0200, Barnabás Pőcze wrote: > Hi > > 2025. 07. 24. 20:32 keltezéssel, Laurent Pinchart írta: > > Hi Barnabás, > > > > Thank you for the patch. > > > > On Tue, Jan 21, 2025 at 07:05:50PM +0000, Barnabás Pőcze wrote: > >> There is no reason to allocate the frame info objects dynamically, > >> and then store raw pointers in the `std::map` in the rkisp1 > >> and ipu3 pipeline handler. > >> > >> 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. > > > > We share the common goal of reducing memory allocations at runtime, and > > this patch helps by turning two allocations into one. However, I think > > it could make it more difficult to get rid of the second allocation. > > > > Ideally, we should preallocate all the frame info at start time and put > > them in a pool. Have you considered how this could be done ? Would it be > > as simple as creating the map with a custom allocator ? > > The map will always allocate nodes, this cannot be avoided. So if you want to > improve still, the data structure should be replaced (e.g. to something intrusive). > With `std::map`, I think the choice is between a single pool of nodes (which embed > the useful data directly), or two pools of nodes (one for nodes, one for the useful data). > > Something like `std::pmr::unsynchronized_pool_resource` seems like the best choice > available in the STL, unfortunately that introduces virtual dispatch for the allocator. > Anything better than that would need a custom allocator wrapper I think. There are > particular requirements for an stl allocators[0] that memory resources do not satisfy, > even if they had the right interface. > > So in summary, no I have not considered preallocating, and I still think it is a step > in the right direction. OK. Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > [0]: https://en.cppreference.com/w/cpp/named_req/Allocator.html > > >> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > >> --- > >> src/libcamera/pipeline/ipu3/frames.cpp | 38 ++++++------ > >> src/libcamera/pipeline/ipu3/frames.h | 2 +- > >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 75 +++++++++++------------- > >> 3 files changed, 53 insertions(+), 62 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp > >> index bc0526a77..f3ea78797 100644 > >> --- a/src/libcamera/pipeline/ipu3/frames.cpp > >> +++ b/src/libcamera/pipeline/ipu3/frames.cpp > >> @@ -64,20 +64,20 @@ IPU3Frames::Info *IPU3Frames::create(Request *request) > >> availableParamBuffers_.pop(); > >> availableStatBuffers_.pop(); > >> > >> - /* \todo Remove the dynamic allocation of Info */ > >> - std::unique_ptr<Info> info = std::make_unique<Info>(); > >> + auto [it, inserted] = frameInfo_.try_emplace(id); > >> + ASSERT(inserted); > >> > >> - info->id = id; > >> - info->request = request; > >> - info->rawBuffer = nullptr; > >> - info->paramBuffer = paramBuffer; > >> - info->statBuffer = statBuffer; > >> - info->paramDequeued = false; > >> - info->metadataProcessed = false; > >> + auto &info = it->second; > >> > >> - frameInfo_[id] = std::move(info); > >> + info.id = id; > >> + info.request = request; > >> + info.rawBuffer = nullptr; > >> + info.paramBuffer = paramBuffer; > >> + info.statBuffer = statBuffer; > >> + info.paramDequeued = false; > >> + info.metadataProcessed = false; > >> > >> - return frameInfo_[id].get(); > >> + return &info; > >> } > >> > >> void IPU3Frames::remove(IPU3Frames::Info *info) > >> @@ -115,7 +115,7 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id) > >> const auto &itInfo = frameInfo_.find(id); > >> > >> if (itInfo != frameInfo_.end()) > >> - return itInfo->second.get(); > >> + return &itInfo->second; > >> > >> LOG(IPU3, Fatal) << "Can't find tracking information for frame " << id; > >> > >> @@ -124,16 +124,14 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id) > >> > >> IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer) > >> { > >> - for (auto const &itInfo : frameInfo_) { > >> - Info *info = itInfo.second.get(); > >> - > >> - for (auto const itBuffers : info->request->buffers()) > >> + for (auto &[id, info] : frameInfo_) { > >> + for (auto const itBuffers : info.request->buffers()) > >> if (itBuffers.second == buffer) > >> - return info; > >> + return &info; > >> > >> - if (info->rawBuffer == buffer || info->paramBuffer == buffer || > >> - info->statBuffer == buffer) > >> - return info; > >> + if (info.rawBuffer == buffer || info.paramBuffer == buffer || > >> + info.statBuffer == buffer) > >> + return &info; > >> } > >> > >> LOG(IPU3, Fatal) << "Can't find tracking information from buffer"; > >> diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h > >> index a347b66f3..1cabd0e64 100644 > >> --- a/src/libcamera/pipeline/ipu3/frames.h > >> +++ b/src/libcamera/pipeline/ipu3/frames.h > >> @@ -61,7 +61,7 @@ private: > >> std::queue<FrameBuffer *> availableParamBuffers_; > >> std::queue<FrameBuffer *> availableStatBuffers_; > >> > >> - std::map<unsigned int, std::unique_ptr<Info>> frameInfo_; > >> + std::map<unsigned int, Info> frameInfo_; > >> }; > >> > >> } /* namespace libcamera */ > >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >> index ca8bbaaf1..953138305 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,46 @@ 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); > >> + auto &info = it->second; > >> > >> - frameInfo_.erase(info->frame); > >> + 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 +319,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 +328,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 +343,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";
diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp index bc0526a77..f3ea78797 100644 --- a/src/libcamera/pipeline/ipu3/frames.cpp +++ b/src/libcamera/pipeline/ipu3/frames.cpp @@ -64,20 +64,20 @@ IPU3Frames::Info *IPU3Frames::create(Request *request) availableParamBuffers_.pop(); availableStatBuffers_.pop(); - /* \todo Remove the dynamic allocation of Info */ - std::unique_ptr<Info> info = std::make_unique<Info>(); + auto [it, inserted] = frameInfo_.try_emplace(id); + ASSERT(inserted); - info->id = id; - info->request = request; - info->rawBuffer = nullptr; - info->paramBuffer = paramBuffer; - info->statBuffer = statBuffer; - info->paramDequeued = false; - info->metadataProcessed = false; + auto &info = it->second; - frameInfo_[id] = std::move(info); + info.id = id; + info.request = request; + info.rawBuffer = nullptr; + info.paramBuffer = paramBuffer; + info.statBuffer = statBuffer; + info.paramDequeued = false; + info.metadataProcessed = false; - return frameInfo_[id].get(); + return &info; } void IPU3Frames::remove(IPU3Frames::Info *info) @@ -115,7 +115,7 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id) const auto &itInfo = frameInfo_.find(id); if (itInfo != frameInfo_.end()) - return itInfo->second.get(); + return &itInfo->second; LOG(IPU3, Fatal) << "Can't find tracking information for frame " << id; @@ -124,16 +124,14 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id) IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer) { - for (auto const &itInfo : frameInfo_) { - Info *info = itInfo.second.get(); - - for (auto const itBuffers : info->request->buffers()) + for (auto &[id, info] : frameInfo_) { + for (auto const itBuffers : info.request->buffers()) if (itBuffers.second == buffer) - return info; + return &info; - if (info->rawBuffer == buffer || info->paramBuffer == buffer || - info->statBuffer == buffer) - return info; + if (info.rawBuffer == buffer || info.paramBuffer == buffer || + info.statBuffer == buffer) + return &info; } LOG(IPU3, Fatal) << "Can't find tracking information from buffer"; diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h index a347b66f3..1cabd0e64 100644 --- a/src/libcamera/pipeline/ipu3/frames.h +++ b/src/libcamera/pipeline/ipu3/frames.h @@ -61,7 +61,7 @@ private: std::queue<FrameBuffer *> availableParamBuffers_; std::queue<FrameBuffer *> availableStatBuffers_; - std::map<unsigned int, std::unique_ptr<Info>> frameInfo_; + std::map<unsigned int, Info> frameInfo_; }; } /* namespace libcamera */ diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index ca8bbaaf1..953138305 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,46 @@ 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); + auto &info = it->second; - frameInfo_.erase(info->frame); + 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 +319,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 +328,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 +343,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";
There is no reason to allocate the frame info objects dynamically, and then store raw pointers in the `std::map` in the rkisp1 and ipu3 pipeline handler. 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. Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- src/libcamera/pipeline/ipu3/frames.cpp | 38 ++++++------ src/libcamera/pipeline/ipu3/frames.h | 2 +- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 75 +++++++++++------------- 3 files changed, 53 insertions(+), 62 deletions(-)