Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify distinctions of 0 values from missing elements for new non-mandatory flag elements #453

Closed
rcombs opened this issue Feb 21, 2021 · 10 comments · Fixed by #455
Closed

Comments

@rcombs
Copy link
Contributor

rcombs commented Feb 21, 2021

Following up on #394 and #449. It sounds like existing flag elements are going to be designated as simple booleans (where not-present always means the same as either 0 or 1, depending on which flag), but when discussing with @mkver, we came to the conclusion that it'd be useful for the newly-added flags (original, commentary, HI/VI, etc) to be able to be explicitly specified as 0, and for this to be distinct from not setting them at all. My understanding is that this is the behavior specified by the current spec and RFC, but it's unclear when looking at the current spec, particularly since the description of the "default" field on the Matroska website doesn't match what's in the RFC. I'm not sure how best to clarify this; maybe something in the notes file? Or possibly just by updating the legend entry for "Default" in the Matroska-Org/Infrastructure repo. Maybe this GHI would be more appropriate in that repo, it's just a little awkward since the documentation text is split across 2 places.

@robUx4
Copy link
Contributor

robUx4 commented Feb 21, 2021

While the website is a good/quick/easy source to find something, it's no longer the most authoritative source. The Matroska RFC was pretty much the website content put in a RFC and then edited/formatted/corrected. The website is likely missing a lot of these changes.

As for the new track flags, you're correct in assuming if they are not present the meaning is different than being present (with a length of 0). They are not mandatory, so nothing written means we don't know.

@rcombs
Copy link
Contributor Author

rcombs commented Feb 21, 2021

Right, just, as-is someone reading the docs can easily come to the incorrect conclusion that 0 and unset are the same here, and I'm concerned that it'll result in improper implementation code. In particular, @mkver pointed out that the current mkvmerge code (and help text) doesn't clearly expose the distinction.

@mbunkus
Copy link
Contributor

mbunkus commented Feb 21, 2021

While the website is a good/quick/easy source to find something, it's no longer the most authoritative source. The Matroska RFC was pretty much the website content put in a RFC and then edited/formatted/corrected. The website is likely missing a lot of these changes.

The whole "Technical/Info" tree of pages is generated directly from the Matroska and EBML RFCs. It is definitely up to date. When in doubt, look at the element specs page. Right below the "Matroska v4 element specification" header there's the information about the revision & date of the spec that was used to build the pages from.

The proper way to fix this heading is the usual one: PR against the matroska-specification repo with the intended changes. I usually update the web site from spec repo changes (by updating the sub-module in the infrastructure project) often enough; if not, just ping me.

In particular, @mkver pointed out that the current mkvmerge code (and help text) doesn't clearly expose the distinction.

True. That's because libebml doesn't handle those elements correctly yet (issues Matroska-Org/libebml#72 and Matroska-Org/libebml#73). Until those two have been fixed, there's no (sensible) way to fix MKVToolNix, unfortunately.

Pretty much all software out there acts according to how things were explained before the EBML RFC came out (element missing = use default value if present, no matter if element is mandatory or not), not just MKVToolNix.

@rcombs
Copy link
Contributor Author

rcombs commented Feb 21, 2021

The whole "Technical/Info" tree of pages is generated directly from the Matroska and EBML RFCs.

The "Legend" section of the "Element Specifications" page isn't, and that's where the issue I'm referring to is; it's raw HTML within ebml_schema2spectable.xsl, which I linked to in the top post here. Maybe that text should be pulled in from a canonical source, though?

@mbunkus
Copy link
Contributor

mbunkus commented Feb 21, 2021

Oh right, sorry. The current "usual way" is to file a PR against the infrastructure repo.

Maybe that text should be pulled in from a canonical source, though?

I guess the problem is that writing content for an RFC isn't exactly the same as writing content for a web page. That being said, if we can find a way to incorporate the information (= the annotation of the table headers) into the RFC, I'd be all for it.

@mkver
Copy link
Contributor

mkver commented Feb 21, 2021

How about we simply delete the default values of the new elements? This would not change anything with parsers compliant to the new specs, but it would not confuse developers with the old default value semantics in mind (and especially for FlagOriginal it seems more natural not to set a default value at all). And it might also allow to fix MKVToolNix immediately, without the need to fix libebml first (after all, non-mandatory elements without default values are nothing new).

Of course it should still be documented that the flag being present with a value of zero and the flag being absent have different meanings.

@robUx4
Copy link
Contributor

robUx4 commented Feb 23, 2021

Yes, elements with a default value but not mandatory could as well do without a default value at all. The length=0 feature is pretty much unused right now and the saving is minimal. Also the fact it's not used is also because there's not much benefit.

That being said, for backward compatibility we should keep the old ones (as least up to v3) the way they are. We will definitely fix libebml as well.

@mbunkus
Copy link
Contributor

mbunkus commented Feb 23, 2021

Yes, elements with a default value but not mandatory could as well do without a default value at all.

I'd welcome that as to avoid confusion. I agree that length=0 isn't used a lot (if at all) and it doesn't really save much space — especially for elements that are solely used once or twice in headers.

Things would obviously be different for elements that are used repeatedly, e.g. in each BlockGroup, where saving space really makes sense.

@mkver
Copy link
Contributor

mkver commented Mar 1, 2021

I just opened PR #455 for this.

@robUx4
Copy link
Contributor

robUx4 commented Apr 11, 2021

Given this issue was related to the new track flags, we can assume this issue is closed ? They no longer have a default value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants