Message ID | 20250812143814.120975-1-stefan.klug@ideasonboard.com |
---|---|
State | Accepted |
Commit | 452fbd2295dea2f1acb4f7e746aa17a0c7b11a73 |
Headers | show |
Series |
|
Related | show |
Quoting Stefan Klug (2025-08-12 15:38:04) > It is often helpful to know which tuning file is used. Add a log > statement with INFO level for that. > > As the core logic has multiple return paths, adding the log statement is > not straight forward. Extract finder logic into a ipaConfigurationFile() > function and call that with the name and optionally the fallbackName > from the configurationFile() function. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > > Hi all, > > I posted a similar patch a while ago here: > https://lists.libcamera.org/pipermail/libcamera-devel/2025-January/048324.html > > We didn't merge it back then because it should be central in ipa_proxy. > Adding it there is a bit ugly because there are a few return paths. I > tried to use a scope_exit but that doesn't work as the exit function is > called after confPath is moved out to the caller. So I settled for > refactoring which imho makes the configurationFile() function also > easier to read. As a related option which could also be complementary to this patch - I've been pondering about putting the tuning file path into the camera properties. That makes it easy to find from either cam -c1 -p - or from camshark! - https://git.uk.ideasonboard.com/kbingham/libcamera/commit/da431b3599311b754d18f9ac6325c98f4ed8f87f - https://git.uk.ideasonboard.com/kbingham/libcamera/commit/04e79e0733c70b065b0cbb070d8263d3c416fc67 But alas, I don't think we can have something like that in a central location easily as there's no common path that has access to the IPA Proxy path and camera properties for all cameras I don't think. > > Best regards, > Stefan > > --- > src/libcamera/ipa_proxy.cpp | 143 ++++++++++++++++++++---------------- > 1 file changed, 79 insertions(+), 64 deletions(-) > > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > index 9907b9615ec7..b5c13a30f1d4 100644 > --- a/src/libcamera/ipa_proxy.cpp > +++ b/src/libcamera/ipa_proxy.cpp > @@ -25,6 +25,78 @@ namespace libcamera { > > LOG_DEFINE_CATEGORY(IPAProxy) > > +namespace { > + > +std::string ipaConfigurationFile(const std::string &ipaName, const std::string &name) > +{ > + /* > + * Start with any user override through the module-specific environment > + * variable. Use the name of the IPA module up to the first '/' to > + * construct the variable name. > + */ > + std::string ipaEnvName = ipaName.substr(0, ipaName.find('/')); > + std::transform(ipaEnvName.begin(), ipaEnvName.end(), ipaEnvName.begin(), > + [](unsigned char c) { return std::toupper(c); }); > + ipaEnvName = "LIBCAMERA_" + ipaEnvName + "_TUNING_FILE"; > + > + char const *configFromEnv = utils::secure_getenv(ipaEnvName.c_str()); > + if (configFromEnv && *configFromEnv != '\0') > + return { configFromEnv }; > + > + struct stat statbuf; > + int ret; > + > + /* > + * Check the directory pointed to by the IPA config path environment > + * variable next. > + */ > + const char *confPaths = utils::secure_getenv("LIBCAMERA_IPA_CONFIG_PATH"); > + if (confPaths) { > + for (const auto &dir : utils::split(confPaths, ":")) { > + if (dir.empty()) > + continue; > + > + std::string confPath = dir + "/" + ipaName + "/" + name; > + ret = stat(confPath.c_str(), &statbuf); > + if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) > + return confPath; > + } > + } > + > + std::string root = utils::libcameraSourcePath(); > + if (!root.empty()) { > + /* > + * When libcamera is used before it is installed, load > + * configuration files from the source directory. The > + * configuration files are then located in the 'data' > + * subdirectory of the corresponding IPA module. > + */ > + std::string ipaConfDir = root + "src/ipa/" + ipaName + "/data"; > + > + LOG(IPAProxy, Info) > + << "libcamera is not installed. Loading IPA configuration from '" > + << ipaConfDir << "'"; > + > + std::string confPath = ipaConfDir + "/" + name; > + ret = stat(confPath.c_str(), &statbuf); > + if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) > + return confPath; > + > + } else { > + /* Else look in the system locations. */ > + for (const auto &dir : utils::split(IPA_CONFIG_DIR, ":")) { > + std::string confPath = dir + "/" + ipaName + "/" + name; > + ret = stat(confPath.c_str(), &statbuf); > + if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) > + return confPath; > + } > + } > + > + return {}; > +} > + > +} /* namespace */ > + > /** > * \class IPAProxy > * \brief IPA Proxy > @@ -103,68 +175,10 @@ std::string IPAProxy::configurationFile(const std::string &name, > * has been validated when loading the module. > */ > const std::string ipaName = ipam_->info().name; > - > - /* > - * Start with any user override through the module-specific environment > - * variable. Use the name of the IPA module up to the first '/' to > - * construct the variable name. > - */ > - std::string ipaEnvName = ipaName.substr(0, ipaName.find('/')); > - std::transform(ipaEnvName.begin(), ipaEnvName.end(), ipaEnvName.begin(), > - [](unsigned char c) { return std::toupper(c); }); > - ipaEnvName = "LIBCAMERA_" + ipaEnvName + "_TUNING_FILE"; > - > - char const *configFromEnv = utils::secure_getenv(ipaEnvName.c_str()); > - if (configFromEnv && *configFromEnv != '\0') > - return { configFromEnv }; > - > - struct stat statbuf; > - int ret; > - > - /* > - * Check the directory pointed to by the IPA config path environment > - * variable next. > - */ > - const char *confPaths = utils::secure_getenv("LIBCAMERA_IPA_CONFIG_PATH"); > - if (confPaths) { > - for (const auto &dir : utils::split(confPaths, ":")) { > - if (dir.empty()) > - continue; > - > - std::string confPath = dir + "/" + ipaName + "/" + name; > - ret = stat(confPath.c_str(), &statbuf); > - if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) > - return confPath; > - } > - } > - > - std::string root = utils::libcameraSourcePath(); > - if (!root.empty()) { > - /* > - * When libcamera is used before it is installed, load > - * configuration files from the source directory. The > - * configuration files are then located in the 'data' > - * subdirectory of the corresponding IPA module. > - */ > - std::string ipaConfDir = root + "src/ipa/" + ipaName + "/data"; > - > - LOG(IPAProxy, Info) > - << "libcamera is not installed. Loading IPA configuration from '" > - << ipaConfDir << "'"; > - > - std::string confPath = ipaConfDir + "/" + name; > - ret = stat(confPath.c_str(), &statbuf); > - if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) > - return confPath; > - > - } else { > - /* Else look in the system locations. */ > - for (const auto &dir : utils::split(IPA_CONFIG_DIR, ":")) { > - std::string confPath = dir + "/" + ipaName + "/" + name; > - ret = stat(confPath.c_str(), &statbuf); > - if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) > - return confPath; > - } > + std::string confPath = ipaConfigurationFile(ipaName, name); > + if (!confPath.empty()) { > + LOG(IPAProxy, Info) << "Using tuning file " << confPath; > + return confPath; > } > > if (fallbackName.empty()) { > @@ -174,11 +188,12 @@ std::string IPAProxy::configurationFile(const std::string &name, > return std::string(); > } > > + confPath = ipaConfigurationFile(ipaName, fallbackName); How come we call this ... ohhhh we were recursive before. Ok - this is nicer indeed! And I can confirm with meld that the moved code is simply moved.... Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > LOG(IPAProxy, Warning) > << "Configuration file '" << name > << "' not found for IPA module '" << ipaName > - << "', falling back to '" << fallbackName << "'"; > - return configurationFile(fallbackName); > + << "', falling back to '" << confPath << "'"; > + return confPath; > } > > /** > -- > 2.48.1 >
2025. 08. 12. 16:38 keltezéssel, Stefan Klug írta: > It is often helpful to know which tuning file is used. Add a log > statement with INFO level for that. > > As the core logic has multiple return paths, adding the log statement is > not straight forward. Extract finder logic into a ipaConfigurationFile() > function and call that with the name and optionally the fallbackName > from the configurationFile() function. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > > Hi all, > > I posted a similar patch a while ago here: > https://lists.libcamera.org/pipermail/libcamera-devel/2025-January/048324.html > > We didn't merge it back then because it should be central in ipa_proxy. > Adding it there is a bit ugly because there are a few return paths. I > tried to use a scope_exit but that doesn't work as the exit function is > called after confPath is moved out to the caller. So I settled for > refactoring which imho makes the configurationFile() function also > easier to read. > > Best regards, > Stefan > > --- Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > src/libcamera/ipa_proxy.cpp | 143 ++++++++++++++++++++---------------- > 1 file changed, 79 insertions(+), 64 deletions(-) > > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > index 9907b9615ec7..b5c13a30f1d4 100644 > --- a/src/libcamera/ipa_proxy.cpp > +++ b/src/libcamera/ipa_proxy.cpp > @@ -25,6 +25,78 @@ namespace libcamera { > > LOG_DEFINE_CATEGORY(IPAProxy) > > +namespace { > + > +std::string ipaConfigurationFile(const std::string &ipaName, const std::string &name) > +{ > + /* > + * Start with any user override through the module-specific environment > + * variable. Use the name of the IPA module up to the first '/' to > + * construct the variable name. > + */ > + std::string ipaEnvName = ipaName.substr(0, ipaName.find('/')); > + std::transform(ipaEnvName.begin(), ipaEnvName.end(), ipaEnvName.begin(), > + [](unsigned char c) { return std::toupper(c); }); > + ipaEnvName = "LIBCAMERA_" + ipaEnvName + "_TUNING_FILE"; > + > + char const *configFromEnv = utils::secure_getenv(ipaEnvName.c_str()); > + if (configFromEnv && *configFromEnv != '\0') > + return { configFromEnv }; > + > + struct stat statbuf; > + int ret; > + > + /* > + * Check the directory pointed to by the IPA config path environment > + * variable next. > + */ > + const char *confPaths = utils::secure_getenv("LIBCAMERA_IPA_CONFIG_PATH"); > + if (confPaths) { > + for (const auto &dir : utils::split(confPaths, ":")) { > + if (dir.empty()) > + continue; > + > + std::string confPath = dir + "/" + ipaName + "/" + name; > + ret = stat(confPath.c_str(), &statbuf); > + if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) > + return confPath; > + } > + } > + > + std::string root = utils::libcameraSourcePath(); > + if (!root.empty()) { > + /* > + * When libcamera is used before it is installed, load > + * configuration files from the source directory. The > + * configuration files are then located in the 'data' > + * subdirectory of the corresponding IPA module. > + */ > + std::string ipaConfDir = root + "src/ipa/" + ipaName + "/data"; > + > + LOG(IPAProxy, Info) > + << "libcamera is not installed. Loading IPA configuration from '" > + << ipaConfDir << "'"; > + > + std::string confPath = ipaConfDir + "/" + name; > + ret = stat(confPath.c_str(), &statbuf); > + if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) > + return confPath; > + > + } else { > + /* Else look in the system locations. */ > + for (const auto &dir : utils::split(IPA_CONFIG_DIR, ":")) { > + std::string confPath = dir + "/" + ipaName + "/" + name; > + ret = stat(confPath.c_str(), &statbuf); > + if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) > + return confPath; > + } > + } > + > + return {}; > +} > + > +} /* namespace */ > + > /** > * \class IPAProxy > * \brief IPA Proxy > @@ -103,68 +175,10 @@ std::string IPAProxy::configurationFile(const std::string &name, > * has been validated when loading the module. > */ > const std::string ipaName = ipam_->info().name; > - > - /* > - * Start with any user override through the module-specific environment > - * variable. Use the name of the IPA module up to the first '/' to > - * construct the variable name. > - */ > - std::string ipaEnvName = ipaName.substr(0, ipaName.find('/')); > - std::transform(ipaEnvName.begin(), ipaEnvName.end(), ipaEnvName.begin(), > - [](unsigned char c) { return std::toupper(c); }); > - ipaEnvName = "LIBCAMERA_" + ipaEnvName + "_TUNING_FILE"; > - > - char const *configFromEnv = utils::secure_getenv(ipaEnvName.c_str()); > - if (configFromEnv && *configFromEnv != '\0') > - return { configFromEnv }; > - > - struct stat statbuf; > - int ret; > - > - /* > - * Check the directory pointed to by the IPA config path environment > - * variable next. > - */ > - const char *confPaths = utils::secure_getenv("LIBCAMERA_IPA_CONFIG_PATH"); > - if (confPaths) { > - for (const auto &dir : utils::split(confPaths, ":")) { > - if (dir.empty()) > - continue; > - > - std::string confPath = dir + "/" + ipaName + "/" + name; > - ret = stat(confPath.c_str(), &statbuf); > - if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) > - return confPath; > - } > - } > - > - std::string root = utils::libcameraSourcePath(); > - if (!root.empty()) { > - /* > - * When libcamera is used before it is installed, load > - * configuration files from the source directory. The > - * configuration files are then located in the 'data' > - * subdirectory of the corresponding IPA module. > - */ > - std::string ipaConfDir = root + "src/ipa/" + ipaName + "/data"; > - > - LOG(IPAProxy, Info) > - << "libcamera is not installed. Loading IPA configuration from '" > - << ipaConfDir << "'"; > - > - std::string confPath = ipaConfDir + "/" + name; > - ret = stat(confPath.c_str(), &statbuf); > - if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) > - return confPath; > - > - } else { > - /* Else look in the system locations. */ > - for (const auto &dir : utils::split(IPA_CONFIG_DIR, ":")) { > - std::string confPath = dir + "/" + ipaName + "/" + name; > - ret = stat(confPath.c_str(), &statbuf); > - if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) > - return confPath; > - } > + std::string confPath = ipaConfigurationFile(ipaName, name); > + if (!confPath.empty()) { > + LOG(IPAProxy, Info) << "Using tuning file " << confPath; > + return confPath; > } > > if (fallbackName.empty()) { > @@ -174,11 +188,12 @@ std::string IPAProxy::configurationFile(const std::string &name, > return std::string(); > } > > + confPath = ipaConfigurationFile(ipaName, fallbackName); > LOG(IPAProxy, Warning) > << "Configuration file '" << name > << "' not found for IPA module '" << ipaName > - << "', falling back to '" << fallbackName << "'"; > - return configurationFile(fallbackName); > + << "', falling back to '" << confPath << "'"; > + return confPath; > } > > /**
diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp index 9907b9615ec7..b5c13a30f1d4 100644 --- a/src/libcamera/ipa_proxy.cpp +++ b/src/libcamera/ipa_proxy.cpp @@ -25,6 +25,78 @@ namespace libcamera { LOG_DEFINE_CATEGORY(IPAProxy) +namespace { + +std::string ipaConfigurationFile(const std::string &ipaName, const std::string &name) +{ + /* + * Start with any user override through the module-specific environment + * variable. Use the name of the IPA module up to the first '/' to + * construct the variable name. + */ + std::string ipaEnvName = ipaName.substr(0, ipaName.find('/')); + std::transform(ipaEnvName.begin(), ipaEnvName.end(), ipaEnvName.begin(), + [](unsigned char c) { return std::toupper(c); }); + ipaEnvName = "LIBCAMERA_" + ipaEnvName + "_TUNING_FILE"; + + char const *configFromEnv = utils::secure_getenv(ipaEnvName.c_str()); + if (configFromEnv && *configFromEnv != '\0') + return { configFromEnv }; + + struct stat statbuf; + int ret; + + /* + * Check the directory pointed to by the IPA config path environment + * variable next. + */ + const char *confPaths = utils::secure_getenv("LIBCAMERA_IPA_CONFIG_PATH"); + if (confPaths) { + for (const auto &dir : utils::split(confPaths, ":")) { + if (dir.empty()) + continue; + + std::string confPath = dir + "/" + ipaName + "/" + name; + ret = stat(confPath.c_str(), &statbuf); + if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) + return confPath; + } + } + + std::string root = utils::libcameraSourcePath(); + if (!root.empty()) { + /* + * When libcamera is used before it is installed, load + * configuration files from the source directory. The + * configuration files are then located in the 'data' + * subdirectory of the corresponding IPA module. + */ + std::string ipaConfDir = root + "src/ipa/" + ipaName + "/data"; + + LOG(IPAProxy, Info) + << "libcamera is not installed. Loading IPA configuration from '" + << ipaConfDir << "'"; + + std::string confPath = ipaConfDir + "/" + name; + ret = stat(confPath.c_str(), &statbuf); + if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) + return confPath; + + } else { + /* Else look in the system locations. */ + for (const auto &dir : utils::split(IPA_CONFIG_DIR, ":")) { + std::string confPath = dir + "/" + ipaName + "/" + name; + ret = stat(confPath.c_str(), &statbuf); + if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) + return confPath; + } + } + + return {}; +} + +} /* namespace */ + /** * \class IPAProxy * \brief IPA Proxy @@ -103,68 +175,10 @@ std::string IPAProxy::configurationFile(const std::string &name, * has been validated when loading the module. */ const std::string ipaName = ipam_->info().name; - - /* - * Start with any user override through the module-specific environment - * variable. Use the name of the IPA module up to the first '/' to - * construct the variable name. - */ - std::string ipaEnvName = ipaName.substr(0, ipaName.find('/')); - std::transform(ipaEnvName.begin(), ipaEnvName.end(), ipaEnvName.begin(), - [](unsigned char c) { return std::toupper(c); }); - ipaEnvName = "LIBCAMERA_" + ipaEnvName + "_TUNING_FILE"; - - char const *configFromEnv = utils::secure_getenv(ipaEnvName.c_str()); - if (configFromEnv && *configFromEnv != '\0') - return { configFromEnv }; - - struct stat statbuf; - int ret; - - /* - * Check the directory pointed to by the IPA config path environment - * variable next. - */ - const char *confPaths = utils::secure_getenv("LIBCAMERA_IPA_CONFIG_PATH"); - if (confPaths) { - for (const auto &dir : utils::split(confPaths, ":")) { - if (dir.empty()) - continue; - - std::string confPath = dir + "/" + ipaName + "/" + name; - ret = stat(confPath.c_str(), &statbuf); - if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) - return confPath; - } - } - - std::string root = utils::libcameraSourcePath(); - if (!root.empty()) { - /* - * When libcamera is used before it is installed, load - * configuration files from the source directory. The - * configuration files are then located in the 'data' - * subdirectory of the corresponding IPA module. - */ - std::string ipaConfDir = root + "src/ipa/" + ipaName + "/data"; - - LOG(IPAProxy, Info) - << "libcamera is not installed. Loading IPA configuration from '" - << ipaConfDir << "'"; - - std::string confPath = ipaConfDir + "/" + name; - ret = stat(confPath.c_str(), &statbuf); - if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) - return confPath; - - } else { - /* Else look in the system locations. */ - for (const auto &dir : utils::split(IPA_CONFIG_DIR, ":")) { - std::string confPath = dir + "/" + ipaName + "/" + name; - ret = stat(confPath.c_str(), &statbuf); - if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG) - return confPath; - } + std::string confPath = ipaConfigurationFile(ipaName, name); + if (!confPath.empty()) { + LOG(IPAProxy, Info) << "Using tuning file " << confPath; + return confPath; } if (fallbackName.empty()) { @@ -174,11 +188,12 @@ std::string IPAProxy::configurationFile(const std::string &name, return std::string(); } + confPath = ipaConfigurationFile(ipaName, fallbackName); LOG(IPAProxy, Warning) << "Configuration file '" << name << "' not found for IPA module '" << ipaName - << "', falling back to '" << fallbackName << "'"; - return configurationFile(fallbackName); + << "', falling back to '" << confPath << "'"; + return confPath; } /**
It is often helpful to know which tuning file is used. Add a log statement with INFO level for that. As the core logic has multiple return paths, adding the log statement is not straight forward. Extract finder logic into a ipaConfigurationFile() function and call that with the name and optionally the fallbackName from the configurationFile() function. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- Hi all, I posted a similar patch a while ago here: https://lists.libcamera.org/pipermail/libcamera-devel/2025-January/048324.html We didn't merge it back then because it should be central in ipa_proxy. Adding it there is a bit ugly because there are a few return paths. I tried to use a scope_exit but that doesn't work as the exit function is called after confPath is moved out to the caller. So I settled for refactoring which imho makes the configurationFile() function also easier to read. Best regards, Stefan --- src/libcamera/ipa_proxy.cpp | 143 ++++++++++++++++++++---------------- 1 file changed, 79 insertions(+), 64 deletions(-)