[libcamera-devel] [PATCH v5 5/8] ipa: raspberrypi: Introduce version 2.0 format for the camera tuning file

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Thu, Jul 14, 2022 at 04:24:06PM +0100, Naushir Patuck via libcamera-devel wrote:
> The existing tuning file format (version 1.0) requires the controller algorithms
> to run in the same order as listied in the JSON structure. The JSON

s/listied/listed/

> specification does not mandate any such ordering, but the Boost JSON parser
> would maintain this order.
> 
> In order to remove this reliance on the parser to provide ordering, introduce a
> new version 2.0 format for the camera tuning file. In this version, the
> algorithms are specified in a top level list node ("algorithms"), which does
> require strict ordering of the elements. Additionally, a "version" node is added
> to distinguish between the version 1.0 and 2.0 formats. The absence of the
> "version" node implies version 1.0.
> 
> Update the controller to support either version of the tuning file by looking
> at the version node. CreateAlgorithm member function to now load and configure
> each algorithm. Additionally, make CreateAlgorithm a private member, it does not
> get called externally.
> 
> If a version 1.0 format tuning file is used, throw a warning message indicating
> it will be soon deprecated.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>

Sounds like a good plan.

A question on the versioning scheme though. In the rkisp1 IPA modules,
we use (for now) a single integer for the version. Do you see use cases
for a semantic versioning scheme (x.y or x.y.z) ? I'm not implying there
are none, but we haven't really considered the versioning scheme yet,
and it seems like a good time to discuss this.

I assume major new versions of tuning files will not be backward
compatible with previous ones, so if an IPA module supports version
[min, max], any tuning file with a version number outside that range
would be rejected.

I can also imagine changes to the tuning file format that would be
backward-compatible (mostly adding new data, when the implementation of
the algorithms can use default values if the data isn't present). In
this case, could an IPA module benefit from knowing in advance, through
a minor version number, that additional data may be present, or would it
only use the major version number only and just use default values for
absent data ? I'm not sure if the minor version number would be useful
for the IPA module, but I could also imagine it may be useful to other
tools, such as tuning tools, to know what to expect.

> ---
>  src/ipa/raspberrypi/controller/controller.cpp | 42 +++++++++++++------
>  src/ipa/raspberrypi/controller/controller.hpp |  4 +-
>  2 files changed, 33 insertions(+), 13 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/controller.cpp b/src/ipa/raspberrypi/controller/controller.cpp
> index 67d650ef0c1b..9a2a3fb01793 100644
> --- a/src/ipa/raspberrypi/controller/controller.cpp
> +++ b/src/ipa/raspberrypi/controller/controller.cpp
> @@ -42,22 +42,40 @@ void Controller::Read(char const *filename)
>  	}
>  
>  	std::unique_ptr<YamlObject> root = YamlParser::parse(file);
> -
> -	for (auto const &[key, value] : root->asDict()) {
> -		Algorithm *algo = CreateAlgorithm(key.c_str());
> -		if (algo) {
> -			algo->Read(value);
> -			algorithms_.push_back(AlgorithmPtr(algo));
> -		} else
> -			LOG(RPiController, Warning)
> -				<< "No algorithm found for \"" << key << "\"";
> +	double version = (*root)["version"].get<double>(1.0);

Interesting idea to consider it as a duoble.

> +	if (version < 2.0) {
> +		LOG(RPiController, Warning)
> +			<< "This format of the tuning file will be deprecated soon!"
> +			<< " Please use the convert_tuning.py utility to update to version 2.0.";
> +		for (auto const &[key, value] : root->asDict()) {
> +			if (!key.compare("version"))
> +				continue;

I think you can drop this check, as v1 tuning files don't specify the
version.

> +			CreateAlgorithm(key, value);
> +		}
> +	} else {

	} else if (version < 3.0) {
		...
	} else {
		/* error */
	}

But maybe a

	switch (std::floor(version)) {
		...
	}

would be more readable ? Uo to you.

> +		if (!root->contains("algorithms")) {
> +			LOG(RPiController, Fatal)
> +				<< "Tuning file " << filename
> +				<< " does not have an \"algorithms\" list!";
> +			return;
> +		}
> +		for (auto const &rootAlgo : (*root)["algorithms"].asList())
> +			for (auto const &[key, value] : rootAlgo.asDict())

I'm tempted to create a asOrderedDict() function to make this easier.

> +				CreateAlgorithm(key, value);
>  	}
>  }
>  
> -Algorithm *Controller::CreateAlgorithm(char const *name)
> +void Controller::CreateAlgorithm(const std::string &name, const YamlObject &params)
>  {
> -	auto it = GetAlgorithms().find(std::string(name));
> -	return it != GetAlgorithms().end() ? (*it->second)(this) : nullptr;
> +	auto it = GetAlgorithms().find(name);
> +	if (it == GetAlgorithms().end()) {
> +		LOG(RPiController, Warning)
> +			<< "No algorithm found for \"" << name << "\"";
> +		return;
> +	}
> +	Algorithm *algo = (*it->second)(this);
> +	algo->Read(params);
> +	algorithms_.push_back(AlgorithmPtr(algo));
>  }
>  
>  void Controller::Initialise()
> diff --git a/src/ipa/raspberrypi/controller/controller.hpp b/src/ipa/raspberrypi/controller/controller.hpp
> index 3b50ae770d11..9091deac5feb 100644
> --- a/src/ipa/raspberrypi/controller/controller.hpp
> +++ b/src/ipa/raspberrypi/controller/controller.hpp
> @@ -15,6 +15,8 @@
>  
>  #include <linux/bcm2835-isp.h>
>  
> +#include "libcamera/internal/yaml_parser.h"

You can also forward-declare YamlObject, up to you.

> +
>  #include "camera_mode.h"
>  #include "device_status.h"
>  #include "metadata.hpp"
> @@ -36,7 +38,6 @@ public:
>  	Controller();
>  	Controller(char const *json_filename);
>  	~Controller();
> -	Algorithm *CreateAlgorithm(char const *name);
>  	void Read(char const *filename);
>  	void Initialise();
>  	void SwitchMode(CameraMode const &camera_mode, Metadata *metadata);
> @@ -46,6 +47,7 @@ public:
>  	Algorithm *GetAlgorithm(std::string const &name) const;
>  
>  protected:
> +	void CreateAlgorithm(const std::string &name, const libcamera::YamlObject &params);
>  	Metadata global_metadata_;
>  	std::vector<AlgorithmPtr> algorithms_;
>  	bool switch_mode_called_;

-- 
Regards,

Laurent Pinchart