[v8,00/18] Software ISP refactoring
mbox series

Message ID 20240927134624.801004-1-mzamazal@redhat.com
Headers show
Series
  • Software ISP refactoring
Related show

Message

Milan Zamazal Sept. 27, 2024, 1:46 p.m. UTC 
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.
- 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.
- Any IPA code sharing with hardware pipelines.  If there is an
  opportunity for this, it can be addressed in followup patches.

Available in git at
https://gitlab.freedesktop.org/camera/libcamera-softisp/-/commits/pdm-algorithm

Changes in v8:
- Dropped unused `black' struct from IPASessionConfiguration.
- Unused <libcamera/controls.h> include removed.
- Some empty lines added.

Changes in v7:
- Redundant import of request.h in software_isp.cpp removed.
- Algorithm initializations moved from init() to configure().
- Black levels stored as pixel values rather than floats.

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               |  35 +++
 src/ipa/simple/algorithms/lut.cpp             |  86 ++++++
 src/ipa/simple/algorithms/lut.h               |  34 +++
 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                  |  65 ++++
 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    |  12 +-
 src/libcamera/software_isp/debayer_cpu.h      |   2 +-
 src/libcamera/software_isp/software_isp.cpp   |  42 ++-
 src/libcamera/software_isp/swstats_cpu.cpp    |   6 +-
 src/libcamera/software_isp/swstats_cpu.h      |   4 +-
 29 files changed, 954 insertions(+), 426 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

Comments

Kieran Bingham Sept. 27, 2024, 3:43 p.m. UTC | #1 
Quoting Milan Zamazal (2024-09-27 14:46:06)
> 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.
> - 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.
> - Any IPA code sharing with hardware pipelines.  If there is an
>   opportunity for this, it can be addressed in followup patches.

I've merged this series. I hope that helps you with the items you are
tackling on top.

--
Kieran
Milan Zamazal Sept. 27, 2024, 4:54 p.m. UTC | #2 
Hi Kieran,

Kieran Bingham <kieran.bingham@ideasonboard.com> writes:

> Quoting Milan Zamazal (2024-09-27 14:46:06)
>> 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.
>> - 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.
>> - Any IPA code sharing with hardware pipelines.  If there is an
>>   opportunity for this, it can be addressed in followup patches.
>
> I've merged this series. I hope that helps you with the items you are
> tackling on top.

Definitely, thank you!

> --
> Kieran