[libcamera-devel] [PATCH v4 02/12] libcamera: pipeline: Add a platform configuration file helper

[Kieran Bingham]

Quoting Naushir Patuck (2023-01-17 08:50:46)
> Hi Kieran,
> 
> Thank you for the feedback!
> 
> On Mon, 16 Jan 2023 at 17:18, Kieran Bingham <
> kieran.bingham at ideasonboard.com> wrote:
> 
> > Quoting Naushir Patuck via libcamera-devel (2022-12-09 09:00:40)
> > > Add a new helper function PipelineHandler::configurationFile() that
> > returns
> > > the full path of a named configuration file. This configuration file may
> > be read
> > > by pipeline handlers for platform specific configuration parameters on
> > > initialisation.
> > >
> > > The mechanism for searching for the configuration file is similar to the
> > IPA
> > > configuration file:
> > >
> > > - In the source tree if libcamera is not installed
> > > - Otherwise in standard system locations (etc and share directories).
> > >
> > > When stored in the source tree, configuration files shall be located in
> > a 'data'
> > > subdirectory of their respective pipeline handler directory.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > >  include/libcamera/internal/pipeline_handler.h |  3 +
> > >  src/libcamera/pipeline_handler.cpp            | 60 +++++++++++++++++++
> > >  2 files changed, 63 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/pipeline_handler.h
> > b/include/libcamera/internal/pipeline_handler.h
> > > index ec4f662d7399..4c4dfe62a680 100644
> > > --- a/include/libcamera/internal/pipeline_handler.h
> > > +++ b/include/libcamera/internal/pipeline_handler.h
> > > @@ -65,6 +65,9 @@ public:
> > >         bool completeBuffer(Request *request, FrameBuffer *buffer);
> > >         void completeRequest(Request *request);
> > >
> > > +       std::string configurationFile(const std::string &subdir,
> > > +                                     const std::string &name) const;
> > > +
> > >         const char *name() const { return name_; }
> > >
> > >  protected:
> > > diff --git a/src/libcamera/pipeline_handler.cpp
> > b/src/libcamera/pipeline_handler.cpp
> > > index cfade4908118..a515ad5ecffb 100644
> > > --- a/src/libcamera/pipeline_handler.cpp
> > > +++ b/src/libcamera/pipeline_handler.cpp
> > > @@ -8,6 +8,7 @@
> > >  #include "libcamera/internal/pipeline_handler.h"
> > >
> > >  #include <chrono>
> > > +#include <sys/stat.h>
> > >  #include <sys/sysmacros.h>
> > >
> > >  #include <libcamera/base/log.h>
> > > @@ -534,6 +535,65 @@ void PipelineHandler::completeRequest(Request
> > *request)
> > >         }
> > >  }
> > >
> > > +/**
> > > + * \brief Retrieve the absolute path to a platform configuration file
> > > + * \param[in] subdir The pipeline handler specific subdirectory name
> > > + * \param[in] name The configuration file name
> > > + *
> > > + * This function locates a named platform configuration file and returns
> > > + * its absolute path to the pipeline handler. It searches the following
> > > + * directories, in order:
> > > + *
> > > + * - If libcamera is not installed, the
> > src/libcamera/pipeline/<subdir>/data/
> > > + *   directory within the source tree ; otherwise
> > > + * - The system data (share/libcamera/pipeline/<subdir>) directory.
> > > + *
> > > + * The system directories are not searched if libcamera is not
> > installed.
> > > + *
> > > + * \return The full path to the pipeline handler configuration file, or
> > an empty
> > > + * string if no configuration file can be found
> > > + */
> > > +std::string PipelineHandler::configurationFile(const std::string
> > &subdir,
> > > +                                              const std::string &name)
> > const
> > > +{
> > > +       std::string confPath;
> > > +       struct stat statbuf;
> > > +       int ret;
> > > +
> > > +       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 pipeline handler.
> > > +                */
> > > +               std::string confDir = root + "src/libcamera/pipeline/";
> > > +               confPath = confDir + subdir + "/data/" + name;
> > > +
> > > +               LOG(Pipeline, Info)
> > > +                       << "libcamera is not installed. Loading platform
> > configuration file from '"
> > > +                       << confPath << "'";
> > > +
> > > +               ret = stat(confPath.c_str(), &statbuf);
> > > +               if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG)
> >
> > Will this stop configuration files being a symlink to an alternative
> > file? (S_IFREG vs S_IFLINK) ? Does that even matter?
> >
> > We could do:
> >
> >         if (File::exists(confPath))
> >                 return confPath;
> >
> > That only checks that the 'name' is not a directory... So ... it's
> > different again, though I would expect the

"I would expect the user of the confPath to use the File class, so it
should be appropriate" ... was supposed to be written here ;)

> 
> 
> I was copying the implementation of IPAProxy::configurationFile()
> for this function to retain consistency, but could change if other folks
> agree?

If it's better we can fix the configurationFile location too. But both
seem functional anyway.
--
Kieran


> 
> 
> >
> >
> >
> > > +                       return confPath;
> > > +       } else {
> > > +               /* Else look in the system locations. */
> > > +               confPath = std::string(LIBCAMERA_DATA_DIR)
> > > +                               + "/pipeline/" + subdir + '/' + name;
> > > +               ret = stat(confPath.c_str(), &statbuf);
> > > +               if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG)
> > > +                       return confPath;
> > > +       }
> > > +
> > > +       LOG(Pipeline, Error)
> > > +               << "Configuration file '" << confPath
> > > +               << "' not found for pipeline handler '" <<
> > PipelineHandler::name() << "'";
> >
> > This sounds like it suddenly makes configuration files mandatory for all
> > pipelines ?
> >
> > (or perhaps only if a pipeline looks for one ?)
> >
> 
> Correct, the config file is only mandatory if a pipeline handler explicitly
> looks for one.
> 

Would you expect pipelines to have a default configuration for all
options?

Anyway, while it reports an error, it's up to the pipeline handler to
decide how to respond if there's no file.

--
Kieran


> Regards,
> Naush
> 
> 
> 
> >
> > > +
> > > +       return std::string();
> > > +}
> > > +
> > >  /**
> > >   * \brief Register a camera to the camera manager and pipeline handler
> > >   * \param[in] camera The camera to be added
> > > --
> > > 2.25.1
> > >
> >