[libcamera-devel] [PATCH v5 5/8] ipa: raspberrypi: Introduce version 2.0 format for the camera tuning file
Naushir Patuck
naush at raspberrypi.com
Fri Jul 15 09:38:17 CEST 2022
Hi Laurent,
Thank you for your feedback.
On Fri, 15 Jul 2022 at 00:26, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> 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.
>
As you spotted below, I used a float type for versioning to allow x.y
schemes.
I never really considered a string for x.y.z to be honest, I doubt we will
need
that level of versioning. However, I am happy to change to a string type if
that feels safer. Maybe David can share his thoughts when he is back.
I agree with your other points, major revisions will not be backward
compatible
with previous revisions, and if the IPA encounters a version outside of its
range
it will have to reject it and fail the application.
Adding new data fields to a tuning file possibly implies bumping up the
minor
revision number. In these cases, reading them through an older IPA library
ought to "just work" as those values will simply be ignored - and probably
throw
a warning message which is a good thing.
Regards,
Naush
>
> > ---
> > 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 ¶ms)
> > {
> > - 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 ¶ms);
> > Metadata global_metadata_;
> > std::vector<AlgorithmPtr> algorithms_;
> > bool switch_mode_called_;
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220715/8c7f669a/attachment.htm>
More information about the libcamera-devel
mailing list