Bug 88187 – Basic support for timestamps within a TextTrackCue

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Victor Carbune]

WebVTT format support timestamps within the cue.
There are multiple applications to this: Karaoke, Paint-on captions.

[Victor Carbune]

Corrected blocking dependency, sorry.

[Victor Carbune]

Created attachment 145481 [details]
Preliminary patch

[Victor Carbune]

Created attachment 145488 [details]
Corrected patch

[Victor Carbune]

Just realized that according to the spec, the ProcessingInstructions nodes must have the original timestamp, not the actual value (this seems to require to parse it each time, for now)

[Eric Carlson]

Comment on attachment 145488 [details]
Corrected patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145488&action=review

> Source/WebCore/ChangeLog:5
> [phone]  Victor Carbune  <[e-mail]>
> +
> +        Need a short description and bug URL (OOPS!)
> +
> +        Reviewed by NOBODY (OOPS!).

Oops,

> Source/WebCore/ChangeLog:36
> [phone]  Victor Carbune  <[e-mail]>
> +
> +        Basic support for timestamps within a TextTrackCue
> +        https://bugs.webkit.org/show_bug.cgi?id=88187
> +
> +        Implemented support for timestamps within a TextTrackCue.
> +        This enables rendering functionality for Karaoke and Paint-on captions.
> +
> +        Reviewed by NOBODY (OOPS!).
> +

you have two ChangeLog entries.

> Source/WebCore/css/mediaControls.css:237
> +video::-webkit-media-text-track-future-nodes {

Do we display future nodes?

> Source/WebCore/css/mediaControls.css:242
> +    color: green;
> +    background-color: rgba(0, 0, 0, 0.8);

If so, why are they green?

> Source/WebCore/html/track/TextTrackCue.cpp:579
> +    for (Node *child = referenceTree->firstChild(); child; child = child->nextSibling()) {
> +        if (child->nodeName() == timestampTag) {
> +            unsigned int position = 0;
> +            String timestamp = child->nodeValue();
> +
> +            double timestampTime = WebVTTParser::create(0, m_scriptExecutionContext)->collectTimeStamp(timestamp, &position);
> +            ASSERT(timestampTime != -1);
> +
> +            if (timestampTime > movieTime)
> +                isPastNode = false;
> +        }

Should you always set "isPastNode", or are timestamps guaranteed to be in ascending time order?

> Source/WebCore/html/track/WebVTTParser.cpp:386
> -            m_currentNode->parserAddChild(ProcessingInstruction::create(document, "timestamp", String(m_token.characters().data(), m_token.characters().size())));
> +            m_currentNode->parserAddChild(ProcessingInstruction::create(document, "timestamp",  String(m_token.characters().data(), m_token.characters().size())));

Is the only difference here an extra space? If so, it is unnecessary.

> Source/WebCore/html/track/WebVTTParser.h:97
> +    double collectTimeStamp(const String&, unsigned*);

Because we are making this a public method, I think we should give it a better name, eg. something like "parseTimeStamp"?

[Victor Carbune]

Created attachment 146418 [details]
Updated patch

[Victor Carbune]

(In reply to comment #5)
> (From update of attachment 145488 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145488&action=review
> 
> > Source/WebCore/ChangeLog:5
> > [phone]  Victor Carbune  <[e-mail]>
> > +
> > +        Need a short description and bug URL (OOPS!)
> > +
> > +        Reviewed by NOBODY (OOPS!).
> 
> Oops,
> 
> > Source/WebCore/ChangeLog:36
> > [phone]  Victor Carbune  <[e-mail]>
> > +
> > +        Basic support for timestamps within a TextTrackCue
> > +        https://bugs.webkit.org/show_bug.cgi?id=88187
> > +
> > +        Implemented support for timestamps within a TextTrackCue.
> > +        This enables rendering functionality for Karaoke and Paint-on captions.
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> 
> you have two ChangeLog entries.
Sorry, fixed.

> > Source/WebCore/css/mediaControls.css:237
> > +video::-webkit-media-text-track-future-nodes {
> 
> Do we display future nodes?
They have visibility:none, so they are not visible, but are attached to the display tree and rendered there.

> > Source/WebCore/css/mediaControls.css:242
> > +    color: green;
> > +    background-color: rgba(0, 0, 0, 0.8);
> 
> If so, why are they green?
Oops, it was mostly for testing. Removed. 

> > Source/WebCore/html/track/TextTrackCue.cpp:579
> > +    for (Node *child = referenceTree->firstChild(); child; child = child->nextSibling()) {
> > +        if (child->nodeName() == timestampTag) {
> > +            unsigned int position = 0;
> > +            String timestamp = child->nodeValue();
> > +
> > +            double timestampTime = WebVTTParser::create(0, m_scriptExecutionContext)->collectTimeStamp(timestamp, &position);
> > +            ASSERT(timestampTime != -1);
> > +
> > +            if (timestampTime > movieTime)
> > +                isPastNode = false;
> > +        }
> 
> Should you always set "isPastNode", or are timestamps guaranteed to be in ascending time order?
Yes, timestamps are guaranteed to be in ascending time order in the spec.

> > Source/WebCore/html/track/WebVTTParser.cpp:386
> > -            m_currentNode->parserAddChild(ProcessingInstruction::create(document, "timestamp", String(m_token.characters().data(), m_token.characters().size())));
> > +            m_currentNode->parserAddChild(ProcessingInstruction::create(document, "timestamp",  String(m_token.characters().data(), m_token.characters().size())));
> 
> Is the only difference here an extra space? If so, it is unnecessary.
Fixed.

> > Source/WebCore/html/track/WebVTTParser.h:97
> > +    double collectTimeStamp(const String&, unsigned*);
> 
> Because we are making this a public method, I think we should give it a better name, eg. something like "parseTimeStamp"?
I would suggest keeping it the same, mostly because "collect" is the term used in the specification (and by the other remaining private methods).

[WebKit Review Bot]

Comment on attachment 146418 [details]
Updated patch

Clearing flags on attachment: 146418

Committed r119907: <http://trac.webkit.org/changeset/119907>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

The new test is failing on EFL port. The diff looks like:
-Current time: 2.25
+Current time: 2.249999761581421
-Current time: 2.75
+Current time: 2.750000238418579
-Current time: 3.25
+Current time: 3.249999761581421
-Current time: 3.75
+Current time: 3.750000238418579

Looks like a rounding issue.

[Chris Dumez]

The new test appears to be failing for GTK port as well. I opened Bug 88725 to track the issue.