| Message ID | 20251017102704.3887-5-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Quoting David Plowman (2025-10-17 12:05:40) > We're going to use a "floating statistics region" to store a full > image Y sum. The VC4 platform actually has no floating region for > this, but we can synthesize such a region as follows in software. > > We know that the 15 AGC regions that we do have are arranged to cover > the whole image, and they cannot be changed. Adding up the R, G and B > values here will get us most of the way to Y. But we do also need to > know the most recent colour gains, so code must also be added to > remember the last AWB status. > > With this change, algorithms can now look at the first floating region > on both VC4 and PiSP platforms to get a full image Y average value. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/ipa/rpi/vc4/vc4.cpp | 33 +++++++++++++++++++++++++++++++-- > 1 file changed, 31 insertions(+), 2 deletions(-) > > diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp > index ba43e474..fe754ace 100644 > --- a/src/ipa/rpi/vc4/vc4.cpp > +++ b/src/ipa/rpi/vc4/vc4.cpp > @@ -43,6 +43,9 @@ public: > IpaVc4() > : IpaBase(), lsTable_(nullptr) > { > + lastAwbStatus_.gainR = 1.0; > + lastAwbStatus_.gainG = 1.0; > + lastAwbStatus_.gainB = 1.0; > } > > ~IpaVc4() > @@ -81,6 +84,9 @@ private: > /* LS table allocation passed in from the pipeline handler. */ > SharedFD lsTableHandle_; > void *lsTable_; > + > + /* Remember the most recent AWB values. */ > + AwbStatus lastAwbStatus_; > }; > > int32_t IpaVc4::platformInit([[maybe_unused]] const InitParams ¶ms, [[maybe_unused]] InitResult *result) > @@ -144,8 +150,10 @@ void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms, > std::unique_lock<RPiController::Metadata> lock(rpiMetadata); > > AwbStatus *awbStatus = rpiMetadata.getLocked<AwbStatus>("awb.status"); > - if (awbStatus) > + if (awbStatus) { > applyAWB(awbStatus, ctrls); > + lastAwbStatus_ = *awbStatus; > + } > > CcmStatus *ccmStatus = rpiMetadata.getLocked<CcmStatus>("ccm.status"); > if (ccmStatus) > @@ -226,7 +234,15 @@ RPiController::StatisticsPtr IpaVc4::platformProcessStats(Span<uint8_t> mem) > LOG(IPARPI, Debug) << "No AGC algorithm - not copying statistics"; > statistics->agcRegions.init(0); > } else { > - statistics->agcRegions.init(hw.agcRegions); > + uint64_t sumR = 0; > + uint64_t sumG = 0; > + uint64_t sumB = 0; > + uint32_t countedSum = 0; > + uint32_t notCountedSum = 0; Why not a RgbyRegions::Region as accumulator? But either way works. Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> Best regards, Stefan > + /* We're going to pretend there's a floating region where we will put a full image Y sum. */ > + const unsigned int numFloating = 1; > + > + statistics->agcRegions.init(hw.agcRegions, numFloating); > const std::vector<double> &weights = agc->getWeights(); > for (i = 0; i < statistics->agcRegions.numRegions(); i++) { > uint64_t rSum = (stats->agc_stats[i].r_sum << scale) * weights[i]; > @@ -237,7 +253,20 @@ RPiController::StatisticsPtr IpaVc4::platformProcessStats(Span<uint8_t> mem) > statistics->agcRegions.set(i, { { rSum, gSum, bSum }, > counted, > notcounted }); > + > + /* Accumulate values for the full image Y sum. */ > + sumR += stats->agc_stats[i].r_sum << scale; > + sumG += stats->agc_stats[i].g_sum << scale; > + sumB += stats->agc_stats[i].b_sum << scale; > + countedSum += stats->agc_stats[i].counted; > + notCountedSum += stats->agc_stats[i].notcounted; > } > + > + /* The "floating" region has the Y sum for the entire image. */ > + uint64_t sumY = sumR * lastAwbStatus_.gainR * 0.299 + sumG * lastAwbStatus_.gainG * 0.587 + > + sumB * lastAwbStatus_.gainB * 0.114; > + statistics->agcRegions.setFloating(0, { { sumR, sumG, sumB, sumY }, > + countedSum, notCountedSum }); > } > > statistics->focusRegions.init(hw.focusRegions); > -- > 2.47.3 >
Hi Stefan Thanks for this (and the other!) reviews. On Mon, 20 Oct 2025 at 11:10, Stefan Klug <stefan.klug@ideasonboard.com> wrote: > > Hi David, > > Quoting David Plowman (2025-10-17 12:05:40) > > We're going to use a "floating statistics region" to store a full > > image Y sum. The VC4 platform actually has no floating region for > > this, but we can synthesize such a region as follows in software. > > > > We know that the 15 AGC regions that we do have are arranged to cover > > the whole image, and they cannot be changed. Adding up the R, G and B > > values here will get us most of the way to Y. But we do also need to > > know the most recent colour gains, so code must also be added to > > remember the last AWB status. > > > > With this change, algorithms can now look at the first floating region > > on both VC4 and PiSP platforms to get a full image Y average value. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/ipa/rpi/vc4/vc4.cpp | 33 +++++++++++++++++++++++++++++++-- > > 1 file changed, 31 insertions(+), 2 deletions(-) > > > > diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp > > index ba43e474..fe754ace 100644 > > --- a/src/ipa/rpi/vc4/vc4.cpp > > +++ b/src/ipa/rpi/vc4/vc4.cpp > > @@ -43,6 +43,9 @@ public: > > IpaVc4() > > : IpaBase(), lsTable_(nullptr) > > { > > + lastAwbStatus_.gainR = 1.0; > > + lastAwbStatus_.gainG = 1.0; > > + lastAwbStatus_.gainB = 1.0; > > } > > > > ~IpaVc4() > > @@ -81,6 +84,9 @@ private: > > /* LS table allocation passed in from the pipeline handler. */ > > SharedFD lsTableHandle_; > > void *lsTable_; > > + > > + /* Remember the most recent AWB values. */ > > + AwbStatus lastAwbStatus_; > > }; > > > > int32_t IpaVc4::platformInit([[maybe_unused]] const InitParams ¶ms, [[maybe_unused]] InitResult *result) > > @@ -144,8 +150,10 @@ void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms, > > std::unique_lock<RPiController::Metadata> lock(rpiMetadata); > > > > AwbStatus *awbStatus = rpiMetadata.getLocked<AwbStatus>("awb.status"); > > - if (awbStatus) > > + if (awbStatus) { > > applyAWB(awbStatus, ctrls); > > + lastAwbStatus_ = *awbStatus; > > + } > > > > CcmStatus *ccmStatus = rpiMetadata.getLocked<CcmStatus>("ccm.status"); > > if (ccmStatus) > > @@ -226,7 +234,15 @@ RPiController::StatisticsPtr IpaVc4::platformProcessStats(Span<uint8_t> mem) > > LOG(IPARPI, Debug) << "No AGC algorithm - not copying statistics"; > > statistics->agcRegions.init(0); > > } else { > > - statistics->agcRegions.init(hw.agcRegions); > > + uint64_t sumR = 0; > > + uint64_t sumG = 0; > > + uint64_t sumB = 0; > > + uint32_t countedSum = 0; > > + uint32_t notCountedSum = 0; > > Why not a RgbyRegions::Region as accumulator? Good point - I'll look into that. Thanks! David > > But either way works. > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > > Best regards, > Stefan > > > + /* We're going to pretend there's a floating region where we will put a full image Y sum. */ > > + const unsigned int numFloating = 1; > > + > > + statistics->agcRegions.init(hw.agcRegions, numFloating); > > const std::vector<double> &weights = agc->getWeights(); > > for (i = 0; i < statistics->agcRegions.numRegions(); i++) { > > uint64_t rSum = (stats->agc_stats[i].r_sum << scale) * weights[i]; > > @@ -237,7 +253,20 @@ RPiController::StatisticsPtr IpaVc4::platformProcessStats(Span<uint8_t> mem) > > statistics->agcRegions.set(i, { { rSum, gSum, bSum }, > > counted, > > notcounted }); > > + > > + /* Accumulate values for the full image Y sum. */ > > + sumR += stats->agc_stats[i].r_sum << scale; > > + sumG += stats->agc_stats[i].g_sum << scale; > > + sumB += stats->agc_stats[i].b_sum << scale; > > + countedSum += stats->agc_stats[i].counted; > > + notCountedSum += stats->agc_stats[i].notcounted; > > } > > + > > + /* The "floating" region has the Y sum for the entire image. */ > > + uint64_t sumY = sumR * lastAwbStatus_.gainR * 0.299 + sumG * lastAwbStatus_.gainG * 0.587 + > > + sumB * lastAwbStatus_.gainB * 0.114; > > + statistics->agcRegions.setFloating(0, { { sumR, sumG, sumB, sumY }, > > + countedSum, notCountedSum }); > > } > > > > statistics->focusRegions.init(hw.focusRegions); > > -- > > 2.47.3 > >
Hi David, On Mon, 20 Oct 2025 at 11:10, Stefan Klug <stefan.klug@ideasonboard.com> wrote: > > Hi David, > > Quoting David Plowman (2025-10-17 12:05:40) > > We're going to use a "floating statistics region" to store a full > > image Y sum. The VC4 platform actually has no floating region for > > this, but we can synthesize such a region as follows in software. > > > > We know that the 15 AGC regions that we do have are arranged to cover > > the whole image, and they cannot be changed. Adding up the R, G and B > > values here will get us most of the way to Y. But we do also need to > > know the most recent colour gains, so code must also be added to > > remember the last AWB status. > > > > With this change, algorithms can now look at the first floating region > > on both VC4 and PiSP platforms to get a full image Y average value. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/ipa/rpi/vc4/vc4.cpp | 33 +++++++++++++++++++++++++++++++-- > > 1 file changed, 31 insertions(+), 2 deletions(-) > > > > diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp > > index ba43e474..fe754ace 100644 > > --- a/src/ipa/rpi/vc4/vc4.cpp > > +++ b/src/ipa/rpi/vc4/vc4.cpp > > @@ -43,6 +43,9 @@ public: > > IpaVc4() > > : IpaBase(), lsTable_(nullptr) > > { > > + lastAwbStatus_.gainR = 1.0; > > + lastAwbStatus_.gainG = 1.0; > > + lastAwbStatus_.gainB = 1.0; > > } > > > > ~IpaVc4() > > @@ -81,6 +84,9 @@ private: > > /* LS table allocation passed in from the pipeline handler. */ > > SharedFD lsTableHandle_; > > void *lsTable_; > > + > > + /* Remember the most recent AWB values. */ > > + AwbStatus lastAwbStatus_; > > }; > > > > int32_t IpaVc4::platformInit([[maybe_unused]] const InitParams ¶ms, [[maybe_unused]] InitResult *result) > > @@ -144,8 +150,10 @@ void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms, > > std::unique_lock<RPiController::Metadata> lock(rpiMetadata); > > > > AwbStatus *awbStatus = rpiMetadata.getLocked<AwbStatus>("awb.status"); > > - if (awbStatus) > > + if (awbStatus) { > > applyAWB(awbStatus, ctrls); > > + lastAwbStatus_ = *awbStatus; > > + } > > > > CcmStatus *ccmStatus = rpiMetadata.getLocked<CcmStatus>("ccm.status"); > > if (ccmStatus) > > @@ -226,7 +234,15 @@ RPiController::StatisticsPtr IpaVc4::platformProcessStats(Span<uint8_t> mem) > > LOG(IPARPI, Debug) << "No AGC algorithm - not copying statistics"; > > statistics->agcRegions.init(0); > > } else { > > - statistics->agcRegions.init(hw.agcRegions); > > + uint64_t sumR = 0; > > + uint64_t sumG = 0; > > + uint64_t sumB = 0; > > + uint32_t countedSum = 0; > > + uint32_t notCountedSum = 0; > > Why not a RgbyRegions::Region as accumulator? Yes, that sounds sensible, and avoids having very similar accumulator names :) With that change: Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > But either way works. > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > > Best regards, > Stefan > > > + /* We're going to pretend there's a floating region where we will put a full image Y sum. */ > > + const unsigned int numFloating = 1; > > + > > + statistics->agcRegions.init(hw.agcRegions, numFloating); > > const std::vector<double> &weights = agc->getWeights(); > > for (i = 0; i < statistics->agcRegions.numRegions(); i++) { > > uint64_t rSum = (stats->agc_stats[i].r_sum << scale) * weights[i]; > > @@ -237,7 +253,20 @@ RPiController::StatisticsPtr IpaVc4::platformProcessStats(Span<uint8_t> mem) > > statistics->agcRegions.set(i, { { rSum, gSum, bSum }, > > counted, > > notcounted }); > > + > > + /* Accumulate values for the full image Y sum. */ > > + sumR += stats->agc_stats[i].r_sum << scale; > > + sumG += stats->agc_stats[i].g_sum << scale; > > + sumB += stats->agc_stats[i].b_sum << scale; > > + countedSum += stats->agc_stats[i].counted; > > + notCountedSum += stats->agc_stats[i].notcounted; > > } > > + > > + /* The "floating" region has the Y sum for the entire image. */ > > + uint64_t sumY = sumR * lastAwbStatus_.gainR * 0.299 + sumG * lastAwbStatus_.gainG * 0.587 + > > + sumB * lastAwbStatus_.gainB * 0.114; > > + statistics->agcRegions.setFloating(0, { { sumR, sumG, sumB, sumY }, > > + countedSum, notCountedSum }); > > } > > > > statistics->focusRegions.init(hw.focusRegions); > > -- > > 2.47.3 > >
diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp index ba43e474..fe754ace 100644 --- a/src/ipa/rpi/vc4/vc4.cpp +++ b/src/ipa/rpi/vc4/vc4.cpp @@ -43,6 +43,9 @@ public: IpaVc4() : IpaBase(), lsTable_(nullptr) { + lastAwbStatus_.gainR = 1.0; + lastAwbStatus_.gainG = 1.0; + lastAwbStatus_.gainB = 1.0; } ~IpaVc4() @@ -81,6 +84,9 @@ private: /* LS table allocation passed in from the pipeline handler. */ SharedFD lsTableHandle_; void *lsTable_; + + /* Remember the most recent AWB values. */ + AwbStatus lastAwbStatus_; }; int32_t IpaVc4::platformInit([[maybe_unused]] const InitParams ¶ms, [[maybe_unused]] InitResult *result) @@ -144,8 +150,10 @@ void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms, std::unique_lock<RPiController::Metadata> lock(rpiMetadata); AwbStatus *awbStatus = rpiMetadata.getLocked<AwbStatus>("awb.status"); - if (awbStatus) + if (awbStatus) { applyAWB(awbStatus, ctrls); + lastAwbStatus_ = *awbStatus; + } CcmStatus *ccmStatus = rpiMetadata.getLocked<CcmStatus>("ccm.status"); if (ccmStatus) @@ -226,7 +234,15 @@ RPiController::StatisticsPtr IpaVc4::platformProcessStats(Span<uint8_t> mem) LOG(IPARPI, Debug) << "No AGC algorithm - not copying statistics"; statistics->agcRegions.init(0); } else { - statistics->agcRegions.init(hw.agcRegions); + uint64_t sumR = 0; + uint64_t sumG = 0; + uint64_t sumB = 0; + uint32_t countedSum = 0; + uint32_t notCountedSum = 0; + /* We're going to pretend there's a floating region where we will put a full image Y sum. */ + const unsigned int numFloating = 1; + + statistics->agcRegions.init(hw.agcRegions, numFloating); const std::vector<double> &weights = agc->getWeights(); for (i = 0; i < statistics->agcRegions.numRegions(); i++) { uint64_t rSum = (stats->agc_stats[i].r_sum << scale) * weights[i]; @@ -237,7 +253,20 @@ RPiController::StatisticsPtr IpaVc4::platformProcessStats(Span<uint8_t> mem) statistics->agcRegions.set(i, { { rSum, gSum, bSum }, counted, notcounted }); + + /* Accumulate values for the full image Y sum. */ + sumR += stats->agc_stats[i].r_sum << scale; + sumG += stats->agc_stats[i].g_sum << scale; + sumB += stats->agc_stats[i].b_sum << scale; + countedSum += stats->agc_stats[i].counted; + notCountedSum += stats->agc_stats[i].notcounted; } + + /* The "floating" region has the Y sum for the entire image. */ + uint64_t sumY = sumR * lastAwbStatus_.gainR * 0.299 + sumG * lastAwbStatus_.gainG * 0.587 + + sumB * lastAwbStatus_.gainB * 0.114; + statistics->agcRegions.setFloating(0, { { sumR, sumG, sumB, sumY }, + countedSum, notCountedSum }); } statistics->focusRegions.init(hw.focusRegions);
We're going to use a "floating statistics region" to store a full image Y sum. The VC4 platform actually has no floating region for this, but we can synthesize such a region as follows in software. We know that the 15 AGC regions that we do have are arranged to cover the whole image, and they cannot be changed. Adding up the R, G and B values here will get us most of the way to Y. But we do also need to know the most recent colour gains, so code must also be added to remember the last AWB status. With this change, algorithms can now look at the first floating region on both VC4 and PiSP platforms to get a full image Y average value. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/ipa/rpi/vc4/vc4.cpp | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-)