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

Clarify definition of epoch and epoch-related offsets #302

Merged
merged 3 commits into from
Jan 4, 2018

Conversation

palemieux
Copy link
Contributor

Close #266

@palemieux palemieux added this to the 3rd Ed milestone Dec 20, 2017
@palemieux palemieux self-assigned this Dec 20, 2017
@nigelmegitt
Copy link
Contributor

Copy link
Contributor

@nigelmegitt nigelmegitt left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me.

spec/ttml1.xml Outdated
<div2 id="time-expression-semantics-clock">
<head>Clock Time Base</head>
<p>When operating with the <code>clock</code> time base, the following semantics should be applied for interpreting time expressions,
as defined by <loc href="#timing-value-timeExpression">&lt;timeExpression&gt;</loc>, and their relationship to media time and local real time.</p>
<p>The clock time base <phrase role="strong"><code>C</code></phrase> is related to local real time <phrase role="strong"><code>R</code></phrase>
expressed in an arbitrary (implementation defined) epoch <phrase role="strong"><code>E</code></phrase>
expressed in the applicable epoch <phrase role="strong"><code>E</code></phrase>
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading this sentence alone, one wonders what "the applicable epoch" is. Why not say "expressed in the epoch defined by the Document Processing Context".

Copy link
Contributor

Choose a reason for hiding this comment

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

That'd work, but the applicable epoch is now defined in the appendix text before the sub-sections begin, so I think it works as is.

spec/ttml1.xml Outdated
<div2 id="time-expression-semantics-clock">
<head>Clock Time Base</head>
<p>When operating with the <code>clock</code> time base, the following semantics should be applied for interpreting time expressions,
as defined by <loc href="#timing-value-timeExpression">&lt;timeExpression&gt;</loc>, and their relationship to media time and local real time.</p>
<p>The clock time base <phrase role="strong"><code>C</code></phrase> is related to local real time <phrase role="strong"><code>R</code></phrase>
expressed in an arbitrary (implementation defined) epoch <phrase role="strong"><code>E</code></phrase>
expressed in the applicable epoch <phrase role="strong"><code>E</code></phrase>
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to see the parenthetical "(implementation defined)" remain in some form.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skynavga Would the text suggested by @cconcolato address your comment since Document Processing Context is implementation-defined?

Copy link
Contributor

Choose a reason for hiding this comment

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

@palemieux yes, that would work

spec/ttml1.xml Outdated
@@ -13406,7 +13405,7 @@ during playback, presentation timing is not affected.
<p>When operating with the <code>media</code> time base, the following semantics should be applied for interpreting time expressions,
as defined by <loc href="#timing-value-timeExpression">&lt;timeExpression&gt;</loc>, and their relationship to media time and local real time.</p>
<p>The media time base <phrase role="strong"><code>M</code></phrase> is related to local real time <phrase role="strong"><code>R</code></phrase>
expressed in the applicable epoch <phrase role="strong"><code>E</code></phrase> as follows:</p>
expressed in the epoch <phrase role="strong"><code>E</code></phrase> (defined by the <emph>Document Processing Context</emph>) as follows:</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

leave "applicable"; remove parenthesis; change to "as defined by ..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"as defined by" will be followed by "as follows", which I think reads funny.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought you were going to use @cconcolato's text, which would be:

expressed in the epoch <phrase role="strong"><code>E</code></phrase> defined by the Document Processing Context as follows:

which is fine with me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're both wrong but also right! The point is not to give the impression that the "as follows" explains how the document processing context defines the epoch. How about:

expressed in the applicable epoch E, where E is defined by the Document Processing Context, as follows:

note the use of a comma after Document Processing Context to separate the sub-clause.

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 added the parentheses to emphasize that "as follows" qualifies epoch E and not Document Processing Context

Copy link
Contributor

Choose a reason for hiding this comment

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

I think proper parentheses are clearer here than commas. I'm still worried that the comma separated sub-clause could be misread.

@skynavga
Copy link
Contributor

skynavga commented Jan 3, 2018

I have reviewed this issue and PR and conclude that no change is needed whatsoever. The original text qualifies epoch with "(implementation defined)", which means exactly what is intended. All of these efforts to point out that "(implementation defined)" means the same as "(document processing context defined)" serves no useful purpose and merely adds clutter to the document.

@palemieux
Copy link
Contributor Author

@skynavga Evidently other members feel differently. The question is whether you plan on formally objecting to the PR as currently presented. Recall that the text is purely informative.

@nigelmegitt
Copy link
Contributor

I'm happy with the changes made after my review approval.

Copy link

@andreastai andreastai left a comment

Choose a reason for hiding this comment

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

I think the latest change fulfills it's purpose and address the request of the commenter. Therefore we should move on and merge the commit.

Copy link
Contributor

@skynavga skynavga left a comment

Choose a reason for hiding this comment

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

In the interest of moving forward, I am approving this change. However, I oppose this change and will not entertain it for TTML2. This change fails to take note that "implementation defined|dependent" is a term in use elsewhere in the document, and adds nothing that is not already stated.

@palemieux palemieux merged commit 697dc5a into master Jan 4, 2018
@skynavga skynavga added editorial and removed agenda labels Jan 4, 2018
@skynavga skynavga deleted the issue-266-epoch-definition branch March 11, 2018 22:49
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.

5 participants