[libcamera-devel] [RFC PATCH v2 05/14] test: yaml-parser: Test dictionary items ordering
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jun 16 10:10:07 CEST 2022
Hi David,
On Mon, Jun 13, 2022 at 12:48:55PM +0300, Laurent Pinchart via libcamera-devel wrote:
> On Mon, Jun 13, 2022 at 10:25:15AM +0100, David Plowman wrote:
> > 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?
>
> This patch series guarantees ordering of entries in mapping (at the
> expense of duplicated key storage in memory, but that could probably be
> fixed), so there should be no breakage. The main drawback is that
> YamlParser and YamlObject will then expose an order that is
> implementation-specific and shouldn't be relied on by applications
> according to the JSON and YAML specifications. There's thus a risk of
> introducing new code that relies on mappings being ordered, which
> wouldn't be the case if the implementation didn't guarantee ordering
> (although one may argue that in the case the users of YamlObject may
> still rely on a different implementation-specific order without
> realizing it, we would need to randomize the order to avoid that, which
> I don't think is a good idea).
>
> With versioned tuning files the IPA could fairly easily support both the
> current format and the new format, so that shouldn't be a problem. I
> think we should then provide a Python script to convert the old format
> to the new one, and print a warning to the log. Would you expect the
> Raspberry Pi IPA module to support the current format forever, or only
> for a fixed duration to help users transition ?
I'll post a v3 series of the YamlObject changes, and I'd like to decide
on which direction to take (if possible :-)). Could you share your
thoughts on this ?
> > On Mon, 13 Jun 2022 at 09:46, Laurent Pinchart wrote:
> > > 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
More information about the libcamera-devel
mailing list