-
Notifications
You must be signed in to change notification settings - Fork 11
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 terminology #49
Conversation
I suggest also changing the name of the document, since a title with the industry term "Timed Metadata" (or "Timed Metadata Requirements" or "Requirements for Timed Metadata") will draw more readers than "Media Timed Events" |
Sorry, -1 to that idea @mavgit , timed media events such as subtitle (cue) changes are clearly in scope, and I feel so strongly about this that I'd actually object to referring to those as metadata. |
Yes, the scope here is broader than DataCue (the proposed API for timed metadata), and DataCue can be used for things that are more event notifications than metadata (e.g., the MPEG-DASH MPD validity expiration event). Feedback on the proposed changes in this PR is welcome. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that seems missing right now is the link between timed metadata and cues. The document defines cues in 5.2.1 "Cues (e.g., TextTrackCue , or VTTCue ) are units of time-sensitive data on a media timeline [ HTML ]" but without reference to timed metadata.
AFAICT, the two terms appear in combination for the first time in 6.1 "The API should allow web applications to subscribe to receive specific timed metadata cues by schema".
I would suggest to:
- move the definition of cues from 5.2.1 to the terminology section;
- clarify that timed metadata typically get exposed as a set of cues in the definition of timed metadata;
- and, for extra bonus points, link back occurrences of "cues" to the definition
index.html
Outdated
Timed metadata may be carried either "in-band", meaning that they are | ||
delivered within the audio or video media container or multiplexed with | ||
the media stream, or "out-of-band", meaning that they are delivered | ||
externally to the media container or media stream. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to drop quotes around "in-band" and "out-of-band" and to wrap them in <a>
instead to link back to the definition of these terms in this document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
index.html
Outdated
from JavaScript events, at specific points on the <a>media timeline</a> | ||
of an audio or video media stream. | ||
There is a need in the media industry for an API to support timed | ||
metadata, i.e., metadata that is synchronized to audio or video media, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap "timed metadata" in <a>
to link back to the definition of the term?
(The document repeats the definition of this term and of "in-band" and "out-of-band" in practice but that seems fine to repeat the definition in the introduction)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@nigelmegitt & @chrisn , I'm not trying to argue for or against the phrase "timed metadata". I'm really making an editorial comment about being consistent with PR #49. After PR #49 is applied:
So, my point is that it makes no sense to title the document with a phrase which is undefined, unused in the document (save once) and unmentioned in the summary introduction. Now, if there's a better replacement for "timed metadata", that's a different, perfectly reasonable discussion. I just don't see the point of keeping the doc named with a phrase that has been essentially eliminated from the doc in PR #49. |
- Add 'cue' to Terminology section - Replace remaining uses of 'media timed events' - Renamed headings under Recommendations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor editorial comments and a bigger question about payload type signalling.
index.html
Outdated
timeline</a>, to be triggered by the user agent. The API should allow | ||
the web application to provide all necessary parameters to define the | ||
cue, including start and end times, schema identifier, and data | ||
payload. The payload should be any data type (e.g., the set of types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to say anything about payload type signalling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this covered by "schema identifier"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not obviously - that sounds like it's something that would help to parse the payload data of the cue, not to select it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest s/schema identifier/schema identifier, type identifier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest update has "cue type identifier", in both the "Subscribing to receive media timed event cues" and "Out-of-band events" sections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change introduces the idea of a "metadata cue" but all the concepts it refers to are more general than metadata, and do even include control data. I don't think control data can ever be considered to be metadata. Also, one of the proposals for resolving this is called DataCue
.
On balance, I think we should use the term "data cue" instead. Apart from that, the changes look good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Responding to questions about payload type signalling - in my view it is not the same as schema identification. Actually the type identification should be less optional than the schema identification.
index.html
Outdated
<p> | ||
The API should allow web applications to subscribe to receive specific | ||
event streams by event type. For example, to support MPEG-DASH | ||
schemas of <a>timed metadata</a> cue. For example, to support MPEG-DASH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest s/schemas/types to be more general. Schema can be taken as a data type definition not something against which to select content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a33e48b replaces "schema" with "cue type"
index.html
Outdated
agent should deliver only those events to a web application for which | ||
the application has subscribed. The API should also allow web | ||
applications to unsubscribe from specific event streams by event type. | ||
receiving the cues opt-in from the application's point of view. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/the cues/cues grouped by type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in a33e48b
index.html
Outdated
timeline</a>, to be triggered by the user agent. The API should allow | ||
the web application to provide all necessary parameters to define the | ||
cue, including start and end times, schema identifier, and data | ||
payload. The payload should be any data type (e.g., the set of types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not obviously - that sounds like it's something that would help to parse the payload data of the cue, not to select it.
index.html
Outdated
timeline</a>, to be triggered by the user agent. The API should allow | ||
the web application to provide all necessary parameters to define the | ||
cue, including start and end times, schema identifier, and data | ||
payload. The payload should be any data type (e.g., the set of types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest s/schema identifier/schema identifier, type identifier
The use of timed metadata as a term seems well established elsewhere, so I was trying to be consistent with that. I agree it's not a great fit for things that are really control messages though - I note that the DASH-IF document talks about "events and timed metadata". |
This avoids describing the DASH specific control messages as timed metadata.
Sorry, I've confused myself with GitHub here and approved a smaller set of changes, only to see afterwards that it approved the whole pull request, and I don't have the opportunity to dismiss my own review! I would like my previous review comments to be addressed, ideally. |
In the latest update I have gone back to using the previous "media timed event" terminology, and added text to the introduction to explain that this encompasses timed metadata and timed control messages. This should address @mavgit's observation that the terminolgy in the document doesn't match the title. Other uses of "event" are either qualified or should be more obvious from context (e.g,. DASH MPD and emsg events, and DOM events) I think this PR is ready to merge. PTAL @cconcolato, @rjksmith, @nigelmegitt. |
I'll merge this PR, and we can raise further issues to address specific points, as needed. |
This PR clarifies the terminology used in the document:
Preview | Diff