| Message ID | 20211116080956.21373-1-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Jean-Michel Hautbois (2021-11-16 08:09:56) > Until f8f07f94 (ipa: ipu3: agc: Improve gain calculation, 2021-11-04) > the gain to apply on the exposure value was only using the histogram. > Now that the global brightness of the frame is estimated too, we don't > need to remove part of the saturated pixels from the equation anymore. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> This is still working quite well here. No perceived regression as far as I can tell, and it's still coping well with backlights and skylights. Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/agc.cpp | 24 ++++++++---------------- > 1 file changed, 8 insertions(+), 16 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > index 4e424857..f67a79d3 100644 > --- a/src/ipa/ipu3/algorithms/agc.cpp > +++ b/src/ipa/ipu3/algorithms/agc.cpp > @@ -58,12 +58,6 @@ static constexpr uint32_t knumHistogramBins = 256; > /* Target value to reach for the top 2% of the histogram */ > static constexpr double kEvGainTarget = 0.5; > > -/* > - * Maximum ratio of saturated pixels in a cell for the cell to be considered > - * non-saturated and counted by the AGC algorithm. > - */ > -static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100; > - > /* Number of frames to wait before calculating stats on minimum exposure */ > static constexpr uint32_t kNumStartupFrames = 10; > > @@ -133,16 +127,14 @@ void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats, > &stats->awb_raw_buffer.meta_data[cellPosition] > ); > > - if (cell->sat_ratio <= kMinCellsPerZoneRatio) { > - uint8_t gr = cell->Gr_avg; > - uint8_t gb = cell->Gb_avg; > - /* > - * Store the average green value to estimate the > - * brightness. Even the overexposed pixels are > - * taken into account. > - */ > - hist[(gr + gb) / 2]++; > - } > + uint8_t gr = cell->Gr_avg; > + uint8_t gb = cell->Gb_avg; > + /* > + * Store the average green value to estimate the > + * brightness. Even the overexposed pixels are > + * taken into account. > + */ > + hist[(gr + gb) / 2]++; > } > } > > -- > 2.32.0 >
Hello, On Tue, Nov 16, 2021 at 02:41:14PM +0000, Kieran Bingham wrote: > Quoting Jean-Michel Hautbois (2021-11-16 08:09:56) > > Until f8f07f94 (ipa: ipu3: agc: Improve gain calculation, 2021-11-04) s/Until/Until commit/ I'd drop the date, and expand the commit ID to 12 characters as usual. > > the gain to apply on the exposure value was only using the histogram. > > Now that the global brightness of the frame is estimated too, we don't > > need to remove part of the saturated pixels from the equation anymore. I'm not sure to understand why the above commit changed the situation, but I also don't understand why we needed to take saturation into account here in the first place, so the change looks good to me overall. Commit f8f07f94 also added a histogram empty check. Do we still need it ? > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > This is still working quite well here. No perceived regression as far > as I can tell, and it's still coping well with backlights and skylights. > > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > src/ipa/ipu3/algorithms/agc.cpp | 24 ++++++++---------------- > > 1 file changed, 8 insertions(+), 16 deletions(-) > > > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > > index 4e424857..f67a79d3 100644 > > --- a/src/ipa/ipu3/algorithms/agc.cpp > > +++ b/src/ipa/ipu3/algorithms/agc.cpp > > @@ -58,12 +58,6 @@ static constexpr uint32_t knumHistogramBins = 256; > > /* Target value to reach for the top 2% of the histogram */ > > static constexpr double kEvGainTarget = 0.5; > > > > -/* > > - * Maximum ratio of saturated pixels in a cell for the cell to be considered > > - * non-saturated and counted by the AGC algorithm. > > - */ > > -static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100; > > - > > /* Number of frames to wait before calculating stats on minimum exposure */ > > static constexpr uint32_t kNumStartupFrames = 10; > > > > @@ -133,16 +127,14 @@ void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats, > > &stats->awb_raw_buffer.meta_data[cellPosition] > > ); > > > > - if (cell->sat_ratio <= kMinCellsPerZoneRatio) { > > - uint8_t gr = cell->Gr_avg; > > - uint8_t gb = cell->Gb_avg; > > - /* > > - * Store the average green value to estimate the > > - * brightness. Even the overexposed pixels are > > - * taken into account. > > - */ > > - hist[(gr + gb) / 2]++; > > - } > > + uint8_t gr = cell->Gr_avg; > > + uint8_t gb = cell->Gb_avg; > > + /* > > + * Store the average green value to estimate the > > + * brightness. Even the overexposed pixels are > > + * taken into account. > > + */ > > + hist[(gr + gb) / 2]++; > > } > > } > >
Hi Laurent, On 16/11/2021 15:59, Laurent Pinchart wrote: > Hello, > > On Tue, Nov 16, 2021 at 02:41:14PM +0000, Kieran Bingham wrote: >> Quoting Jean-Michel Hautbois (2021-11-16 08:09:56) >>> Until f8f07f94 (ipa: ipu3: agc: Improve gain calculation, 2021-11-04) > > s/Until/Until commit/ > > I'd drop the date, and expand the commit ID to 12 characters as usual. OK, this is what git log --pretty=reference returned, I will use --abbrev instead. > >>> the gain to apply on the exposure value was only using the histogram. >>> Now that the global brightness of the frame is estimated too, we don't >>> need to remove part of the saturated pixels from the equation anymore. > > I'm not sure to understand why the above commit changed the situation, > but I also don't understand why we needed to take saturation into > account here in the first place, so the change looks good to me overall. > > Commit f8f07f94 also added a histogram empty check. Do we still need it > ? I don't think we need it anymore... as I can't see why the histogram could be empty now. I may remove it too in v2 then. > >>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >> >> This is still working quite well here. No perceived regression as far >> as I can tell, and it's still coping well with backlights and skylights. >> >> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>> --- >>> src/ipa/ipu3/algorithms/agc.cpp | 24 ++++++++---------------- >>> 1 file changed, 8 insertions(+), 16 deletions(-) >>> >>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp >>> index 4e424857..f67a79d3 100644 >>> --- a/src/ipa/ipu3/algorithms/agc.cpp >>> +++ b/src/ipa/ipu3/algorithms/agc.cpp >>> @@ -58,12 +58,6 @@ static constexpr uint32_t knumHistogramBins = 256; >>> /* Target value to reach for the top 2% of the histogram */ >>> static constexpr double kEvGainTarget = 0.5; >>> >>> -/* >>> - * Maximum ratio of saturated pixels in a cell for the cell to be considered >>> - * non-saturated and counted by the AGC algorithm. >>> - */ >>> -static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100; >>> - >>> /* Number of frames to wait before calculating stats on minimum exposure */ >>> static constexpr uint32_t kNumStartupFrames = 10; >>> >>> @@ -133,16 +127,14 @@ void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats, >>> &stats->awb_raw_buffer.meta_data[cellPosition] >>> ); >>> >>> - if (cell->sat_ratio <= kMinCellsPerZoneRatio) { >>> - uint8_t gr = cell->Gr_avg; >>> - uint8_t gb = cell->Gb_avg; >>> - /* >>> - * Store the average green value to estimate the >>> - * brightness. Even the overexposed pixels are >>> - * taken into account. >>> - */ >>> - hist[(gr + gb) / 2]++; >>> - } >>> + uint8_t gr = cell->Gr_avg; >>> + uint8_t gb = cell->Gb_avg; >>> + /* >>> + * Store the average green value to estimate the >>> + * brightness. Even the overexposed pixels are >>> + * taken into account. >>> + */ >>> + hist[(gr + gb) / 2]++; >>> } >>> } >>> >
diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp index 4e424857..f67a79d3 100644 --- a/src/ipa/ipu3/algorithms/agc.cpp +++ b/src/ipa/ipu3/algorithms/agc.cpp @@ -58,12 +58,6 @@ static constexpr uint32_t knumHistogramBins = 256; /* Target value to reach for the top 2% of the histogram */ static constexpr double kEvGainTarget = 0.5; -/* - * Maximum ratio of saturated pixels in a cell for the cell to be considered - * non-saturated and counted by the AGC algorithm. - */ -static constexpr uint32_t kMinCellsPerZoneRatio = 255 * 20 / 100; - /* Number of frames to wait before calculating stats on minimum exposure */ static constexpr uint32_t kNumStartupFrames = 10; @@ -133,16 +127,14 @@ void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats, &stats->awb_raw_buffer.meta_data[cellPosition] ); - if (cell->sat_ratio <= kMinCellsPerZoneRatio) { - uint8_t gr = cell->Gr_avg; - uint8_t gb = cell->Gb_avg; - /* - * Store the average green value to estimate the - * brightness. Even the overexposed pixels are - * taken into account. - */ - hist[(gr + gb) / 2]++; - } + uint8_t gr = cell->Gr_avg; + uint8_t gb = cell->Gb_avg; + /* + * Store the average green value to estimate the + * brightness. Even the overexposed pixels are + * taken into account. + */ + hist[(gr + gb) / 2]++; } }
Until f8f07f94 (ipa: ipu3: agc: Improve gain calculation, 2021-11-04) the gain to apply on the exposure value was only using the histogram. Now that the global brightness of the frame is estimated too, we don't need to remove part of the saturated pixels from the equation anymore. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/ipa/ipu3/algorithms/agc.cpp | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-)