[libcamera-devel] [RFC PATCH v2 05/14] test: yaml-parser: Test dictionary items ordering

[David Plowman]

Hi Laurent, everyone

Thanks for the suggestions. I'm fine to change the syntax to make
things clearer, but I wonder if we could avoid breaking existing JSON
files? There are probably low numbers of them out there beyond the
ones that we've supplied, but you never quite know and backwards
compatibility is still a nice thing. Do you think that's something we
can arrange?

David

On Mon, 13 Jun 2022 at 09:46, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Naush,
>
> On Mon, Jun 13, 2022 at 09:05:21AM +0100, Naushir Patuck wrote:
> > On Mon, 13 Jun 2022 at 07:18, Laurent Pinchart wrote:
> > > On Mon, Jun 13, 2022 at 01:22:19PM +0900, paul.elder at ideasonboard.com wrote:
> > > > On Sat, Jun 04, 2022 at 09:59:30PM +0300, Laurent Pinchart wrote:
> > > > > The order of items in a YAML dictionary may matter. Update the test to
> > > > > ensure that it is preserved. The test currently fails at the YamlParser
> > > >
> > > > My understanding is that YAML mappings are unordered [1] [2], and if
> > > > order in the mapping is significant, then either a sequence of mappings
> > > > [3] or flow mapping adjacent values [4] should be used.
> > >
> > > That's a very good point. [5] even mentions
> > >
> > > "while imposing a order on mapping keys is necessary for flattening YAML
> > > representations to a sequential access medium, this serialization detail
> > > must not be used to convey application level information."
> > >
> > > The same applies to JSON ([6]).
> > >
> > > Fixing this would require changing the syntax of the tuning files. It's
> > > inconvenient, but not doing so opens the door to more issues in the
> > > future :-S
> >
> > Given that the IPA does require ordering and we have been lucky with
> > Boost preserving order in the JSON parser, I think we probably ought
> > to specify ordering in the config file with a specific key.
>
> Sounds good to me.
>
> It could be done through a priority key in each algorithm, or by
> converting the mapping to a list. In YAML format, that would be moving
> from
>
> {
>     "rpi.black_level":
>     {
>         "black_level": 4096
>     },
>     "rpi.noise":
>     {
>         "reference_constant": 0,
>         "reference_slope": 3.67
>     },
> }
>
> to
>
> [
>     {
>         "rpi.black_level":
>         {
>             "black_level": 4096
>         },
>     },
>     {
>         "rpi.noise":
>         {
>             "reference_constant": 0,
>             "reference_slope": 3.67
>         },
>     }
> ]
>
> In YAML format, it would translate as a move from
>
> rpi.black_level:
>   black_level: 4096
> rpi.noise:
>   reference_constant: 0
>   reference_slope: 3.67
>
> to
>
> - rpi.black_level:
>     black_level: 4096
> - rpi.noise:
>     reference_constant: 0
>     reference_slope: 3.67
>
> As we have to change the tuning files, I'd like to take this as an
> opportunity to add a format version. Something along those lines maybe ?
>
> version: 1.0
> algorithms:
>   - rpi.black_level:
>       black_level: 4096
>   - rpi.noise:
>       reference_constant: 0
>       reference_slope: 3.67
>
> And while we're discussing this, does someone know about best practice
> rules to design JSON/YAML grammars ? I've been wondering for a long time
> if the following grammar would have any advantage:
>
> version: 1.0
> algorithms:
>   - name: rpi.black_level
>     data:
>       black_level: 4096
>   - name: rpi.noise:
>     data:
>       reference_constant: 0
>       reference_slope: 3.67
>
> > When I get a
> > change, I'll look to add a patch to allow this on the existing codebase,
> > and this series ought to "just work" after that.
> >
> > > > [1] https://yaml.org/spec/1.2.2/#mapping
> > > > [2] https://yaml.org/spec/1.2.2/#3221-mapping-key-order
> > > > [3] https://yaml.org/spec/1.2.2/#example-ordered-mappings
> > > > [4] https://yaml.org/spec/1.2.2/#example-flow-mapping-adjacent-values
> > >
> > > [5] https://yaml.org/spec/1.2.2/#32-information-models
> > > [6] https://www.json.org/json-en.html
> > >
> > > > > doesn't correctly preserve the order, this will be fixed by the next
> > > > > commit.
> > > > >
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > > ---
> > > > >  test/yaml-parser.cpp | 7 +++----
> > > > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/test/yaml-parser.cpp b/test/yaml-parser.cpp
> > > > > index 5ff4c3236dbf..582c9caed836 100644
> > > > > --- a/test/yaml-parser.cpp
> > > > > +++ b/test/yaml-parser.cpp
> > > > > @@ -29,8 +29,8 @@ static const string testYaml =
> > > > >     "  - Mary\n"
> > > > >     "dictionary:\n"
> > > > >     "  a: 1\n"
> > > > > -   "  b: 2\n"
> > > > >     "  c: 3\n"
> > > > > +   "  b: 2\n"
> > > > >     "level1:\n"
> > > > >     "  level2:\n"
> > > > >     "    - [1, 2]\n"
> > > > > @@ -428,7 +428,6 @@ protected:
> > > > >             }
> > > > >
> > > > >             auto memeberNames = dictObj.memberNames();
> > > > > -           sort(memeberNames.begin(), memeberNames.end());
> > > > >
> > > > >             if (memeberNames.size() != 3) {
> > > > >                     cerr << "Dictionary object fail to extra member names" << std::endl;
> > > > > @@ -436,8 +435,8 @@ protected:
> > > > >             }
> > > > >
> > > > >             if (memeberNames[0] != "a" ||
> > > > > -               memeberNames[1] != "b" ||
> > > > > -               memeberNames[2] != "c") {
> > > > > +               memeberNames[1] != "c" ||
> > > > > +               memeberNames[2] != "b") {
> > > > >                     cerr << "Dictionary object fail to parse member names" << std::endl;
> > > > >                     return TestFail;
> > > > >             }
>
> --
> Regards,
>
> Laurent Pinchart