sony / nmos-cpp

An NMOS (Networked Media Open Specifications) Registry and Node in C++ (IS-04, IS-05)
Apache License 2.0
136 stars 79 forks source link

Strip leading zeroes from SDP #336

Closed maweit closed 11 months ago

maweit commented 1 year ago

There are IS-05 compatible devices out in the field which supply SDP files where e.g. the session-id comes with leading zeroes (looks like: o=- 00098979940036). This patch strips our leading zeroes from session-id and session-version.

garethsb commented 1 year ago

Just paraphrasing comments from off-line thread:

If we look at the RFC 4566 ABNF grammar, there are several instances of 1*DIGIT, which therefore could have leading zeros, and there are a few instances of e.g. POS-DIGIT *DIGIT which don’t allow leading zeros.

1*DIGIT is used for

For all of these except sess-id and sess-version, it doesn’t seem important to me to maintain knowledge of the leading zeros, even though we couldn’t roundtrip the exact SDP text format from our JSON representation.

But since sess-id is a “numeric string” would it matter if we couldn’t output leading zeros? I.e. if we just fix the parsing to accept/ignore leading zeros, if you parsed and regenerated the example SDP file, the leading zeros will be gone. The only way to get roundtrip is to keep the values that allow leading zeros as strings, I think… But that changes the JSON data model. ... If we accept the inability to roundtrip. I don't think it's quite as simple as stripping leading zeros, because the fix should probably be applied in number_converter, which must also be able to handle floating point, and simple integer '0', so we need to avoid stripping a final zero before end of string or before decimal point, maybe? Test cases needed!

garethsb commented 1 year ago

To be clear, at the moment, leading zeros are rejected with an sdp_exception, so we definitely need to do something!

garethsb commented 1 year ago

https://grammarhow.com/zeroes-or-zeros/ 😄

garethsb commented 1 year ago

In terms of implementation, I think having a separate converter for 1*DIGIT than for POS-DIGIT *DIGIT as @maweit proposes makes sense.

I think js_number_converter isn't the right name for the former. number_with_leading_zeros_converter is too long-winded... how about digits_converter?

Then we need to decide whether to also use it for the other instances of 1*DIGIT that I identified, e.g. <proto-version>, <bandwidth>, <port> and so on...

lo-simon commented 11 months ago

Fixed in #340