<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 17 Jan 2023 at 09:40, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Quoting Naushir Patuck (2023-01-17 08:50:46)<br>
> Hi Kieran,<br>
> <br>
> Thank you for the feedback!<br>
> <br>
> On Mon, 16 Jan 2023 at 17:18, Kieran Bingham <<br>
> <a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>> wrote:<br>
> <br>
> > Quoting Naushir Patuck via libcamera-devel (2022-12-09 09:00:40)<br>
> > > Add a new helper function PipelineHandler::configurationFile() that<br>
> > returns<br>
> > > the full path of a named configuration file. This configuration file may<br>
> > be read<br>
> > > by pipeline handlers for platform specific configuration parameters on<br>
> > > initialisation.<br>
> > ><br>
> > > The mechanism for searching for the configuration file is similar to the<br>
> > IPA<br>
> > > configuration file:<br>
> > ><br>
> > > - In the source tree if libcamera is not installed<br>
> > > - Otherwise in standard system locations (etc and share directories).<br>
> > ><br>
> > > When stored in the source tree, configuration files shall be located in<br>
> > a 'data'<br>
> > > subdirectory of their respective pipeline handler directory.<br>
> > ><br>
> > > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> > > ---<br>
> > > include/libcamera/internal/pipeline_handler.h | 3 +<br>
> > > src/libcamera/pipeline_handler.cpp | 60 +++++++++++++++++++<br>
> > > 2 files changed, 63 insertions(+)<br>
> > ><br>
> > > diff --git a/include/libcamera/internal/pipeline_handler.h<br>
> > b/include/libcamera/internal/pipeline_handler.h<br>
> > > index ec4f662d7399..4c4dfe62a680 100644<br>
> > > --- a/include/libcamera/internal/pipeline_handler.h<br>
> > > +++ b/include/libcamera/internal/pipeline_handler.h<br>
> > > @@ -65,6 +65,9 @@ public:<br>
> > > bool completeBuffer(Request *request, FrameBuffer *buffer);<br>
> > > void completeRequest(Request *request);<br>
> > ><br>
> > > + std::string configurationFile(const std::string &subdir,<br>
> > > + const std::string &name) const;<br>
> > > +<br>
> > > const char *name() const { return name_; }<br>
> > ><br>
> > > protected:<br>
> > > diff --git a/src/libcamera/pipeline_handler.cpp<br>
> > b/src/libcamera/pipeline_handler.cpp<br>
> > > index cfade4908118..a515ad5ecffb 100644<br>
> > > --- a/src/libcamera/pipeline_handler.cpp<br>
> > > +++ b/src/libcamera/pipeline_handler.cpp<br>
> > > @@ -8,6 +8,7 @@<br>
> > > #include "libcamera/internal/pipeline_handler.h"<br>
> > ><br>
> > > #include <chrono><br>
> > > +#include <sys/stat.h><br>
> > > #include <sys/sysmacros.h><br>
> > ><br>
> > > #include <libcamera/base/log.h><br>
> > > @@ -534,6 +535,65 @@ void PipelineHandler::completeRequest(Request<br>
> > *request)<br>
> > > }<br>
> > > }<br>
> > ><br>
> > > +/**<br>
> > > + * \brief Retrieve the absolute path to a platform configuration file<br>
> > > + * \param[in] subdir The pipeline handler specific subdirectory name<br>
> > > + * \param[in] name The configuration file name<br>
> > > + *<br>
> > > + * This function locates a named platform configuration file and returns<br>
> > > + * its absolute path to the pipeline handler. It searches the following<br>
> > > + * directories, in order:<br>
> > > + *<br>
> > > + * - If libcamera is not installed, the<br>
> > src/libcamera/pipeline/<subdir>/data/<br>
> > > + * directory within the source tree ; otherwise<br>
> > > + * - The system data (share/libcamera/pipeline/<subdir>) directory.<br>
> > > + *<br>
> > > + * The system directories are not searched if libcamera is not<br>
> > installed.<br>
> > > + *<br>
> > > + * \return The full path to the pipeline handler configuration file, or<br>
> > an empty<br>
> > > + * string if no configuration file can be found<br>
> > > + */<br>
> > > +std::string PipelineHandler::configurationFile(const std::string<br>
> > &subdir,<br>
> > > + const std::string &name)<br>
> > const<br>
> > > +{<br>
> > > + std::string confPath;<br>
> > > + struct stat statbuf;<br>
> > > + int ret;<br>
> > > +<br>
> > > + std::string root = utils::libcameraSourcePath();<br>
> > > + if (!root.empty()) {<br>
> > > + /*<br>
> > > + * When libcamera is used before it is installed, load<br>
> > > + * configuration files from the source directory. The<br>
> > > + * configuration files are then located in the 'data'<br>
> > > + * subdirectory of the corresponding pipeline handler.<br>
> > > + */<br>
> > > + std::string confDir = root + "src/libcamera/pipeline/";<br>
> > > + confPath = confDir + subdir + "/data/" + name;<br>
> > > +<br>
> > > + LOG(Pipeline, Info)<br>
> > > + << "libcamera is not installed. Loading platform<br>
> > configuration file from '"<br>
> > > + << confPath << "'";<br>
> > > +<br>
> > > + ret = stat(confPath.c_str(), &statbuf);<br>
> > > + if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG)<br>
> ><br>
> > Will this stop configuration files being a symlink to an alternative<br>
> > file? (S_IFREG vs S_IFLINK) ? Does that even matter?<br>
> ><br>
> > We could do:<br>
> ><br>
> > if (File::exists(confPath))<br>
> > return confPath;<br>
> ><br>
> > That only checks that the 'name' is not a directory... So ... it's<br>
> > different again, though I would expect the<br>
<br>
"I would expect the user of the confPath to use the File class, so it<br>
should be appropriate" ... was supposed to be written here ;)<br>
<br>
> <br>
> <br>
> I was copying the implementation of IPAProxy::configurationFile()<br>
> for this function to retain consistency, but could change if other folks<br>
> agree?<br>
<br>
If it's better we can fix the configurationFile location too. But both<br>
seem functional anyway.<br>
--<br>
Kieran<br>
<br>
<br>
> <br>
> <br>
> ><br>
> ><br>
> ><br>
> > > + return confPath;<br>
> > > + } else {<br>
> > > + /* Else look in the system locations. */<br>
> > > + confPath = std::string(LIBCAMERA_DATA_DIR)<br>
> > > + + "/pipeline/" + subdir + '/' + name;<br>
> > > + ret = stat(confPath.c_str(), &statbuf);<br>
> > > + if (ret == 0 && (statbuf.st_mode & S_IFMT) == S_IFREG)<br>
> > > + return confPath;<br>
> > > + }<br>
> > > +<br>
> > > + LOG(Pipeline, Error)<br>
> > > + << "Configuration file '" << confPath<br>
> > > + << "' not found for pipeline handler '" <<<br>
> > PipelineHandler::name() << "'";<br>
> ><br>
> > This sounds like it suddenly makes configuration files mandatory for all<br>
> > pipelines ?<br>
> ><br>
> > (or perhaps only if a pipeline looks for one ?)<br>
> ><br>
> <br>
> Correct, the config file is only mandatory if a pipeline handler explicitly<br>
> looks for one.<br>
> <br>
<br>
Would you expect pipelines to have a default configuration for all<br>
options?<br>
<br>
Anyway, while it reports an error, it's up to the pipeline handler to<br>
decide how to respond if there's no file.<br></blockquote><div><br></div><div>In an earlier version of the patch, we did have a default.yaml file that loaded <br>a default configuration if no other file was provided. However, Laurent suggested<br>(and I agree) that we remove that and hard-code the default configuration in the<br>source itself.<br></div><div><br></div><div>As you said, it's up to the individual pipeline handlers to decide the behavior.</div><div><br></div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
--<br>
Kieran<br>
<br>
<br>
> Regards,<br>
> Naush<br>
> <br>
> <br>
> <br>
> ><br>
> > > +<br>
> > > + return std::string();<br>
> > > +}<br>
> > > +<br>
> > > /**<br>
> > > * \brief Register a camera to the camera manager and pipeline handler<br>
> > > * \param[in] camera The camera to be added<br>
> > > --<br>
> > > 2.25.1<br>
> > ><br>
> ><br>
</blockquote></div></div>