[v4,0/6] ipa: software_isp: AGC: Fix AGC oscillation bug

24533 mbox series

Hi All,

Due to a combination of not having correct control-delay information for
various sensors as well as the generic nature of the simple-pipeline +
software-ISP meaning that any pipeline delays are unknown it is impossible
to get reliable control-delay values.

Wrong control-delay values can lead to pretty bad oscilation. See the v1
cover-letter for more details.

This series fixes some unrelated issues in the softIPA AGC algorithm and
in the last 2 patches addresses the oscillation issue. Note v2 and later
take a new approach to fixing the oscilation by only generating statistics
for every 4th frame and having the IPA only run its algorithms when there
actually are statistics. This saves CPU time, while at the same time
avoiding the oscillation issue.

v4 has successfully passed CI, see:
https://gitlab.freedesktop.org/camera/libcamera-softisp/-/commit/e749d265ff6ec85752198dc1ab23ef2a42ab36af/pipelines?ref=softisp-agc-oscillation-fixes-v4

Changes in v4:
- AGC: Add an empty line after the agc substruct declaration
- AGC: Init active-state agc gain and exposue from sensor values in case
  updateExposure() does not run for the first frame
- SwStats: Document why to skip 3 frames / why once every 4 frames
- Pass frame number to SwStatsCpu::startFrame() SwStatsCpu::processLine?()
  and move all skipping handling to inside the SwStatsCpu class

Changes in v3:
- Drop if (!stats_->valid) early exit from IPASoftSimple::processStats()
- Save last AGC calculated new gain and exposure values and re-use these
  for frames without statistics, this avoids delayedCtrl history underruns
- Improve SwIspStats.valid documentation

Changes in v2:
- Skip running IPA algorithms when there are no statistics
- Only generate statistics for every 4th frame

Note the speed of the AGC algorithm converges on the desired brightness
level is unchanged compared to v1 since in v1 the AGC algorithm would
skip 3 frames after every gain/exposure change.

Regards,

Hans


Hans de Goede (6):
  ipa: software_isp: Fix context_.configuration.agc.againMin init
  ipa: software_isp: AGC: Do not lower gain below 1.0
  ipa: software_isp: AGC: Raise exposure or gain not both at the same
    time
  ipa: software_isp: AGC: Only use integers for exposure calculations
  libcamera: software_isp: Add valid flag to struct SwIspStats
  libcamera: software_isp: Run sw-statistics once every 4th frame

 .../internal/software_isp/swisp_stats.h       |  5 ++
 src/ipa/simple/algorithms/agc.cpp             | 47 ++++++++++++++-----
 src/ipa/simple/algorithms/awb.cpp             |  3 ++
 src/ipa/simple/algorithms/blc.cpp             |  3 ++
 src/ipa/simple/ipa_context.h                  |  7 ++-
 src/ipa/simple/soft_simple.cpp                |  7 ++-
 src/libcamera/software_isp/debayer_cpu.cpp    | 18 +++----
 src/libcamera/software_isp/debayer_cpu.h      |  4 +-
 src/libcamera/software_isp/swstats_cpu.cpp    | 18 +++++--
 src/libcamera/software_isp/swstats_cpu.h      | 20 ++++++--
 10 files changed, 100 insertions(+), 32 deletions(-)