[PATCH v6 00/18] Software ISP refactoring

[Kieran Bingham]

Quoting Milan Zamazal (2024-09-06 13:09:09)
> The purpose of this patch series is to bring software ISP code
> structuring closer to the hardware pipelines.  Most notably, the API
> around algorithm.h is used now.  This should make software ISP easier to
> understand, extend, maintain, and code-share with the other pipelines.
> 
> What is omitted in the patch series:
> 
> - Any bigger or unrelated functional changes.  The purpose of this
>   series is code restructuring, which is enough already.

Agreed, and I think trying to land this as soon as possible is helpful

> - Stats and params buffers are still used in the original way.  Making
>   their handling closer to the hardware pipelines will be a subject of
>   followup patches.  This series is a preparation step for that.

Indeed, though I do wonder if this will use 'parameter buffers' in the
same way. But I guess a defined structure interface will be much the
same as a parameter buffer so probably good to keep the naming
consistent.

> - Any IPA code sharing with hardware pipelines.  If there is an
>   opportunity for this, it can be addressed in followup patches.

Agreed

> 
> Available in git at
> https://gitlab.freedesktop.org/camera/libcamera-softisp/-/commits/pdm-algorithm
> 
> Pending, more input from reviewers needed (see v3 review discussions):
> - Should `frame' argument name be kept for consistency with other
>   pipelines for now or already renamed to something like
>   `sequenceNumber'?

I'm fine keeping it as 'frame' for now - but I'm constantly worried
about the different 'clock domains' of sequences between the sensor and
the requests.

We'll be tackling this as part of per-frame control 'soon' so I think
keeping it now is easiest.

> - Should black level be stored as a 0..1 number or a pixel value
>   number?

We store Black levels as pixel numbers, I think that's what you're doing
already so I'd keep it that way.

Toying around to test this branch earlier - we took a couple of
screenshots/captures running through camshark which now detects macbeth
charts and details the delta-e for each colour as a live overlay
(awesome work from Stefan!)

https://cloud.ideasonboard.com/apps/photos/public/ewHIg37E78qmqGPOwh6uCHAuM1svX3Jx

The reds seem to be a bit lost - I think that's where we'll see benefits
from starting to add some CCM support.  But that will be into sensor
specific tunings.



