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

Remove some default values/make some elements mandatory #499

Merged
merged 6 commits into from
Jun 6, 2021

Conversation

robUx4
Copy link
Contributor

@robUx4 robUx4 commented Apr 22, 2021

Having a default value when an element is not mandatory is confusing. We don't want to allow 0-length elements (#453).

Following the analysis in #310 here are some easy fixes.

Each commit explains one of the changes.

@robUx4 robUx4 added clarifications spec_main Main Matroska spec document target XML Schema EBML Schema validation labels Apr 22, 2021
Copy link
Contributor

@hubblec4 hubblec4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mbunkus
Copy link
Contributor

mbunkus commented May 30, 2021

@robUx4 Branch has conflicts. Can you please rebase & then just merge? Thanks.

robUx4 added 6 commits June 6, 2021 10:18
This is likely unused and the fact it's a reversed number of lace is counter intuitive.
it's a tricky feature to use when it's not 0 so it's better to let people away from it
It's monoscopic video unless specified otherwise. There should not be an undefined.
The default value is not even the behavior video players do.
There should not be an undefined value and by default the DisplayWidth/Height are in pixels
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.

Since we need to differentiate both, it cannot be mandatory and therefore the default value is useless.
@robUx4 robUx4 force-pushed the mandatory-defaults branch from fe42cec to 9262b7b Compare June 6, 2021 08:20
@robUx4 robUx4 merged commit e5b6f22 into master Jun 6, 2021
@robUx4 robUx4 deleted the mandatory-defaults branch June 6, 2021 08:22
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 XML Schema EBML Schema validation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants