libcamera: layer_manager: Add support for global configuration

26048 diff mbox series

Enable configuring the layer system with the global configuration file,
while retaining the ability to overwrite the global configuration via
environment variables. This allows users to more easily retain layer
setups without having to always set environment variables.

To achieve this, the CameraManager needs to manually construct the
LayerManager to feed it the global configuration.

While at it, add documentation for the layer configuration options

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
This patch is on top of v6 of "Add Layers support" [0]

[0] https://patchwork.libcamera.org/project/libcamera/list/?series=5753

 Documentation/runtime_configuration.rst     | 13 +++++
 include/libcamera/internal/camera_manager.h |  4 +-
 include/libcamera/internal/layer_manager.h  | 10 +++-
 src/libcamera/camera_manager.cpp            |  1 +
 src/libcamera/layer_manager.cpp             | 61 ++++++++++-----------
 5 files changed, 53 insertions(+), 36 deletions(-)

Hi Paul,

the idea looks generally fine to me.  A few comments below, others may
have more comments.

Paul Elder <paul.elder@ideasonboard.com> writes:

> Enable configuring the layer system with the global configuration file,
> while retaining the ability to overwrite the global configuration via
> environment variables. This allows users to more easily retain layer
> setups without having to always set environment variables.
>
> To achieve this, the CameraManager needs to manually construct the
> LayerManager to feed it the global configuration.
>
> While at it, add documentation for the layer configuration options
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
> This patch is on top of v6 of "Add Layers support" [0]
>
> [0] https://patchwork.libcamera.org/project/libcamera/list/?series=5753
>
>  Documentation/runtime_configuration.rst     | 13 +++++
>  include/libcamera/internal/camera_manager.h |  4 +-
>  include/libcamera/internal/layer_manager.h  | 10 +++-
>  src/libcamera/camera_manager.cpp            |  1 +
>  src/libcamera/layer_manager.cpp             | 61 ++++++++++-----------
>  5 files changed, 53 insertions(+), 36 deletions(-)
>
> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst
> index e99ef2fb9561..a74a738803d6 100644
> --- a/Documentation/runtime_configuration.rst
> +++ b/Documentation/runtime_configuration.rst
> @@ -139,6 +139,19 @@ LIBCAMERA_<NAME>_TUNING_FILE
>  
>     Example value: ``/usr/local/share/libcamera/ipa/rpi/vc4/custom_sensor.json``
>  
> +LIBCAMERA_LAYER_PATH, layer.path
> +   Define custom search locations for Layer implementations.
> +
> +   Example value: ``${HOME}/.libcamera/share/layer:/opt/libcamera/vendor/share/layer``
> +
> +LIBCAMERA_LAYERS_ENABLE, layer.layers
> +  Define an ordered list of Layers to load. The layer names are declared in the
> +  'name' field of their repsective LayerInfo structs. The layers declared first

s/repsective/respective/

> +  are closer to the application, and the layers declared later are closer to
> +  libcamera.
> +
> +  Example value: ``inject_controls,sync``
> +
>  pipelines.simple.supported_devices.driver, pipelines.simple.supported_devices.software_isp
>     Override whether software ISP is enabled for the given driver.
>  
> diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h
> index cc3ede02507f..00afcd2177e7 100644
> --- a/include/libcamera/internal/camera_manager.h
> +++ b/include/libcamera/internal/camera_manager.h
> @@ -46,7 +46,7 @@ public:
>  	}
>  
>  	IPAManager *ipaManager() const { return ipaManager_.get(); }
> -	const LayerManager *layerManager() const { return &layerManager_; }
> +	const LayerManager *layerManager() const { return layerManager_.get(); }

It doesn't look like a real problem but still, it feels like an abuse of
unique_ptr.  Should shared_ptr be used?

