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 range attribute from element with a (large) restriction section #515

Merged
merged 1 commit into from
Jun 13, 2021

Conversation

robUx4
Copy link
Contributor

@robUx4 robUx4 commented May 30, 2021

The restriction mentions the value that are allowed. The range gives the lower
and upper limits. But without an indication on what to do with values in the
range but with an unknown value we have more confusion than necessary.

If the restriction values are later expanded older parser will ignore the new
values no matter what. The range is only useful to tell if the value is legit
or not, but there's still plenty of changes it is not.

Somehow fixes the issue raised in ietf-wg-cellar/ebml-specification#410

@robUx4 robUx4 added enhancement clarifications spec_main Main Matroska spec document target labels May 30, 2021
@hubblec4
Copy link
Contributor

hubblec4 commented May 30, 2021

Yes we should remove the range attribute if a restriction is present.
We have more then these two elements: here a list:

  • TrackType
  • FlagInterlaced: the range matches the restriction but should also removed
  • FieldOrder
  • ProjectionType: the range matches the restriction but should also removed
  • ContentEncodingScope

Copy link
Contributor

@mbunkus mbunkus left a comment

Choose a reason for hiding this comment

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

I agree that the ranges should go if restrictions exist.

Please extend the patch to cover all such elements. I've looked through ebml_matroska.xml, too, and the list by @hubblec4 looks to be complete.

@hubblec4 I've edited your comment slightly for improved formatting only.

The restriction mentions the value that are allowed. The range gives the lower
and upper limits. But without an indication on what to do with values in the
range but with an unknown value we have more confusion than necessary.

If the restriction values are later expanded older parser will ignore the new
values no matter what. The range is only useful to tell if the value is legit
or not, but there's still plenty of changes it is not.
@robUx4 robUx4 force-pushed the remove-restriction-range branch from 52f1435 to 8633f5a Compare June 6, 2021 06:47
@robUx4
Copy link
Contributor Author

robUx4 commented Jun 6, 2021

Done

@robUx4 robUx4 requested a review from mbunkus June 6, 2021 06:47
@robUx4 robUx4 merged commit a0e503c into master Jun 13, 2021
@robUx4 robUx4 deleted the remove-restriction-range branch June 13, 2021 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarifications enhancement spec_main Main Matroska spec document target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants