| Message ID | 20240906120927.4071508-1-mzamazal@redhat.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
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 >
Hi Kieran, thank you for reviews. Kieran Bingham <kieran.bingham@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 >>