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

Chapter updates #190

Merged
merged 3 commits into from
Sep 25, 2017
Merged

Chapter updates #190

merged 3 commits into from
Sep 25, 2017

Conversation

dericed
Copy link
Contributor

@dericed dericed commented Sep 16, 2017

No description provided.

The make process doesn’t support tick-quotes or links in section names.
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.

LGTM.

chapters.md Outdated

#### `EditionFlagOrdered`
The `EditionFlagOrdered Flag` is a significant feature as it enables an Edition of `Ordered Chapters` which define and arrange a virtual timeline rather than simply labeling points within the timeline. For example, with `Editions` of `Ordered Chapters` a single `Matroska file` can present multiple edits of a film without duplicating content. Alternatively if a videotape is digitized in full, one `Ordered Edition` could present the full content (including colorbars, countdown, slate, a feature presentation, and black frames), while another `Edition` of `Ordered Chapters` can use `Chapters` that only mark the intended presentation with the colorbars and other ancilary visual information excluded. If an `Edition` of `Ordered Chapters` is enabled then the Matroska Player MUST play those Chapters in their stored order from the timecode marked in the `ChapterTimeStart Element` to the timecode marked in to `ChapterTimeEnd Element`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Edition is capitalized but not backticked in the first line/sentence

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like more clarity that the define/arrange description in the first sentence is speaking of the Ordered Chapters in general and not the Edition -- otherwise should be defines/arranges

Copy link
Collaborator

Choose a reason for hiding this comment

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

And Chapters in the last sentence is capitalized but not backticked as well!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think from the schema it feels like the concept should be called Ordered Edition but Ordered Chapters is what has more common usage. will backtick later

@dericed
Copy link
Contributor Author

dericed commented Sep 16, 2017

pinging @hubblec4 for review

@dericed
Copy link
Contributor Author

dericed commented Sep 16, 2017

I updated the PR according to @ablwr's comments. I also added a little more rewriting, so please re-review.

chapters.md Outdated

If the `EditionFlagOrdered Flag` is set to `false`, "Matroska Simple-Chapters" are used and only the `ChapterTimeStart` of a chapter is used as chapter mark to jump to the predefined point in the timeline.
Some chapter elements must be now ignored by the playback application. All these elements are now informational only.
The `EditionFlagOrdered Flag` is a significant feature as it enables an `Edition` of `Ordered Chapters` which defines and arranges a virtual timeline rather than simply labeling points within the timeline. For example, with `Editions` of `Ordered Chapters` a single `Matroska file` can present multiple edits of a film without duplicating content. Alternatively if a videotape is digitized in full, one `Ordered Edition` could present the full content (including colorbars, countdown, slate, a feature presentation, and black frames), while another `Edition` of `Ordered Chapters` can use `Chapters` that only mark the intended presentation with the colorbars and other ancilary visual information excluded. If an `Edition` of `Ordered Chapters` is enabled then the Matroska Player MUST play those Chapters in their stored order from the timecode marked in the `ChapterTimeStart Element` to the timecode marked in to `ChapterTimeEnd Element`.
Copy link
Contributor

Choose a reason for hiding this comment

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

ancillary with two l

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm basically fine with this new description.
I wanted to insert an example myself, but I was not sure if this is too much text for the Specs.

chapters.md Outdated

If the `EditionFlagOrdered Flag` is set to `true`, the "Matroska Ordered-Chapters" feature is activated.
A playback application must now also read the `ChapterTimeEnd Element`, and a new virtual timeline is used.
If the `EditionFlagOrdered Flag` is set to `false`, `Simple Chapters` are used and only the `ChapterTimeStart` of a `Chapter` is used as Chapter mark to jump to the predefined point in the timeline. With `Simple Chapters`, a Matroska Plyaer MUST ignore certain `Chapter Elements`. All these elements are now informational only.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: Player

Copy link
Contributor

Choose a reason for hiding this comment

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

