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

[Laurent Pinchart]

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