Bug 109822 – Extending WebVTT Rendering with Regions

• Component: Media
• Status: RESOLVED FIXED

[Victor Carbune]

Extend the rules for updating the display of WebVTT text tracks.

[Glenn Adams]

no spec, no extension; propose something in the W3C or WHATWG CG, get it written into a spec, then reopen this bug

[Eric Carlson]

As noted in the master bug, there is a proposed solution [1] [2] which has had a great deal of discussion in the text community group mailing list (eg. [3]).

Please don't close bugs without discussion. 

[1] https://dvcs.w3.org/hg/text-tracks/raw-file/default/608toVTT/region.html
[2] http://www.w3.org/community/texttracks/wiki/MultiCueBox
[3] http://lists.w3.org/Archives/Public/public-texttracks/2012Dec/0000.html

[Brent Fulgham]

This work was landed in Chromium under commit 0cdebc1d76d41bfcb9c4b022e54a4fbff132225d, and should be merged.

[Brent Fulgham]

Created attachment 226603 [details]
WIP Patch Not Tested Yet

[Brent Fulgham]

Created attachment 226754 [details]
Patch

[Eric Carlson]

Comment on attachment 226754 [details]
Patch

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

> Source/WebCore/html/shadow/MediaControlElements.cpp:1301
> +        TextTrackRegion* region = cue->track()->regions()->getRegionById(regionId);

Both TextTrackCue::track and TextTrack::regions() can return NULL, either of which will make this crash.

> Source/WebCore/html/track/TextTrackRegion.cpp:334
> +    DEPRECATED_DEFINE_STATIC_LOCAL(const AtomicString, trackRegionCueContainerScrollingClass, ("scrolling", AtomicString::ConstructFromLiteral));

Nit: you might as well go straight to NeverDestroyed<T>.

> Source/WebCore/html/track/TextTrackRegion.cpp:341
> +    DEPRECATED_DEFINE_STATIC_LOCAL(const AtomicString, trackRegionCueContainerPseudoId,

Ditto.

> Source/WebCore/html/track/TextTrackRegion.cpp:349
> +    DEPRECATED_DEFINE_STATIC_LOCAL(const AtomicString, trackRegionShadowPseudoId,

Ditto.

> Source/WebCore/html/track/TextTrackRegion.cpp:366
> +    LOG(Media, "TextTrackRegion::displayLastTextTrackCueBox");

Nit: won't this will be call really frequently? if so, you might not want to log here.

> Source/WebCore/html/track/VTTCue.cpp:223
> +    displayTreeInternal()->remove(ASSERT_NO_EXCEPTION);

displayTreeInternal allocates the display tree if it is NULL, so you should return "if (!hasDisplayTree())"

> Source/WebCore/html/track/VTTCue.cpp:747
> +    TextTrackRegion* region = track()->regions()->getRegionById(m_regionId);

TextTrackCue::track() and TextTrack::regions() can both return NULL

[Eric Carlson]

Comment on attachment 226754 [details]
Patch

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

>> Source/WebCore/html/track/TextTrackRegion.cpp:334
>> +    DEPRECATED_DEFINE_STATIC_LOCAL(const AtomicString, trackRegionCueContainerScrollingClass, ("scrolling", AtomicString::ConstructFromLiteral));
> 
> Nit: you might as well go straight to NeverDestroyed<T>.

Or maybe not, see https://bugs.webkit.org/show_bug.cgi?id=130256#c3.

[Brent Fulgham]

Comment on attachment 226754 [details]
Patch

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

>> Source/WebCore/html/shadow/MediaControlElements.cpp:1301
>> +        TextTrackRegion* region = cue->track()->regions()->getRegionById(regionId);
> 
> Both TextTrackCue::track and TextTrack::regions() can return NULL, either of which will make this crash.

Yikes! cue->track() is tested a few lines above, but I'll add code for the regions() == nullptr case.

[Brent Fulgham]

Committed r165765: <http://trac.webkit.org/changeset/165765>

[Brent Fulgham]

*** Bug 117469 has been marked as a duplicate of this bug. ***