| Message ID | 20260703153819.1088752-1-barnabas.pocze@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabas, let's discuss the architecture before getting into the details of each single patch On Fri, Jul 03, 2026 at 05:38:02PM +0200, Barnabás Pőcze wrote: > This is a first early of the AGC rework that is supposed to remove duplication > of AGC related code from multiple IPA modules. > > It also enables the use of AgcMeanLuminance in the simple pipeline handler > if a camera sensor helper is available. > > This applies on top of the current master branch, not rebased on any of > the libipa rework yet. Conflicts should be minimal. This series implements libIPA agc support as: +--------------+ | AgcAlgorithm | +--------------+ ^ | | +-----------------+ +------------------+ | AgcMeanLuminance|-------o| AgcMeanLuminance | | Algorithm | +------------------+ +-----------------+ o | | +-----------------+ | RkISP1 AGC | +-----------------+ AgcAlgorithm - defines and interface similar in spirit to libipa::Algorithm - implements the controls handling state machine - Defines ActiveState FrameContext and SessionConfiguration AgcMeanLuminanceAlgorithm: - inherits from AgcAlgorithm - extends ActiveState FrameContext and SessionConfiguration with AeConstraintModeEnum and AeExposureModeEnum - implements handling of AeExposureModeEnum and AeExposureModeEnum and ExposureValue - embeds and instance of AgcMeanLuminance AgcMeanLuminance: - implements the algorithm logic Let me start with a comparison with Agc and Lsc. They do implement something like +--------------+ +------------------+ | LscAlgorithm |-----o| LscImplementation| +--------------+ +------------------+ o ^ | | | ------------------ +---------------+ | | | RkISP1 LSC | +------+ +------------+ +---------------+ | Grid | | Polynomial | +------+ +------------+ The difference here is that IPA algorithm use by composition and instance of [Awb|Lsc]Algorithm which instantiate an instance of one the back-ends according to the tuning file content: Gray or Bayes for Awb and Grid or Polynomial for Lsc. This allows to add more back-ends behding a single [Awb|Lsc]Implementation and hide the implementation details inside the [Awb|Lsc]Algorithm class. In the Agc implementation instead the IPA embeds an AgcMeanLuminanceAlgorithm and decides then to use the MeanLuminance method. If any other back-end is introduced, IPAs will have to be modified instead of simply updating the tuning files. The same goes for the IPA context. IPAs will use AgcMeanLuminanceAlgorithm::FrameContext and AgcMeanLuminanceAlgorithm::ActiveState and once they do so, they're bound to use AgcMeanLuminance. What are the reasons that prevent implementing a logic-agnostic AgcAlgorithm that instantiate AgcMeanLuminance internally ? Do we expext more implementations of AgcAlgorithm or will MeanLuminance be the only one in libIPA so we don't need to abstract it away ? What do other think ? A few more questions more specifical to the implmentation: ------------------------------------------------------------------------------- class AgcAlgorithm { public: struct Session { }; struct ActiveState { }; struct FrameContext { }; struct ConfigurationParams { }; }; For all the other algorithms the Session, ActiveState and FrameContext definitions that will be used by IPAs in ipa_context.h have been placed outside of the class defintion in a dedicated namespace. namespace libcamera { namespace ipa { namespace awb { struct Session { }; struct ActiveState { }; struct FrameContext { }; } /* namespace awb */ class AwbAlgorithm { }; } /* namespace ipa */ } /* namespace libcamera */ What are the reasons to do it differently here ? ------------------------------------------------------------------------------- ------------------------------------------------------------------------------- You have split AgcMeanLuminance in two classes. What is the purpose of AgcMeanLuminanceAlgorithm and AgcMeanLuminance. Can the two be used separately ? ------------------------------------------------------------------------------- ------------------------------------------------------------------------------- Why are libcamera generic controls like ExposureValue, AeConstraintMode and AeExposureMode handled in AgcMeanLuminanceAlgorithm and not in AgcAlgorithm like other ones ? What's special about them ? ------------------------------------------------------------------------------- I would like to clarify these questions before getting into a more detailed review if possible. Thanks j > > TODO: handling of DigitalGain + AeEnable needs to be clarified > > Barnabás Pőcze (17): > ipa: simple: Fix control presence sanity check > ipa: rkisp1: Remove `IPASessionConfiguration::sensor.defVBlank` > ipa: simple: agc: Do not overwrite sensor exposure/gain > ipa: libipa: agc_mean_luminance: Remove the need for inheritance > ipa: rkisp1: Move sensor control info map to context > ipa: rkisp1: Update sensor info when configuring > ipa: rkisp1: updateControls(): Use sensor info from context > ipa: rkisp1: Move AGC related controls into AGC algorithm > ipa: mali-c55: Remove `DigitalGain` support > ipa: libipa: Add `AgcMeanLuminance` wrapper > ipa: rkisp1: Port to `AgcMeanLuminanceAlgorithm` > ipa: mali-c55: Port to `AgcMeanLuminanceAlgorithm` > ipa: ipu3: Port to `AgcMeanLuminanceAlgorithm` > ipa: libipa: agc_mean_luminance: controls(): Return const ref > ipa: libipa: Add `AgcAlgorithm` > ipa: simple: agc: Port to `AgcAlgorithm` > ipa: simple: agc: Use `AgcMeanLuminance` if sensor helper is available > > src/ipa/ipu3/algorithms/agc.cpp | 199 +++++------ > src/ipa/ipu3/algorithms/agc.h | 19 +- > src/ipa/ipu3/ipa_context.cpp | 49 +-- > src/ipa/ipu3/ipa_context.h | 27 +- > src/ipa/ipu3/ipu3.cpp | 138 ++------ > src/ipa/libipa/agc.cpp | 488 ++++++++++++++++++++++++++ > src/ipa/libipa/agc.h | 100 ++++++ > src/ipa/libipa/agc_mean_luminance.cpp | 268 ++++++++++++-- > src/ipa/libipa/agc_mean_luminance.h | 65 +++- > src/ipa/libipa/meson.build | 2 + > src/ipa/mali-c55/algorithms/agc.cpp | 205 +++-------- > src/ipa/mali-c55/algorithms/agc.h | 7 +- > src/ipa/mali-c55/ipa_context.h | 38 +- > src/ipa/mali-c55/mali-c55.cpp | 163 ++------- > src/ipa/rkisp1/algorithms/agc.cpp | 478 ++++++------------------- > src/ipa/rkisp1/algorithms/agc.h | 13 +- > src/ipa/rkisp1/algorithms/lux.cpp | 2 +- > src/ipa/rkisp1/algorithms/wdr.cpp | 2 +- > src/ipa/rkisp1/ipa_context.cpp | 104 +----- > src/ipa/rkisp1/ipa_context.h | 48 +-- > src/ipa/rkisp1/rkisp1.cpp | 120 +------ > src/ipa/simple/agc_simple.cpp | 197 +++++++++++ > src/ipa/simple/agc_simple.h | 59 ++++ > src/ipa/simple/algorithms/agc.cpp | 296 ++++++++-------- > src/ipa/simple/algorithms/agc.h | 16 +- > src/ipa/simple/ipa_context.h | 25 +- > src/ipa/simple/meson.build | 1 + > src/ipa/simple/soft_simple.cpp | 80 ++--- > 28 files changed, 1743 insertions(+), 1466 deletions(-) > create mode 100644 src/ipa/libipa/agc.cpp > create mode 100644 src/ipa/libipa/agc.h > create mode 100644 src/ipa/simple/agc_simple.cpp > create mode 100644 src/ipa/simple/agc_simple.h > > -- > 2.54.0
2026. 07. 06. 14:34 keltezéssel, Jacopo Mondi írta: > Hi Barnabas, > let's discuss the architecture before getting into the details of > each single patch > > On Fri, Jul 03, 2026 at 05:38:02PM +0200, Barnabás Pőcze wrote: >> This is a first early of the AGC rework that is supposed to remove duplication >> of AGC related code from multiple IPA modules. >> >> It also enables the use of AgcMeanLuminance in the simple pipeline handler >> if a camera sensor helper is available. >> >> This applies on top of the current master branch, not rebased on any of >> the libipa rework yet. Conflicts should be minimal. > > This series implements libIPA agc support as: > > +--------------+ > | AgcAlgorithm | > +--------------+ > ^ > | > | > +-----------------+ +------------------+ > | AgcMeanLuminance|-------o| AgcMeanLuminance | > | Algorithm | +------------------+ > +-----------------+ > o > | > | > +-----------------+ > | RkISP1 AGC | > +-----------------+ > > AgcAlgorithm > - defines and interface similar in spirit to libipa::Algorithm > - implements the controls handling state machine > - Defines ActiveState FrameContext and SessionConfiguration > > AgcMeanLuminanceAlgorithm: > - inherits from AgcAlgorithm > - extends ActiveState FrameContext and SessionConfiguration with > AeConstraintModeEnum and AeExposureModeEnum > - implements handling of AeExposureModeEnum and AeExposureModeEnum > and ExposureValue > - embeds and instance of AgcMeanLuminance > > AgcMeanLuminance: > - implements the algorithm logic > > Let me start with a comparison with Agc and Lsc. > They do implement something like > > +--------------+ +------------------+ > | LscAlgorithm |-----o| LscImplementation| > +--------------+ +------------------+ > o ^ > | | > | ------------------ > +---------------+ | | > | RkISP1 LSC | +------+ +------------+ > +---------------+ | Grid | | Polynomial | > +------+ +------------+ > > > The difference here is that IPA algorithm use by composition and > instance of [Awb|Lsc]Algorithm which instantiate an instance of > one the back-ends according to the tuning file content: Gray or Bayes > for Awb and Grid or Polynomial for Lsc. > > This allows to add more back-ends behding a single > [Awb|Lsc]Implementation and hide the implementation details inside the > [Awb|Lsc]Algorithm class. > > In the Agc implementation instead the IPA embeds an > AgcMeanLuminanceAlgorithm and decides then to use the MeanLuminance > method. If any other back-end is introduced, IPAs will have to be > modified instead of simply updating the tuning files. > > The same goes for the IPA context. IPAs will use > AgcMeanLuminanceAlgorithm::FrameContext and > AgcMeanLuminanceAlgorithm::ActiveState and once they do so, they're > bound to use AgcMeanLuminance. > > What are the reasons that prevent implementing a logic-agnostic > AgcAlgorithm that instantiate AgcMeanLuminance internally ? > > Do we expext more implementations of AgcAlgorithm or will > MeanLuminance be the only one in libIPA so we don't need to abstract > it away ? > > What do other think ? The way I see it, different algorithms will need different inputs / have different requirements. Hence I am not sure one can easily define a generic interface that works for all. This is already somewhat visible with `AgcMeanLuminance` and the current algorithm in the simple ipa module. Another reason was to keep these changes simpler. > > A few more questions more specifical to the implmentation: > > ------------------------------------------------------------------------------- > > class AgcAlgorithm > { > public: > struct Session { > }; > > struct ActiveState { > }; > > struct FrameContext { > }; > > struct ConfigurationParams { > }; > }; > > For all the other algorithms the Session, ActiveState and FrameContext > definitions that will be used by IPAs in ipa_context.h have been > placed outside of the class defintion in a dedicated namespace. > > namespace libcamera { > namespace ipa { > namespace awb { > > struct Session { > }; > > struct ActiveState { > }; > > struct FrameContext { > }; > > } /* namespace awb */ > > class AwbAlgorithm > { > > }; > > } /* namespace ipa */ > > } /* namespace libcamera */ > > What are the reasons to do it differently here ? > ------------------------------------------------------------------------------- I think it was done this way because there are multiple ones for each kind, in AgcAlgorithm, AgcMeanLuminanceAlgorithm, etc. > > ------------------------------------------------------------------------------- > You have split AgcMeanLuminance in two classes. What is the purpose of > AgcMeanLuminanceAlgorithm and AgcMeanLuminance. Can the two be used > separately ? > ------------------------------------------------------------------------------- The idea was to keep the actual algorithm in `AgcMeanLuminance` and not modify it too much. And then what's needed to fit into the libipa algorithm format is in `AgcMeanLuminanceAlgorithm`. > > ------------------------------------------------------------------------------- > Why are libcamera generic controls like ExposureValue, > AeConstraintMode and AeExposureMode handled in > AgcMeanLuminanceAlgorithm and not in AgcAlgorithm like other ones ? > > What's special about them ? > ------------------------------------------------------------------------------- For example, I would argue that the constraint mode handling is very much defined with `AgcMeanLuminance` in mind. So I took the short path and kept them there instead of trying to generalize them. Similar considerations apply to the others. > > I would like to clarify these questions before getting into a more > detailed review if possible. > > Thanks > j > > >> >> TODO: handling of DigitalGain + AeEnable needs to be clarified >> >> Barnabás Pőcze (17): >> ipa: simple: Fix control presence sanity check >> ipa: rkisp1: Remove `IPASessionConfiguration::sensor.defVBlank` >> ipa: simple: agc: Do not overwrite sensor exposure/gain >> ipa: libipa: agc_mean_luminance: Remove the need for inheritance >> ipa: rkisp1: Move sensor control info map to context >> ipa: rkisp1: Update sensor info when configuring >> ipa: rkisp1: updateControls(): Use sensor info from context >> ipa: rkisp1: Move AGC related controls into AGC algorithm >> ipa: mali-c55: Remove `DigitalGain` support >> ipa: libipa: Add `AgcMeanLuminance` wrapper >> ipa: rkisp1: Port to `AgcMeanLuminanceAlgorithm` >> ipa: mali-c55: Port to `AgcMeanLuminanceAlgorithm` >> ipa: ipu3: Port to `AgcMeanLuminanceAlgorithm` >> ipa: libipa: agc_mean_luminance: controls(): Return const ref >> ipa: libipa: Add `AgcAlgorithm` >> ipa: simple: agc: Port to `AgcAlgorithm` >> ipa: simple: agc: Use `AgcMeanLuminance` if sensor helper is available >> >> src/ipa/ipu3/algorithms/agc.cpp | 199 +++++------ >> src/ipa/ipu3/algorithms/agc.h | 19 +- >> src/ipa/ipu3/ipa_context.cpp | 49 +-- >> src/ipa/ipu3/ipa_context.h | 27 +- >> src/ipa/ipu3/ipu3.cpp | 138 ++------ >> src/ipa/libipa/agc.cpp | 488 ++++++++++++++++++++++++++ >> src/ipa/libipa/agc.h | 100 ++++++ >> src/ipa/libipa/agc_mean_luminance.cpp | 268 ++++++++++++-- >> src/ipa/libipa/agc_mean_luminance.h | 65 +++- >> src/ipa/libipa/meson.build | 2 + >> src/ipa/mali-c55/algorithms/agc.cpp | 205 +++-------- >> src/ipa/mali-c55/algorithms/agc.h | 7 +- >> src/ipa/mali-c55/ipa_context.h | 38 +- >> src/ipa/mali-c55/mali-c55.cpp | 163 ++------- >> src/ipa/rkisp1/algorithms/agc.cpp | 478 ++++++------------------- >> src/ipa/rkisp1/algorithms/agc.h | 13 +- >> src/ipa/rkisp1/algorithms/lux.cpp | 2 +- >> src/ipa/rkisp1/algorithms/wdr.cpp | 2 +- >> src/ipa/rkisp1/ipa_context.cpp | 104 +----- >> src/ipa/rkisp1/ipa_context.h | 48 +-- >> src/ipa/rkisp1/rkisp1.cpp | 120 +------ >> src/ipa/simple/agc_simple.cpp | 197 +++++++++++ >> src/ipa/simple/agc_simple.h | 59 ++++ >> src/ipa/simple/algorithms/agc.cpp | 296 ++++++++-------- >> src/ipa/simple/algorithms/agc.h | 16 +- >> src/ipa/simple/ipa_context.h | 25 +- >> src/ipa/simple/meson.build | 1 + >> src/ipa/simple/soft_simple.cpp | 80 ++--- >> 28 files changed, 1743 insertions(+), 1466 deletions(-) >> create mode 100644 src/ipa/libipa/agc.cpp >> create mode 100644 src/ipa/libipa/agc.h >> create mode 100644 src/ipa/simple/agc_simple.cpp >> create mode 100644 src/ipa/simple/agc_simple.h >> >> -- >> 2.54.0