| Message ID | 20260817134042.277168-6-johannes.goede@oss.qualcomm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
2026. 08. 17. 15:40 keltezéssel, Hans de Goede írta: > Currently createIPA() / IPAManager::module() assume that there is a 1:1 > relationship between pipeline handlers and IPAs and IPA matching is done > based on matching the pipe's name to ipaModuleInfo.name[]. > > This coupling between pipeline name and which IPA to create is undesirable > and makes it impossible to for example use the softISP as a fallback in > other pipeline handlers than the simple pipeline handler. > > The actual API contract between the IPA and the pipeline handler is given > by the IPA proxy type. Add a static IPAProxyXXX::name() to the generated > IPAProxyXXX classes and use this in PipelineHandler::createIPA() instead > of using PipelineHandler::name(). > > This requires changing the name, as well as the subdirectory of the softISP > IPA from "simple" to "softisp", which is the more sensible name anyway. > > Note: this means that softISP IPA config files will now be looked for under > /usr/share/libcamera/ipa/softisp instead of /usr/share/libcamera/ipa/simple! > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com> > --- I feel like the renaming should be separated and possibly merged with the later patches 6 and 7. Any thoughts? In any case, it looks ok to me. Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > Changes in v8: > - Fix CI failure caused by extra trailing ';' > > Changes in v7: > - Rename the softISP IPA from "simple" to "softisp" instead of "soft" > - Mark PipelineHandler::createIPA() overload with an explicit IPA name > argument as deprecated > > Changes in v6: > - New patch in v6 of this patch-series > --- > Documentation/Doxyfile-internal.in | 2 +- > include/libcamera/internal/pipeline_handler.h | 2 +- > include/libcamera/internal/software_isp/software_isp.h | 4 ++-- > include/libcamera/ipa/meson.build | 2 +- > include/libcamera/ipa/{soft.mojom => softisp.mojom} | 0 > meson_options.txt | 4 ++-- > src/ipa/meson.build | 2 +- > src/ipa/{simple => softisp}/algorithms/adjust.cpp | 0 > src/ipa/{simple => softisp}/algorithms/adjust.h | 0 > src/ipa/{simple => softisp}/algorithms/agc.cpp | 0 > src/ipa/{simple => softisp}/algorithms/agc.h | 0 > src/ipa/{simple => softisp}/algorithms/algorithm.h | 0 > src/ipa/{simple => softisp}/algorithms/awb.cpp | 0 > src/ipa/{simple => softisp}/algorithms/awb.h | 2 +- > src/ipa/{simple => softisp}/algorithms/blc.cpp | 0 > src/ipa/{simple => softisp}/algorithms/blc.h | 0 > src/ipa/{simple => softisp}/algorithms/ccm.cpp | 0 > src/ipa/{simple => softisp}/algorithms/ccm.h | 0 > src/ipa/{simple => softisp}/algorithms/meson.build | 0 > src/ipa/{simple => softisp}/data/meson.build | 2 +- > src/ipa/{simple => softisp}/data/uncalibrated.yaml | 0 > src/ipa/{simple => softisp}/ipa_context.cpp | 0 > src/ipa/{simple => softisp}/ipa_context.h | 0 > src/ipa/{simple => softisp}/meson.build | 0 > src/ipa/{simple => softisp}/module.h | 2 +- > src/ipa/{simple => softisp}/soft_simple.cpp | 4 ++-- > src/libcamera/pipeline_handler.cpp | 8 +++++--- > .../libcamera_templates/module_ipa_proxy.h.tmpl | 2 ++ > 28 files changed, 20 insertions(+), 16 deletions(-) > rename include/libcamera/ipa/{soft.mojom => softisp.mojom} (100%) > rename src/ipa/{simple => softisp}/algorithms/adjust.cpp (100%) > rename src/ipa/{simple => softisp}/algorithms/adjust.h (100%) > rename src/ipa/{simple => softisp}/algorithms/agc.cpp (100%) > rename src/ipa/{simple => softisp}/algorithms/agc.h (100%) > rename src/ipa/{simple => softisp}/algorithms/algorithm.h (100%) > rename src/ipa/{simple => softisp}/algorithms/awb.cpp (100%) > rename src/ipa/{simple => softisp}/algorithms/awb.h (97%) > rename src/ipa/{simple => softisp}/algorithms/blc.cpp (100%) > rename src/ipa/{simple => softisp}/algorithms/blc.h (100%) > rename src/ipa/{simple => softisp}/algorithms/ccm.cpp (100%) > rename src/ipa/{simple => softisp}/algorithms/ccm.h (100%) > rename src/ipa/{simple => softisp}/algorithms/meson.build (100%) > rename src/ipa/{simple => softisp}/data/meson.build (79%) > rename src/ipa/{simple => softisp}/data/uncalibrated.yaml (100%) > rename src/ipa/{simple => softisp}/ipa_context.cpp (100%) > rename src/ipa/{simple => softisp}/ipa_context.h (100%) > rename src/ipa/{simple => softisp}/meson.build (100%) > rename src/ipa/{simple => softisp}/module.h (91%) > rename src/ipa/{simple => softisp}/soft_simple.cpp (99%) > > diff --git a/Documentation/Doxyfile-internal.in b/Documentation/Doxyfile-internal.in > index b64b6a6fe..70b7e7ef1 100644 > --- a/Documentation/Doxyfile-internal.in > +++ b/Documentation/Doxyfile-internal.in > @@ -28,7 +28,7 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/base/span.h \ > @TOP_SRCDIR@/src/libcamera/sensor/camera_sensor_raw.cpp \ > @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \ > @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \ > - @TOP_BUILDDIR@/include/libcamera/ipa/soft_ipa_interface.h \ > + @TOP_BUILDDIR@/include/libcamera/ipa/softisp_ipa_interface.h \ > @TOP_BUILDDIR@/src/libcamera/proxy/ > > HTML_OUTPUT = internal-api > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index b60c07b13..2a2ba95a4 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -76,7 +76,7 @@ public: > std::unique_ptr<T> createIPA(uint32_t minVersion, uint32_t maxVersion) > { > IPAManager *ipaManager = manager_->_d()->ipaManager(); > - return ipaManager->createIPA<T>(name_, minVersion, maxVersion); > + return ipaManager->createIPA<T>(T::name(), minVersion, maxVersion); > } > > template<typename T> > diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h > index 4f72dce9b..3205a2e14 100644 > --- a/include/libcamera/internal/software_isp/software_isp.h > +++ b/include/libcamera/internal/software_isp/software_isp.h > @@ -25,8 +25,8 @@ > #include <libcamera/geometry.h> > #include <libcamera/pixel_format.h> > > -#include <libcamera/ipa/soft_ipa_interface.h> > -#include <libcamera/ipa/soft_ipa_proxy.h> > +#include <libcamera/ipa/softisp_ipa_interface.h> > +#include <libcamera/ipa/softisp_ipa_proxy.h> > > #include "libcamera/internal/camera_sensor.h" > #include "libcamera/internal/dma_buf_allocator.h" > diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build > index 3ee3ada30..fb518526a 100644 > --- a/include/libcamera/ipa/meson.build > +++ b/include/libcamera/ipa/meson.build > @@ -68,7 +68,7 @@ pipeline_ipa_mojom_mapping = { > 'rkisp1': 'rkisp1.mojom', > 'rpi/pisp': 'raspberrypi.mojom', > 'rpi/vc4': 'raspberrypi.mojom', > - 'simple': 'soft.mojom', > + 'simple': 'softisp.mojom', > 'vimc': 'vimc.mojom', > } > > diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/softisp.mojom > similarity index 100% > rename from include/libcamera/ipa/soft.mojom > rename to include/libcamera/ipa/softisp.mojom > diff --git a/meson_options.txt b/meson_options.txt > index 20baacc4f..e829ebe60 100644 > --- a/meson_options.txt > +++ b/meson_options.txt > @@ -48,8 +48,8 @@ option('gstreamer', > > option('ipas', > type : 'array', > - choices : ['ipu3', 'mali-c55', 'rkisp1', 'rpi/pisp', 'rpi/vc4', 'simple', > - 'vimc'], > + choices : ['ipu3', 'mali-c55', 'rkisp1', 'rpi/pisp', 'rpi/vc4', > + 'softisp', 'vimc'], > description : 'Select which IPA modules to build') > > option('lc-compliance', > diff --git a/src/ipa/meson.build b/src/ipa/meson.build > index c583c7efd..ab4a1897f 100644 > --- a/src/ipa/meson.build > +++ b/src/ipa/meson.build > @@ -30,7 +30,7 @@ supported_ipas = { > 'rkisp1': 'rkisp1', > 'rpi/pisp': 'rpi/pisp', > 'rpi/vc4': 'rpi/vc4', > - 'simple': 'simple', > + 'simple': 'softisp', > 'vimc': 'vimc' > } > > diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/softisp/algorithms/adjust.cpp > similarity index 100% > rename from src/ipa/simple/algorithms/adjust.cpp > rename to src/ipa/softisp/algorithms/adjust.cpp > diff --git a/src/ipa/simple/algorithms/adjust.h b/src/ipa/softisp/algorithms/adjust.h > similarity index 100% > rename from src/ipa/simple/algorithms/adjust.h > rename to src/ipa/softisp/algorithms/adjust.h > diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/softisp/algorithms/agc.cpp > similarity index 100% > rename from src/ipa/simple/algorithms/agc.cpp > rename to src/ipa/softisp/algorithms/agc.cpp > diff --git a/src/ipa/simple/algorithms/agc.h b/src/ipa/softisp/algorithms/agc.h > similarity index 100% > rename from src/ipa/simple/algorithms/agc.h > rename to src/ipa/softisp/algorithms/agc.h > diff --git a/src/ipa/simple/algorithms/algorithm.h b/src/ipa/softisp/algorithms/algorithm.h > similarity index 100% > rename from src/ipa/simple/algorithms/algorithm.h > rename to src/ipa/softisp/algorithms/algorithm.h > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/softisp/algorithms/awb.cpp > similarity index 100% > rename from src/ipa/simple/algorithms/awb.cpp > rename to src/ipa/softisp/algorithms/awb.cpp > diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/softisp/algorithms/awb.h > similarity index 97% > rename from src/ipa/simple/algorithms/awb.h > rename to src/ipa/softisp/algorithms/awb.h > index ff0a7a28e..59ab6fd79 100644 > --- a/src/ipa/simple/algorithms/awb.h > +++ b/src/ipa/softisp/algorithms/awb.h > @@ -14,7 +14,7 @@ > > #include "libipa/awb.h" > #include "libipa/fixedpoint.h" > -#include "simple/ipa_context.h" > +#include "softisp/ipa_context.h" > > #include "algorithm.h" > > diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/softisp/algorithms/blc.cpp > similarity index 100% > rename from src/ipa/simple/algorithms/blc.cpp > rename to src/ipa/softisp/algorithms/blc.cpp > diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/softisp/algorithms/blc.h > similarity index 100% > rename from src/ipa/simple/algorithms/blc.h > rename to src/ipa/softisp/algorithms/blc.h > diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/softisp/algorithms/ccm.cpp > similarity index 100% > rename from src/ipa/simple/algorithms/ccm.cpp > rename to src/ipa/softisp/algorithms/ccm.cpp > diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/softisp/algorithms/ccm.h > similarity index 100% > rename from src/ipa/simple/algorithms/ccm.h > rename to src/ipa/softisp/algorithms/ccm.h > diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/softisp/algorithms/meson.build > similarity index 100% > rename from src/ipa/simple/algorithms/meson.build > rename to src/ipa/softisp/algorithms/meson.build > diff --git a/src/ipa/simple/data/meson.build b/src/ipa/softisp/data/meson.build > similarity index 79% > rename from src/ipa/simple/data/meson.build > rename to src/ipa/softisp/data/meson.build > index 92795ee4c..fd9aa0fab 100644 > --- a/src/ipa/simple/data/meson.build > +++ b/src/ipa/softisp/data/meson.build > @@ -6,5 +6,5 @@ conf_files = files([ > > # The install_dir must match the name from the IPAModuleInfo > install_data(conf_files, > - install_dir : ipa_data_dir / 'simple', > + install_dir : ipa_data_dir / 'softisp', > install_tag : 'runtime') > diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/softisp/data/uncalibrated.yaml > similarity index 100% > rename from src/ipa/simple/data/uncalibrated.yaml > rename to src/ipa/softisp/data/uncalibrated.yaml > diff --git a/src/ipa/simple/ipa_context.cpp b/src/ipa/softisp/ipa_context.cpp > similarity index 100% > rename from src/ipa/simple/ipa_context.cpp > rename to src/ipa/softisp/ipa_context.cpp > diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/softisp/ipa_context.h > similarity index 100% > rename from src/ipa/simple/ipa_context.h > rename to src/ipa/softisp/ipa_context.h > diff --git a/src/ipa/simple/meson.build b/src/ipa/softisp/meson.build > similarity index 100% > rename from src/ipa/simple/meson.build > rename to src/ipa/softisp/meson.build > diff --git a/src/ipa/simple/module.h b/src/ipa/softisp/module.h > similarity index 91% > rename from src/ipa/simple/module.h > rename to src/ipa/softisp/module.h > index 8d4d53fb2..b5d99ba08 100644 > --- a/src/ipa/simple/module.h > +++ b/src/ipa/softisp/module.h > @@ -9,7 +9,7 @@ > > #include <libcamera/controls.h> > > -#include <libcamera/ipa/soft_ipa_interface.h> > +#include <libcamera/ipa/softisp_ipa_interface.h> > > #include "libcamera/internal/software_isp/debayer_params.h" > #include "libcamera/internal/software_isp/swisp_stats.h" > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/softisp/soft_simple.cpp > similarity index 99% > rename from src/ipa/simple/soft_simple.cpp > rename to src/ipa/softisp/soft_simple.cpp > index d4ab91e30..e84feeea7 100644 > --- a/src/ipa/simple/soft_simple.cpp > +++ b/src/ipa/softisp/soft_simple.cpp > @@ -20,7 +20,7 @@ > > #include <libcamera/ipa/ipa_interface.h> > #include <libcamera/ipa/ipa_module_info.h> > -#include <libcamera/ipa/soft_ipa_interface.h> > +#include <libcamera/ipa/softisp_ipa_interface.h> > > #include "libcamera/internal/software_isp/debayer_params.h" > #include "libcamera/internal/software_isp/swisp_stats.h" > @@ -342,7 +342,7 @@ extern "C" { > const struct IPAModuleInfo ipaModuleInfo = { > IPA_MODULE_API_VERSION, > 0, > - "simple", > + "softisp", > }; > > IPAInterface *ipaCreate() > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 25fc11989..a803f6208 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -844,8 +844,10 @@ void PipelineHandler::disconnect() > * > * Create an IPA module using \a ipaName as the matching identifier. This > * overload allows pipeline handlers to create an IPA module by specifying its > - * name instead of relying on the fact that the IPA module matches the pipeline > - * handler's one. > + * name instead of relying on the fact that the IPA module matches the IPA > + * proxy's type name. > + * > + * \deprecated This method is deprecated and *MUST* not be used in new code. > * > * \return A newly created IPA proxy, or nullptr if no matching IPA module is > * found or if the IPA proxy fails to initialize > @@ -858,7 +860,7 @@ void PipelineHandler::disconnect() > * \param[in] minVersion Minimum acceptable version of IPA module > * \param[in] maxVersion Maximum acceptable version of IPA module > * > - * Create an IPA module using the pipeline handler name as the matching > + * Create an IPA module using the IPA proxy's type name as the matching > * identifier. This overload allows pipeline handler to create an IPA module > * whose name matches the pipeline handler one. > * > diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl > index d48b90dcf..259ed47c3 100644 > --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl > +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl > @@ -40,6 +40,8 @@ class {{proxy_name}}Isolated; > class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object > { > public: > + static constexpr const char *name() { return "{{module_name}}"; } > + > using Threaded = {{proxy_name}}Threaded; > using Isolated = {{proxy_name}}Isolated; >
Hi Barnabás, Thank you for all the reviews. On 17-Aug-26 21:08, Barnabás Pőcze wrote: > 2026. 08. 17. 15:40 keltezéssel, Hans de Goede írta: >> Currently createIPA() / IPAManager::module() assume that there is a 1:1 >> relationship between pipeline handlers and IPAs and IPA matching is done >> based on matching the pipe's name to ipaModuleInfo.name[]. >> >> This coupling between pipeline name and which IPA to create is undesirable >> and makes it impossible to for example use the softISP as a fallback in >> other pipeline handlers than the simple pipeline handler. >> >> The actual API contract between the IPA and the pipeline handler is given >> by the IPA proxy type. Add a static IPAProxyXXX::name() to the generated >> IPAProxyXXX classes and use this in PipelineHandler::createIPA() instead >> of using PipelineHandler::name(). >> >> This requires changing the name, as well as the subdirectory of the softISP >> IPA from "simple" to "softisp", which is the more sensible name anyway. >> >> Note: this means that softISP IPA config files will now be looked for under >> /usr/share/libcamera/ipa/softisp instead of /usr/share/libcamera/ipa/simple! >> >> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com> >> --- > > I feel like the renaming should be separated and possibly merged with the later patches 6 and 7. > Any thoughts? A minimal rename is necessary here to not leave the tree in a non-working state (important for git bisectability). I tried to do the minium necessary here, while at the same time balancing that with not needing to touch the same lines / rename the same files a second time. As for the further split of the remaining renaming work into 2 patches that results in much easier to review patches, so I plan to keep that split as is. > > In any case, it looks ok to me. > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Thx. Regards, Hans >> Changes in v8: >> - Fix CI failure caused by extra trailing ';' >> >> Changes in v7: >> - Rename the softISP IPA from "simple" to "softisp" instead of "soft" >> - Mark PipelineHandler::createIPA() overload with an explicit IPA name >> argument as deprecated >> >> Changes in v6: >> - New patch in v6 of this patch-series >> --- >> Documentation/Doxyfile-internal.in | 2 +- >> include/libcamera/internal/pipeline_handler.h | 2 +- >> include/libcamera/internal/software_isp/software_isp.h | 4 ++-- >> include/libcamera/ipa/meson.build | 2 +- >> include/libcamera/ipa/{soft.mojom => softisp.mojom} | 0 >> meson_options.txt | 4 ++-- >> src/ipa/meson.build | 2 +- >> src/ipa/{simple => softisp}/algorithms/adjust.cpp | 0 >> src/ipa/{simple => softisp}/algorithms/adjust.h | 0 >> src/ipa/{simple => softisp}/algorithms/agc.cpp | 0 >> src/ipa/{simple => softisp}/algorithms/agc.h | 0 >> src/ipa/{simple => softisp}/algorithms/algorithm.h | 0 >> src/ipa/{simple => softisp}/algorithms/awb.cpp | 0 >> src/ipa/{simple => softisp}/algorithms/awb.h | 2 +- >> src/ipa/{simple => softisp}/algorithms/blc.cpp | 0 >> src/ipa/{simple => softisp}/algorithms/blc.h | 0 >> src/ipa/{simple => softisp}/algorithms/ccm.cpp | 0 >> src/ipa/{simple => softisp}/algorithms/ccm.h | 0 >> src/ipa/{simple => softisp}/algorithms/meson.build | 0 >> src/ipa/{simple => softisp}/data/meson.build | 2 +- >> src/ipa/{simple => softisp}/data/uncalibrated.yaml | 0 >> src/ipa/{simple => softisp}/ipa_context.cpp | 0 >> src/ipa/{simple => softisp}/ipa_context.h | 0 >> src/ipa/{simple => softisp}/meson.build | 0 >> src/ipa/{simple => softisp}/module.h | 2 +- >> src/ipa/{simple => softisp}/soft_simple.cpp | 4 ++-- >> src/libcamera/pipeline_handler.cpp | 8 +++++--- >> .../libcamera_templates/module_ipa_proxy.h.tmpl | 2 ++ >> 28 files changed, 20 insertions(+), 16 deletions(-) >> rename include/libcamera/ipa/{soft.mojom => softisp.mojom} (100%) >> rename src/ipa/{simple => softisp}/algorithms/adjust.cpp (100%) >> rename src/ipa/{simple => softisp}/algorithms/adjust.h (100%) >> rename src/ipa/{simple => softisp}/algorithms/agc.cpp (100%) >> rename src/ipa/{simple => softisp}/algorithms/agc.h (100%) >> rename src/ipa/{simple => softisp}/algorithms/algorithm.h (100%) >> rename src/ipa/{simple => softisp}/algorithms/awb.cpp (100%) >> rename src/ipa/{simple => softisp}/algorithms/awb.h (97%) >> rename src/ipa/{simple => softisp}/algorithms/blc.cpp (100%) >> rename src/ipa/{simple => softisp}/algorithms/blc.h (100%) >> rename src/ipa/{simple => softisp}/algorithms/ccm.cpp (100%) >> rename src/ipa/{simple => softisp}/algorithms/ccm.h (100%) >> rename src/ipa/{simple => softisp}/algorithms/meson.build (100%) >> rename src/ipa/{simple => softisp}/data/meson.build (79%) >> rename src/ipa/{simple => softisp}/data/uncalibrated.yaml (100%) >> rename src/ipa/{simple => softisp}/ipa_context.cpp (100%) >> rename src/ipa/{simple => softisp}/ipa_context.h (100%) >> rename src/ipa/{simple => softisp}/meson.build (100%) >> rename src/ipa/{simple => softisp}/module.h (91%) >> rename src/ipa/{simple => softisp}/soft_simple.cpp (99%) >> >> diff --git a/Documentation/Doxyfile-internal.in b/Documentation/Doxyfile-internal.in >> index b64b6a6fe..70b7e7ef1 100644 >> --- a/Documentation/Doxyfile-internal.in >> +++ b/Documentation/Doxyfile-internal.in >> @@ -28,7 +28,7 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/base/span.h \ >> @TOP_SRCDIR@/src/libcamera/sensor/camera_sensor_raw.cpp \ >> @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \ >> @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \ >> - @TOP_BUILDDIR@/include/libcamera/ipa/soft_ipa_interface.h \ >> + @TOP_BUILDDIR@/include/libcamera/ipa/softisp_ipa_interface.h \ >> @TOP_BUILDDIR@/src/libcamera/proxy/ >> HTML_OUTPUT = internal-api >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h >> index b60c07b13..2a2ba95a4 100644 >> --- a/include/libcamera/internal/pipeline_handler.h >> +++ b/include/libcamera/internal/pipeline_handler.h >> @@ -76,7 +76,7 @@ public: >> std::unique_ptr<T> createIPA(uint32_t minVersion, uint32_t maxVersion) >> { >> IPAManager *ipaManager = manager_->_d()->ipaManager(); >> - return ipaManager->createIPA<T>(name_, minVersion, maxVersion); >> + return ipaManager->createIPA<T>(T::name(), minVersion, maxVersion); >> } >> template<typename T> >> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h >> index 4f72dce9b..3205a2e14 100644 >> --- a/include/libcamera/internal/software_isp/software_isp.h >> +++ b/include/libcamera/internal/software_isp/software_isp.h >> @@ -25,8 +25,8 @@ >> #include <libcamera/geometry.h> >> #include <libcamera/pixel_format.h> >> -#include <libcamera/ipa/soft_ipa_interface.h> >> -#include <libcamera/ipa/soft_ipa_proxy.h> >> +#include <libcamera/ipa/softisp_ipa_interface.h> >> +#include <libcamera/ipa/softisp_ipa_proxy.h> >> #include "libcamera/internal/camera_sensor.h" >> #include "libcamera/internal/dma_buf_allocator.h" >> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build >> index 3ee3ada30..fb518526a 100644 >> --- a/include/libcamera/ipa/meson.build >> +++ b/include/libcamera/ipa/meson.build >> @@ -68,7 +68,7 @@ pipeline_ipa_mojom_mapping = { >> 'rkisp1': 'rkisp1.mojom', >> 'rpi/pisp': 'raspberrypi.mojom', >> 'rpi/vc4': 'raspberrypi.mojom', >> - 'simple': 'soft.mojom', >> + 'simple': 'softisp.mojom', >> 'vimc': 'vimc.mojom', >> } >> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/softisp.mojom >> similarity index 100% >> rename from include/libcamera/ipa/soft.mojom >> rename to include/libcamera/ipa/softisp.mojom >> diff --git a/meson_options.txt b/meson_options.txt >> index 20baacc4f..e829ebe60 100644 >> --- a/meson_options.txt >> +++ b/meson_options.txt >> @@ -48,8 +48,8 @@ option('gstreamer', >> option('ipas', >> type : 'array', >> - choices : ['ipu3', 'mali-c55', 'rkisp1', 'rpi/pisp', 'rpi/vc4', 'simple', >> - 'vimc'], >> + choices : ['ipu3', 'mali-c55', 'rkisp1', 'rpi/pisp', 'rpi/vc4', >> + 'softisp', 'vimc'], >> description : 'Select which IPA modules to build') >> option('lc-compliance', >> diff --git a/src/ipa/meson.build b/src/ipa/meson.build >> index c583c7efd..ab4a1897f 100644 >> --- a/src/ipa/meson.build >> +++ b/src/ipa/meson.build >> @@ -30,7 +30,7 @@ supported_ipas = { >> 'rkisp1': 'rkisp1', >> 'rpi/pisp': 'rpi/pisp', >> 'rpi/vc4': 'rpi/vc4', >> - 'simple': 'simple', >> + 'simple': 'softisp', >> 'vimc': 'vimc' >> } >> diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/softisp/algorithms/adjust.cpp >> similarity index 100% >> rename from src/ipa/simple/algorithms/adjust.cpp >> rename to src/ipa/softisp/algorithms/adjust.cpp >> diff --git a/src/ipa/simple/algorithms/adjust.h b/src/ipa/softisp/algorithms/adjust.h >> similarity index 100% >> rename from src/ipa/simple/algorithms/adjust.h >> rename to src/ipa/softisp/algorithms/adjust.h >> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/softisp/algorithms/agc.cpp >> similarity index 100% >> rename from src/ipa/simple/algorithms/agc.cpp >> rename to src/ipa/softisp/algorithms/agc.cpp >> diff --git a/src/ipa/simple/algorithms/agc.h b/src/ipa/softisp/algorithms/agc.h >> similarity index 100% >> rename from src/ipa/simple/algorithms/agc.h >> rename to src/ipa/softisp/algorithms/agc.h >> diff --git a/src/ipa/simple/algorithms/algorithm.h b/src/ipa/softisp/algorithms/algorithm.h >> similarity index 100% >> rename from src/ipa/simple/algorithms/algorithm.h >> rename to src/ipa/softisp/algorithms/algorithm.h >> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/softisp/algorithms/awb.cpp >> similarity index 100% >> rename from src/ipa/simple/algorithms/awb.cpp >> rename to src/ipa/softisp/algorithms/awb.cpp >> diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/softisp/algorithms/awb.h >> similarity index 97% >> rename from src/ipa/simple/algorithms/awb.h >> rename to src/ipa/softisp/algorithms/awb.h >> index ff0a7a28e..59ab6fd79 100644 >> --- a/src/ipa/simple/algorithms/awb.h >> +++ b/src/ipa/softisp/algorithms/awb.h >> @@ -14,7 +14,7 @@ >> #include "libipa/awb.h" >> #include "libipa/fixedpoint.h" >> -#include "simple/ipa_context.h" >> +#include "softisp/ipa_context.h" >> #include "algorithm.h" >> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/softisp/algorithms/blc.cpp >> similarity index 100% >> rename from src/ipa/simple/algorithms/blc.cpp >> rename to src/ipa/softisp/algorithms/blc.cpp >> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/softisp/algorithms/blc.h >> similarity index 100% >> rename from src/ipa/simple/algorithms/blc.h >> rename to src/ipa/softisp/algorithms/blc.h >> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/softisp/algorithms/ccm.cpp >> similarity index 100% >> rename from src/ipa/simple/algorithms/ccm.cpp >> rename to src/ipa/softisp/algorithms/ccm.cpp >> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/softisp/algorithms/ccm.h >> similarity index 100% >> rename from src/ipa/simple/algorithms/ccm.h >> rename to src/ipa/softisp/algorithms/ccm.h >> diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/softisp/algorithms/meson.build >> similarity index 100% >> rename from src/ipa/simple/algorithms/meson.build >> rename to src/ipa/softisp/algorithms/meson.build >> diff --git a/src/ipa/simple/data/meson.build b/src/ipa/softisp/data/meson.build >> similarity index 79% >> rename from src/ipa/simple/data/meson.build >> rename to src/ipa/softisp/data/meson.build >> index 92795ee4c..fd9aa0fab 100644 >> --- a/src/ipa/simple/data/meson.build >> +++ b/src/ipa/softisp/data/meson.build >> @@ -6,5 +6,5 @@ conf_files = files([ >> # The install_dir must match the name from the IPAModuleInfo >> install_data(conf_files, >> - install_dir : ipa_data_dir / 'simple', >> + install_dir : ipa_data_dir / 'softisp', >> install_tag : 'runtime') >> diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/softisp/data/uncalibrated.yaml >> similarity index 100% >> rename from src/ipa/simple/data/uncalibrated.yaml >> rename to src/ipa/softisp/data/uncalibrated.yaml >> diff --git a/src/ipa/simple/ipa_context.cpp b/src/ipa/softisp/ipa_context.cpp >> similarity index 100% >> rename from src/ipa/simple/ipa_context.cpp >> rename to src/ipa/softisp/ipa_context.cpp >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/softisp/ipa_context.h >> similarity index 100% >> rename from src/ipa/simple/ipa_context.h >> rename to src/ipa/softisp/ipa_context.h >> diff --git a/src/ipa/simple/meson.build b/src/ipa/softisp/meson.build >> similarity index 100% >> rename from src/ipa/simple/meson.build >> rename to src/ipa/softisp/meson.build >> diff --git a/src/ipa/simple/module.h b/src/ipa/softisp/module.h >> similarity index 91% >> rename from src/ipa/simple/module.h >> rename to src/ipa/softisp/module.h >> index 8d4d53fb2..b5d99ba08 100644 >> --- a/src/ipa/simple/module.h >> +++ b/src/ipa/softisp/module.h >> @@ -9,7 +9,7 @@ >> #include <libcamera/controls.h> >> -#include <libcamera/ipa/soft_ipa_interface.h> >> +#include <libcamera/ipa/softisp_ipa_interface.h> >> #include "libcamera/internal/software_isp/debayer_params.h" >> #include "libcamera/internal/software_isp/swisp_stats.h" >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/softisp/soft_simple.cpp >> similarity index 99% >> rename from src/ipa/simple/soft_simple.cpp >> rename to src/ipa/softisp/soft_simple.cpp >> index d4ab91e30..e84feeea7 100644 >> --- a/src/ipa/simple/soft_simple.cpp >> +++ b/src/ipa/softisp/soft_simple.cpp >> @@ -20,7 +20,7 @@ >> #include <libcamera/ipa/ipa_interface.h> >> #include <libcamera/ipa/ipa_module_info.h> >> -#include <libcamera/ipa/soft_ipa_interface.h> >> +#include <libcamera/ipa/softisp_ipa_interface.h> >> #include "libcamera/internal/software_isp/debayer_params.h" >> #include "libcamera/internal/software_isp/swisp_stats.h" >> @@ -342,7 +342,7 @@ extern "C" { >> const struct IPAModuleInfo ipaModuleInfo = { >> IPA_MODULE_API_VERSION, >> 0, >> - "simple", >> + "softisp", >> }; >> IPAInterface *ipaCreate() >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp >> index 25fc11989..a803f6208 100644 >> --- a/src/libcamera/pipeline_handler.cpp >> +++ b/src/libcamera/pipeline_handler.cpp >> @@ -844,8 +844,10 @@ void PipelineHandler::disconnect() >> * >> * Create an IPA module using \a ipaName as the matching identifier. This >> * overload allows pipeline handlers to create an IPA module by specifying its >> - * name instead of relying on the fact that the IPA module matches the pipeline >> - * handler's one. >> + * name instead of relying on the fact that the IPA module matches the IPA >> + * proxy's type name. >> + * >> + * \deprecated This method is deprecated and *MUST* not be used in new code. >> * >> * \return A newly created IPA proxy, or nullptr if no matching IPA module is >> * found or if the IPA proxy fails to initialize >> @@ -858,7 +860,7 @@ void PipelineHandler::disconnect() >> * \param[in] minVersion Minimum acceptable version of IPA module >> * \param[in] maxVersion Maximum acceptable version of IPA module >> * >> - * Create an IPA module using the pipeline handler name as the matching >> + * Create an IPA module using the IPA proxy's type name as the matching >> * identifier. This overload allows pipeline handler to create an IPA module >> * whose name matches the pipeline handler one. >> * >> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl >> index d48b90dcf..259ed47c3 100644 >> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl >> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl >> @@ -40,6 +40,8 @@ class {{proxy_name}}Isolated; >> class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object >> { >> public: >> + static constexpr const char *name() { return "{{module_name}}"; } >> + >> using Threaded = {{proxy_name}}Threaded; >> using Isolated = {{proxy_name}}Isolated; >> >
2026. 08. 18. 10:35 keltezéssel, Hans de Goede írta: > Hi Barnabás, > > Thank you for all the reviews. > > On 17-Aug-26 21:08, Barnabás Pőcze wrote: >> 2026. 08. 17. 15:40 keltezéssel, Hans de Goede írta: >>> Currently createIPA() / IPAManager::module() assume that there is a 1:1 >>> relationship between pipeline handlers and IPAs and IPA matching is done >>> based on matching the pipe's name to ipaModuleInfo.name[]. >>> >>> This coupling between pipeline name and which IPA to create is undesirable >>> and makes it impossible to for example use the softISP as a fallback in >>> other pipeline handlers than the simple pipeline handler. >>> >>> The actual API contract between the IPA and the pipeline handler is given >>> by the IPA proxy type. Add a static IPAProxyXXX::name() to the generated >>> IPAProxyXXX classes and use this in PipelineHandler::createIPA() instead >>> of using PipelineHandler::name(). >>> >>> This requires changing the name, as well as the subdirectory of the softISP >>> IPA from "simple" to "softisp", which is the more sensible name anyway. >>> >>> Note: this means that softISP IPA config files will now be looked for under >>> /usr/share/libcamera/ipa/softisp instead of /usr/share/libcamera/ipa/simple! >>> >>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com> >>> --- >> >> I feel like the renaming should be separated and possibly merged with the later patches 6 and 7. >> Any thoughts? > > A minimal rename is necessary here to not leave the tree in a non-working > state (important for git bisectability). I see. Wouldn't forcing the name (temporarily) be an option here as well, like in the case of rpi? > > I tried to do the minium necessary here, while at the same time balancing > that with not needing to touch the same lines / rename the same files > a second time. > > As for the further split of the remaining renaming work into 2 > patches that results in much easier to review patches, so I plan > to keep that split as is. > >> >> In any case, it looks ok to me. >> >> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > Thx. > > Regards, > > Hans > > > >>> Changes in v8: >>> - Fix CI failure caused by extra trailing ';' >>> >>> Changes in v7: >>> - Rename the softISP IPA from "simple" to "softisp" instead of "soft" >>> - Mark PipelineHandler::createIPA() overload with an explicit IPA name >>> argument as deprecated >>> >>> Changes in v6: >>> - New patch in v6 of this patch-series >>> --- >>> Documentation/Doxyfile-internal.in | 2 +- >>> include/libcamera/internal/pipeline_handler.h | 2 +- >>> include/libcamera/internal/software_isp/software_isp.h | 4 ++-- >>> include/libcamera/ipa/meson.build | 2 +- >>> include/libcamera/ipa/{soft.mojom => softisp.mojom} | 0 >>> meson_options.txt | 4 ++-- >>> src/ipa/meson.build | 2 +- >>> src/ipa/{simple => softisp}/algorithms/adjust.cpp | 0 >>> src/ipa/{simple => softisp}/algorithms/adjust.h | 0 >>> src/ipa/{simple => softisp}/algorithms/agc.cpp | 0 >>> src/ipa/{simple => softisp}/algorithms/agc.h | 0 >>> src/ipa/{simple => softisp}/algorithms/algorithm.h | 0 >>> src/ipa/{simple => softisp}/algorithms/awb.cpp | 0 >>> src/ipa/{simple => softisp}/algorithms/awb.h | 2 +- >>> src/ipa/{simple => softisp}/algorithms/blc.cpp | 0 >>> src/ipa/{simple => softisp}/algorithms/blc.h | 0 >>> src/ipa/{simple => softisp}/algorithms/ccm.cpp | 0 >>> src/ipa/{simple => softisp}/algorithms/ccm.h | 0 >>> src/ipa/{simple => softisp}/algorithms/meson.build | 0 >>> src/ipa/{simple => softisp}/data/meson.build | 2 +- >>> src/ipa/{simple => softisp}/data/uncalibrated.yaml | 0 >>> src/ipa/{simple => softisp}/ipa_context.cpp | 0 >>> src/ipa/{simple => softisp}/ipa_context.h | 0 >>> src/ipa/{simple => softisp}/meson.build | 0 >>> src/ipa/{simple => softisp}/module.h | 2 +- >>> src/ipa/{simple => softisp}/soft_simple.cpp | 4 ++-- >>> src/libcamera/pipeline_handler.cpp | 8 +++++--- >>> .../libcamera_templates/module_ipa_proxy.h.tmpl | 2 ++ >>> 28 files changed, 20 insertions(+), 16 deletions(-) >>> rename include/libcamera/ipa/{soft.mojom => softisp.mojom} (100%) >>> rename src/ipa/{simple => softisp}/algorithms/adjust.cpp (100%) >>> rename src/ipa/{simple => softisp}/algorithms/adjust.h (100%) >>> rename src/ipa/{simple => softisp}/algorithms/agc.cpp (100%) >>> rename src/ipa/{simple => softisp}/algorithms/agc.h (100%) >>> rename src/ipa/{simple => softisp}/algorithms/algorithm.h (100%) >>> rename src/ipa/{simple => softisp}/algorithms/awb.cpp (100%) >>> rename src/ipa/{simple => softisp}/algorithms/awb.h (97%) >>> rename src/ipa/{simple => softisp}/algorithms/blc.cpp (100%) >>> rename src/ipa/{simple => softisp}/algorithms/blc.h (100%) >>> rename src/ipa/{simple => softisp}/algorithms/ccm.cpp (100%) >>> rename src/ipa/{simple => softisp}/algorithms/ccm.h (100%) >>> rename src/ipa/{simple => softisp}/algorithms/meson.build (100%) >>> rename src/ipa/{simple => softisp}/data/meson.build (79%) >>> rename src/ipa/{simple => softisp}/data/uncalibrated.yaml (100%) >>> rename src/ipa/{simple => softisp}/ipa_context.cpp (100%) >>> rename src/ipa/{simple => softisp}/ipa_context.h (100%) >>> rename src/ipa/{simple => softisp}/meson.build (100%) >>> rename src/ipa/{simple => softisp}/module.h (91%) >>> rename src/ipa/{simple => softisp}/soft_simple.cpp (99%) >>> >>> diff --git a/Documentation/Doxyfile-internal.in b/Documentation/Doxyfile-internal.in >>> index b64b6a6fe..70b7e7ef1 100644 >>> --- a/Documentation/Doxyfile-internal.in >>> +++ b/Documentation/Doxyfile-internal.in >>> @@ -28,7 +28,7 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/base/span.h \ >>> @TOP_SRCDIR@/src/libcamera/sensor/camera_sensor_raw.cpp \ >>> @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \ >>> @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \ >>> - @TOP_BUILDDIR@/include/libcamera/ipa/soft_ipa_interface.h \ >>> + @TOP_BUILDDIR@/include/libcamera/ipa/softisp_ipa_interface.h \ >>> @TOP_BUILDDIR@/src/libcamera/proxy/ >>> HTML_OUTPUT = internal-api >>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h >>> index b60c07b13..2a2ba95a4 100644 >>> --- a/include/libcamera/internal/pipeline_handler.h >>> +++ b/include/libcamera/internal/pipeline_handler.h >>> @@ -76,7 +76,7 @@ public: >>> std::unique_ptr<T> createIPA(uint32_t minVersion, uint32_t maxVersion) >>> { >>> IPAManager *ipaManager = manager_->_d()->ipaManager(); >>> - return ipaManager->createIPA<T>(name_, minVersion, maxVersion); >>> + return ipaManager->createIPA<T>(T::name(), minVersion, maxVersion); >>> } >>> template<typename T> >>> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h >>> index 4f72dce9b..3205a2e14 100644 >>> --- a/include/libcamera/internal/software_isp/software_isp.h >>> +++ b/include/libcamera/internal/software_isp/software_isp.h >>> @@ -25,8 +25,8 @@ >>> #include <libcamera/geometry.h> >>> #include <libcamera/pixel_format.h> >>> -#include <libcamera/ipa/soft_ipa_interface.h> >>> -#include <libcamera/ipa/soft_ipa_proxy.h> >>> +#include <libcamera/ipa/softisp_ipa_interface.h> >>> +#include <libcamera/ipa/softisp_ipa_proxy.h> >>> #include "libcamera/internal/camera_sensor.h" >>> #include "libcamera/internal/dma_buf_allocator.h" >>> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build >>> index 3ee3ada30..fb518526a 100644 >>> --- a/include/libcamera/ipa/meson.build >>> +++ b/include/libcamera/ipa/meson.build >>> @@ -68,7 +68,7 @@ pipeline_ipa_mojom_mapping = { >>> 'rkisp1': 'rkisp1.mojom', >>> 'rpi/pisp': 'raspberrypi.mojom', >>> 'rpi/vc4': 'raspberrypi.mojom', >>> - 'simple': 'soft.mojom', >>> + 'simple': 'softisp.mojom', >>> 'vimc': 'vimc.mojom', >>> } >>> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/softisp.mojom >>> similarity index 100% >>> rename from include/libcamera/ipa/soft.mojom >>> rename to include/libcamera/ipa/softisp.mojom >>> diff --git a/meson_options.txt b/meson_options.txt >>> index 20baacc4f..e829ebe60 100644 >>> --- a/meson_options.txt >>> +++ b/meson_options.txt >>> @@ -48,8 +48,8 @@ option('gstreamer', >>> option('ipas', >>> type : 'array', >>> - choices : ['ipu3', 'mali-c55', 'rkisp1', 'rpi/pisp', 'rpi/vc4', 'simple', >>> - 'vimc'], >>> + choices : ['ipu3', 'mali-c55', 'rkisp1', 'rpi/pisp', 'rpi/vc4', >>> + 'softisp', 'vimc'], >>> description : 'Select which IPA modules to build') >>> option('lc-compliance', >>> diff --git a/src/ipa/meson.build b/src/ipa/meson.build >>> index c583c7efd..ab4a1897f 100644 >>> --- a/src/ipa/meson.build >>> +++ b/src/ipa/meson.build >>> @@ -30,7 +30,7 @@ supported_ipas = { >>> 'rkisp1': 'rkisp1', >>> 'rpi/pisp': 'rpi/pisp', >>> 'rpi/vc4': 'rpi/vc4', >>> - 'simple': 'simple', >>> + 'simple': 'softisp', >>> 'vimc': 'vimc' >>> } >>> diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/softisp/algorithms/adjust.cpp >>> similarity index 100% >>> rename from src/ipa/simple/algorithms/adjust.cpp >>> rename to src/ipa/softisp/algorithms/adjust.cpp >>> diff --git a/src/ipa/simple/algorithms/adjust.h b/src/ipa/softisp/algorithms/adjust.h >>> similarity index 100% >>> rename from src/ipa/simple/algorithms/adjust.h >>> rename to src/ipa/softisp/algorithms/adjust.h >>> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/softisp/algorithms/agc.cpp >>> similarity index 100% >>> rename from src/ipa/simple/algorithms/agc.cpp >>> rename to src/ipa/softisp/algorithms/agc.cpp >>> diff --git a/src/ipa/simple/algorithms/agc.h b/src/ipa/softisp/algorithms/agc.h >>> similarity index 100% >>> rename from src/ipa/simple/algorithms/agc.h >>> rename to src/ipa/softisp/algorithms/agc.h >>> diff --git a/src/ipa/simple/algorithms/algorithm.h b/src/ipa/softisp/algorithms/algorithm.h >>> similarity index 100% >>> rename from src/ipa/simple/algorithms/algorithm.h >>> rename to src/ipa/softisp/algorithms/algorithm.h >>> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/softisp/algorithms/awb.cpp >>> similarity index 100% >>> rename from src/ipa/simple/algorithms/awb.cpp >>> rename to src/ipa/softisp/algorithms/awb.cpp >>> diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/softisp/algorithms/awb.h >>> similarity index 97% >>> rename from src/ipa/simple/algorithms/awb.h >>> rename to src/ipa/softisp/algorithms/awb.h >>> index ff0a7a28e..59ab6fd79 100644 >>> --- a/src/ipa/simple/algorithms/awb.h >>> +++ b/src/ipa/softisp/algorithms/awb.h >>> @@ -14,7 +14,7 @@ >>> #include "libipa/awb.h" >>> #include "libipa/fixedpoint.h" >>> -#include "simple/ipa_context.h" >>> +#include "softisp/ipa_context.h" >>> #include "algorithm.h" >>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/softisp/algorithms/blc.cpp >>> similarity index 100% >>> rename from src/ipa/simple/algorithms/blc.cpp >>> rename to src/ipa/softisp/algorithms/blc.cpp >>> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/softisp/algorithms/blc.h >>> similarity index 100% >>> rename from src/ipa/simple/algorithms/blc.h >>> rename to src/ipa/softisp/algorithms/blc.h >>> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/softisp/algorithms/ccm.cpp >>> similarity index 100% >>> rename from src/ipa/simple/algorithms/ccm.cpp >>> rename to src/ipa/softisp/algorithms/ccm.cpp >>> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/softisp/algorithms/ccm.h >>> similarity index 100% >>> rename from src/ipa/simple/algorithms/ccm.h >>> rename to src/ipa/softisp/algorithms/ccm.h >>> diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/softisp/algorithms/meson.build >>> similarity index 100% >>> rename from src/ipa/simple/algorithms/meson.build >>> rename to src/ipa/softisp/algorithms/meson.build >>> diff --git a/src/ipa/simple/data/meson.build b/src/ipa/softisp/data/meson.build >>> similarity index 79% >>> rename from src/ipa/simple/data/meson.build >>> rename to src/ipa/softisp/data/meson.build >>> index 92795ee4c..fd9aa0fab 100644 >>> --- a/src/ipa/simple/data/meson.build >>> +++ b/src/ipa/softisp/data/meson.build >>> @@ -6,5 +6,5 @@ conf_files = files([ >>> # The install_dir must match the name from the IPAModuleInfo >>> install_data(conf_files, >>> - install_dir : ipa_data_dir / 'simple', >>> + install_dir : ipa_data_dir / 'softisp', >>> install_tag : 'runtime') >>> diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/softisp/data/uncalibrated.yaml >>> similarity index 100% >>> rename from src/ipa/simple/data/uncalibrated.yaml >>> rename to src/ipa/softisp/data/uncalibrated.yaml >>> diff --git a/src/ipa/simple/ipa_context.cpp b/src/ipa/softisp/ipa_context.cpp >>> similarity index 100% >>> rename from src/ipa/simple/ipa_context.cpp >>> rename to src/ipa/softisp/ipa_context.cpp >>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/softisp/ipa_context.h >>> similarity index 100% >>> rename from src/ipa/simple/ipa_context.h >>> rename to src/ipa/softisp/ipa_context.h >>> diff --git a/src/ipa/simple/meson.build b/src/ipa/softisp/meson.build >>> similarity index 100% >>> rename from src/ipa/simple/meson.build >>> rename to src/ipa/softisp/meson.build >>> diff --git a/src/ipa/simple/module.h b/src/ipa/softisp/module.h >>> similarity index 91% >>> rename from src/ipa/simple/module.h >>> rename to src/ipa/softisp/module.h >>> index 8d4d53fb2..b5d99ba08 100644 >>> --- a/src/ipa/simple/module.h >>> +++ b/src/ipa/softisp/module.h >>> @@ -9,7 +9,7 @@ >>> #include <libcamera/controls.h> >>> -#include <libcamera/ipa/soft_ipa_interface.h> >>> +#include <libcamera/ipa/softisp_ipa_interface.h> >>> #include "libcamera/internal/software_isp/debayer_params.h" >>> #include "libcamera/internal/software_isp/swisp_stats.h" >>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/softisp/soft_simple.cpp >>> similarity index 99% >>> rename from src/ipa/simple/soft_simple.cpp >>> rename to src/ipa/softisp/soft_simple.cpp >>> index d4ab91e30..e84feeea7 100644 >>> --- a/src/ipa/simple/soft_simple.cpp >>> +++ b/src/ipa/softisp/soft_simple.cpp >>> @@ -20,7 +20,7 @@ >>> #include <libcamera/ipa/ipa_interface.h> >>> #include <libcamera/ipa/ipa_module_info.h> >>> -#include <libcamera/ipa/soft_ipa_interface.h> >>> +#include <libcamera/ipa/softisp_ipa_interface.h> >>> #include "libcamera/internal/software_isp/debayer_params.h" >>> #include "libcamera/internal/software_isp/swisp_stats.h" >>> @@ -342,7 +342,7 @@ extern "C" { >>> const struct IPAModuleInfo ipaModuleInfo = { >>> IPA_MODULE_API_VERSION, >>> 0, >>> - "simple", >>> + "softisp", >>> }; >>> IPAInterface *ipaCreate() >>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp >>> index 25fc11989..a803f6208 100644 >>> --- a/src/libcamera/pipeline_handler.cpp >>> +++ b/src/libcamera/pipeline_handler.cpp >>> @@ -844,8 +844,10 @@ void PipelineHandler::disconnect() >>> * >>> * Create an IPA module using \a ipaName as the matching identifier. This >>> * overload allows pipeline handlers to create an IPA module by specifying its >>> - * name instead of relying on the fact that the IPA module matches the pipeline >>> - * handler's one. >>> + * name instead of relying on the fact that the IPA module matches the IPA >>> + * proxy's type name. >>> + * >>> + * \deprecated This method is deprecated and *MUST* not be used in new code. >>> * >>> * \return A newly created IPA proxy, or nullptr if no matching IPA module is >>> * found or if the IPA proxy fails to initialize >>> @@ -858,7 +860,7 @@ void PipelineHandler::disconnect() >>> * \param[in] minVersion Minimum acceptable version of IPA module >>> * \param[in] maxVersion Maximum acceptable version of IPA module >>> * >>> - * Create an IPA module using the pipeline handler name as the matching >>> + * Create an IPA module using the IPA proxy's type name as the matching >>> * identifier. This overload allows pipeline handler to create an IPA module >>> * whose name matches the pipeline handler one. >>> * >>> diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl >>> index d48b90dcf..259ed47c3 100644 >>> --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl >>> +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl >>> @@ -40,6 +40,8 @@ class {{proxy_name}}Isolated; >>> class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object >>> { >>> public: >>> + static constexpr const char *name() { return "{{module_name}}"; } >>> + >>> using Threaded = {{proxy_name}}Threaded; >>> using Isolated = {{proxy_name}}Isolated; >>> >> >
Hi Barnabás, On 18-Aug-26 10:39, Barnabás Pőcze wrote: > 2026. 08. 18. 10:35 keltezéssel, Hans de Goede írta: >> Hi Barnabás, >> >> Thank you for all the reviews. >> >> On 17-Aug-26 21:08, Barnabás Pőcze wrote: >>> 2026. 08. 17. 15:40 keltezéssel, Hans de Goede írta: >>>> Currently createIPA() / IPAManager::module() assume that there is a 1:1 >>>> relationship between pipeline handlers and IPAs and IPA matching is done >>>> based on matching the pipe's name to ipaModuleInfo.name[]. >>>> >>>> This coupling between pipeline name and which IPA to create is undesirable >>>> and makes it impossible to for example use the softISP as a fallback in >>>> other pipeline handlers than the simple pipeline handler. >>>> >>>> The actual API contract between the IPA and the pipeline handler is given >>>> by the IPA proxy type. Add a static IPAProxyXXX::name() to the generated >>>> IPAProxyXXX classes and use this in PipelineHandler::createIPA() instead >>>> of using PipelineHandler::name(). >>>> >>>> This requires changing the name, as well as the subdirectory of the softISP >>>> IPA from "simple" to "softisp", which is the more sensible name anyway. >>>> >>>> Note: this means that softISP IPA config files will now be looked for under >>>> /usr/share/libcamera/ipa/softisp instead of /usr/share/libcamera/ipa/simple! >>>> >>>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>>> Signed-off-by: Hans de Goede <johannes.goede@oss.qualcomm.com> >>>> --- >>> >>> I feel like the renaming should be separated and possibly merged with the later patches 6 and 7. >>> Any thoughts? >> >> A minimal rename is necessary here to not leave the tree in a non-working >> state (important for git bisectability). > > I see. Wouldn't forcing the name (temporarily) be an option here as well, like in the case of rpi? That would result in changing the createIPA() call only to change it right back in the next patch, I'm not a fan of that. Also this series has been very long in the making, so I would like to get it upstream rather then have more "churn" in it. Note the resulting code after applying the whole series will look exactly the same either way. So I'm going to do a v9 addressing your comment about to no longer needed #include in patch 1/8 and leave the rest as is. Regards, Hans
diff --git a/Documentation/Doxyfile-internal.in b/Documentation/Doxyfile-internal.in index b64b6a6fe..70b7e7ef1 100644 --- a/Documentation/Doxyfile-internal.in +++ b/Documentation/Doxyfile-internal.in @@ -28,7 +28,7 @@ EXCLUDE = @TOP_SRCDIR@/include/libcamera/base/span.h \ @TOP_SRCDIR@/src/libcamera/sensor/camera_sensor_raw.cpp \ @TOP_SRCDIR@/src/libcamera/tracepoints.cpp \ @TOP_BUILDDIR@/include/libcamera/internal/tracepoints.h \ - @TOP_BUILDDIR@/include/libcamera/ipa/soft_ipa_interface.h \ + @TOP_BUILDDIR@/include/libcamera/ipa/softisp_ipa_interface.h \ @TOP_BUILDDIR@/src/libcamera/proxy/ HTML_OUTPUT = internal-api diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h index b60c07b13..2a2ba95a4 100644 --- a/include/libcamera/internal/pipeline_handler.h +++ b/include/libcamera/internal/pipeline_handler.h @@ -76,7 +76,7 @@ public: std::unique_ptr<T> createIPA(uint32_t minVersion, uint32_t maxVersion) { IPAManager *ipaManager = manager_->_d()->ipaManager(); - return ipaManager->createIPA<T>(name_, minVersion, maxVersion); + return ipaManager->createIPA<T>(T::name(), minVersion, maxVersion); } template<typename T> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h index 4f72dce9b..3205a2e14 100644 --- a/include/libcamera/internal/software_isp/software_isp.h +++ b/include/libcamera/internal/software_isp/software_isp.h @@ -25,8 +25,8 @@ #include <libcamera/geometry.h> #include <libcamera/pixel_format.h> -#include <libcamera/ipa/soft_ipa_interface.h> -#include <libcamera/ipa/soft_ipa_proxy.h> +#include <libcamera/ipa/softisp_ipa_interface.h> +#include <libcamera/ipa/softisp_ipa_proxy.h> #include "libcamera/internal/camera_sensor.h" #include "libcamera/internal/dma_buf_allocator.h" diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build index 3ee3ada30..fb518526a 100644 --- a/include/libcamera/ipa/meson.build +++ b/include/libcamera/ipa/meson.build @@ -68,7 +68,7 @@ pipeline_ipa_mojom_mapping = { 'rkisp1': 'rkisp1.mojom', 'rpi/pisp': 'raspberrypi.mojom', 'rpi/vc4': 'raspberrypi.mojom', - 'simple': 'soft.mojom', + 'simple': 'softisp.mojom', 'vimc': 'vimc.mojom', } diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/softisp.mojom similarity index 100% rename from include/libcamera/ipa/soft.mojom rename to include/libcamera/ipa/softisp.mojom diff --git a/meson_options.txt b/meson_options.txt index 20baacc4f..e829ebe60 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -48,8 +48,8 @@ option('gstreamer', option('ipas', type : 'array', - choices : ['ipu3', 'mali-c55', 'rkisp1', 'rpi/pisp', 'rpi/vc4', 'simple', - 'vimc'], + choices : ['ipu3', 'mali-c55', 'rkisp1', 'rpi/pisp', 'rpi/vc4', + 'softisp', 'vimc'], description : 'Select which IPA modules to build') option('lc-compliance', diff --git a/src/ipa/meson.build b/src/ipa/meson.build index c583c7efd..ab4a1897f 100644 --- a/src/ipa/meson.build +++ b/src/ipa/meson.build @@ -30,7 +30,7 @@ supported_ipas = { 'rkisp1': 'rkisp1', 'rpi/pisp': 'rpi/pisp', 'rpi/vc4': 'rpi/vc4', - 'simple': 'simple', + 'simple': 'softisp', 'vimc': 'vimc' } diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/softisp/algorithms/adjust.cpp similarity index 100% rename from src/ipa/simple/algorithms/adjust.cpp rename to src/ipa/softisp/algorithms/adjust.cpp diff --git a/src/ipa/simple/algorithms/adjust.h b/src/ipa/softisp/algorithms/adjust.h similarity index 100% rename from src/ipa/simple/algorithms/adjust.h rename to src/ipa/softisp/algorithms/adjust.h diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/softisp/algorithms/agc.cpp similarity index 100% rename from src/ipa/simple/algorithms/agc.cpp rename to src/ipa/softisp/algorithms/agc.cpp diff --git a/src/ipa/simple/algorithms/agc.h b/src/ipa/softisp/algorithms/agc.h similarity index 100% rename from src/ipa/simple/algorithms/agc.h rename to src/ipa/softisp/algorithms/agc.h diff --git a/src/ipa/simple/algorithms/algorithm.h b/src/ipa/softisp/algorithms/algorithm.h similarity index 100% rename from src/ipa/simple/algorithms/algorithm.h rename to src/ipa/softisp/algorithms/algorithm.h diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/softisp/algorithms/awb.cpp similarity index 100% rename from src/ipa/simple/algorithms/awb.cpp rename to src/ipa/softisp/algorithms/awb.cpp diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/softisp/algorithms/awb.h similarity index 97% rename from src/ipa/simple/algorithms/awb.h rename to src/ipa/softisp/algorithms/awb.h index ff0a7a28e..59ab6fd79 100644 --- a/src/ipa/simple/algorithms/awb.h +++ b/src/ipa/softisp/algorithms/awb.h @@ -14,7 +14,7 @@ #include "libipa/awb.h" #include "libipa/fixedpoint.h" -#include "simple/ipa_context.h" +#include "softisp/ipa_context.h" #include "algorithm.h" diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/softisp/algorithms/blc.cpp similarity index 100% rename from src/ipa/simple/algorithms/blc.cpp rename to src/ipa/softisp/algorithms/blc.cpp diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/softisp/algorithms/blc.h similarity index 100% rename from src/ipa/simple/algorithms/blc.h rename to src/ipa/softisp/algorithms/blc.h diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/softisp/algorithms/ccm.cpp similarity index 100% rename from src/ipa/simple/algorithms/ccm.cpp rename to src/ipa/softisp/algorithms/ccm.cpp diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/softisp/algorithms/ccm.h similarity index 100% rename from src/ipa/simple/algorithms/ccm.h rename to src/ipa/softisp/algorithms/ccm.h diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/softisp/algorithms/meson.build similarity index 100% rename from src/ipa/simple/algorithms/meson.build rename to src/ipa/softisp/algorithms/meson.build diff --git a/src/ipa/simple/data/meson.build b/src/ipa/softisp/data/meson.build similarity index 79% rename from src/ipa/simple/data/meson.build rename to src/ipa/softisp/data/meson.build index 92795ee4c..fd9aa0fab 100644 --- a/src/ipa/simple/data/meson.build +++ b/src/ipa/softisp/data/meson.build @@ -6,5 +6,5 @@ conf_files = files([ # The install_dir must match the name from the IPAModuleInfo install_data(conf_files, - install_dir : ipa_data_dir / 'simple', + install_dir : ipa_data_dir / 'softisp', install_tag : 'runtime') diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/softisp/data/uncalibrated.yaml similarity index 100% rename from src/ipa/simple/data/uncalibrated.yaml rename to src/ipa/softisp/data/uncalibrated.yaml diff --git a/src/ipa/simple/ipa_context.cpp b/src/ipa/softisp/ipa_context.cpp similarity index 100% rename from src/ipa/simple/ipa_context.cpp rename to src/ipa/softisp/ipa_context.cpp diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/softisp/ipa_context.h similarity index 100% rename from src/ipa/simple/ipa_context.h rename to src/ipa/softisp/ipa_context.h diff --git a/src/ipa/simple/meson.build b/src/ipa/softisp/meson.build similarity index 100% rename from src/ipa/simple/meson.build rename to src/ipa/softisp/meson.build diff --git a/src/ipa/simple/module.h b/src/ipa/softisp/module.h similarity index 91% rename from src/ipa/simple/module.h rename to src/ipa/softisp/module.h index 8d4d53fb2..b5d99ba08 100644 --- a/src/ipa/simple/module.h +++ b/src/ipa/softisp/module.h @@ -9,7 +9,7 @@ #include <libcamera/controls.h> -#include <libcamera/ipa/soft_ipa_interface.h> +#include <libcamera/ipa/softisp_ipa_interface.h> #include "libcamera/internal/software_isp/debayer_params.h" #include "libcamera/internal/software_isp/swisp_stats.h" diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/softisp/soft_simple.cpp similarity index 99% rename from src/ipa/simple/soft_simple.cpp rename to src/ipa/softisp/soft_simple.cpp index d4ab91e30..e84feeea7 100644 --- a/src/ipa/simple/soft_simple.cpp +++ b/src/ipa/softisp/soft_simple.cpp @@ -20,7 +20,7 @@ #include <libcamera/ipa/ipa_interface.h> #include <libcamera/ipa/ipa_module_info.h> -#include <libcamera/ipa/soft_ipa_interface.h> +#include <libcamera/ipa/softisp_ipa_interface.h> #include "libcamera/internal/software_isp/debayer_params.h" #include "libcamera/internal/software_isp/swisp_stats.h" @@ -342,7 +342,7 @@ extern "C" { const struct IPAModuleInfo ipaModuleInfo = { IPA_MODULE_API_VERSION, 0, - "simple", + "softisp", }; IPAInterface *ipaCreate() diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 25fc11989..a803f6208 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -844,8 +844,10 @@ void PipelineHandler::disconnect() * * Create an IPA module using \a ipaName as the matching identifier. This * overload allows pipeline handlers to create an IPA module by specifying its - * name instead of relying on the fact that the IPA module matches the pipeline - * handler's one. + * name instead of relying on the fact that the IPA module matches the IPA + * proxy's type name. + * + * \deprecated This method is deprecated and *MUST* not be used in new code. * * \return A newly created IPA proxy, or nullptr if no matching IPA module is * found or if the IPA proxy fails to initialize @@ -858,7 +860,7 @@ void PipelineHandler::disconnect() * \param[in] minVersion Minimum acceptable version of IPA module * \param[in] maxVersion Maximum acceptable version of IPA module * - * Create an IPA module using the pipeline handler name as the matching + * Create an IPA module using the IPA proxy's type name as the matching * identifier. This overload allows pipeline handler to create an IPA module * whose name matches the pipeline handler one. * diff --git a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl index d48b90dcf..259ed47c3 100644 --- a/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl +++ b/utils/codegen/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl @@ -40,6 +40,8 @@ class {{proxy_name}}Isolated; class {{proxy_name}} : public IPAProxy, public {{interface_name}}, public Object { public: + static constexpr const char *name() { return "{{module_name}}"; } + using Threaded = {{proxy_name}}Threaded; using Isolated = {{proxy_name}}Isolated;