[PATCH 00/10] Centralise Agc into libipa

[Laurent Pinchart]

On Fri, Mar 22, 2024 at 03:33:18PM +0100, Stefan Klug wrote:
> Hi Daniel,
> 
> On Fri, Mar 22, 2024 at 01:14:41PM +0000, 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.
> 
> Just a wild guess (and the numbers fit nicely). This might be dependent on
> whether gamma is included in the calculation or not. A value of 0.16
> corresponds to 0.4 for gamma=2.

Both the IPU3 and RkISP1 are supposed to produce stats computed on
linear-space data. We don't linearize the sensor output at the moment,
but it should be mostly linear to start with.

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