[PATCH 00/10] Centralise Agc into libipa

[Laurent Pinchart]

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.

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

-- 
Regards,

Laurent Pinchart