| Message ID | 20250326124856.75709-2-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Milan Zamazal (2025-03-26 12:48:50) > Hardware pipelines track requests and other information related to > particular frames. This hasn't been needed in software ISP so far. But > in order to be able to track metadata corresponding to a given frame, > frame-request tracking mechanism starts being useful. This patch > introduces the basic tracking structure, actual metadata handling is > added in the following patch. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> In v3, Barnabas clarified that the fix I was worried about was incorporated here already so: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> # Lenovo X13s Concerns about duplicating this framework concept in each pipeline handler can be aligned on top. > --- > src/libcamera/pipeline/simple/simple.cpp | 85 ++++++++++++++++++++++-- > 1 file changed, 80 insertions(+), 5 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 6e039bf3..2281c0a6 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -181,6 +181,64 @@ LOG_DEFINE_CATEGORY(SimplePipeline) > > class SimplePipelineHandler; > > +struct SimpleFrameInfo { > + uint32_t frame; > + Request *request; > +}; > + > +class SimpleFrames > +{ > +public: > + void create(Request *request); > + void destroy(uint32_t frame); > + void clear(); > + > + SimpleFrameInfo *find(uint32_t frame); > + SimpleFrameInfo *find(Request *request); > + > +private: > + std::map<uint32_t, SimpleFrameInfo> frameInfo_; > +}; > + > +void SimpleFrames::create(Request *request) > +{ > + uint32_t frame = request->sequence(); > + auto [it, inserted] = frameInfo_.try_emplace(request->sequence()); > + ASSERT(inserted); > + it->second.frame = frame; > + it->second.request = request; > +} > + > +void SimpleFrames::destroy(uint32_t frame) > +{ > + frameInfo_.erase(frame); > +} > + > +void SimpleFrames::clear() > +{ > + frameInfo_.clear(); > +} > + > +SimpleFrameInfo *SimpleFrames::find(uint32_t frame) > +{ > + auto info = frameInfo_.find(frame); > + if (info == frameInfo_.end()) > + return nullptr; > + return &info->second; > +} > + > +SimpleFrameInfo *SimpleFrames::find(Request *request) > +{ > + for (auto &itInfo : frameInfo_) { > + SimpleFrameInfo *info = &itInfo.second; > + > + if (info->request == request) > + return info; > + } > + > + return nullptr; > +} > + > struct SimplePipelineInfo { > const char *driver; > /* > @@ -293,11 +351,13 @@ public: > > std::unique_ptr<Converter> converter_; > std::unique_ptr<SoftwareIsp> swIsp_; > + SimpleFrames frameInfo_; > > private: > void tryPipeline(unsigned int code, const Size &size); > static std::vector<const MediaPad *> routedSourcePads(MediaPad *sink); > > + void completeRequest(Request *request); > void conversionInputDone(FrameBuffer *buffer); > void conversionOutputDone(FrameBuffer *buffer); > > @@ -785,7 +845,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > /* No conversion, just complete the request. */ > Request *request = buffer->request(); > pipe->completeBuffer(request, buffer); > - pipe->completeRequest(request); > + completeRequest(request); > return; > } > > @@ -803,7 +863,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > const RequestOutputs &outputs = conversionQueue_.front(); > for (auto &[stream, buf] : outputs.outputs) > pipe->completeBuffer(outputs.request, buf); > - pipe->completeRequest(outputs.request); > + completeRequest(outputs.request); > conversionQueue_.pop(); > > return; > @@ -861,7 +921,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > > /* Otherwise simply complete the request. */ > pipe->completeBuffer(request, buffer); > - pipe->completeRequest(request); > + completeRequest(request); > } > > void SimpleCameraData::clearIncompleteRequests() > @@ -872,6 +932,18 @@ void SimpleCameraData::clearIncompleteRequests() > } > } > > +void SimpleCameraData::completeRequest(Request *request) > +{ > + SimpleFrameInfo *info = frameInfo_.find(request); > + if (!info) { > + /* Something is really wrong, let's return. */ > + return; > + } > + > + frameInfo_.destroy(info->frame); > + pipe()->completeRequest(request); > +} > + > void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) > { > /* Queue the input buffer back for capture. */ > @@ -885,7 +957,7 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) > /* Complete the buffer and the request. */ > Request *request = buffer->request(); > if (pipe->completeBuffer(request, buffer)) > - pipe->completeRequest(request); > + completeRequest(request); > } > > void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId) > @@ -1398,6 +1470,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera) > > video->bufferReady.disconnect(data, &SimpleCameraData::imageBufferReady); > > + data->frameInfo_.clear(); > data->clearIncompleteRequests(); > data->conversionBuffers_.clear(); > > @@ -1428,8 +1501,10 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) > > if (data->useConversion_) { > data->conversionQueue_.push({ request, std::move(buffers) }); > - if (data->swIsp_) > + if (data->swIsp_) { > + data->frameInfo_.create(request); > data->swIsp_->queueRequest(request->sequence(), request->controls()); > + } > } > > return 0; > -- > 2.49.0 >
Hi Milan, Thank you for the patch. On Wed, Mar 26, 2025 at 01:48:50PM +0100, Milan Zamazal wrote: > Hardware pipelines track requests and other information related to > particular frames. This hasn't been needed in software ISP so far. But > in order to be able to track metadata corresponding to a given frame, > frame-request tracking mechanism starts being useful. This patch > introduces the basic tracking structure, actual metadata handling is > added in the following patch. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 85 ++++++++++++++++++++++-- > 1 file changed, 80 insertions(+), 5 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 6e039bf3..2281c0a6 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -181,6 +181,64 @@ LOG_DEFINE_CATEGORY(SimplePipeline) > > class SimplePipelineHandler; > > +struct SimpleFrameInfo { If you add a constructor here: SimpleFrameInfo(uint32_t f, Request *r) : frame(f), request(r) { } ... > + uint32_t frame; > + Request *request; > +}; > + > +class SimpleFrames > +{ > +public: > + void create(Request *request); > + void destroy(uint32_t frame); > + void clear(); > + > + SimpleFrameInfo *find(uint32_t frame); > + SimpleFrameInfo *find(Request *request); > + > +private: > + std::map<uint32_t, SimpleFrameInfo> frameInfo_; > +}; > + > +void SimpleFrames::create(Request *request) > +{ > + uint32_t frame = request->sequence(); > + auto [it, inserted] = frameInfo_.try_emplace(request->sequence()); const uint32_t frame = request->sequence(); auto [it, inserted] = frameInfo_.try_emplace(frame); > + ASSERT(inserted); > + it->second.frame = frame; > + it->second.request = request; ... then you can write auto [it, inserted] = frameInfo_.try_emplace(frame, frame, request); ASSERT(inserted); > +} > + > +void SimpleFrames::destroy(uint32_t frame) > +{ > + frameInfo_.erase(frame); > +} > + > +void SimpleFrames::clear() > +{ > + frameInfo_.clear(); > +} > + > +SimpleFrameInfo *SimpleFrames::find(uint32_t frame) > +{ > + auto info = frameInfo_.find(frame); > + if (info == frameInfo_.end()) > + return nullptr; > + return &info->second; > +} > + > +SimpleFrameInfo *SimpleFrames::find(Request *request) > +{ > + for (auto &itInfo : frameInfo_) { > + SimpleFrameInfo *info = &itInfo.second; > + > + if (info->request == request) > + return info; > + } That's fairly inefficient. Can't you drop this function and replace find(request) with find(request->sequence()) ? Or keep it as a convenience and simply return find(request->sequence()); Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > + return nullptr; > +} > + > struct SimplePipelineInfo { > const char *driver; > /* > @@ -293,11 +351,13 @@ public: > > std::unique_ptr<Converter> converter_; > std::unique_ptr<SoftwareIsp> swIsp_; > + SimpleFrames frameInfo_; > > private: > void tryPipeline(unsigned int code, const Size &size); > static std::vector<const MediaPad *> routedSourcePads(MediaPad *sink); > > + void completeRequest(Request *request); > void conversionInputDone(FrameBuffer *buffer); > void conversionOutputDone(FrameBuffer *buffer); > > @@ -785,7 +845,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > /* No conversion, just complete the request. */ > Request *request = buffer->request(); > pipe->completeBuffer(request, buffer); > - pipe->completeRequest(request); > + completeRequest(request); > return; > } > > @@ -803,7 +863,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > const RequestOutputs &outputs = conversionQueue_.front(); > for (auto &[stream, buf] : outputs.outputs) > pipe->completeBuffer(outputs.request, buf); > - pipe->completeRequest(outputs.request); > + completeRequest(outputs.request); > conversionQueue_.pop(); > > return; > @@ -861,7 +921,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > > /* Otherwise simply complete the request. */ > pipe->completeBuffer(request, buffer); > - pipe->completeRequest(request); > + completeRequest(request); > } > > void SimpleCameraData::clearIncompleteRequests() > @@ -872,6 +932,18 @@ void SimpleCameraData::clearIncompleteRequests() > } > } > > +void SimpleCameraData::completeRequest(Request *request) > +{ > + SimpleFrameInfo *info = frameInfo_.find(request); > + if (!info) { > + /* Something is really wrong, let's return. */ > + return; > + } > + > + frameInfo_.destroy(info->frame); > + pipe()->completeRequest(request); > +} > + > void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) > { > /* Queue the input buffer back for capture. */ > @@ -885,7 +957,7 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) > /* Complete the buffer and the request. */ > Request *request = buffer->request(); > if (pipe->completeBuffer(request, buffer)) > - pipe->completeRequest(request); > + completeRequest(request); > } > > void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId) > @@ -1398,6 +1470,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera) > > video->bufferReady.disconnect(data, &SimpleCameraData::imageBufferReady); > > + data->frameInfo_.clear(); > data->clearIncompleteRequests(); > data->conversionBuffers_.clear(); > > @@ -1428,8 +1501,10 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) > > if (data->useConversion_) { > data->conversionQueue_.push({ request, std::move(buffers) }); > - if (data->swIsp_) > + if (data->swIsp_) { > + data->frameInfo_.create(request); > data->swIsp_->queueRequest(request->sequence(), request->controls()); > + } > } > > return 0;
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 6e039bf3..2281c0a6 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -181,6 +181,64 @@ LOG_DEFINE_CATEGORY(SimplePipeline) class SimplePipelineHandler; +struct SimpleFrameInfo { + uint32_t frame; + Request *request; +}; + +class SimpleFrames +{ +public: + void create(Request *request); + void destroy(uint32_t frame); + void clear(); + + SimpleFrameInfo *find(uint32_t frame); + SimpleFrameInfo *find(Request *request); + +private: + std::map<uint32_t, SimpleFrameInfo> frameInfo_; +}; + +void SimpleFrames::create(Request *request) +{ + uint32_t frame = request->sequence(); + auto [it, inserted] = frameInfo_.try_emplace(request->sequence()); + ASSERT(inserted); + it->second.frame = frame; + it->second.request = request; +} + +void SimpleFrames::destroy(uint32_t frame) +{ + frameInfo_.erase(frame); +} + +void SimpleFrames::clear() +{ + frameInfo_.clear(); +} + +SimpleFrameInfo *SimpleFrames::find(uint32_t frame) +{ + auto info = frameInfo_.find(frame); + if (info == frameInfo_.end()) + return nullptr; + return &info->second; +} + +SimpleFrameInfo *SimpleFrames::find(Request *request) +{ + for (auto &itInfo : frameInfo_) { + SimpleFrameInfo *info = &itInfo.second; + + if (info->request == request) + return info; + } + + return nullptr; +} + struct SimplePipelineInfo { const char *driver; /* @@ -293,11 +351,13 @@ public: std::unique_ptr<Converter> converter_; std::unique_ptr<SoftwareIsp> swIsp_; + SimpleFrames frameInfo_; private: void tryPipeline(unsigned int code, const Size &size); static std::vector<const MediaPad *> routedSourcePads(MediaPad *sink); + void completeRequest(Request *request); void conversionInputDone(FrameBuffer *buffer); void conversionOutputDone(FrameBuffer *buffer); @@ -785,7 +845,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) /* No conversion, just complete the request. */ Request *request = buffer->request(); pipe->completeBuffer(request, buffer); - pipe->completeRequest(request); + completeRequest(request); return; } @@ -803,7 +863,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) const RequestOutputs &outputs = conversionQueue_.front(); for (auto &[stream, buf] : outputs.outputs) pipe->completeBuffer(outputs.request, buf); - pipe->completeRequest(outputs.request); + completeRequest(outputs.request); conversionQueue_.pop(); return; @@ -861,7 +921,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) /* Otherwise simply complete the request. */ pipe->completeBuffer(request, buffer); - pipe->completeRequest(request); + completeRequest(request); } void SimpleCameraData::clearIncompleteRequests() @@ -872,6 +932,18 @@ void SimpleCameraData::clearIncompleteRequests() } } +void SimpleCameraData::completeRequest(Request *request) +{ + SimpleFrameInfo *info = frameInfo_.find(request); + if (!info) { + /* Something is really wrong, let's return. */ + return; + } + + frameInfo_.destroy(info->frame); + pipe()->completeRequest(request); +} + void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) { /* Queue the input buffer back for capture. */ @@ -885,7 +957,7 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer) /* Complete the buffer and the request. */ Request *request = buffer->request(); if (pipe->completeBuffer(request, buffer)) - pipe->completeRequest(request); + completeRequest(request); } void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId) @@ -1398,6 +1470,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera) video->bufferReady.disconnect(data, &SimpleCameraData::imageBufferReady); + data->frameInfo_.clear(); data->clearIncompleteRequests(); data->conversionBuffers_.clear(); @@ -1428,8 +1501,10 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) if (data->useConversion_) { data->conversionQueue_.push({ request, std::move(buffers) }); - if (data->swIsp_) + if (data->swIsp_) { + data->frameInfo_.create(request); data->swIsp_->queueRequest(request->sequence(), request->controls()); + } } return 0;
Hardware pipelines track requests and other information related to particular frames. This hasn't been needed in software ISP so far. But in order to be able to track metadata corresponding to a given frame, frame-request tracking mechanism starts being useful. This patch introduces the basic tracking structure, actual metadata handling is added in the following patch. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/libcamera/pipeline/simple/simple.cpp | 85 ++++++++++++++++++++++-- 1 file changed, 80 insertions(+), 5 deletions(-)