-
Notifications
You must be signed in to change notification settings - Fork 40
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
Added unbounded TextTrackCue.endTime #493
Conversation
@nigelmegitt @gkatsev I was asked to raise this PR to maintain I now see that there's an IPR issue as I'm not a member of TTWG, though I am an IE in SDWIG and have been working with MTE TF for a while. There's no urgency at the moment, but I'd like to resolve this blocking issue and would welcome your guidance on how best to proceed. Thanks. cc @chrisn |
Thanks @tidoust! I think updating index.bs` would also trigger the PR Preview feature. |
@tidoust @gkatsev Thanks for your guidance. I've reverted my changes (7e4b64c) to I suspect that the Bikeshed tool has been updated since Please let me know the process to sign a non-participant contribution agreement to cover IPR so we can resolve the blocking issue. Thanks. |
Error is from WebIDL change at whatwg/webidl#798, which is supposed to be fixed by #492 |
@himorin let me know if you need any assistance with the non-participant contribution agreement process - it should be relatively well-identified starting from https://labs.w3.org/repo-manager/pr/id/w3c/webvtt/493 |
ah, yes. For now, this is kept stop for repo management side (reorg, webidl alignment...), but should be fixed shortly. |
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 change is good to go, once we fix up the WebIDL issue we'll circle back about the IPR stuff and get this change in.
We've updated the |
The Timed Text Working Group just discussed
The full IRC log of that discussion<nigel> Topic: [WebVTT] Added unbounded TextTrackCue.endTime #493<nigel> github: https://github.com//pull/493 <nigel> Gary: The idea is that a cue can start with a defined time but with an undefined (infinity) end time. <nigel> .. The idea is for live streams where a cue, say a chapter, can show up, but then because the end isn't in the live stream yet <nigel> .. you want the cue to reflect the end time of the video, which is also infinity for the video. <nigel> .. It also aligns it with WebVMT which is a fork of WebVTT which has done that already. <nigel> .. I think the change makes sense and is fairly uncontroversial. <nigel> Nigel: Is the semantic well defined for updating the end time to a definite value? <nigel> Gary: I believe it is, but the fun part is that is defined in HTML! <nigel> .. I think it does account for updating the times of the cue. <nigel> Nigel: So the change was already made in HTML and we are just tracking it here, is that right? <nigel> Gary: There's a pull request out for it, so I guess they want to get it all in at once. <nigel> Cyril: Just a remark - do you know that there's a related change request on the carriage of WebVTT in MP4 by Dave Singer, to match this? <nigel> Gary: I do not <nigel> Cyril: I can tell you there is! <nigel> Gary: Makes sense. <nigel> Cyril: I see that the HTML pull request is not approved - should we wait for that before we do it in case they make a change? <nigel> Gary: Given that the change in WebVTT is 3 words, basically, we can wait to merge it until the WebVTT HTML part is approved. <nigel> .. It would be good to make sure all the ducks are in a row on WebVTT, which means figuring out the IPR and checking no-one objects to the change. <nigel> Nigel: Unless there is a circular reference it must be clear which one has to move first. <nigel> Gary: They sound kind of separate. The HTML one updates Text Track Cue constructor and object to allow the end time to be unbounded. <nigel> .. VTTCue is separate. No, I guess you need both. <cyril> rrsagent, pointer <RRSAgent> See https://www.w3.org/2021/03/04-tt-irc#T16-30-44-1 <nigel> Nigel: I can see that the change is to the type of the end time, to be unrestricted double, and defining the interpretation. <nigel> Gary: Yes <nigel> Cyril: I don't see a change to the serialisation - how would it be shown, as Infinity? <nigel> Pierre: JSON can represent infinity <nigel> s/can/cannot <nigel> Nigel: Right, but this is not JSON <nigel> Pierre: Right, but having gone through something similar recently, unbounded/infinity is poorly supported. <nigel> .. There's no widely accepted representation - just a data point. <nigel> .. For instance as long as you stay within Javascript you can deal with Infinity, assuming the underlying platform supports IEEE floats. <nigel> Gary: I think the way WebVMT does it is by omitting the end time after the -> <nigel> Cyril: For WebVTT that's a breaking change though. <nigel> Gary: Yes, it's possible this is why it is just in the contract for the value, and not in the actual syntax itself. <nigel> Cyril: Maybe there should be information about ignoring the end value or having a special value or something? <nigel> Gary: Yeah you're right we should figure out whether and how we want to have a syntax for this. <nigel> .. Maybe not having the end time, even though it is a change, maybe it can be added on top without breaking things, I'm not sure. <nigel> Nigel: Is the requirement only for constructing the VTTCue object in client code, or for providing a serialised fragment of WebVTT? <nigel> Gary: Yeah if we only care about the javascript the current change should be enough, and maybe that's a good place to separate it. <nigel> .. Do the Javascript contract first and then figure out the syntax. <nigel> Nigel: Cyril, does the change you mentioned before about MP4 imply some form of serialisation requirement? <nigel> Cyril: I'll have to check. <nigel> Gary: Chris Needham's example is Javascript code constructing the cue object. <gkatsev> -> https://github.com/whatwg/html/issues/5297 HTML issue for Add TextTrackCue end time representing end of media <nigel> Cyril: The text I can see doesn't talk about serialisation - it talks about what to do if a cue with indefinite duration exists. <nigel> .. I don't understand it - it will need review. I don't think it answers your question Nigel, and it raises more. <nigel> Nigel: Perhaps it's just explaining how to handle the case, without saying how it might happen, so it's future-proof in that case! <nigel> Nigel: Interesting to know how a syntax change would be handled by existing parsers. <nigel> Pierre: Have any tests been provided? That might help answer the question. <nigel> Gary: The requested change is only to the Javascript contract. <nigel> Pierre: So there are no tests?. <nigel> s/?// <nigel> Gary: No, [checks wpt] there are no tests. <nigel> Nigel: Is it testable? <nigel> Gary: Yes, it's just create a new VTTCue with an infinite end time and verify that one was created with infinity for end time. <nigel> .. For the syntax it is possible that if we add the syntax of no end time the error correction might just ignore that cue. I need to check. <nigel> Nigel: Good shout about tests - our working model changed a couple of years ago to try to write tests alongside substantive <nigel> .. changes at the time of the PR, so we're not rushing to back-fill the IR at the end when we've forgotten the context. <nigel> Gary: Yes <nigel> SUMMARY: 1. Make sure we have tests for the change; 2. Enquire whether a syntax change is needed, and if so, investigate if it can be added in a non-breaking way. |
WebVMT already handles unbounded cues as they are a common use case for live streaming. The cue end time is simply omitted as in the Tower of London example. This has the advantage that an end time can easily be added to make the cue bounded when it becomes known at some future time. |
@rjksmith yeah, I brought up how WebVMT does it. For the tests, we'd want to make a PR against web-platform-tests to update the constructor to allow endTime to be Infinity https://github.com/web-platform-tests/wpt/blob/master/webvtt/api/VTTCue/constructor.html. |
Oh, just realized that maybe TextTrackCue has tests too and sure enough it does. https://github.com/web-platform-tests/wpt/blob/master/html/semantics/embedded-content/media-elements/interfaces/TextTrackCue/endTime.html I'll add a comment on the html repo PR too. |
Also, I just verified that adopting WebVMT's syntax for unbounded cues can happen in a backwards compatible way. Parsers that don't recognize a time header without an end time (i.e. all current webvtt parsers) ignore that entire cue block but still show all subsequent ones. Tested in Chrome, Firefox, Safari, and vtt.js. |
@gkatsev Thanks for confirming. That sounds encouraging. I'll take a look at the web-platform-tests too. |
Is there any news on this or can you post a link to the agreement? Thanks |
I think you might have received an email from W3C non-participant contribution system at Mar 11, 2021, to an email address you registered to W3C account. |
It would be interesting to know whether it's useful to have a way to represent unbound end-time in a way that old parsers handle things intelligently. But it may not be. I think this was discussed a bit at the M&EIG meeting that, unfortunately, had to miss. |
I'm not convinced that skipping the entire cue counts as "backwards compatible". I'd suggest a more backwards compatible approach here is one where a cue end time is specified, that works for all parsers, and additional syntax is available, that is ignored by parsers that don't recognise it, and used by those that do, where that syntax has the semantic "set the end time to infinity". Then authors can specify a fallback end time of their choosing. I'd propose a new WebVTT Cue Setting to satisfy this, because unknown cue settings should be ignored by parsers that don't recognise them. For example:
Current parser generates a cue whose end time is |
I think it counts as backwards compatible, but it's definitely not the best behavior. I was verifying whether the current syntax that WebVMT used could be used and not wreak havoc on existing parsers, like having it fail to parse the rest of the file or something. |
@himorin Many thanks. I've found it. |
Using unbounded cues is not compulsory. If backward compatibility is an issue then a long bounded cue can be used instead.
This is fully backward compatible and is the current solution. WebVMT also allows unbounded cues to be superseded which is an integral part of interpolation for live streaming use cases as discussed at TPAC 2020. |
Indeed, but this issue is to allow the semantic of an unbounded cue to be expressed, right? |
I think that Nigel's point is that it's hard to tell for all cases what the best backwards-compatible behavior should be:
However, I am not sure what the effect of (2) would be. Experiment is needed. It might cause players to conclude that the presentation really is 99 years long, and display e.g. a scroll-bar that long. |
Yes I agree. However, displaying an unbounded cue indefinitely may not be the correct behaviour. An unbounded cue is defined as a cue with an unspecified future end time. The current WHATWG spec represents the duration of an unbounded stream with Infinity: media.duration = Infinity. The stream demonstrably has a finite duration, so Infinity is being used to represent an unspecified future end time and the same definition applies to an unbounded cue - as previously discussed. This can be illustrated with a simple use case. For example, the score during a live sporting event is "0-0" at the start of the game. This may change to "1-0" or "0-1" at an unknown future time or may stay the same, and the duration of the match may be extended. There is no way of knowing the duration of the "0-0" cue until the score changes or the game ends, so this is an unbounded cue. The point is that it is not possible to predict the end time of an unbounded cue (or stream) because it is unspecified by definition, which is reflected in the WebVMT cue syntax:
This is a new HTML feature and if there is a requirement for backward compatibility, bounded cues can be used instead to ensure the desired result. |
Um, the example you give doesn't seem to work, unless there is some provision to update a cue that's already been issued, decoded and acted on. The score 0-0 will last forever, even after a goal is scored. Using captions for state is, I think, problematic. |
Reformatted index.bs & regenerated index.html
0ae61d6
to
92ef31c
Compare
Added tests for unbounded TextTrackCue endTime changes proposed in whatwg/html#5953 & w3c/webvtt#493
Rebased to main |
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'm not an editor of this spec, but I used to be, so some feedback. Looks good, close to the finish line now :)
index.bs
Outdated
@@ -5544,7 +5545,8 @@ enum DirectionSetting { "" /* horizontal */, "rl", "lr" }; | |||
enum LineAlignSetting { "start", "center", "end" }; | |||
enum PositionAlignSetting { "line-left", "center", "line-right", "auto" }; | |||
enum AlignSetting { "start", "center", "end", "left", "right" }; | |||
[Exposed=Window] | |||
[Exposed=Window, | |||
Constructor(double startTime, unrestricted double endTime, DOMString text)] |
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.
Constructor an extended attribute has been removed from Web IDL. Instead of this, you'll need to add "unrestricted" below, I'll make a suggestion.
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.
And this change here is no longer needed. Just the constructor operation below.
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.
Changed back to [Exposed=Window]
as requested
index.bs
Outdated
@@ -5544,7 +5545,8 @@ enum DirectionSetting { "" /* horizontal */, "rl", "lr" }; | |||
enum LineAlignSetting { "start", "center", "end" }; | |||
enum PositionAlignSetting { "line-left", "center", "line-right", "auto" }; | |||
enum AlignSetting { "start", "center", "end", "left", "right" }; | |||
[Exposed=Window] | |||
[Exposed=Window, | |||
Constructor(double startTime, unrestricted double endTime, DOMString text)] | |||
interface VTTCue : TextTrackCue { | |||
constructor(double startTime, double endTime, DOMString text); |
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.
constructor(double startTime, double endTime, DOMString text); | |
constructor(double startTime, unrestricted double endTime, DOMString text); |
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.
Thanks. Good spot. Fixed
index.bs
Outdated
@@ -5693,7 +5695,8 @@ interface VTTCue : TextTrackCue { | |||
interpreted as a time in seconds.</p></li> | |||
|
|||
<li><p>Let |cue|'s <a>text track cue end time</a> be the value of the |endTime| argument, | |||
interpreted as a time in seconds.</p></li> | |||
interpreted as a time in seconds or positive Infinity for an <a>unbounded text track |
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.
Like in HTML, you'll need to check if the argument is -Infinity of NaN and throw a TypeError. That's because the endTime
setter defined in HTML isn't called here, rather it's directly setting the internal "text track cue end time".
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.
Interesting. I'd assumed that this behaviour would be inherited from TextTrackCue.endTime
in HTML. Suitable wording added. Thanks.
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.
Ah, so it's not the endTime
setter that needs this, that is indeed inherited (in a sense) from TextTrackCue
. It's the VTTCue
constructor that needs to check for -Infinity and NaN.
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.
Removed endTime
setter wording and added equivalent check to step 3 of the VTTCue
constructor for endTime
attribute
Added unrestricted to VTTCue constructor endTime argument Added argument check for VTTCue.endTime setter
Removed Constructor from Exposed VTTCue clause Added endTime argument check to VTTCue constructor Removed on setting check from endTime attribute
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.
Looks good, just one remaining comment from me.
I'm not sure who will review and merge this, hope you know who to poke, @rjksmith :)
index.bs
Outdated
then throw a <a | ||
href="https://tc39.es/ecma262/#sec-native-error-types-used-in-this-standard-typeerror">TypeError | ||
</a> exception. Otherwise, let |cue|'s <a>text track cue end time</a> be the value of the |endTime| | ||
argument, interpreted as a time in seconds or positive Infinity for an <a>unbounded text track |
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.
Like in HTML, I don't think it's necessary to say "interpreted as a time in seconds or positive Infinity", instead just set the value.
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 agree. I was trying to match the existing wording for startTime
, but it's unnecessary. Fixed.
Removed unnecessary wording
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 now looks done to me. Thanks for persisting, @rjksmith!
Excellent. You're welcome and thanks for your guidance @foolip |
@gkatsev This PR is now ready for review and merge - as are whatwg/html#5953 and web-platform-tests/wpt#28394. Please co-ordinate with @foolip to synchronise these changes and let me know if there are any problems. Thanks. |
sorry on late reply. it takes some time (depends on health and delays - periodical processing of many of back end services) for IPP, and please count some time for these... (and seems fine for now on IPP / ipr.) |
Added tests for unbounded TextTrackCue endTime changes proposed in whatwg/html#5953 & w3c/webvtt#493
@gkatsev whatwg/html#5953 and web-platform-tests/wpt#28394 have been merged. As mentioned above, Web Platform Tests rely on VTTCue and so will fail spuriously without this PR. Please now review and merge #493 to avoid a mismatch with WPT and let me know if there are any problems. Thanks. |
…only Automatic update from web-platform-tests Tests for unbounded cue end time (#28394) Added tests for unbounded TextTrackCue endTime changes proposed in whatwg/html#5953 & w3c/webvtt#493 -- wpt-commits: 786c430165d916bdab740ab491ac541afb3f8bf5 wpt-pr: 28394
This change looks good to me, and seems straightforward to align with the TextTrackCue change in HTML. Is it OK to merge? Can the questions around the WebVTT document format be followed up separately to merging this change? |
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.
@@ -54,6 +54,7 @@ urlPrefix: https://html.spec.whatwg.org/multipage/ | |||
text: rules for extracting the chapter title | |||
text: text track cue start time | |||
text: text track cue end time | |||
text: unbounded text track cue |
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.
Turns out this term isn't linked any longer, so this line could be removed. Or maybe a note could be added saying that a cue with endTime
Infinity is an unbounded text track cue, linking to 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.
Well spotted. I'll either remove this or add a note as you suggest.
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.
Added note with a link to unbounded text track cue, though I'm unsure whether my wording is correct as I was unable to find a comparable example.
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.
Sounds like reasonable wording to me.
Added note linking to unbounded text track cue
I'm happy to join too if someone can send me an invite. Thanks. |
The Timed Text Working Group just discussed
The full IRC log of that discussion<nigel> Topic: WebVTT Add unbounded TextTrackCue.endTime #493<nigel> github: https://github.com//pull/493 <nigel> Gary: When we last spoke about the unbounded end time for the TextTrackCue there wasn't much concern. <nigel> .. The hold-up was whether there were tests and whether it was approved on the HTML spec side. <nigel> .. Last week the [PRs on] tests and the HTML side were merged so now WebVTT is the last hold-out for the API change, <nigel> .. which is this pull request right now. <RobSmith> q+ <nigel> ack R <nigel> Rob: I concur. Firstly this is my first TTWG meeting, thank you for the invite. Thanks Gary for the quick summary. <nigel> .. I'd add that having done the platform tests I notice there's a dependency between the TextTrackCue tests and the VTTCue tests <nigel> .. because the TextTrackCue relies on VTTCue for the constructor. There's a crossover or interdependency that results. <nigel> .. The WebVTT change is required to make the Web Platform Tests work as well. <nigel> Gary: Thanks Rob that makes sense. <nigel> .. In terms of process, I haven't heard any objections from anyone. Do we need to wait before merging for the resolution period? <nigel> Nigel: Our normal practice is to consider the opening of a PR as effectively a Call for Consensus <nigel> .. so that the Decision Review period expires 2 weeks after opening, obviously assuming there is consensus at that time. <nigel> .. What this means is that, since this PR has been open since November, if the Chair declares consensus, then the Editor can go ahead and merge. <RobSmith> q+ <nigel> .. I'd just point out that there was a commit just 23 hours ago so that should be considered. <nigel> Gary: I plan to look through it again after this meeting and in that case go ahead and merge. <nigel> ack R <cpn> q+ to ask about the VTT format discussion <nigel> Rob: That recent addition was to address Philip Jaegenstadt's comment that there was a reference to unbounded TextTrackCue which was <nigel> .. not used. We tried to keep it as simple as possible and ended up removing all references to unbounded cues in the process. <nigel> .. The way it is at the moment it is the one thing we are adding and do not mention, so I thought it worth adding. <cpn> s/Jaegenstadt/Jaegenstedt/ <nigel> .. I couldn't find any similar example and struggled with the wording so would welcome guidance. <nigel> Gary: Earlier I thought it was fine but will take a closer look. <nigel> Rob: Thank you <nigel> q++ nigel <nigel> ack n <nigel> ack + <nigel> Gary: Then for the pull request the resolution is for me to look over it and merge after approval. <RobSmith> q+ <cpn> q- cpn <nigel> ack Rob <nigel> Rob: Gary, if there's change to be made on the wording I'm happy to work with you to get that resolved and complete this. <nigel> Gary: Thank you Rob <nigel> RESOLUTION: Gary to do final Editorial pass and merge |
@@ -54,6 +54,7 @@ urlPrefix: https://html.spec.whatwg.org/multipage/ | |||
text: rules for extracting the chapter title | |||
text: text track cue start time | |||
text: text track cue end time | |||
text: unbounded text track cue |
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.
Sounds like reasonable wording to me.
@gkatsev Fantastic. Many thanks for your swift response. This enables the WPT update in web-platform-tests/wpt#28394 which allows browser implementers to validate their code for unbounded cue support, and concludes the changes required for whatwg/html#5953. I look forward to the constructive discussion of possible changes to WebVTT in #496. |
Added support for unbounded TextTrackCue - see whatwg/html#5953
Whitespace removed by Atom
Preview | Diff