| Message ID | 20240418124632.652163-4-dan.scally@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Daniel, On 18/04/2024 14:46, Daniel Scally wrote: > The IPU3 IPA module uses a green histogram for the AGC algorithm's > constraint mode calculations. Switch instead to a luminance histogram > calculated using the Rec.601 coefficients. > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/agc.cpp | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > index a59b73fb..76d2bb60 100644 > --- a/src/ipa/ipu3/algorithms/agc.cpp > +++ b/src/ipa/ipu3/algorithms/agc.cpp > @@ -152,18 +152,19 @@ Histogram Agc::parseStatistics(const ipu3_uapi_stats_3a *stats, > reinterpret_cast<const ipu3_uapi_awb_set_item *>( > &stats->awb_raw_buffer.meta_data[cellPosition]); > > - rgbTriples_.push_back({ > - cell->R_avg, > - (cell->Gr_avg + cell->Gb_avg) / 2, > - cell->B_avg > - }); > + uint8_t G_avg = (cell->Gr_avg + cell->Gb_avg) / 2; > + > + rgbTriples_.push_back({cell->R_avg, G_avg, cell->B_avg}); > > /* > - * Store the average green value to estimate the > + * Store the average luminance value to estimate the > * brightness. Even the overexposed pixels are > * taken into account. > */ > - hist[(cell->Gr_avg + cell->Gb_avg) / 2]++; > + uint8_t y = (cell->R_avg * .299) + > + (G_avg * .587) + > + (cell->B_avg * .114); > + hist[y]++; Is there a benefit to have the "real" Y value and not only the green parts (which reflect this Y value quite nicely as it is ~60% of the value) ? Could you measure it ? No increase in CPU time for big stats grids ? JM
Hi Jean-Michel - sorry for the delay replying to you On 18/04/2024 13:54, Jean-Michel Hautbois wrote: > Hi Daniel, > > On 18/04/2024 14:46, Daniel Scally wrote: >> The IPU3 IPA module uses a green histogram for the AGC algorithm's >> constraint mode calculations. Switch instead to a luminance histogram >> calculated using the Rec.601 coefficients. >> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >> --- >> src/ipa/ipu3/algorithms/agc.cpp | 15 ++++++++------- >> 1 file changed, 8 insertions(+), 7 deletions(-) >> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp >> index a59b73fb..76d2bb60 100644 >> --- a/src/ipa/ipu3/algorithms/agc.cpp >> +++ b/src/ipa/ipu3/algorithms/agc.cpp >> @@ -152,18 +152,19 @@ Histogram Agc::parseStatistics(const ipu3_uapi_stats_3a *stats, >> reinterpret_cast<const ipu3_uapi_awb_set_item *>( >> &stats->awb_raw_buffer.meta_data[cellPosition]); >> - rgbTriples_.push_back({ >> - cell->R_avg, >> - (cell->Gr_avg + cell->Gb_avg) / 2, >> - cell->B_avg >> - }); >> + uint8_t G_avg = (cell->Gr_avg + cell->Gb_avg) / 2; >> + >> + rgbTriples_.push_back({cell->R_avg, G_avg, cell->B_avg}); >> /* >> - * Store the average green value to estimate the >> + * Store the average luminance value to estimate the >> * brightness. Even the overexposed pixels are >> * taken into account. >> */ >> - hist[(cell->Gr_avg + cell->Gb_avg) / 2]++; >> + uint8_t y = (cell->R_avg * .299) + >> + (G_avg * .587) + >> + (cell->B_avg * .114); >> + hist[y]++; > > Is there a benefit to have the "real" Y value and not only the green parts (which reflect this Y > value quite nicely as it is ~60% of the value) ? Only that it more accurately matches the Y-value that's used in the rest of the algorithm; estimateLuminance() uses the same Rec.601 coefficients and the gain that's derived from that calculation is compared against one derived from this Histogram. > Could you measure it ? No increase in CPU time for big stats grids ? I didn't check specifically but I can do - I didn't lose any framerate on my Surface Go2...but I imagine that the Imgu can run much faster than that is doing currently. Cheers Dan > > JM
Quoting Dan Scally (2024-05-04 20:58:57) > Hi Jean-Michel - sorry for the delay replying to you > > On 18/04/2024 13:54, Jean-Michel Hautbois wrote: > > Hi Daniel, > > > > On 18/04/2024 14:46, Daniel Scally wrote: > >> The IPU3 IPA module uses a green histogram for the AGC algorithm's > >> constraint mode calculations. Switch instead to a luminance histogram > >> calculated using the Rec.601 coefficients. > >> > >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > >> --- > >> src/ipa/ipu3/algorithms/agc.cpp | 15 ++++++++------- > >> 1 file changed, 8 insertions(+), 7 deletions(-) > >> > >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > >> index a59b73fb..76d2bb60 100644 > >> --- a/src/ipa/ipu3/algorithms/agc.cpp > >> +++ b/src/ipa/ipu3/algorithms/agc.cpp > >> @@ -152,18 +152,19 @@ Histogram Agc::parseStatistics(const ipu3_uapi_stats_3a *stats, > >> reinterpret_cast<const ipu3_uapi_awb_set_item *>( > >> &stats->awb_raw_buffer.meta_data[cellPosition]); > >> - rgbTriples_.push_back({ > >> - cell->R_avg, > >> - (cell->Gr_avg + cell->Gb_avg) / 2, > >> - cell->B_avg > >> - }); > >> + uint8_t G_avg = (cell->Gr_avg + cell->Gb_avg) / 2; > >> + > >> + rgbTriples_.push_back({cell->R_avg, G_avg, cell->B_avg}); > >> /* > >> - * Store the average green value to estimate the > >> + * Store the average luminance value to estimate the > >> * brightness. Even the overexposed pixels are > >> * taken into account. > >> */ > >> - hist[(cell->Gr_avg + cell->Gb_avg) / 2]++; > >> + uint8_t y = (cell->R_avg * .299) + > >> + (G_avg * .587) + > >> + (cell->B_avg * .114); Somewhere in libipa we should really have helpers or classes for the colour space coefficients to make this more expressive. > >> + hist[y]++; > > > > Is there a benefit to have the "real" Y value and not only the green > > parts (which reflect this Y value quite nicely as it is ~60% of the > > value) ? > > > Only that it more accurately matches the Y-value that's used in the > rest of the algorithm; estimateLuminance() uses the same Rec.601 > coefficients and the gain that's derived from that calculation is > compared against one derived from this Histogram. Presumably I would get different results if I were in a Red or Green t-shirt ... so I certainly think it's more appropriate to use the luminance correctly. > > Could you measure it ? No increase in CPU time for big stats grids ? > > > I didn't check specifically but I can do - I didn't lose any framerate > on my Surface Go2...but I imagine that the Imgu can run much faster > than that is doing currently. I don't think I'm worried about doing the correct thing here. It's once per frame, on the stats - not per pixel in the image. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Cheers > > Dan > > > > > > > JM
On Wed, May 08, 2024 at 01:52:32PM +0100, Kieran Bingham wrote: > Quoting Dan Scally (2024-05-04 20:58:57) > > On 18/04/2024 13:54, Jean-Michel Hautbois wrote: > > > On 18/04/2024 14:46, Daniel Scally wrote: > > >> The IPU3 IPA module uses a green histogram for the AGC algorithm's > > >> constraint mode calculations. Switch instead to a luminance histogram > > >> calculated using the Rec.601 coefficients. > > >> > > >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > > >> --- > > >> src/ipa/ipu3/algorithms/agc.cpp | 15 ++++++++------- > > >> 1 file changed, 8 insertions(+), 7 deletions(-) > > >> > > >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > > >> index a59b73fb..76d2bb60 100644 > > >> --- a/src/ipa/ipu3/algorithms/agc.cpp > > >> +++ b/src/ipa/ipu3/algorithms/agc.cpp > > >> @@ -152,18 +152,19 @@ Histogram Agc::parseStatistics(const ipu3_uapi_stats_3a *stats, > > >> reinterpret_cast<const ipu3_uapi_awb_set_item *>( > > >> &stats->awb_raw_buffer.meta_data[cellPosition]); > > >> - rgbTriples_.push_back({ > > >> - cell->R_avg, > > >> - (cell->Gr_avg + cell->Gb_avg) / 2, > > >> - cell->B_avg > > >> - }); > > >> + uint8_t G_avg = (cell->Gr_avg + cell->Gb_avg) / 2; > > >> + > > >> + rgbTriples_.push_back({cell->R_avg, G_avg, cell->B_avg}); > > >> /* > > >> - * Store the average green value to estimate the > > >> + * Store the average luminance value to estimate the > > >> * brightness. Even the overexposed pixels are > > >> * taken into account. > > >> */ > > >> - hist[(cell->Gr_avg + cell->Gb_avg) / 2]++; > > >> + uint8_t y = (cell->R_avg * .299) + > > >> + (G_avg * .587) + > > >> + (cell->B_avg * .114); > > Somewhere in libipa we should really have helpers or classes for the > colour space coefficients to make this more expressive. > > > >> + hist[y]++; > > > > > > Is there a benefit to have the "real" Y value and not only the green > > > parts (which reflect this Y value quite nicely as it is ~60% of the > > > value) ? > > > > Only that it more accurately matches the Y-value that's used in the > > rest of the algorithm; estimateLuminance() uses the same Rec.601 > > coefficients and the gain that's derived from that calculation is > > compared against one derived from this Histogram. > > Presumably I would get different results if I were in a Red or Green > t-shirt ... so I certainly think it's more appropriate to use the > luminance correctly. Except it would be too easy if it could be simply done this way :-) It all depends on how you define luminance. Not only do we have different definitions in different colour spaces, but the above calculation would need to be done after applying the CCM in order to account for differences in spectral response between sensors. This being said, I'm not opposed to taking the three colour components into account, but I also wonder as Jean-Michel if it really improves the AGC operation compared to using the green channel only. Dan, have you observed as qualitative or quantitative improvements with this patch ? > > > Could you measure it ? No increase in CPU time for big stats grids ? > > > > I didn't check specifically but I can do - I didn't lose any framerate > > on my Surface Go2...but I imagine that the Imgu can run much faster > > than that is doing currently. > > I don't think I'm worried about doing the correct thing here. It's once > per frame, on the stats - not per pixel in the image. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp index a59b73fb..76d2bb60 100644 --- a/src/ipa/ipu3/algorithms/agc.cpp +++ b/src/ipa/ipu3/algorithms/agc.cpp @@ -152,18 +152,19 @@ Histogram Agc::parseStatistics(const ipu3_uapi_stats_3a *stats, reinterpret_cast<const ipu3_uapi_awb_set_item *>( &stats->awb_raw_buffer.meta_data[cellPosition]); - rgbTriples_.push_back({ - cell->R_avg, - (cell->Gr_avg + cell->Gb_avg) / 2, - cell->B_avg - }); + uint8_t G_avg = (cell->Gr_avg + cell->Gb_avg) / 2; + + rgbTriples_.push_back({cell->R_avg, G_avg, cell->B_avg}); /* - * Store the average green value to estimate the + * Store the average luminance value to estimate the * brightness. Even the overexposed pixels are * taken into account. */ - hist[(cell->Gr_avg + cell->Gb_avg) / 2]++; + uint8_t y = (cell->R_avg * .299) + + (G_avg * .587) + + (cell->B_avg * .114); + hist[y]++; } }
The IPU3 IPA module uses a green histogram for the AGC algorithm's constraint mode calculations. Switch instead to a luminance histogram calculated using the Rec.601 coefficients. Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> --- src/ipa/ipu3/algorithms/agc.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)