<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 15 Jul 2022 at 00:26, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@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">Hi Naush,<br>
<br>
Thank you for the patch.<br>
<br>
On Thu, Jul 14, 2022 at 04:24:06PM +0100, Naushir Patuck via libcamera-devel wrote:<br>
> The existing tuning file format (version 1.0) requires the controller algorithms<br>
> to run in the same order as listied in the JSON structure. The JSON<br>
<br>
s/listied/listed/<br>
<br>
> specification does not mandate any such ordering, but the Boost JSON parser<br>
> would maintain this order.<br>
> <br>
> In order to remove this reliance on the parser to provide ordering, introduce a<br>
> new version 2.0 format for the camera tuning file. In this version, the<br>
> algorithms are specified in a top level list node ("algorithms"), which does<br>
> require strict ordering of the elements. Additionally, a "version" node is added<br>
> to distinguish between the version 1.0 and 2.0 formats. The absence of the<br>
> "version" node implies version 1.0.<br>
> <br>
> Update the controller to support either version of the tuning file by looking<br>
> at the version node. CreateAlgorithm member function to now load and configure<br>
> each algorithm. Additionally, make CreateAlgorithm a private member, it does not<br>
> get called externally.<br>
> <br>
> If a version 1.0 format tuning file is used, throw a warning message indicating<br>
> it will be soon deprecated.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
<br>
Sounds like a good plan.<br>
<br>
A question on the versioning scheme though. In the rkisp1 IPA modules,<br>
we use (for now) a single integer for the version. Do you see use cases<br>
for a semantic versioning scheme (x.y or x.y.z) ? I'm not implying there<br>
are none, but we haven't really considered the versioning scheme yet,<br>
and it seems like a good time to discuss this.<br>
<br>
I assume major new versions of tuning files will not be backward<br>
compatible with previous ones, so if an IPA module supports version<br>
[min, max], any tuning file with a version number outside that range<br>
would be rejected.<br>
<br>
I can also imagine changes to the tuning file format that would be<br>
backward-compatible (mostly adding new data, when the implementation of<br>
the algorithms can use default values if the data isn't present). In<br>
this case, could an IPA module benefit from knowing in advance, through<br>
a minor version number, that additional data may be present, or would it<br>
only use the major version number only and just use default values for<br>
absent data ? I'm not sure if the minor version number would be useful<br>
for the IPA module, but I could also imagine it may be useful to other<br>
tools, such as tuning tools, to know what to expect.<br></blockquote><div><br></div><div>As you spotted below, I used a float type for versioning to allow x.y schemes.</div><div>I never really considered a string for x.y.z to be honest, I doubt we will need</div><div>that level of versioning. However, I am happy to change to a string type if</div><div>that feels safer. Maybe David can share his thoughts when he is back.</div><div><br></div><div>I agree with your other points, major revisions will not be backward compatible</div><div>with previous revisions, and if the IPA encounters a version outside of its range</div><div>it will have to reject it and fail the application.</div><div><br></div><div>Adding new data fields to a tuning file possibly implies bumping up the minor</div><div>revision number. In these cases, reading them through an older IPA library</div><div>ought to "just work" as those values will simply be ignored - and probably throw</div><div>a warning message which is a good thing.</div><div><br></div><div>Regards,</div><div>Naush</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>
> src/ipa/raspberrypi/controller/controller.cpp | 42 +++++++++++++------<br>
> src/ipa/raspberrypi/controller/controller.hpp | 4 +-<br>
> 2 files changed, 33 insertions(+), 13 deletions(-)<br>
> <br>
> diff --git a/src/ipa/raspberrypi/controller/controller.cpp b/src/ipa/raspberrypi/controller/controller.cpp<br>
> index 67d650ef0c1b..9a2a3fb01793 100644<br>
> --- a/src/ipa/raspberrypi/controller/controller.cpp<br>
> +++ b/src/ipa/raspberrypi/controller/controller.cpp<br>
> @@ -42,22 +42,40 @@ void Controller::Read(char const *filename)<br>
> }<br>
> <br>
> std::unique_ptr<YamlObject> root = YamlParser::parse(file);<br>
> -<br>
> - for (auto const &[key, value] : root->asDict()) {<br>
> - Algorithm *algo = CreateAlgorithm(key.c_str());<br>
> - if (algo) {<br>
> - algo->Read(value);<br>
> - algorithms_.push_back(AlgorithmPtr(algo));<br>
> - } else<br>
> - LOG(RPiController, Warning)<br>
> - << "No algorithm found for \"" << key << "\"";<br>
> + double version = (*root)["version"].get<double>(1.0);<br>
<br>
Interesting idea to consider it as a duoble.<br>
<br>
> + if (version < 2.0) {<br>
> + LOG(RPiController, Warning)<br>
> + << "This format of the tuning file will be deprecated soon!"<br>
> + << " Please use the convert_tuning.py utility to update to version 2.0.";<br>
> + for (auto const &[key, value] : root->asDict()) {<br>
> + if (!key.compare("version"))<br>
> + continue;<br>
<br>
I think you can drop this check, as v1 tuning files don't specify the<br>
version.<br>
<br>
> + CreateAlgorithm(key, value);<br>
> + }<br>
> + } else {<br>
<br>
} else if (version < 3.0) {<br>
...<br>
} else {<br>
/* error */<br>
}<br>
<br>
But maybe a<br>
<br>
switch (std::floor(version)) {<br>
...<br>
}<br>
<br>
would be more readable ? Uo to you.<br>
<br>
> + if (!root->contains("algorithms")) {<br>
> + LOG(RPiController, Fatal)<br>
> + << "Tuning file " << filename<br>
> + << " does not have an \"algorithms\" list!";<br>
> + return;<br>
> + }<br>
> + for (auto const &rootAlgo : (*root)["algorithms"].asList())<br>
> + for (auto const &[key, value] : rootAlgo.asDict())<br>
<br>
I'm tempted to create a asOrderedDict() function to make this easier.<br>
<br>
> + CreateAlgorithm(key, value);<br>
> }<br>
> }<br>
> <br>
> -Algorithm *Controller::CreateAlgorithm(char const *name)<br>
> +void Controller::CreateAlgorithm(const std::string &name, const YamlObject ¶ms)<br>
> {<br>
> - auto it = GetAlgorithms().find(std::string(name));<br>
> - return it != GetAlgorithms().end() ? (*it->second)(this) : nullptr;<br>
> + auto it = GetAlgorithms().find(name);<br>
> + if (it == GetAlgorithms().end()) {<br>
> + LOG(RPiController, Warning)<br>
> + << "No algorithm found for \"" << name << "\"";<br>
> + return;<br>
> + }<br>
> + Algorithm *algo = (*it->second)(this);<br>
> + algo->Read(params);<br>
> + algorithms_.push_back(AlgorithmPtr(algo));<br>
> }<br>
> <br>
> void Controller::Initialise()<br>
> diff --git a/src/ipa/raspberrypi/controller/controller.hpp b/src/ipa/raspberrypi/controller/controller.hpp<br>
> index 3b50ae770d11..9091deac5feb 100644<br>
> --- a/src/ipa/raspberrypi/controller/controller.hpp<br>
> +++ b/src/ipa/raspberrypi/controller/controller.hpp<br>
> @@ -15,6 +15,8 @@<br>
> <br>
> #include <linux/bcm2835-isp.h><br>
> <br>
> +#include "libcamera/internal/yaml_parser.h"<br>
<br>
You can also forward-declare YamlObject, up to you.<br>
<br>
> +<br>
> #include "camera_mode.h"<br>
> #include "device_status.h"<br>
> #include "metadata.hpp"<br>
> @@ -36,7 +38,6 @@ public:<br>
> Controller();<br>
> Controller(char const *json_filename);<br>
> ~Controller();<br>
> - Algorithm *CreateAlgorithm(char const *name);<br>
> void Read(char const *filename);<br>
> void Initialise();<br>
> void SwitchMode(CameraMode const &camera_mode, Metadata *metadata);<br>
> @@ -46,6 +47,7 @@ public:<br>
> Algorithm *GetAlgorithm(std::string const &name) const;<br>
> <br>
> protected:<br>
> + void CreateAlgorithm(const std::string &name, const libcamera::YamlObject ¶ms);<br>
> Metadata global_metadata_;<br>
> std::vector<AlgorithmPtr> algorithms_;<br>
> bool switch_mode_called_;<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>