> Changes in v6:
> 
> - Use data->swIsp_ only if it is defined.
> - IPASoftSimple::prepare renamed to IPASoftSimple::fillParamsBuffer.
> - soft_ipa_interface.h include moved to the right patch.
> - Unneeded includes in algorithm *.h files dropped.
> - Duplicate kGammaLookupSize constant removed and gammaTable.size() used
>   instead.
> - Gamma algorithm merged to Lut algorithm.
> - Duplicate logging of AWB gains removed.
> - Miscellaneous formatting and cosmetic changes suggested by Laurent.
> 
> Changes in v5:
> - Construction of color lookup tables moved to a separate algorithm.
> 
> Changes in v4:
> - Removed the mistakenly included patches by Umang.
> - IPASoftInterface::prepare() is async now.
> - IPAConfigInfo definition moved to an earlier patch to avoid
>   unnecessary confusion.
> - colors.cpp/h split to gamma and awb.
> - Using the right black level variable in Agc::process.
> - Documentation of the newly introduced SwStatsCpu::finishFrame
>   arguments added.
> - Formatting changes as requested.
> - A forgotten obsolete source code comment removed.
> - Improvements to some commit messages.
> 
> Changes in v3:
> - SoftwareIsp::queueRequest changed to async.
> - IPAActiveState docstring reworded.
> - Minor formatting fixes.
> 
> Changes in v2:
> - Several cosmetic changes and patch arrangement problems pointed out by
>   Umang applied.
> - Added ipa_context.cpp, as a documentation file for ipa_context.h.
> - Added a clarification source comment why SoftwareIsp::queueBuffers needs
>   to get the frame number as a separate argument.
> - core_ipa_interface.h no longer included in module.h.
> - The context used by "14/19 Move black level to an algorithm module"
>   was changed and the black level changes tracking was put closer to the
>   pre-refactoring version, which makes more sense.
> 
> Milan Zamazal (18):
>   libcamera: software_isp: Remove superfluous includes
>   libcamera: software_isp: Move BlackLevel to libcamera::ipa::soft
>   libcamera: software_isp: Define skeletons for IPA refactoring
>   libcamera: software_isp: Let IPASoftSimple inherit Module
>   libcamera: software_isp: Make stats frame and buffer aware
>   libcamera: software_isp: Remove final dots in debayer.cpp docstrings
>   libcamera: software_isp: Track and pass frame ids
>   libcamera: software_isp: Create algorithms
>   libcamera: software_isp: Call Algorithm::configure
>   libcamera: software_isp: Call Algorithm::queueRequest
>   libcamera: software_isp: Call Algorithm::prepare
>   libcamera: software_isp: Call Algorithm::process
>   libcamera: software_isp: Move black level to an algorithm module
>   libcamera: software_isp: Move color handling to an algorithm module
>   libcamera: software_isp: Use floating point for color parameters
>   libcamera: software_isp: Use DelayedControls
>   libcamera: software_isp: Move exposure+gain to an algorithm module
>   libcamera: software_isp: Update black level only on exposure changes
> 
>  .../internal/software_isp/software_isp.h      |  15 +-
>  include/libcamera/ipa/soft.mojom              |  12 +-
>  src/ipa/simple/algorithms/agc.cpp             | 139 +++++++++
>  src/ipa/simple/algorithms/agc.h               |  33 ++
>  src/ipa/simple/algorithms/algorithm.h         |  22 ++
>  src/ipa/simple/algorithms/awb.cpp             |  69 +++++
>  src/ipa/simple/algorithms/awb.h               |  32 ++
>  src/ipa/simple/algorithms/blc.cpp             |  78 +++++
>  src/ipa/simple/algorithms/blc.h               |  36 +++
>  src/ipa/simple/algorithms/lut.cpp             |  81 +++++
>  src/ipa/simple/algorithms/lut.h               |  35 +++
>  src/ipa/simple/algorithms/meson.build         |   8 +
>  src/ipa/simple/black_level.cpp                |  88 ------
>  src/ipa/simple/black_level.h                  |  29 --
>  src/ipa/simple/data/uncalibrated.yaml         |   5 +
>  src/ipa/simple/ipa_context.cpp                | 102 +++++++
>  src/ipa/simple/ipa_context.h                  |  69 +++++
>  src/ipa/simple/meson.build                    |   9 +-
>  src/ipa/simple/module.h                       |  30 ++
>  src/ipa/simple/soft_simple.cpp                | 283 ++++++------------
>  src/libcamera/pipeline/simple/simple.cpp      |  48 ++-
>  src/libcamera/software_isp/TODO               |  39 ---
>  src/libcamera/software_isp/debayer.cpp        |  51 ++--
>  src/libcamera/software_isp/debayer.h          |   2 +-
>  src/libcamera/software_isp/debayer_cpu.cpp    |   9 +-
>  src/libcamera/software_isp/debayer_cpu.h      |   2 +-
>  src/libcamera/software_isp/software_isp.cpp   |  43 ++-
>  src/libcamera/software_isp/swstats_cpu.cpp    |   6 +-
>  src/libcamera/software_isp/swstats_cpu.h      |   4 +-
>  29 files changed, 955 insertions(+), 424 deletions(-)
>  create mode 100644 src/ipa/simple/algorithms/agc.cpp
>  create mode 100644 src/ipa/simple/algorithms/agc.h
>  create mode 100644 src/ipa/simple/algorithms/algorithm.h
>  create mode 100644 src/ipa/simple/algorithms/awb.cpp
>  create mode 100644 src/ipa/simple/algorithms/awb.h
>  create mode 100644 src/ipa/simple/algorithms/blc.cpp
>  create mode 100644 src/ipa/simple/algorithms/blc.h
>  create mode 100644 src/ipa/simple/algorithms/lut.cpp
>  create mode 100644 src/ipa/simple/algorithms/lut.h
>  create mode 100644 src/ipa/simple/algorithms/meson.build
>  delete mode 100644 src/ipa/simple/black_level.cpp
>  delete mode 100644 src/ipa/simple/black_level.h
>  create mode 100644 src/ipa/simple/ipa_context.cpp
>  create mode 100644 src/ipa/simple/ipa_context.h
>  create mode 100644 src/ipa/simple/module.h
> 
> -- 
> 2.44.1
>