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

Non-mandatory elements with default values #310

Closed
mkver opened this issue Feb 12, 2019 · 11 comments · Fixed by #502
Closed

Non-mandatory elements with default values #310

mkver opened this issue Feb 12, 2019 · 11 comments · Fixed by #502
Labels
clarifications spec_main Main Matroska spec document target

Comments

@mkver
Copy link
Contributor

mkver commented Feb 12, 2019

I have just made a surprising observation: The Language (TrackLanguage) element isn't mandatory, although it has a default value (namely "eng") and although most tools (including MKVToolNix and the foundation-source tools and ffmpeg/libavformat) treat it as if this element were mandatory with default value "eng". Given the abundance of files that were created with this interpretation, the specs should be changed to really make the element mandatory.

And then I looked a bit deeper into this topic and found that there are 41 non-mandatory elements with default values:

  • Elements that IMO should be made mandatory (7): Language, PixelCrop* (4x) (If these values aren't mandatory, then the PixelCrop* values are completely indetermined. But they may still be used for the evaluation of the default value of DisplayWidth/Height, so they need to be mandatory.), DisplayUnit (mkvmerge, ffmpeg and mkclean treat files as if DisplayUnit were mandatory with default value 0), ContentEncAlgo (see Revert "always have a ContentEncAlgo in there is a ContentEncryption" #303),
  • Deprecated and irrelevant elements (9): LaceNumber, FrameNumber, BlockAdditionID, Delay, SliceDuration, TrackOffset, TrickTrackFlag, CueRefNumber, CueRefCodecState
  • Special elements (3): BlockDuration: Whether it is mandatory depends upon other elements (whether the track has a DefaultDuration). This looks like a violation of the principle of separation of layers (EBML and Matroska) to me; but this is unavoidable now.
    DisplayWidth/Height: The description here says that the default value only applies if DisplayUnit is 0. But given that these elements are not mandatory, the only way for this default value to apply would be if there were empty DisplayWidth/Height elements present in the file (and if DisplayUnit were 0). But the common tools (mkvmerge, mkclean, ffmpeg) all treat DisplayWidth/Height as mandatory (at least if DisplayUnit == 0). Therefore I think that one should make this element mandatory with its current default value if DisplayUnit == 0. (Yes, this breaks the separation between EBML and Matroska, too.)
  • The rest (22): CodecDelay, Stereomode, Alphamode, AspectRatioType, OutputSamplingFrequency, MatrixCoefficients, BitsPerChannel, ChromaSitingHorz, ChromaSitingVert, Range, TransferCharacteristics, Primaries, ContentSigAlgo, ContentSigHashAlgo, CueBlockNumber, CueCodecState, EditionFlagOrdered, TargetTypeValue, TagTrackUID, TagEditionUID, TagChapterUID, TagAttachmentUID
@robUx4 robUx4 added clarifications spec_main Main Matroska spec document target labels Apr 23, 2019
@mcr
Copy link
Contributor

mcr commented Jun 25, 2019

Either "mandatory" or "default", but not both.
Three case:

  1. no-default, not-mandatory (if present, it has a value. If not present, no assumed value)
  2. mandatory, but has no default value, so writer MUST write that value to the file.
  3. has default, so reader can always assume a value (either as found in the file, or default value)

Also master elements (examples in the text), have mandatory, but no normal elements can be inside.
e.g: "Targets". but does not have to contain anything.
In practice, this element is skipped/voided and can be omitted if empty.
Maybe a better question is: why is this Master Element mandatory?

--> "Every mandatory value must have a default value" <-- believed not to be true.

@hubblec4
Copy link
Contributor

hubblec4 commented Jun 26, 2019

Case 2 is a bit unsafe, like discussed at Matroska meeting. If a writer do not write this element the Matroska file is invalid.
Again, Steve, could you explain it here which element you mean, which must be mandatory but without a default value?

@mcr
Copy link
Contributor

mcr commented Jun 26, 2019

It was an element that could contain sub-elements. At least one of those sub-elements must be present, but no specific sub-element is mandatory.

@dericed
Copy link
Contributor

dericed commented Jun 26, 2019

The description for Targets already says "If empty or not present, then the Tag describes everything in the Segment." so we know what to do if it is "not present" so what's wrong with a proposal to simply say this Element is not actually mandatory?

@mbunkus
Copy link
Contributor

mbunkus commented Jun 26, 2019

Case 2 isn't unsafe, it is a necessity as there are a lot of properties for which no sensible default can ever exist, e.g.

  • Codec ID
  • Video width & height

Without those decoding is simply impossible. Saying such files missing such elements are invalid is the only sensible way to go about things.

@hubblec4
Copy link
Contributor

A Codec ID is a String in Matroska (not an Integer) as I know, and when a writer not write this element you have no info about the codec for this stream (and additional the Matroska file is invalid ). But a default value for this mandatory element could be "null" or "empty" or simple an empty string "". A reader has now ever a value. And is the default value used, this means there is no info. A reader could parse the stream data to get this info.

Same with Video width & height. A default value of 0 (or -1) is also possible and means this info is not available. A reader could search this info again in the stream data.

Targets:
Yes, the description is fine and declare the usage.
However, I see a problem when writing this element. The data size is 0, because there is no sub-element.
A data size of 0 is not allowed, right?
Or is this correct/valid 0x63 0xC0 0x00 0xNext-Element?
On the other hand a writer could omit the empty Targets element, but the specs say it have to write.

@JeromeMartinez
Copy link
Contributor

But a default value for this mandatory element could be "null" or "empty" or simple an empty string ""

In my opinion this is different cases.
In theory "" is a valid value, as there is no limitation on the string itself.
null is more like not present so not valid.
Having such difference between having or not having a default value permits clear check of a valid file.

  1. A missing non mandatory field without default value is valid
  2. A missing mandatory field without default value is invalid
  3. A missing (non mandatory or mandatory is not applicable there) field with default value is valid

@hubblec4
Copy link
Contributor

hubblec4 commented Jun 30, 2019

I think the descriptions

Three case:

  1. no-default, not-mandatory (if present, it has a value. If not present, no assumed value)
  2. mandatory, but has no default value, so writer MUST write that value to the file.
  3. has default, so reader can always assume a value (either as found in the file, or default value)

and

  1. A missing non mandatory field without default value is valid
  2. A missing mandatory field without default value is invalid
  3. A missing (non mandatory or mandatory is not applicable there) field with default value is valid

are perfect to avoid misusing of a mandatory element.
We could/should insert this descriptions into the specs.

... so what's wrong with a proposal to simply say this Element is not actually mandatory?

I think also this element should not be mandatory but Steve say there is a case where it is necessary.

@robUx4
Copy link
Contributor

robUx4 commented Apr 22, 2021

Summary of the issues solved/still present.

  • Language is mandatory since Make the TrackEntry\Language mandatory #451.

  • PixelCrop* can become mandatory, Their default value is 0 and that's the value that should have been assumed if they were not present (so same behavior as a mandatory element).

  • DisplayUnit can become mandatory. The default value is 0 (pixels) which is assumed by all readers anyway, only the value 3 (Display Aspect Ratio) is also commonly used.

  • Video\DisplayWidth and Video\DisplayHeight are not mandatory but could/should be with an implementation_note. With DisplayUnit mandatory these values need to be known as well. The default value is already specified that way. The def

  • TimeSlice\LaceNumber can become mandatory to 0 or remove the default value. I don't think anyone used it.

  • BlockGroup\BlockDuration is marked as mandatory in some implementation_note (add implementation_notes on conditional element attributes #272)

  • TrackEntry\CodecDelay can become mandatory to 0 or remove the default value. Not being present is like having a value of 0, so it could be mandatory as its' handled that way even silently.

  • Video\StereoMode can become mandatory to 0 or remove the default value. Not being present is like having a value of 0, so it could be mandatory as its' handled that way even silently.

  • Video\AlphaMode should probably not have a default value. The value 0 is not even explained. This is a WebM thing.

  • Video\AspectRatioType forces the player to respect the aspect ratio is some ways. I don't think anyone ever used it. I would deprecate it. The "free resizing" as opposed to "keep aspect ratio" is not even what every player does (respect the aspect ratio).

  • CuePoint\CueTrackPositions\CueBlockNumber has a default value of 1. Not being present is different as being present. When not present the CuePoint is still valid but we don't know which Block is the one we're looking for.

  • CuePoint\CueTrackPositions\CueCodecState has default value of 0 which means we don't use any CodecState. Not being present is like having a value of 0, so it could be mandatory as its' handled that way even silently.

  • EditionEntry\EditionFlagOrdered has a default value of 0 which means not ordered. It should become mandatory as an edition has to be ordered or non-ordered, not undefined.

  • Tag\Targets\TargetTypeValue has a default value of 50 (album/movie). Not having the value is not defined. I'm not sure if it should be undefined or a type is necessary.

  • Tag\Targets\TagTrackUID, Tag\Targets\TagEditionUID, Tag\Targets\TagChapterUID and Tag\Targets\TagAttachmentUID may be used without the others. So they might be omitted on purpose. The default value of 0 specifies the tag applies to all tracks/editions/chapters/attachment of the segment. IMO it would be better to omit it and specify what it means in that case.

Some Video\Colour elements have a default value which means "unspecified". Not storing the element has the same meaning whether the element is mandatory or not: we don't know the actual value.

  • MatrixCoefficients has a default value of 2 = "unspecified".
  • TransferCharacteristics has a default value of 2 = "unspecified".
  • Primaries has a default value of 2 = "unspecified".
  • ChromaSitingHorz and ChromaSitingVert have a default value of 0 = "unspecified".
  • Range has a default value of 0 = "unspecified".
  • BitsPerChannel has a default value of 0 = "unspecified". This is an informational flag which is mandatory for raw video.

Deprecated elements that should not be used in any version will be listed separately and their default value has no use. (#487)

  • TimeSlice\FrameNumber
  • TimeSlice\BlockAdditionID
  • TimeSlice\Delay
  • TimeSlice\SliceDuration
  • TrackEntry\TrackOffset
  • TrackEntry\TrickTrackFlag
  • CueReference\CueRefNumber
  • CueReference\CueRefCodecState

Some signature elements should become deprecated (#491)

  • ContentEncryption\ContentSigAlgo
  • ContentEncryption\ContentSigHashAlgo

@robUx4
Copy link
Contributor

robUx4 commented Apr 23, 2021

Given the proposed rule in #500 MatrixCoefficients, TransferCharacteristics, Primaries, CueBlockNumber, TargetTypeValue cannot have a non zero default value and not be mandatory. Given we can't change the default value now, we should make them mandatory, except for CueBlockNumber which has a different meaning if it's present or not.

@mbunkus do you use CueBlockNumber at all ? I'd like to remove the default value or even deprecate it. It's not used by parsers in VLC and libavformat. It's possible that when writing a value of 1, it might have been stored as an Empty Element (zero length) and read back as a value of 0. I'm not sure if anyone is writing this value and even less reading it to use it.

@mbunkus
Copy link
Contributor

mbunkus commented May 30, 2021

do you use CueBlockNumber at all ?

Only mkvinfo reads & outputs it, nothing else in MKVToolNix. Let's get rid (deprecate) of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarifications spec_main Main Matroska spec document target
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants