[PATCH 3/3] ipa: ipu3: Use a Y histogram instead of green

[Kieran Bingham]

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 at 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 at ideasonboard.com>


> 
> 
> Cheers
> 
> Dan
> 
> 
> 
> >
> > JM