[3/3] ipa: ipu3: Use a Y histogram instead of green
diff mbox series

Message ID 20240418124632.652163-4-dan.scally@ideasonboard.com
State New
Headers show
Series
  • Minor AGC fixes
Related show

Commit Message

Daniel Scally April 18, 2024, 12:46 p.m. UTC
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(-)

Comments

Jean-Michel Hautbois April 18, 2024, 12:54 p.m. UTC | #1
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
Daniel Scally May 4, 2024, 7:58 p.m. UTC | #2
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
Kieran Bingham May 8, 2024, 12:52 p.m. UTC | #3
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
Laurent Pinchart May 8, 2024, 2:07 p.m. UTC | #4
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>

Patch
diff mbox series

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]++;
 		}
 	}