| Message ID | 20221213114836.15473-6-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Naushir Patuck via libcamera-devel (2022-12-13 11:48:36) > The VC4 ISP uses a pipeline bit-depth of 13-bits. The AGC algorithm needs to > know this bit-depth when computing the Y value for the image. > > Instead of hardcoding the VC4 bit-depth in the AGC source code, normalise all > region sums to 16-bits when filling the Statistics structure. AWB and ALSC are > agnostic about pipeline depth, so do not need changing. pipeline bit-depth to not confuse pipeline depth with the number of buffers as this term has been used for previously? > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/ipa/raspberrypi/controller/rpi/agc.cpp | 10 ++++------ > src/ipa/raspberrypi/raspberrypi.cpp | 16 ++++++++++------ > 2 files changed, 14 insertions(+), 12 deletions(-) > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp > index 868c30f03d66..7ec8292d32ff 100644 > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp > @@ -28,8 +28,6 @@ LOG_DEFINE_CATEGORY(RPiAgc) > > #define NAME "rpi.agc" > > -static constexpr unsigned int PipelineBits = 13; /* seems to be a 13-bit pipeline */ > - > int AgcMeteringMode::read(const libcamera::YamlObject ¶ms) > { > const YamlObject &yamlWeights = params["weights"]; > @@ -593,9 +591,9 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb, > double rSum = 0, gSum = 0, bSum = 0, pixelSum = 0; > for (unsigned int i = 0; i < stats->agcRegions.numRegions(); i++) { > auto ®ion = stats->agcRegions.get(i); > - double rAcc = std::min<double>(region.val.rSum * gain, ((1 << PipelineBits) - 1) * region.counted); > - double gAcc = std::min<double>(region.val.gSum * gain, ((1 << PipelineBits) - 1) * region.counted); > - double bAcc = std::min<double>(region.val.bSum * gain, ((1 << PipelineBits) - 1) * region.counted); > + double rAcc = std::min<double>(region.val.rSum * gain, ((1 << 16) - 1) * region.counted); > + double gAcc = std::min<double>(region.val.gSum * gain, ((1 << 16) - 1) * region.counted); > + double bAcc = std::min<double>(region.val.bSum * gain, ((1 << 16) - 1) * region.counted); 1 << 16 - 1 now looks a bit 'magic', and it may not be clear to a reader what it's refering to? Before it referenced PipelineBits so it was clearer. I expect this ((1 << 16) - 1) could be a constexpr somewhere at the top of the file, or a helper to wrap the scaling of region.counted? Hrm ... it 'was' already a constexpr at the top... Is there a reason you chose to hardcode the value here instead of update (/and or rename) the existing PipelineBits ? Aside from that: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > rSum += rAcc * weights[i]; > gSum += gAcc * weights[i]; > bSum += bAcc * weights[i]; > @@ -608,7 +606,7 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb, > double ySum = rSum * awb.gainR * .299 + > gSum * awb.gainG * .587 + > bSum * awb.gainB * .114; > - return ySum / pixelSum / (1 << PipelineBits); > + return ySum / pixelSum / (1 << 16); > } > > /* > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index cff079bbafb3..50fdeb4f0478 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -1154,11 +1154,15 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co > /* RGB histograms are not used, so do not populate them. */ > statistics->yHist = std::move(RPiController::Histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS)); > > + /* > + * All region sums are based on a 13-bit pipeline bit-depth. Normalise > + * this to 16-bits for the AGC/AWB/ALSC algorithms. > + */ > statistics->awbRegions.init({ DEFAULT_AWB_REGIONS_X, DEFAULT_AWB_REGIONS_Y }); > for (i = 0; i < statistics->awbRegions.numRegions(); i++) > - statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum, > - stats->awb_stats[i].g_sum, > - stats->awb_stats[i].b_sum }, > + statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum << 3, > + stats->awb_stats[i].g_sum << 3, > + stats->awb_stats[i].b_sum << 3 }, > stats->awb_stats[i].counted, > stats->awb_stats[i].notcounted }); > > @@ -1168,9 +1172,9 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co > */ > statistics->agcRegions.init(15); > for (i = 0; i < statistics->agcRegions.numRegions(); i++) > - statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum, > - stats->agc_stats[i].g_sum, > - stats->agc_stats[i].b_sum }, > + statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum << 3, > + stats->agc_stats[i].g_sum << 3, > + stats->agc_stats[i].b_sum << 3 }, > stats->agc_stats[i].counted, > stats->awb_stats[i].notcounted }); > > -- > 2.25.1 >
Hi Kieran, On Mon, 30 Jan 2023 at 13:31, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Naushir Patuck via libcamera-devel (2022-12-13 11:48:36) > > The VC4 ISP uses a pipeline bit-depth of 13-bits. The AGC algorithm needs to > > know this bit-depth when computing the Y value for the image. > > > > Instead of hardcoding the VC4 bit-depth in the AGC source code, normalise all > > region sums to 16-bits when filling the Statistics structure. AWB and ALSC are > > agnostic about pipeline depth, so do not need changing. > > pipeline bit-depth to not confuse pipeline depth with the number of > buffers as this term has been used for previously? > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 10 ++++------ > > src/ipa/raspberrypi/raspberrypi.cpp | 16 ++++++++++------ > > 2 files changed, 14 insertions(+), 12 deletions(-) > > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp > > index 868c30f03d66..7ec8292d32ff 100644 > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp > > @@ -28,8 +28,6 @@ LOG_DEFINE_CATEGORY(RPiAgc) > > > > #define NAME "rpi.agc" > > > > -static constexpr unsigned int PipelineBits = 13; /* seems to be a 13-bit pipeline */ > > - > > int AgcMeteringMode::read(const libcamera::YamlObject ¶ms) > > { > > const YamlObject &yamlWeights = params["weights"]; > > @@ -593,9 +591,9 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb, > > double rSum = 0, gSum = 0, bSum = 0, pixelSum = 0; > > for (unsigned int i = 0; i < stats->agcRegions.numRegions(); i++) { > > auto ®ion = stats->agcRegions.get(i); > > - double rAcc = std::min<double>(region.val.rSum * gain, ((1 << PipelineBits) - 1) * region.counted); > > - double gAcc = std::min<double>(region.val.gSum * gain, ((1 << PipelineBits) - 1) * region.counted); > > - double bAcc = std::min<double>(region.val.bSum * gain, ((1 << PipelineBits) - 1) * region.counted); > > + double rAcc = std::min<double>(region.val.rSum * gain, ((1 << 16) - 1) * region.counted); > > + double gAcc = std::min<double>(region.val.gSum * gain, ((1 << 16) - 1) * region.counted); > > + double bAcc = std::min<double>(region.val.bSum * gain, ((1 << 16) - 1) * region.counted); > > 1 << 16 - 1 now looks a bit 'magic', and it may not be clear to a > reader what it's refering to? > > Before it referenced PipelineBits so it was clearer. I expect this > ((1 << 16) - 1) > > could be a constexpr somewhere at the top of the file, or a helper to > wrap the scaling of region.counted? > > > Hrm ... it 'was' already a constexpr at the top... > Is there a reason you chose to hardcode the value here instead of update > (/and or rename) the existing PipelineBits ? Since the generalised statistics are now normalised to 16-bits, perhaps what is needed is to have a constexpr for (1 << 16) - 1 to be (possibly) defined in the statistics structure to indicate the normalisation used? Naush > > Aside from that: > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > rSum += rAcc * weights[i]; > > gSum += gAcc * weights[i]; > > bSum += bAcc * weights[i]; > > @@ -608,7 +606,7 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb, > > double ySum = rSum * awb.gainR * .299 + > > gSum * awb.gainG * .587 + > > bSum * awb.gainB * .114; > > - return ySum / pixelSum / (1 << PipelineBits); > > + return ySum / pixelSum / (1 << 16); > > } > > > > /* > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > index cff079bbafb3..50fdeb4f0478 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -1154,11 +1154,15 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co > > /* RGB histograms are not used, so do not populate them. */ > > statistics->yHist = std::move(RPiController::Histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS)); > > > > + /* > > + * All region sums are based on a 13-bit pipeline bit-depth. Normalise > > + * this to 16-bits for the AGC/AWB/ALSC algorithms. > > + */ > > statistics->awbRegions.init({ DEFAULT_AWB_REGIONS_X, DEFAULT_AWB_REGIONS_Y }); > > for (i = 0; i < statistics->awbRegions.numRegions(); i++) > > - statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum, > > - stats->awb_stats[i].g_sum, > > - stats->awb_stats[i].b_sum }, > > + statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum << 3, > > + stats->awb_stats[i].g_sum << 3, > > + stats->awb_stats[i].b_sum << 3 }, > > stats->awb_stats[i].counted, > > stats->awb_stats[i].notcounted }); > > > > @@ -1168,9 +1172,9 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co > > */ > > statistics->agcRegions.init(15); > > for (i = 0; i < statistics->agcRegions.numRegions(); i++) > > - statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum, > > - stats->agc_stats[i].g_sum, > > - stats->agc_stats[i].b_sum }, > > + statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum << 3, > > + stats->agc_stats[i].g_sum << 3, > > + stats->agc_stats[i].b_sum << 3 }, > > stats->agc_stats[i].counted, > > stats->awb_stats[i].notcounted }); > > > > -- > > 2.25.1 > >
Quoting Naushir Patuck (2023-01-31 12:29:07) > Hi Kieran, > > On Mon, 30 Jan 2023 at 13:31, Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: > > > > Quoting Naushir Patuck via libcamera-devel (2022-12-13 11:48:36) > > > The VC4 ISP uses a pipeline bit-depth of 13-bits. The AGC algorithm needs to > > > know this bit-depth when computing the Y value for the image. > > > > > > Instead of hardcoding the VC4 bit-depth in the AGC source code, normalise all > > > region sums to 16-bits when filling the Statistics structure. AWB and ALSC are > > > agnostic about pipeline depth, so do not need changing. > > > > pipeline bit-depth to not confuse pipeline depth with the number of > > buffers as this term has been used for previously? > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 10 ++++------ > > > src/ipa/raspberrypi/raspberrypi.cpp | 16 ++++++++++------ > > > 2 files changed, 14 insertions(+), 12 deletions(-) > > > > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp > > > index 868c30f03d66..7ec8292d32ff 100644 > > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp > > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp > > > @@ -28,8 +28,6 @@ LOG_DEFINE_CATEGORY(RPiAgc) > > > > > > #define NAME "rpi.agc" > > > > > > -static constexpr unsigned int PipelineBits = 13; /* seems to be a 13-bit pipeline */ > > > - > > > int AgcMeteringMode::read(const libcamera::YamlObject ¶ms) > > > { > > > const YamlObject &yamlWeights = params["weights"]; > > > @@ -593,9 +591,9 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb, > > > double rSum = 0, gSum = 0, bSum = 0, pixelSum = 0; > > > for (unsigned int i = 0; i < stats->agcRegions.numRegions(); i++) { > > > auto ®ion = stats->agcRegions.get(i); > > > - double rAcc = std::min<double>(region.val.rSum * gain, ((1 << PipelineBits) - 1) * region.counted); > > > - double gAcc = std::min<double>(region.val.gSum * gain, ((1 << PipelineBits) - 1) * region.counted); > > > - double bAcc = std::min<double>(region.val.bSum * gain, ((1 << PipelineBits) - 1) * region.counted); > > > + double rAcc = std::min<double>(region.val.rSum * gain, ((1 << 16) - 1) * region.counted); > > > + double gAcc = std::min<double>(region.val.gSum * gain, ((1 << 16) - 1) * region.counted); > > > + double bAcc = std::min<double>(region.val.bSum * gain, ((1 << 16) - 1) * region.counted); > > > > 1 << 16 - 1 now looks a bit 'magic', and it may not be clear to a > > reader what it's refering to? > > > > Before it referenced PipelineBits so it was clearer. I expect this > > ((1 << 16) - 1) > > > > could be a constexpr somewhere at the top of the file, or a helper to > > wrap the scaling of region.counted? > > > > > > Hrm ... it 'was' already a constexpr at the top... > > Is there a reason you chose to hardcode the value here instead of update > > (/and or rename) the existing PipelineBits ? > > Since the generalised statistics are now normalised to 16-bits, > perhaps what is needed is to have a constexpr for (1 << 16) - 1 to be > (possibly) defined in the statistics structure to indicate the > normalisation used? Perhaps, but we're in src/ipa/raspberrypi/ so it's all up to your preference. I'll merge this series, and if you think it deserves a cleanup on top, that should be easy to treat independently. -- Kieran > > Naush > > > > > Aside from that: > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > rSum += rAcc * weights[i]; > > > gSum += gAcc * weights[i]; > > > bSum += bAcc * weights[i]; > > > @@ -608,7 +606,7 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb, > > > double ySum = rSum * awb.gainR * .299 + > > > gSum * awb.gainG * .587 + > > > bSum * awb.gainB * .114; > > > - return ySum / pixelSum / (1 << PipelineBits); > > > + return ySum / pixelSum / (1 << 16); > > > } > > > > > > /* > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > > index cff079bbafb3..50fdeb4f0478 100644 > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > @@ -1154,11 +1154,15 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co > > > /* RGB histograms are not used, so do not populate them. */ > > > statistics->yHist = std::move(RPiController::Histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS)); > > > > > > + /* > > > + * All region sums are based on a 13-bit pipeline bit-depth. Normalise > > > + * this to 16-bits for the AGC/AWB/ALSC algorithms. > > > + */ > > > statistics->awbRegions.init({ DEFAULT_AWB_REGIONS_X, DEFAULT_AWB_REGIONS_Y }); > > > for (i = 0; i < statistics->awbRegions.numRegions(); i++) > > > - statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum, > > > - stats->awb_stats[i].g_sum, > > > - stats->awb_stats[i].b_sum }, > > > + statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum << 3, > > > + stats->awb_stats[i].g_sum << 3, > > > + stats->awb_stats[i].b_sum << 3 }, > > > stats->awb_stats[i].counted, > > > stats->awb_stats[i].notcounted }); > > > > > > @@ -1168,9 +1172,9 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co > > > */ > > > statistics->agcRegions.init(15); > > > for (i = 0; i < statistics->agcRegions.numRegions(); i++) > > > - statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum, > > > - stats->agc_stats[i].g_sum, > > > - stats->agc_stats[i].b_sum }, > > > + statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum << 3, > > > + stats->agc_stats[i].g_sum << 3, > > > + stats->agc_stats[i].b_sum << 3 }, > > > stats->agc_stats[i].counted, > > > stats->awb_stats[i].notcounted }); > > > > > > -- > > > 2.25.1 > > >
Quoting Kieran Bingham (2023-01-31 13:52:40) > Quoting Naushir Patuck (2023-01-31 12:29:07) > > Hi Kieran, > > > > On Mon, 30 Jan 2023 at 13:31, Kieran Bingham > > <kieran.bingham@ideasonboard.com> wrote: > > > > > > Quoting Naushir Patuck via libcamera-devel (2022-12-13 11:48:36) > > > > The VC4 ISP uses a pipeline bit-depth of 13-bits. The AGC algorithm needs to > > > > know this bit-depth when computing the Y value for the image. > > > > > > > > Instead of hardcoding the VC4 bit-depth in the AGC source code, normalise all > > > > region sums to 16-bits when filling the Statistics structure. AWB and ALSC are > > > > agnostic about pipeline depth, so do not need changing. > > > > > > pipeline bit-depth to not confuse pipeline depth with the number of > > > buffers as this term has been used for previously? > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 10 ++++------ > > > > src/ipa/raspberrypi/raspberrypi.cpp | 16 ++++++++++------ > > > > 2 files changed, 14 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp > > > > index 868c30f03d66..7ec8292d32ff 100644 > > > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp > > > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp > > > > @@ -28,8 +28,6 @@ LOG_DEFINE_CATEGORY(RPiAgc) > > > > > > > > #define NAME "rpi.agc" > > > > > > > > -static constexpr unsigned int PipelineBits = 13; /* seems to be a 13-bit pipeline */ > > > > - > > > > int AgcMeteringMode::read(const libcamera::YamlObject ¶ms) > > > > { > > > > const YamlObject &yamlWeights = params["weights"]; > > > > @@ -593,9 +591,9 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb, > > > > double rSum = 0, gSum = 0, bSum = 0, pixelSum = 0; > > > > for (unsigned int i = 0; i < stats->agcRegions.numRegions(); i++) { > > > > auto ®ion = stats->agcRegions.get(i); > > > > - double rAcc = std::min<double>(region.val.rSum * gain, ((1 << PipelineBits) - 1) * region.counted); > > > > - double gAcc = std::min<double>(region.val.gSum * gain, ((1 << PipelineBits) - 1) * region.counted); > > > > - double bAcc = std::min<double>(region.val.bSum * gain, ((1 << PipelineBits) - 1) * region.counted); > > > > + double rAcc = std::min<double>(region.val.rSum * gain, ((1 << 16) - 1) * region.counted); > > > > + double gAcc = std::min<double>(region.val.gSum * gain, ((1 << 16) - 1) * region.counted); > > > > + double bAcc = std::min<double>(region.val.bSum * gain, ((1 << 16) - 1) * region.counted); > > > > > > 1 << 16 - 1 now looks a bit 'magic', and it may not be clear to a > > > reader what it's refering to? > > > > > > Before it referenced PipelineBits so it was clearer. I expect this > > > ((1 << 16) - 1) > > > > > > could be a constexpr somewhere at the top of the file, or a helper to > > > wrap the scaling of region.counted? > > > > > > > > > Hrm ... it 'was' already a constexpr at the top... > > > Is there a reason you chose to hardcode the value here instead of update > > > (/and or rename) the existing PipelineBits ? > > > > Since the generalised statistics are now normalised to 16-bits, > > perhaps what is needed is to have a constexpr for (1 << 16) - 1 to be > > (possibly) defined in the statistics structure to indicate the > > normalisation used? > > Perhaps, but we're in src/ipa/raspberrypi/ so it's all up to your > preference. > > I'll merge this series, and if you think it deserves a cleanup on top, > that should be easy to treat independently. > aha - we crossed paths. I'll make sure your new patch still works, and use that instead of this one. ;-) -- Kieran > -- > Kieran > > > > > > Naush > > > > > > > > Aside from that: > > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > > rSum += rAcc * weights[i]; > > > > gSum += gAcc * weights[i]; > > > > bSum += bAcc * weights[i]; > > > > @@ -608,7 +606,7 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb, > > > > double ySum = rSum * awb.gainR * .299 + > > > > gSum * awb.gainG * .587 + > > > > bSum * awb.gainB * .114; > > > > - return ySum / pixelSum / (1 << PipelineBits); > > > > + return ySum / pixelSum / (1 << 16); > > > > } > > > > > > > > /* > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > > > index cff079bbafb3..50fdeb4f0478 100644 > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > > @@ -1154,11 +1154,15 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co > > > > /* RGB histograms are not used, so do not populate them. */ > > > > statistics->yHist = std::move(RPiController::Histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS)); > > > > > > > > + /* > > > > + * All region sums are based on a 13-bit pipeline bit-depth. Normalise > > > > + * this to 16-bits for the AGC/AWB/ALSC algorithms. > > > > + */ > > > > statistics->awbRegions.init({ DEFAULT_AWB_REGIONS_X, DEFAULT_AWB_REGIONS_Y }); > > > > for (i = 0; i < statistics->awbRegions.numRegions(); i++) > > > > - statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum, > > > > - stats->awb_stats[i].g_sum, > > > > - stats->awb_stats[i].b_sum }, > > > > + statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum << 3, > > > > + stats->awb_stats[i].g_sum << 3, > > > > + stats->awb_stats[i].b_sum << 3 }, > > > > stats->awb_stats[i].counted, > > > > stats->awb_stats[i].notcounted }); > > > > > > > > @@ -1168,9 +1172,9 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co > > > > */ > > > > statistics->agcRegions.init(15); > > > > for (i = 0; i < statistics->agcRegions.numRegions(); i++) > > > > - statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum, > > > > - stats->agc_stats[i].g_sum, > > > > - stats->agc_stats[i].b_sum }, > > > > + statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum << 3, > > > > + stats->agc_stats[i].g_sum << 3, > > > > + stats->agc_stats[i].b_sum << 3 }, > > > > stats->agc_stats[i].counted, > > > > stats->awb_stats[i].notcounted }); > > > > > > > > -- > > > > 2.25.1 > > > >
On Tue, 31 Jan 2023 at 13:53, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Kieran Bingham (2023-01-31 13:52:40) > > Quoting Naushir Patuck (2023-01-31 12:29:07) > > > Hi Kieran, > > > > > > On Mon, 30 Jan 2023 at 13:31, Kieran Bingham > > > <kieran.bingham@ideasonboard.com> wrote: > > > > > > > > Quoting Naushir Patuck via libcamera-devel (2022-12-13 11:48:36) > > > > > The VC4 ISP uses a pipeline bit-depth of 13-bits. The AGC algorithm needs to > > > > > know this bit-depth when computing the Y value for the image. > > > > > > > > > > Instead of hardcoding the VC4 bit-depth in the AGC source code, normalise all > > > > > region sums to 16-bits when filling the Statistics structure. AWB and ALSC are > > > > > agnostic about pipeline depth, so do not need changing. > > > > > > > > pipeline bit-depth to not confuse pipeline depth with the number of > > > > buffers as this term has been used for previously? > > > > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > > --- > > > > > src/ipa/raspberrypi/controller/rpi/agc.cpp | 10 ++++------ > > > > > src/ipa/raspberrypi/raspberrypi.cpp | 16 ++++++++++------ > > > > > 2 files changed, 14 insertions(+), 12 deletions(-) > > > > > > > > > > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp > > > > > index 868c30f03d66..7ec8292d32ff 100644 > > > > > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp > > > > > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp > > > > > @@ -28,8 +28,6 @@ LOG_DEFINE_CATEGORY(RPiAgc) > > > > > > > > > > #define NAME "rpi.agc" > > > > > > > > > > -static constexpr unsigned int PipelineBits = 13; /* seems to be a 13-bit pipeline */ > > > > > - > > > > > int AgcMeteringMode::read(const libcamera::YamlObject ¶ms) > > > > > { > > > > > const YamlObject &yamlWeights = params["weights"]; > > > > > @@ -593,9 +591,9 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb, > > > > > double rSum = 0, gSum = 0, bSum = 0, pixelSum = 0; > > > > > for (unsigned int i = 0; i < stats->agcRegions.numRegions(); i++) { > > > > > auto ®ion = stats->agcRegions.get(i); > > > > > - double rAcc = std::min<double>(region.val.rSum * gain, ((1 << PipelineBits) - 1) * region.counted); > > > > > - double gAcc = std::min<double>(region.val.gSum * gain, ((1 << PipelineBits) - 1) * region.counted); > > > > > - double bAcc = std::min<double>(region.val.bSum * gain, ((1 << PipelineBits) - 1) * region.counted); > > > > > + double rAcc = std::min<double>(region.val.rSum * gain, ((1 << 16) - 1) * region.counted); > > > > > + double gAcc = std::min<double>(region.val.gSum * gain, ((1 << 16) - 1) * region.counted); > > > > > + double bAcc = std::min<double>(region.val.bSum * gain, ((1 << 16) - 1) * region.counted); > > > > > > > > 1 << 16 - 1 now looks a bit 'magic', and it may not be clear to a > > > > reader what it's refering to? > > > > > > > > Before it referenced PipelineBits so it was clearer. I expect this > > > > ((1 << 16) - 1) > > > > > > > > could be a constexpr somewhere at the top of the file, or a helper to > > > > wrap the scaling of region.counted? > > > > > > > > > > > > Hrm ... it 'was' already a constexpr at the top... > > > > Is there a reason you chose to hardcode the value here instead of update > > > > (/and or rename) the existing PipelineBits ? > > > > > > Since the generalised statistics are now normalised to 16-bits, > > > perhaps what is needed is to have a constexpr for (1 << 16) - 1 to be > > > (possibly) defined in the statistics structure to indicate the > > > normalisation used? > > > > Perhaps, but we're in src/ipa/raspberrypi/ so it's all up to your > > preference. > > > > I'll merge this series, and if you think it deserves a cleanup on top, > > that should be easy to treat independently. > > > > aha - we crossed paths. I'll make sure your new patch still works, and > use that instead of this one. ;-) Perfect, thanks! :-) > > -- > Kieran > > > > -- > > Kieran > > > > > > > > > > Naush > > > > > > > > > > > Aside from that: > > > > > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > > > > > rSum += rAcc * weights[i]; > > > > > gSum += gAcc * weights[i]; > > > > > bSum += bAcc * weights[i]; > > > > > @@ -608,7 +606,7 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb, > > > > > double ySum = rSum * awb.gainR * .299 + > > > > > gSum * awb.gainG * .587 + > > > > > bSum * awb.gainB * .114; > > > > > - return ySum / pixelSum / (1 << PipelineBits); > > > > > + return ySum / pixelSum / (1 << 16); > > > > > } > > > > > > > > > > /* > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > > > > > index cff079bbafb3..50fdeb4f0478 100644 > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > > > > @@ -1154,11 +1154,15 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co > > > > > /* RGB histograms are not used, so do not populate them. */ > > > > > statistics->yHist = std::move(RPiController::Histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS)); > > > > > > > > > > + /* > > > > > + * All region sums are based on a 13-bit pipeline bit-depth. Normalise > > > > > + * this to 16-bits for the AGC/AWB/ALSC algorithms. > > > > > + */ > > > > > statistics->awbRegions.init({ DEFAULT_AWB_REGIONS_X, DEFAULT_AWB_REGIONS_Y }); > > > > > for (i = 0; i < statistics->awbRegions.numRegions(); i++) > > > > > - statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum, > > > > > - stats->awb_stats[i].g_sum, > > > > > - stats->awb_stats[i].b_sum }, > > > > > + statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum << 3, > > > > > + stats->awb_stats[i].g_sum << 3, > > > > > + stats->awb_stats[i].b_sum << 3 }, > > > > > stats->awb_stats[i].counted, > > > > > stats->awb_stats[i].notcounted }); > > > > > > > > > > @@ -1168,9 +1172,9 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co > > > > > */ > > > > > statistics->agcRegions.init(15); > > > > > for (i = 0; i < statistics->agcRegions.numRegions(); i++) > > > > > - statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum, > > > > > - stats->agc_stats[i].g_sum, > > > > > - stats->agc_stats[i].b_sum }, > > > > > + statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum << 3, > > > > > + stats->agc_stats[i].g_sum << 3, > > > > > + stats->agc_stats[i].b_sum << 3 }, > > > > > stats->agc_stats[i].counted, > > > > > stats->awb_stats[i].notcounted }); > > > > > > > > > > -- > > > > > 2.25.1 > > > > >
diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp index 868c30f03d66..7ec8292d32ff 100644 --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp @@ -28,8 +28,6 @@ LOG_DEFINE_CATEGORY(RPiAgc) #define NAME "rpi.agc" -static constexpr unsigned int PipelineBits = 13; /* seems to be a 13-bit pipeline */ - int AgcMeteringMode::read(const libcamera::YamlObject ¶ms) { const YamlObject &yamlWeights = params["weights"]; @@ -593,9 +591,9 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb, double rSum = 0, gSum = 0, bSum = 0, pixelSum = 0; for (unsigned int i = 0; i < stats->agcRegions.numRegions(); i++) { auto ®ion = stats->agcRegions.get(i); - double rAcc = std::min<double>(region.val.rSum * gain, ((1 << PipelineBits) - 1) * region.counted); - double gAcc = std::min<double>(region.val.gSum * gain, ((1 << PipelineBits) - 1) * region.counted); - double bAcc = std::min<double>(region.val.bSum * gain, ((1 << PipelineBits) - 1) * region.counted); + double rAcc = std::min<double>(region.val.rSum * gain, ((1 << 16) - 1) * region.counted); + double gAcc = std::min<double>(region.val.gSum * gain, ((1 << 16) - 1) * region.counted); + double bAcc = std::min<double>(region.val.bSum * gain, ((1 << 16) - 1) * region.counted); rSum += rAcc * weights[i]; gSum += gAcc * weights[i]; bSum += bAcc * weights[i]; @@ -608,7 +606,7 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb, double ySum = rSum * awb.gainR * .299 + gSum * awb.gainG * .587 + bSum * awb.gainB * .114; - return ySum / pixelSum / (1 << PipelineBits); + return ySum / pixelSum / (1 << 16); } /* diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index cff079bbafb3..50fdeb4f0478 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -1154,11 +1154,15 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co /* RGB histograms are not used, so do not populate them. */ statistics->yHist = std::move(RPiController::Histogram(stats->hist[0].g_hist, NUM_HISTOGRAM_BINS)); + /* + * All region sums are based on a 13-bit pipeline bit-depth. Normalise + * this to 16-bits for the AGC/AWB/ALSC algorithms. + */ statistics->awbRegions.init({ DEFAULT_AWB_REGIONS_X, DEFAULT_AWB_REGIONS_Y }); for (i = 0; i < statistics->awbRegions.numRegions(); i++) - statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum, - stats->awb_stats[i].g_sum, - stats->awb_stats[i].b_sum }, + statistics->awbRegions.set(i, { { stats->awb_stats[i].r_sum << 3, + stats->awb_stats[i].g_sum << 3, + stats->awb_stats[i].b_sum << 3 }, stats->awb_stats[i].counted, stats->awb_stats[i].notcounted }); @@ -1168,9 +1172,9 @@ RPiController::StatisticsPtr IPARPi::fillStatistics(bcm2835_isp_stats *stats) co */ statistics->agcRegions.init(15); for (i = 0; i < statistics->agcRegions.numRegions(); i++) - statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum, - stats->agc_stats[i].g_sum, - stats->agc_stats[i].b_sum }, + statistics->agcRegions.set(i, { { stats->agc_stats[i].r_sum << 3, + stats->agc_stats[i].g_sum << 3, + stats->agc_stats[i].b_sum << 3 }, stats->agc_stats[i].counted, stats->awb_stats[i].notcounted });