Bug 88187 - Basic support for timestamps within a TextTrackCue
Summary: Basic support for timestamps within a TextTrackCue
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Victor Carbune
URL:
Keywords:
Depends on:
Blocks: 79347
  Show dependency treegraph
 
Reported: 2012-06-03 04:28 PDT by Victor Carbune
Modified: 2012-06-10 00:20 PDT (History)
9 users (show)

See Also:


Attachments
Preliminary patch (21.69 KB, patch)
2012-06-03 04:37 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
Corrected patch (23.85 KB, patch)
2012-06-03 10:56 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
Updated patch (22.10 KB, patch)
2012-06-07 16:53 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Carbune 2012-06-03 04:28:20 PDT
WebVTT format support timestamps within the cue.
There are multiple applications to this: Karaoke, Paint-on captions.
Comment 1 Victor Carbune 2012-06-03 04:30:48 PDT
Corrected blocking dependency, sorry.
Comment 2 Victor Carbune 2012-06-03 04:37:09 PDT
Created attachment 145481 [details]
Preliminary patch
Comment 3 Victor Carbune 2012-06-03 10:56:36 PDT
Created attachment 145488 [details]
Corrected patch
Comment 4 Victor Carbune 2012-06-03 10:59:02 PDT
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)
Comment 5 Eric Carlson 2012-06-05 20:57:18 PDT
Comment on attachment 145488 [details]
Corrected patch

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

> Source/WebCore/ChangeLog:5
> +2012-06-03  Victor Carbune  <vcarbune@adobe.com>
> +
> +        Need a short description and bug URL (OOPS!)
> +
> +        Reviewed by NOBODY (OOPS!).

Oops,

> Source/WebCore/ChangeLog:36
> +2012-06-03  Victor Carbune  <victor@rosedu.org>
> +
> +        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"?
Comment 6 Victor Carbune 2012-06-07 16:53:42 PDT
Created attachment 146418 [details]
Updated patch
Comment 7 Victor Carbune 2012-06-07 16:58:24 PDT
(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
> > +2012-06-03  Victor Carbune  <vcarbune@adobe.com>
> > +
> > +        Need a short description and bug URL (OOPS!)
> > +
> > +        Reviewed by NOBODY (OOPS!).
> 
> Oops,
> 
> > Source/WebCore/ChangeLog:36
> > +2012-06-03  Victor Carbune  <victor@rosedu.org>
> > +
> > +        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).
Comment 8 WebKit Review Bot 2012-06-09 10:01:59 PDT
Comment on attachment 146418 [details]
Updated patch

Clearing flags on attachment: 146418

Committed r119907: <http://trac.webkit.org/changeset/119907>
Comment 9 WebKit Review Bot 2012-06-09 10:02:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Chris Dumez 2012-06-10 00:10:39 PDT
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.
Comment 11 Chris Dumez 2012-06-10 00:20:23 PDT
The new test appears to be failing for GTK port as well. I opened Bug 88725 to track the issue.