>  protected:
>  	void run() override;
> @@ -73,7 +73,7 @@ private:
>  	std::unique_ptr<DeviceEnumerator> enumerator_;
>  
>  	std::unique_ptr<IPAManager> ipaManager_;
> -	LayerManager layerManager_;
> +	std::unique_ptr<LayerManager> layerManager_;
>  
>  	const GlobalConfiguration configuration_;
>  };
> diff --git a/include/libcamera/internal/layer_manager.h b/include/libcamera/internal/layer_manager.h
> index 8427ec3eb5d9..82a6a1d82461 100644
> --- a/include/libcamera/internal/layer_manager.h
> +++ b/include/libcamera/internal/layer_manager.h
> @@ -26,6 +26,8 @@
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  
> +#include "libcamera/internal/global_configuration.h"
> +
>  namespace libcamera {
>  
>  LOG_DECLARE_CATEGORY(LayerLoaded)
> @@ -150,7 +152,9 @@ struct LayerInstance {
>  class LayerController
>  {
>  public:
> -	LayerController(const Camera *camera, const ControlList &properties,
> +	LayerController(const Camera *camera,
> +			const GlobalConfiguration &configuration,

There are plans to generally pass CameraManager instead.  This is a part
of the huge Laurent's patch series, which may hang around for a while
before it gets merged.  I'd suggest keeping eye on it and change the
argument if/when needed.

> +			const ControlList &properties,
>  			const ControlInfoMap &controlInfoMap,
>  			const std::map<std::string, std::shared_ptr<LayerLoaded>> &layers);
>  	~LayerController();
> @@ -190,7 +194,7 @@ private:
>  class LayerManager
>  {
>  public:
> -	LayerManager();
> +	LayerManager(const GlobalConfiguration &configuration);
>  	~LayerManager() = default;
>  
>  	std::unique_ptr<LayerController>
> @@ -200,6 +204,8 @@ public:
>  
>  private:
>  	std::map<std::string, std::shared_ptr<LayerLoaded>> layers_;
> +
> +	const GlobalConfiguration &configuration_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 554cd935339a..a8e7cfe9daff 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -43,6 +43,7 @@ CameraManager::Private::Private()
>  	: Thread("CameraManager"), initialized_(false)
>  {
>  	ipaManager_ = std::make_unique<IPAManager>(this->configuration());
> +	layerManager_ = std::make_unique<LayerManager>(this->configuration());
>  }
>  
>  int CameraManager::Private::start()
> diff --git a/src/libcamera/layer_manager.cpp b/src/libcamera/layer_manager.cpp
> index bef69f6bd04b..445e26234b23 100644
> --- a/src/libcamera/layer_manager.cpp
> +++ b/src/libcamera/layer_manager.cpp
> @@ -25,6 +25,7 @@
>  #include <libcamera/control_ids.h>
>  #include <libcamera/layer.h>
>  
> +#include "libcamera/internal/global_configuration.h"
>  #include "libcamera/internal/utils.h"
>  
>  /**
> @@ -276,6 +277,7 @@ LayerLoaded::LayerLoaded(const std::string &filename)
>  /**
>   * \brief Initialize the Layers
>   * \param[in] camera The Camera for whom to initialize layers
> + * \param[in] configuration The global configuration
>   * \param[in] properties The Camera properties
>   * \param[in] controlInfoMap The Camera controls
>   * \param[in] layers Map of available layers
> @@ -290,29 +292,26 @@ LayerLoaded::LayerLoaded(const std::string &filename)
>   * efficiently returned at properties() and controls(), respectively.
>   */
>  LayerController::LayerController(const Camera *camera,
> +				 const GlobalConfiguration &configuration,
>  				 const ControlList &properties,
>  				 const ControlInfoMap &controlInfoMap,
>  				 const std::map<std::string, std::shared_ptr<LayerLoaded>> &layers)
>  {
>  	/* Order the layers */
> -	/* \todo Document this. First is closer to application, last is closer to libcamera */
> -	/* \todo Get this from configuration file */
> -	const char *layerList = utils::secure_getenv("LIBCAMERA_LAYERS_ENABLE");
> -	if (layerList) {
> -		for (const auto &layerName : utils::split(layerList, ",")) {
> -			if (layerName.empty())
> -				continue;
> -
> -			const auto &it = layers.find(layerName);
> -			if (it == layers.end()) {
> -				LOG(LayerController, Warning)
> -					<< "Requested layer '" << layerName
> -					<< "' not found";
> -				continue;
> -			}
> -
> -			executionQueue_.emplace_back(std::make_unique<LayerInstance>(it->second));
> +	std::vector<std::string> configLayers =
> +		configuration.envListOption("LIBCAMERA_LAYERS_ENABLE", { "layer", "layers" }, ",")
> +			     .value_or(std::vector<std::string>());
> +
> +	for (const std::string &layerName : configLayers) {
> +		const auto &it = layers.find(layerName);
> +		if (it == layers.end()) {
> +			LOG(LayerController, Warning)
> +				<< "Requested layer '" << layerName
> +				<< "' not found";
> +			continue;
>  		}
> +
> +		executionQueue_.emplace_back(std::make_unique<LayerInstance>(it->second));
>  	}
>  
>  	for (std::unique_ptr<LayerInstance> &layer : executionQueue_)
> @@ -532,7 +531,8 @@ void LayerController::stop()
>   * LayerController is responsible for organizing them into queues to be
>   * executed, and for managing closures for each Camera that they belong to.
>   */
> -LayerManager::LayerManager()
> +LayerManager::LayerManager(const GlobalConfiguration &configuration)
> +	: configuration_(configuration)
>  {
>  	/* This is so that we can capture it in the lambda below */
>  	std::map<std::string, std::shared_ptr<LayerLoaded>> &layers = layers_;
> @@ -560,19 +560,15 @@ LayerManager::LayerManager()
>  	};
>  
>  	/* User-specified paths take precedence. */
> -	/* \todo Document this */
> -	const char *layerPaths = utils::secure_getenv("LIBCAMERA_LAYER_PATH");
> -	if (layerPaths) {
> -		for (const auto &dir : utils::split(layerPaths, ":")) {
> -			if (dir.empty())
> -				continue;
> -
> -			/*
> -			 * \todo Move the shared objects into one directory
> -			 * instead of each in their own subdir
> -			 */
> -			utils::findSharedObjects(dir.c_str(), 1, soHandler);
> -		}
> +	std::vector<std::string> layerPaths =
> +		configuration.envListOption("LIBCAMERA_LAYER_PATH", { "layer", "path" })
> +			     .value_or(std::vector<std::string>());
> +	for (const auto &dir : layerPaths) {
> +		/*
> +		 * \todo Move the shared objects into one directory
> +		 * instead of each in their own subdir
> +		 */
> +		utils::findSharedObjects(dir.c_str(), 1, soHandler);
>  	}
>  
>  	/*
> @@ -606,7 +602,8 @@ LayerManager::createController(const Camera *camera,
>  			       const ControlList &properties,
>  			       const ControlInfoMap &controlInfoMap) const
>  {
> -	return std::make_unique<LayerController>(camera, properties, controlInfoMap, layers_);
> +	return std::make_unique<LayerController>(camera, configuration_,
> +						 properties, controlInfoMap, layers_);
>  }
>  
>  } /* namespace libcamera */