[libcamera-devel] [PATCH 5/5] ipa: ipu3: agc: Saturate the averages when computing relative luminance

[Laurent Pinchart]

On Fri, Nov 19, 2021 at 02:35:44PM +0000, Kieran Bingham wrote:
> Quoting Kieran Bingham (2021-11-18 09:53:22)
> > Quoting Jean-Michel Hautbois (2021-11-17 10:26:59)
> > > Hi Laurent,
> > > 
> > > Thanks for the patch !
> > > 
> > > On 16/11/2021 17:26, Laurent Pinchart wrote:
> > > > The relative luminance is calculated using an iterative process to
> > > > account for saturation in the sensor, as multiplying pixels by a gain
> > > > doesn't increase the relative luminance by the same factor if some
> > > > regions are saturated. Relative luminance estimation doesn't apply a
> > > > saturation, which produces a value that doesn't match what the sensor
> > > > will output, and defeats the point of the iterative process. Fix it.
> > > > 
> > > > Fixes: f8f07f9468c6 ("ipa: ipu3: agc: Improve gain calculation")
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > ---
> > > >   src/ipa/ipu3/algorithms/agc.cpp | 25 ++++++++++++++++++-------
> > > >   1 file changed, 18 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > > > index 71398fdd96a6..46aa1b14a100 100644
> > > > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > > > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > > > @@ -252,10 +252,19 @@ void Agc::computeExposure(IPAFrameContext &frameContext, double yGain,
> > > >    * \param[in] gain The analogue gain to apply to the frame
> > > >    * \return The relative luminance
> > > >    *
> > > > - * Luma is the weighted sum of gamma-compressed R′G′B′ components of a color
> > > > - * video. The luma values are normalized as 0.0 to 1.0, with 1.0 being a
> > > > - * theoretical perfect reflector of 100% reference white. We use the Rec. 601
> > > > - * luma here.
> > > > + * This function estimates the average relative luminance of the frame that
> > > > + * would be output by the sensor if an additional analogue \a gain was applied.
> > > 
> > > s/additional analogue \a gain/additional \a gain/
> > > 
> > > > + *
> > > > + * The estimation is based on the AWB statistics for the current frame. Red,
> > > > + * green and blue averages for all cells are first multiplied by the gain, and
> > > > + * then saturated to approximate the sensor behaviour at high brightness
> > > > + * values. The approximation is quitte rough, as it doesn't take into account
> > 
> > /quitte/quite/
> > 
> > > > + * non-linearities when approaching saturation.
> > > > + *
> > > > + * The relative luminance (Y) is computed from the linear RGB components using
> > > > + * the Rec. 601 formula. The values is normalized to the [0.0, 1.0] range,
> > 
> > /values is/values are/
> > 
> > > > + * where 1.0 corresponds to a theoretical perfect reflector of 100% reference
> > > > + * white.
> > > >    *
> > > >    * More detailed information can be found in:
> > > >    * https://en.wikipedia.org/wiki/Relative_luminance
> > > > @@ -267,6 +276,7 @@ double Agc::estimateLuminance(IPAFrameContext &frameContext,
> > > >   {
> > > >       double redSum = 0, greenSum = 0, blueSum = 0;
> > > >   
> > > > +     /* Sum the per-channel averages, saturated to 255. */
> > > >       for (unsigned int cellY = 0; cellY < grid.height; cellY++) {
> > > >               for (unsigned int cellX = 0; cellX < grid.width; cellX++) {
> > > >                       uint32_t cellPosition = cellY * stride_ + cellX;
> > > > @@ -275,10 +285,11 @@ double Agc::estimateLuminance(IPAFrameContext &frameContext,
> > > >                               reinterpret_cast<const ipu3_uapi_awb_set_item *>(
> > > >                                       &stats->awb_raw_buffer.meta_data[cellPosition]
> > > >                               );
> > > > +                     const uint8_t G_avg = (cell->Gr_avg + cell->Gb_avg) / 2;
> > > >   
> > > > -                     redSum += cell->R_avg * gain;
> > > > -                     greenSum += (cell->Gr_avg + cell->Gb_avg) / 2 * gain;
> > > > -                     blueSum += cell->B_avg * gain;
> > > > +                     redSum += std::min(cell->R_avg * gain, 255.0);
> > > > +                     greenSum += std::min(G_avg * gain, 255.0);
> > > > +                     blueSum += std::min(cell->B_avg * gain, 255.0);
> 
> Seeing this constant in the new RKISP series again, wouldn't it make
> sense to keep the kMaximumLuminance constant that you remove at the
> beginning of the series and use it here so that it's self-documented
> that the min operation is clamping to the maximum luminance value?
> 
> Or is 255 really not a luminance value here?

255 here is UINT8_MAX, as the red, green and blue averages are stored as
8-bit values. The maximum values for the sums are then UINT8_MAX *
number of cells, and the maximum value for the luminance is the same
given that the sum of the three coefficients is 1.0.

> > > >               }
> > > >       }
> > > >   
> > > > 
> > 
> > Still works well in my harsh backlight office conditions too.
> > 
> > Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > 
> > > Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> > > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>

-- 
Regards,

Laurent Pinchart