personally not sure that used as Chapter mark and a Matroska Player should have capital C and P…

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree to Reto.
Chapter mark -> chapter mark
Matroska Player -> Matroska player (What is wrong with "playback application"?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

agree with lowercase chapter.

Matroska Player is used elsewhere in this part of the spec. However at the top level, under Security Concerns,Matroska Reader is used and backticked. In the cues.md section, "Matroska reader" is used. We should pick and standardize this. "playback application" is also used in a few spots other than here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The EBML document defines "EBML Reader" in the Notations and Conventions section, along with other phrases used within the document. I'm inclined to consistently use "Matroska Reader" also.

Definition:
"EBML Reader": An "EBML Reader" is a data parser that interprets the
semantics of an "EBML Document" and creates a way for programs to use
"EBML".

https://datatracker.ietf.org/doc/draft-ietf-cellar-ebml/?include_text=1

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't want to hold up this PR with this issue though -- should I ask the CELLAR list?

Copy link
Contributor

Choose a reason for hiding this comment

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

Keep in mind that every reading application is not a player (e.g. mkvmerge), and that every issue is not a playback issue. Meaning that two different wordings "reader" and "player" actually make sense.

Here's an example:

All Matroska readers MUST handle default values properly.

All Matroska players must play Edition so-and-so under circumstances so-and-so.

Copy link
Collaborator

Choose a reason for hiding this comment

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

True, thanks for that point.

Copy link
Contributor

Choose a reason for hiding this comment

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

With this new info I think Matroska Player for a playback application is fine, but Matroska Player sounds to specific so I would prefer playback application.
Matroska Reader for the reading applications sounds good.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer the generic "playback application" too!

chapters.md Outdated

It is recommended that only one Edition's `EditionFlagDefault Flag` is set to `true`. If more then one `Default Edition` is present, the first `Default Edition` MUST be used.
If an `EditionFlagDefault Flag` is set to `true`, this `Edition` MUST be used. When all `EditionFlagDefault Flags` are set to `false`, the first Edition MUST be used.
It is RECOMMENDED that no more than one `Edition` have an `EditionFlagDefault Flag` set to `true`. The first `Edition` with the `EditionFlagDefault Flag` set to `true` is the `Default Edition`. When all `EditionFlagDefault Flags` are set to `false`, then the first `Edition` is the `Default Edition`.

Copy link
Contributor

@hubblec4 hubblec4 Sep 17, 2017

Choose a reason for hiding this comment

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

Here, I have made some tests with EditionFlagDefault set to true AND the EditionFlagHidden set to true.
Simple mkv with 3 editions. 2. edition is set to default and hidden.
The default Edition is not used! Instead the first edition is played.
If the 3. edition deflaut Flag also set to true, then the 3. edition is used.

Maybe we should mention this behaviour.

If the Default Edititon's EditionFlagHidden is set to true then the first Edition is the Default Edition.
When more then one Edition EditionFlagDefault Flag is set to true then the first Default Edition is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hubblec4: I updated the language to say:

The first Edition with both the EditionFlagDefault Flag set to true and the EditionFlagHidden Flag not set to false is the Default Edition.

Does this address your comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

The first Edition with both the EditionFlagDefault Flag set to true and the EditionFlagHidden Flag "not - is not correct" set to false is the Default Edition.
The Edition is hidden when you set the HiddenFlag to true(or not to false) and so the Edition can't be the Default one.

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'm still good with this. @ablwr what about you after @dericed's latest changes?

@ablwr
Copy link
Collaborator

ablwr commented Sep 24, 2017

I would like to see @retokromer's advice incorporated into this PR first!

@ablwr
Copy link
Collaborator

ablwr commented Sep 24, 2017

Even if just "ancilary" and "Plyaer" corrected. Although I'd like to continue the discussion proposed by @hubblec4 about Matroska reader/player/application, that can be covered separately.

What about @hubblec4's note on Line 28?

@dericed
Copy link
Contributor Author

dericed commented Sep 24, 2017

"ancilary" and "Plyaer" corrected locally, will force push soon

@dericed
Copy link
Contributor Author

dericed commented Sep 24, 2017

The PR is rebased and forced pushed to address the typos noted and to add language to address @hubblec4's comment at #190 (comment). Please re-review.

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.

I'm fine with the changes. Only one small issue with the new changes by @dericed, I have commented.

chapters.md Outdated

It is recommended that only one Edition's `EditionFlagDefault Flag` is set to `true`. If more then one `Default Edition` is present, the first `Default Edition` MUST be used.
If an `EditionFlagDefault Flag` is set to `true`, this `Edition` MUST be used. When all `EditionFlagDefault Flags` are set to `false`, the first Edition MUST be used.
It is RECOMMENDED that no more than one `Edition` have an `EditionFlagDefault Flag` set to `true`. The first `Edition` with the `EditionFlagDefault Flag` set to `true` is the `Default Edition`. When all `EditionFlagDefault Flags` are set to `false`, then the first `Edition` is the `Default Edition`.

Copy link
Contributor

Choose a reason for hiding this comment

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

The first Edition with both the EditionFlagDefault Flag set to true and the EditionFlagHidden Flag "not - is not correct" set to false is the Default Edition.
The Edition is hidden when you set the HiddenFlag to true(or not to false) and so the Edition can't be the Default one.

@robUx4 robUx4 merged commit 2ebf56f into master Sep 25, 2017
@robUx4 robUx4 deleted the chapter-updates branch September 25, 2017 11:37
@hubblec4
Copy link
Contributor

  • The first Edition with both the EditionFlagDefault Flag set to true and the EditionFlagHidden Flag not set to false is the Default Edition.

Here ist the "not" too much, the both flags must be set to Deflaut=true and Hidden=false, otherwise the Edition is not the Default one.

  • The first Edition with both the EditionFlagDefault Flag set to true and the EditionFlagHidden Flag set to false is the Default Edition.

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

Successfully merging this pull request may close these issues.

None yet

6 participants