[libcamera-devel] [PATCH v4 02/12] libcamera: pipeline: Add a platform configuration file helper
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Jan 17 10:56:48 CET 2023
Quoting Naushir Patuck (2023-01-17 09:44:12)
> On Tue, 17 Jan 2023 at 09:40, Kieran Bingham <
> kieran.bingham at ideasonboard.com> wrote:
>
> > 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.
> >
>
> In an earlier version of the patch, we did have a default.yaml file that
> loaded
> a default configuration if no other file was provided. However, Laurent
> suggested
> (and I agree) that we remove that and hard-code the default configuration
> in the
> source itself.
Absolutely, yes I think the defaults should be handled by the pipeline
handler itself.
--
Kieran
> As you said, it's up to the individual pipeline handlers to decide the
> behavior.
>
> Naush
>
>
>
> >
> > --
> > 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
> > > > >
> > > >
> >
More information about the libcamera-devel
mailing list