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

Default values of FlagOriginal, FlagCommentary etc. #449

Closed
mkver opened this issue Jan 31, 2021 · 10 comments
Closed

Default values of FlagOriginal, FlagCommentary etc. #449

mkver opened this issue Jan 31, 2021 · 10 comments
Labels

Comments

@mkver
Copy link
Contributor

mkver commented Jan 31, 2021

The elements recently added in #447 (namely FlagOriginal, FlagCommentary, FlagHearingImpaired, FlagVisualImpaired and FlagTextDescriptions) all have a default value of zero. This means that all tracks not having any of these flags (i.e. any track in any Matroska file ever made before this) is now marked as dubbed, as not a commentary, as unsuitable for the hearing impaired, as unsuitable for the visually impaired and as not containing textual descriptions of video content. This changes the semantics of all these files (often in unintended ways) which obviously mustn't happen.

I see two ways to fix this: Remove the default values or set them to something that means "not indicated". Which way is the preferred one?

@rcombs
Copy link
Contributor

rcombs commented Feb 1, 2021

Hmmmmm, the intent on these flags was that they'd be interpreted as "not indicated" on any file where no track has a given flag set, but I can see how it'd be useful to more explicitly mark a track as not having one of them (though that wouldn't be possible to expose in e.g. libavformat, where these flags already exist as simple container-generic booleans and no matroska track has them).
I've got no objection to a default value change, though.

@robUx4
Copy link
Contributor

robUx4 commented Feb 1, 2021

These elements don't have minOccurs=1, meaning they are minOccurs=0. They are not mandatory elements. So if they are not in the file, the value cannot be assumed to be one way or another.

In this case the default value is useful to avoid writing the value (saving 1 octet per element). Like [ID][Length=0] instead of [ID][Length=1][0].

@robUx4 robUx4 added the bug label Feb 1, 2021
@mbunkus
Copy link
Contributor

mbunkus commented Feb 1, 2021

Hmmmmm, the intent on these flags was that they'd be interpreted as "not indicated" on any file where no track has a given flag set

Traditionally we use the absence of an element that has no default value to indicate that the property isn't known/indicated/doesn't apply. For example:

  <element name="OutputSamplingFrequency" path="\Segment\Tracks\TrackEntry\Audio\OutputSamplingFrequency" id="0x78B5" type="float" range="&gt; 0x0p+0" maxOccurs="1">

though that wouldn't be possible to expose in e.g. libavformat

The limitations of certain API designs should not really be a concern for file format design, I think.

They are not mandatory elements. So if they are not in the file, the value cannot be assumed to be one way or another.

Then having default values for those elements doesn't make any sense at all and only confuses. It also clashes with the fact that we have a lot of elements for which there is a default value but which are not mandatory. For example:

  <element name="CodecDelay" path="\Segment\Tracks\TrackEntry\CodecDelay" id="0x56AA" type="uinteger" minver="4" default="0" maxOccurs="1">
  <element name="StereoMode" path="\Segment\Tracks\TrackEntry\Video\StereoMode" id="0x53B8" type="uinteger" minver="3" default="0" maxOccurs="1">
  <element name="PixelCropLeft" path="\Segment\Tracks\TrackEntry\Video\PixelCropLeft" id="0x54CC" type="uinteger" default="0" maxOccurs="1">

In this case the default value is useful to avoid writing the value

Now you're contradicting yourself. First you say that a reader cannot assume a value if the element isn't present, and next you say that you can save space by not writing the element and assuming the value.


I see two possible solutions:

  1. Reword the specs to indicate that a value of 0 (either by the element being absent or by it being present and set to 0) means that the property is not known/indicated.
  2. Simply remove the default values.

I vastly prefer 2 as 1 is really confusing, especially for elements that are supposed to be used as flags.

If a software uses an API that only signals "yes" or "unknown" via implementation-defined values 1 and 0, those should simply be mapped to "write element with value 1" and "do not write element" respectively.

@retokromer
Copy link
Contributor

I vastly prefer 2 as 1 is really confusing, especially for elements that are supposed to be used as flags.

I agree.

@rcombs
Copy link
Contributor

rcombs commented Feb 1, 2021

I think @robUx4 meant that the "default value" of one of these elements isn't meant to represent "value if the element is not present", but instead to represent "value if the element is present, but has zero size". If this is the case, I think it needs to be documented more clearly in the spec notes.

@mbunkus
Copy link
Contributor

mbunkus commented Feb 1, 2021

Ugh, right, I hadn't thought of that possibility. Thanks. That's one corner of the EBML specs I regularly forget about, unfortunately. As do others, it seems.

Taking that into account means that there isn't actually a bug. The elements can be absent, and the semantic meaning is "no information is known about this property", even though the elements do have default values — which only come into play if the elements are present and have a size of 0.

@dericed
Copy link
Contributor

dericed commented Feb 1, 2021

In other elements such as FieldOrder and some of the Colour ones, 0 indicates false, 1 as true, and 2 as unknown with the default as 2.

@mkver
Copy link
Contributor Author

mkver commented Feb 1, 2021

Thanks for the reminder that having a default value does not imply being mandatory. There is no bug, sorry for the noise. I'll close this.

@mkver mkver closed this as completed Feb 1, 2021
@robUx4
Copy link
Contributor

robUx4 commented Feb 2, 2021

minOccurs may have just two values, but the value has different meaning depending on the context (how very EBML).
We should probably have added a table like this in the EBML RFC:

x minOccurs=0 & no default minOccurs=1 & no default minOccurs=0 & has default minOccurs=1 & has default
not written not defined invalid file not defined default value
written length=0 invalid file invalid file default value default value
written value value value value value

@mbunkus
Copy link
Contributor

mbunkus commented Feb 2, 2021

OK, I now understand why I was confused earlier: the RFC changed semantics wrt. to default values. Prior to the RFC all elements with defaults values were treated the same way. With the RFC non-mandaotry are treated differently than they were before.

See the issue I filed for details.

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

No branches or pull requests

6 participants