[PATCH v6 00/18] Software ISP refactoring
Milan Zamazal
mzamazal at redhat.com
Thu Sep 19 20:58:52 CEST 2024
Hi Kieran,
thank you for reviews.
Kieran Bingham <kieran.bingham at ideasonboard.com> writes:
> 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.
Yes. The idea of parameter buffers is expressed in the following TODO,
which I handle in the followup patch series (I'll rebase, adjust and
repost it once this refactoring is finished):
5. Store ISP parameters in per-frame buffers
> /**
> * \fn void Debayer::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)
> * \brief Process the bayer data into the requested format.
> * \param[in] input The input buffer.
> * \param[in] output The output buffer.
> * \param[in] params The parameters to be used in debayering.
> *
> * \note DebayerParams is passed by value deliberately so that a copy is passed
> * when this is run in another thread by invokeMethod().
> */
Possibly something to address later, by storing ISP parameters in
per-frame buffers like we do for hardware ISPs.
>> - 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.
OK.
> 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
Oh, cool!
> 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.
I'm sure we have a lot of things to improve. :-)
>> 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
>>
More information about the libcamera-devel
mailing list