Bug 62886 – Basic display of WebVTT captions

• Component: Media
• Status: RESOLVED FIXED

[Anna Cavender]

This change will enable rendering of text track cues on <video>s.

[Eric Carlson]

Because of the complexity of rendering WebVTT and because the rendering section of the spec is still in flux, I think it makes sense to implement a simple text track renderer using default positioning and style only. 

FWIW, this is the approach that Microsoft has taken for the beta (at least) of IE 10.

[Eric Carlson]

<rdar://problem/10320025>

[Eric Carlson]

Created attachment 119786 [details]
Proposed patch

[WebKit Review Bot]

Attachment 119786 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1

Source/WebCore/html/shadow/MediaControlElements.h:500:  The parameter name "arena" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 119786 [details]
Proposed patch

Attachment 119786 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10933352

[Sam Weinig]

Comment on attachment 119786 [details]
Proposed patch

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

r- due to using innerHTML.  We should at the very least have tests which show the effect of using it.

> Source/WebCore/html/HTMLMediaElement.cpp:1007
> +        HTMLTrackElement* trackElement;
> +        for (Node* node = firstChild(); node; node = node->nextSibling()) {
> +            if (!node->hasTagName(trackTag))
> +                continue;
> +            trackElement = static_cast<HTMLTrackElement*>(node);
> +            if (trackElement->track() != track)
> +                continue;

I don't think it is necessary to have the trackElement outside of the loop.

> Source/WebCore/html/HTMLMediaElement.h:70
> +typedef PODIntervalTree <double, TextTrackCue*> CueIntervalTree;

Don't want the space after PODIntervalTree.

> Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:379
> +    StringBuilder cues;
> +    for (size_t i = 0; i < visibleCues.size(); ++i) {
> +        if (i > 0 && cues.length())
> +            cues.append("<br>");
> +
> +        TextTrackCue* cue = visibleCues[i].data();
> +        ASSERT(cue->isActive());
> +        if (!cue->track() || cue->track()->mode() != TextTrack::SHOWING)
> +            continue;
> +
> +        cues.append(cue->getCueAsSource());
> +    }
> +
> +    ExceptionCode e; 
> +    m_textTrackDisplay->setInnerHTML(cues.toString(), e);
> +    if (cues.length())
> +        m_textDisplayContainer->show();
> +    else
> +        m_textDisplayContainer->hide();

I think it would be nicer/safer to use programatic DOM APIs (appendChild, etc) instead of building a string and using innerHTML.  If you do need to parse the cues as HTML, I would recommend doing it one cue at a time and not as a concatenated string.

[Eric Carlson]

Created attachment 119795 [details]
Updated patch

[Eric Carlson]

(In reply to comment #6)
> (From update of attachment 119786 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119786&action=review
> 
> r- due to using innerHTML.  We should at the very least have tests which show the effect of using it.
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:1007
> > +        HTMLTrackElement* trackElement;
> > +        for (Node* node = firstChild(); node; node = node->nextSibling()) {
> > +            if (!node->hasTagName(trackTag))
> > +                continue;
> > +            trackElement = static_cast<HTMLTrackElement*>(node);
> > +            if (trackElement->track() != track)
> > +                continue;
> 
> I don't think it is necessary to have the trackElement outside of the loop.
> 
Fixed.

> > Source/WebCore/html/HTMLMediaElement.h:70
> > +typedef PODIntervalTree <double, TextTrackCue*> CueIntervalTree;
> 
> Don't want the space after PODIntervalTree.
> 
Fixed.

> > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:379
> > +    StringBuilder cues;
> > +    for (size_t i = 0; i < visibleCues.size(); ++i) {
> > +        if (i > 0 && cues.length())
> > +            cues.append("<br>");
> > +
> > +        TextTrackCue* cue = visibleCues[i].data();
> > +        ASSERT(cue->isActive());
> > +        if (!cue->track() || cue->track()->mode() != TextTrack::SHOWING)
> > +            continue;
> > +
> > +        cues.append(cue->getCueAsSource());
> > +    }
> > +
> > +    ExceptionCode e; 
> > +    m_textTrackDisplay->setInnerHTML(cues.toString(), e);
> > +    if (cues.length())
> > +        m_textDisplayContainer->show();
> > +    else
> > +        m_textDisplayContainer->hide();
> 
> I think it would be nicer/safer to use programatic DOM APIs (appendChild, etc) instead of building a string and using innerHTML.  If you do need to parse the cues as HTML, I would recommend doing it one cue at a time and not as a concatenated string.

Fixed.

Thanks!

[WebKit Review Bot]

Comment on attachment 119795 [details]
Updated patch

Attachment 119795 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10935368

[Sam Weinig]

Comment on attachment 119795 [details]
Updated patch

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

r=me, but please consider adding a test with tags in the content.  (Also, make sure all the parts of the test are there.)

> LayoutTests/media/track/track-cue-rendering.html:65
> +            testTrack = document.querySelectorAll('track')[0];

Nit: This could just use querySelector('track') if you so felt.

[Eric Carlson]

Created attachment 119806 [details]
Updated, one more time.

[Eric Carlson]

Comment on attachment 119806 [details]
Updated, one more time.

Remove r? because Sam already r+'d the previous version. This one is just so the ews bots can tell me if the Chromium specific code compiles.

[WebKit Review Bot]

Comment on attachment 119806 [details]
Updated, one more time.

Attachment 119806 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10932449

[Eric Carlson]

Created attachment 119820 [details]
One more patch for the EWS bot

[WebKit Review Bot]

Comment on attachment 119820 [details]
One more patch for the EWS bot

Attachment 119820 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10921510

[Eric Carlson]

Created attachment 119822 [details]
YAP

[WebKit Review Bot]

Comment on attachment 119822 [details]
YAP

Attachment 119822 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10942012

[Eric Carlson]

Created attachment 119867 [details]
Updated patch

Updated to fix a problem the Chromium EWS bot found.

[Eric Carlson]

 http://trac.webkit.org/changeset/103242