[libcamera-devel] [PATCH] libcamera: ipa_proxy: Allow a prefix for the configuration file

[Naushir Patuck]

Hi Jacopo,

On Thu, 27 Apr 2023 at 09:28, Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> Naush,
>    what would be the implications of setting the IPA module name to
> "rpi/vc4" ? That would make this patch not needed, right ? Are there
> undesired implications ?

I did try to do this, but sadly it does not work.
>From https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/ipa_module.cpp#n303

/* Validate the IPA module name. */
std::string ipaName = info_.name;
auto iter = std::find_if_not(ipaName.begin(), ipaName.end(),
     [](unsigned char c) -> bool {
     return isprint(c) && c != '/' &&
    c != '?' && c != '*' &&
    c != '\\';
     });
if (iter != ipaName.end()) {
LOG(IPAModule, Error)
<< "Invalid IPA module name '" << ipaName << "'";
return -EINVAL;
}

This bit of code complains if the name string has a "/" character.


>
> On Thu, Apr 27, 2023 at 09:24:16AM +0100, Naushir Patuck wrote:
> > On Thu, 27 Apr 2023 at 09:10, Jacopo Mondi
> > <jacopo.mondi at ideasonboard.com> wrote:
> > >
> > > Hi Naush
> > >
> > > On Thu, Apr 27, 2023 at 08:16:52AM +0100, Naushir Patuck via libcamera-devel wrote:
> > > > Add a prefix parameter to IPAProxy::configurationFile(). This prefix is
> > > > added to the search path when locating IPA configuration files in the
> > > > system directories.
> > > >
> > > > For example, the system directories etc/libcamera/ipa/<prefix>/ and
> > > > share/libcamera/ipa/<prefix>/ will be used to search for the IPA
> > > > configuration files.
> > > >
> > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > ---
> > > >  include/libcamera/internal/ipa_proxy.h |  3 ++-
> > > >  src/libcamera/ipa_proxy.cpp            | 11 +++++++----
> > > >  2 files changed, 9 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h
> > > > index 781c8b623605..9235b582edad 100644
> > > > --- a/include/libcamera/internal/ipa_proxy.h
> > > > +++ b/include/libcamera/internal/ipa_proxy.h
> > > > @@ -31,7 +31,8 @@ public:
> > > >
> > > >       bool isValid() const { return valid_; }
> > > >
> > > > -     std::string configurationFile(const std::string &file) const;
> > > > +     std::string configurationFile(const std::string &file,
> > > > +                                   const std::string &prefix = "") const;
> > > >
> > > >  protected:
> > > >       std::string resolvePath(const std::string &file) const;
> > > > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
> > > > index 3f2cc6b89f60..4a27b0a993fa 100644
> > > > --- a/src/libcamera/ipa_proxy.cpp
> > > > +++ b/src/libcamera/ipa_proxy.cpp
> > > > @@ -72,6 +72,7 @@ IPAProxy::~IPAProxy()
> > > >  /**
> > > >   * \brief Retrieve the absolute path to an IPA configuration file
> > > >   * \param[in] name The configuration file name
> > > > + * \param[in] prefix The configuration directory prefix when searching system paths
> > > >   *
> > > >   * This function locates the configuration file for an IPA and returns its
> > > >   * absolute path. It searches the following directories, in order:
> > > > @@ -80,8 +81,8 @@ IPAProxy::~IPAProxy()
> > > >   *   environment variable ; or
> > > >   * - If libcamera is not installed, the src/ipa/ directory within the source
> > > >   *   tree ; otherwise
> > > > - * - The system sysconf (etc/libcamera/ipa) and the data (share/libcamera/ipa/)
> > > > - *   directories.
> > > > + * - The system sysconf (etc/libcamera/ipa/<prefix>/) and the data
> > > > + *   (share/libcamera/ipa/<prefix>/) directories.
> > >
> > > Doxygen doesn't seem happy with <prefix>
> > >
> > > src/libcamera/ipa_proxy.cpp:83: warning: Unsupported xml/html tag <prefix> found
> > > src/libcamera/ipa_proxy.cpp:84: warning: Unsupported xml/html tag <prefix> found
> >
> > Oops! Should I just use "prefix" instead?
> >
> > ""
> >  The system sysconf (etc/libcamera/ipa/prefix/) and the data
> >  (share/libcamera/ipa/prefix/) directories.
> > ""
>
> Not sure it <> could be escaped with \< and \>

Yes, it can!

Annoyingly, I forgot that I fixed this very thing for
PipelineHandler::configurationFile()
some time back!

I'll make the update.  Given that there are a number of changes now, should I
post a v2 with your tags collected?

Regards,
Naush

> >
> > Regards,
> > Naush
> >
> > >
> > >
> > > >   *
> > > >   * The system directories are not searched if libcamera is not installed.
> > > >   *
> > > > @@ -92,7 +93,8 @@ IPAProxy::~IPAProxy()
> > > >   * \return The full path to the IPA configuration file, or an empty string if
> > > >   * no configuration file can be found
> > > >   */
> > > > -std::string IPAProxy::configurationFile(const std::string &name) const
> > > > +std::string IPAProxy::configurationFile(const std::string &name,
> > > > +                                     const std::string &prefix) const
> > > >  {
> > > >       struct stat statbuf;
> > > >       int ret;
> > > > @@ -139,7 +141,8 @@ std::string IPAProxy::configurationFile(const std::string &name) const
> > > >       } else {
> > > >               /* Else look in the system locations. */
> > > >               for (const auto &dir : utils::split(IPA_CONFIG_DIR, ":")) {
> > > > -                     std::string confPath = dir + "/" + ipaName + "/" + name;
> > > > +                     std::string confPath = dir + "/" + prefix + "/" +
> > > > +                                            ipaName + "/" + name;
> > > >                       ret = stat(confPath.c_str(), &statbuf);
> > > >                       if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG)
> > > >                               return confPath;
> > > > --
> > > > 2.34.1
> > > >