[v2,13/16] ipa: rkisp1: Switch histogram to RGB combined mode

24084 diff mbox series

The Y mode of the histogram gets captured at the ISP output, before the
output formatter.  This has the side effect that the first and the last
bins are empty in case of limited YUV range.  Another side effect is
that gamma and GWDR processing is included in the histogram which makes
algorithm development very difficult. In RGB mode the histogram is taken
after xtalk (CCM) and is therefore independent of gamma and WDR. The
limited range issue also does not apply. In the ISP reference it is
however stated that "it is not possible to calculate a luminance or
grayscale histogram from an RGB histogram since the position information
is lost during its generation".

During testing the RGB histogram provided good data and better
algorithmic stability at a possible (but not measured) inaccuracy.

Another option would be to pass the color space information into the IPA
and strip the histogram accordingly. For ease of implementation switch to
the RGB mode.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/agc.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoting Stefan Klug (2025-08-08 15:12:51)
> The Y mode of the histogram gets captured at the ISP output, before the
> output formatter.  This has the side effect that the first and the last
> bins are empty in case of limited YUV range.  Another side effect is
> that gamma and GWDR processing is included in the histogram which makes

This patch doesn't modify  anything in regards to gamma adaptations.
Does that mean we currently mis-handle gamma in the statistics? or that
this patch 'fixes' the handling?

> algorithm development very difficult. In RGB mode the histogram is taken
> after xtalk (CCM) and is therefore independent of gamma and WDR. The
> limited range issue also does not apply. In the ISP reference it is

So it sounds like this is a good fix all round ...

> however stated that "it is not possible to calculate a luminance or
> grayscale histogram from an RGB histogram since the position information
> is lost during its generation".
> 
> During testing the RGB histogram provided good data and better
> algorithmic stability at a possible (but not measured) inaccuracy.
> 
> Another option would be to pass the color space information into the IPA
> and strip the histogram accordingly. For ease of implementation switch to
> the RGB mode.

I feel like some of this needs to be captured in a comment in the code
for future reference. Perhaps even just taking a summary of the above
issues in a block comment above the mode setting?

> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index d34c041f9fe1..8227d60213c2 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -396,7 +396,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,
>         hstConfig.setEnabled(true);
>  
>         hstConfig->meas_window = context.configuration.agc.measureWindow;
> -       hstConfig->mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_Y_HISTOGRAM;
> +       hstConfig->mode = RKISP1_CIF_ISP_HISTOGRAM_MODE_RGB_COMBINED;
>  
>         Span<uint8_t> weights{
>                 hstConfig->hist_weight,
> -- 
> 2.48.1
>