[libcamera-devel] [PATCH v4 02/12] libcamera: pipeline: Add a platform configuration file helper
Naushir Patuck
naush at raspberrypi.com
Tue Jan 17 10:44:12 CET 2023
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.
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
> > > >
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20230117/a8ee49e3/attachment.htm>
More information about the libcamera-devel
mailing list