[PATCH 00/10] Centralise Agc into libipa
Dan Scally
dan.scally at ideasonboard.com
Mon Apr 8 16:32:31 CEST 2024
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
More information about the libcamera-devel
mailing list