[PATCH 00/10] Centralise Agc into libipa

[Dan Scally]

Hi Laurent

On 05/04/2024 22:19, Laurent Pinchart wrote:
> Hi Dan,
>
> On Fri, Mar 22, 2024 at 02:48:06PM +0000, Daniel Scally wrote:
>> 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.
> Then we clearly have something to fix, as the code shouldn't contain
> compile-time constants that depend on the 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.
> Wouldn't it be better to define how the algorithm expects the luminance
> to be estimated, and require each platform-specific specialization to
> compute the luminance estimate accordingly ? Injecting ill-defined
> luminance estimates with ill-defined parameters to tweak the behaviour
> of the calculations doesn't seem very scientific.

That approach also has problems though; at minimum it would force the IPU3 IPA to calculate a 
histogram using the formula that's internally used by the RkISP1 (since we couldn't calculate a 
histogram using the BT.601 values for RkISP1) and forcing one IPA to adhere to the hardware 
constraints of another doesn't sound right. I **think** (but am less sure about) that the statistics 
formats are too different to make a truly equivalent calculation anyway; meaning for the same input 
data from a sensor I don't think you'd be able to arrive at precisely the same luminance estimation 
across each ISP. But is that a problem? I think as long as we understand that and make it clear that 
that's what's happening it ought to be fine.


@Stefan, @Jacopo we talked about this on the call that we had together...but I'm afraid I left it 
too long to revisit the series and now can't remember what we concluded about this difference in 
luminance targets between ISPs and the likely explanation for it.

>
>>> 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