| Message ID | 20240626072100.55497-11-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan On 26/06/24 12:50 pm, Milan Zamazal wrote: > This patch adds Algorithm::process call for the defined algorithms. > This is preparation only since there are currently no Algorithm based > algorithms defined. > > As software ISP currently doesn't produce any metadata, a dummy and > unused metadata instance is created to satisfy Algorithm::process API. > This should be changed in future. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/ipa/simple/soft_simple.cpp | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > index 6ddb4004..c0cb6769 100644 > --- a/src/ipa/simple/soft_simple.cpp > +++ b/src/ipa/simple/soft_simple.cpp > @@ -265,10 +265,21 @@ void IPASoftSimple::stop() > } > > void IPASoftSimple::processStats( > - [[maybe_unused]] const uint32_t frame, > + const uint32_t frame, > [[maybe_unused]] const uint32_t bufferId, > const ControlList &sensorControls) > { > + IPAFrameContext &frameContext = context_.frameContexts.get(frame); I was trying to infer from the previous patches where the frameContexts were allocated per frame.. only to find out it is done in later patch 12/19. the Algorithms::* patches ideally would be arranged in the order of their logical calling sequencing ... but it's not the end of the world. :) So far looks good to me. > + /* > + * \todo Software ISP currently doesn't produce any metadata so it is > + * possible to use a dummy metadata instance here. But metadata should be > + * properly handled in future. > + */ > + ControlList metadata(controls::controls); > + for (auto const &algo : algorithms()) { > + algo->process(context_, frame, frameContext, stats_, metadata); > + } > + > SwIspStats::Histogram histogram = stats_->yHistogram; > if (ignoreUpdates_ > 0) > blackLevel_.update(histogram);
Hi Milan, Thank you for the patch. On 26/06/24 12:50 pm, Milan Zamazal wrote: > This patch adds Algorithm::process call for the defined algorithms. > This is preparation only since there are currently no Algorithm based > algorithms defined. > > As software ISP currently doesn't produce any metadata, a dummy and > unused metadata instance is created to satisfy Algorithm::process API. > This should be changed in future. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/ipa/simple/soft_simple.cpp | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > index 6ddb4004..c0cb6769 100644 > --- a/src/ipa/simple/soft_simple.cpp > +++ b/src/ipa/simple/soft_simple.cpp > @@ -265,10 +265,21 @@ void IPASoftSimple::stop() > } > > void IPASoftSimple::processStats( > - [[maybe_unused]] const uint32_t frame, > + const uint32_t frame, > [[maybe_unused]] const uint32_t bufferId, > const ControlList &sensorControls) > { > + IPAFrameContext &frameContext = context_.frameContexts.get(frame); > + /* > + * \todo Software ISP currently doesn't produce any metadata so it is > + * possible to use a dummy metadata instance here. But metadata should be > + * properly handled in future. > + */ Can be possibly reworded to: /* * Software ISP currently does not produce any metadata. Use a empty * ControlList for now. * * \todo Implement proper metadata handling */ > + ControlList metadata(controls::controls); > + for (auto const &algo : algorithms()) { > + algo->process(context_, frame, frameContext, stats_, metadata); > + } {...} can be dropped Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > + > SwIspStats::Histogram histogram = stats_->yHistogram; > if (ignoreUpdates_ > 0) > blackLevel_.update(histogram);
Hi Umang, thank you for review. Umang Jain <umang.jain@ideasonboard.com> writes: > Hi Milan > > On 26/06/24 12:50 pm, Milan Zamazal wrote: >> This patch adds Algorithm::process call for the defined algorithms. >> This is preparation only since there are currently no Algorithm based >> algorithms defined. >> >> As software ISP currently doesn't produce any metadata, a dummy and >> unused metadata instance is created to satisfy Algorithm::process API. >> This should be changed in future. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/ipa/simple/soft_simple.cpp | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >> index 6ddb4004..c0cb6769 100644 >> --- a/src/ipa/simple/soft_simple.cpp >> +++ b/src/ipa/simple/soft_simple.cpp >> @@ -265,10 +265,21 @@ void IPASoftSimple::stop() >> } >> void IPASoftSimple::processStats( >> - [[maybe_unused]] const uint32_t frame, >> + const uint32_t frame, >> [[maybe_unused]] const uint32_t bufferId, >> const ControlList &sensorControls) >> { >> + IPAFrameContext &frameContext = context_.frameContexts.get(frame); > > I was trying to infer from the previous patches where the frameContexts were > allocated per frame.. only to find out it is done in later patch 12/19. > > the Algorithms::* patches ideally would be arranged in the order of their logical > calling sequencing ... but it's not the end of the world. :) I thought arranging them from simpler to more complicated might be easier but since it's actually more confusing, I'll change the order. > So far looks good to me. > >> + /* >> + * \todo Software ISP currently doesn't produce any metadata so it is >> + * possible to use a dummy metadata instance here. But metadata should be >> + * properly handled in future. >> + */ >> + ControlList metadata(controls::controls); >> + for (auto const &algo : algorithms()) { >> + algo->process(context_, frame, frameContext, stats_, metadata); >> + } >> + >> SwIspStats::Histogram histogram = stats_->yHistogram; >> if (ignoreUpdates_ > 0) >> blackLevel_.update(histogram);
diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp index 6ddb4004..c0cb6769 100644 --- a/src/ipa/simple/soft_simple.cpp +++ b/src/ipa/simple/soft_simple.cpp @@ -265,10 +265,21 @@ void IPASoftSimple::stop() } void IPASoftSimple::processStats( - [[maybe_unused]] const uint32_t frame, + const uint32_t frame, [[maybe_unused]] const uint32_t bufferId, const ControlList &sensorControls) { + IPAFrameContext &frameContext = context_.frameContexts.get(frame); + /* + * \todo Software ISP currently doesn't produce any metadata so it is + * possible to use a dummy metadata instance here. But metadata should be + * properly handled in future. + */ + ControlList metadata(controls::controls); + for (auto const &algo : algorithms()) { + algo->process(context_, frame, frameContext, stats_, metadata); + } + SwIspStats::Histogram histogram = stats_->yHistogram; if (ignoreUpdates_ > 0) blackLevel_.update(histogram);
This patch adds Algorithm::process call for the defined algorithms. This is preparation only since there are currently no Algorithm based algorithms defined. As software ISP currently doesn't produce any metadata, a dummy and unused metadata instance is created to satisfy Algorithm::process API. This should be changed in future. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/ipa/simple/soft_simple.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)