[PATCH 00/10] Centralise Agc into libipa

[Dan Scally]

On 22/03/2024 13:14, Daniel Scally wrote:
> Hello all
>
> The IPU3 and RkISP1 IPAs do Agc the same way (following the Rpi method closely),
> but the implementations are wholly separate. This introduces the potential for
> divergence and also makes both maintenance and improvement more difficult. This
> series unifies them as derivations of a new MeanLuminanceAgc class within
> libipa. The algorithm itself is done through functions of the base class
> with the IPA's derived classes providing a function through which the mean
> luminance input to the algorithm can be calculated from their specific stats
> formats.
>
> The old implementations effectively hard coded values for the AeConstraintMode
> and AeExposureMode controls; they adhered to a single lower-bound constraint of
> 0.5 across the top 2% of a Histogram and acted as though the AeExposureMode
> expected shutter time to be driven to its maximum before touching gain at all.
> The base class additionally adds infrastructure to discover the supported values
> for AeConstraintMode and AeExposureMode controls from a camera sensor tuning
> file and uses the values defined in those files with the algorithm instead,
> though as no tuning files are modified in this series the behaviour currently
> remains as it was before.
>
> The series **does** alter the calculated shutter time and gain for both the
> IPU3 and RkISP1 compared to their bespoke implementations, for two reasons:
>
> 1. A bug in the way they were implemented meant that an over-exposed image was
>     corrected more slowly than an under-exposed one. This is fixed and will
>     improve both IPAs' response to a too-bright image.
> 2. The default kRelativeLuminanceTarget is centrally set to 0.16 which matches
>     the value from the IPU3 implementation. In the RkISP1 implementation that
>     value was set to 0.4 with a \todo to see why they were different. My belief
>     is that they were different because they were implemented in different
>     lighting conditions. In my very imperfect testing setup the 0.16 target
>     produced reasonable images on both.


Actually the more I think about this, the more I think we'll need a value for each IPA anyway 
because it's a figure that depends on how they calculate things. RkISP1 takes an average of an array 
of values which are the themselves the average Luminance in a zone, where Y=(R+G+B) * 0.332. IPU3 
takes sums the average R/G/B value in each zone of red/blue/green pixels and adjusts them by the 
BT.601 values. Rpi I **think** gets R/G/B sums across an image and adjusts them by BT.601. All of 
those methods seem bound to produce different estimations of Y even with completely identical 
scenes, so I'm leaning towards the view that we probably need to define the default 
kRelativeLuminanceTarget in each IPA's implementation of Agc separately.

>
> Without those two changes, the shutter time and gain calculated after this
> series matches those calculated by their independent implementations.
>
> Thanks
> Dan
>
> Daniel Scally (9):
>    ipa: libipa: Allow creation of empty Histogram
>    libcamera: controls: Generate enum value-name maps
>    ipa: libipa: Add MeanLuminanceAgc base class
>    ipa: ipu3: Parse and store Agc stats
>    ipa: ipu3: Derive ipu3::algorithms::Agc from MeanLuminanceAgc
>    ipa: ipu3: Remove bespoke AGC functions from IPU3
>    ipa: rkisp1: Add parseStatistics() to Agc
>    ipa: rkisp1: Derive rkisp1::algorithms::Agc from MeanLuminanceAgc
>    ipa: rkisp1: Remove bespoke Agc functions
>
> Paul Elder (1):
>    ipa: libipa: Add ExposureModeHelper
>
>   include/libcamera/control_ids.h.in      |   2 +
>   include/libcamera/property_ids.h.in     |   2 +
>   src/ipa/ipu3/algorithms/agc.cpp         | 306 ++++----------
>   src/ipa/ipu3/algorithms/agc.h           |  27 +-
>   src/ipa/ipu3/ipa_context.cpp            |   3 +
>   src/ipa/ipu3/ipa_context.h              |   5 +
>   src/ipa/ipu3/ipu3.cpp                   |   2 +-
>   src/ipa/libipa/agc.cpp                  | 526 ++++++++++++++++++++++++
>   src/ipa/libipa/agc.h                    |  82 ++++
>   src/ipa/libipa/exposure_mode_helper.cpp | 307 ++++++++++++++
>   src/ipa/libipa/exposure_mode_helper.h   |  61 +++
>   src/ipa/libipa/histogram.h              |   1 +
>   src/ipa/libipa/meson.build              |   4 +
>   src/ipa/rkisp1/algorithms/agc.cpp       | 303 ++++----------
>   src/ipa/rkisp1/algorithms/agc.h         |  19 +-
>   src/ipa/rkisp1/ipa_context.h            |   5 +
>   src/ipa/rkisp1/rkisp1.cpp               |   2 +-
>   utils/gen-controls.py                   |  19 +
>   18 files changed, 1219 insertions(+), 457 deletions(-)
>   create mode 100644 src/ipa/libipa/agc.cpp
>   create mode 100644 src/ipa/libipa/agc.h
>   create mode 100644 src/ipa/libipa/exposure_mode_helper.cpp
>   create mode 100644 src/ipa/libipa/exposure_mode_helper.h
>