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

[Jacopo Mondi]

On Thu, Apr 27, 2023 at 09:47:02AM +0100, Naushir Patuck via libcamera-devel wrote:
> 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.
>

AH right. I wonder if we should reconsider that..
For now, unless others are strongly in favour of changing this, let's
leave it this way

>
> >
> > 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
> > > > >