| Message ID | 20250113133405.12167-2-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan, Thank you for the patch. On Mon, Jan 13, 2025 at 02:34:00PM +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 | 81 ++++++++++++++++++++++-- > 1 file changed, 76 insertions(+), 5 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 8ac24e6e..123179b6 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); > > @@ -799,7 +859,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; > } > > @@ -817,7 +877,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; > @@ -875,7 +935,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > > /* Otherwise simply complete the request. */ > pipe->completeBuffer(request, buffer); > - pipe->completeRequest(request); > + completeRequest(request); > } > > void SimpleCameraData::clearIncompleteRequests() > @@ -886,6 +946,14 @@ void SimpleCameraData::clearIncompleteRequests() > } > } > > +void SimpleCameraData::completeRequest(Request *request) > +{ > + SimpleFrameInfo *info = frameInfo_.find(request); > + if (info) If there's no frame info something is really wrong. I don't think completing the request makes much sense in that case. > + frameInfo_.destroy(info->frame); > + pipe()->completeRequest(request); > +} > + > void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) > { > /* Queue the input buffer back for capture. */ > @@ -899,7 +967,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) > @@ -1412,6 +1480,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera) > > video->bufferReady.disconnect(data, &SimpleCameraData::imageBufferReady); > > + data->frameInfo_.clear(); > data->clearIncompleteRequests(); > data->conversionBuffers_.clear(); > > @@ -1442,8 +1511,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;
Quoting Laurent Pinchart (2025-01-27 06:59:36) > Hi Milan, > > Thank you for the patch. > > On Mon, Jan 13, 2025 at 02:34:00PM +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. > > I'm also weary that Barnabás made some fixes to other IPA 'frame handlers' recently. I think somehow this concept needs to be templated to make sure we don't have different bugs and different implementation in each IPA... Not a fault of 'this' patch, but I'm really weary about seeing multiple implementations of the same object. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > > --- > > src/libcamera/pipeline/simple/simple.cpp | 81 ++++++++++++++++++++++-- > > 1 file changed, 76 insertions(+), 5 deletions(-) > > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > > index 8ac24e6e..123179b6 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); > > > > @@ -799,7 +859,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; > > } > > > > @@ -817,7 +877,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; > > @@ -875,7 +935,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > > > > /* Otherwise simply complete the request. */ > > pipe->completeBuffer(request, buffer); > > - pipe->completeRequest(request); > > + completeRequest(request); > > } > > > > void SimpleCameraData::clearIncompleteRequests() > > @@ -886,6 +946,14 @@ void SimpleCameraData::clearIncompleteRequests() > > } > > } > > > > +void SimpleCameraData::completeRequest(Request *request) > > +{ > > + SimpleFrameInfo *info = frameInfo_.find(request); > > + if (info) > > If there's no frame info something is really wrong. I don't think > completing the request makes much sense in that case. > > > + frameInfo_.destroy(info->frame); > > + pipe()->completeRequest(request); > > +} > > + > > void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) > > { > > /* Queue the input buffer back for capture. */ > > @@ -899,7 +967,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) > > @@ -1412,6 +1480,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera) > > > > video->bufferReady.disconnect(data, &SimpleCameraData::imageBufferReady); > > > > + data->frameInfo_.clear(); > > data->clearIncompleteRequests(); > > data->conversionBuffers_.clear(); > > > > @@ -1442,8 +1511,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; > > -- > Regards, > > Laurent Pinchart
2025. január 27., hétfő 19:35 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta: > Quoting Laurent Pinchart (2025-01-27 06:59:36) > > Hi Milan, > > > > Thank you for the patch. > > > > On Mon, Jan 13, 2025 at 02:34:00PM +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. > > > > > I'm also weary that Barnabás made some fixes to other IPA 'frame > handlers' recently. The changes in the proposed https://patchwork.libcamera.org/patch/22610/ patch have already been incorporated into this patch set. > > I think somehow this concept needs to be templated to make sure we don't > have different bugs and different implementation in each IPA... > > Not a fault of 'this' patch, but I'm really weary about seeing multiple > implementations of the same object. I agree. Regards, Barnabás Pőcze > > > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > > > --- > > > src/libcamera/pipeline/simple/simple.cpp | 81 ++++++++++++++++++++++-- > > > 1 file changed, 76 insertions(+), 5 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > > > index 8ac24e6e..123179b6 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); > > > > > > @@ -799,7 +859,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; > > > } > > > > > > @@ -817,7 +877,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; > > > @@ -875,7 +935,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) > > > > > > /* Otherwise simply complete the request. */ > > > pipe->completeBuffer(request, buffer); > > > - pipe->completeRequest(request); > > > + completeRequest(request); > > > } > > > > > > void SimpleCameraData::clearIncompleteRequests() > > > @@ -886,6 +946,14 @@ void SimpleCameraData::clearIncompleteRequests() > > > } > > > } > > > > > > +void SimpleCameraData::completeRequest(Request *request) > > > +{ > > > + SimpleFrameInfo *info = frameInfo_.find(request); > > > + if (info) > > > > If there's no frame info something is really wrong. I don't think > > completing the request makes much sense in that case. > > > > > + frameInfo_.destroy(info->frame); > > > + pipe()->completeRequest(request); > > > +} > > > + > > > void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) > > > { > > > /* Queue the input buffer back for capture. */ > > > @@ -899,7 +967,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) > > > @@ -1412,6 +1480,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera) > > > > > > video->bufferReady.disconnect(data, &SimpleCameraData::imageBufferReady); > > > > > > + data->frameInfo_.clear(); > > > data->clearIncompleteRequests(); > > > data->conversionBuffers_.clear(); > > > > > > @@ -1442,8 +1511,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; > > > > -- > > Regards, > > > > Laurent Pinchart >
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 8ac24e6e..123179b6 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); @@ -799,7 +859,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; } @@ -817,7 +877,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; @@ -875,7 +935,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer) /* Otherwise simply complete the request. */ pipe->completeBuffer(request, buffer); - pipe->completeRequest(request); + completeRequest(request); } void SimpleCameraData::clearIncompleteRequests() @@ -886,6 +946,14 @@ void SimpleCameraData::clearIncompleteRequests() } } +void SimpleCameraData::completeRequest(Request *request) +{ + SimpleFrameInfo *info = frameInfo_.find(request); + if (info) + frameInfo_.destroy(info->frame); + pipe()->completeRequest(request); +} + void SimpleCameraData::conversionInputDone(FrameBuffer *buffer) { /* Queue the input buffer back for capture. */ @@ -899,7 +967,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) @@ -1412,6 +1480,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera) video->bufferReady.disconnect(data, &SimpleCameraData::imageBufferReady); + data->frameInfo_.clear(); data->clearIncompleteRequests(); data->conversionBuffers_.clear(); @@ -1442,8 +1511,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 | 81 ++++++++++++++++++++++-- 1 file changed, 76 insertions(+), 5 deletions(-)