Bug 109822

Summary: Extending WebVTT Rendering with Regions
Product: WebKit Reporter: Victor Carbune <vcarbune>
Component: MediaAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, calvaris, cdumez, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, kondapallykalyan, macpherson, menard, philipj, rniwa, sergio, silviapf
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 109570, 128412    
Attachments:
Description Flags
WIP Patch Not Tested Yet
none
Patch eric.carlson: review+

Victor Carbune
Reported 2013-02-14 05:57:01 PST
Extend the rules for updating the display of WebVTT text tracks.
Attachments
WIP Patch Not Tested Yet (32.19 KB, patch)
2014-03-13 11:35 PDT, Brent Fulgham 		no flags
DetailsFormatted DiffDiff
Patch (25.83 KB, patch)
2014-03-14 12:57 PDT, Brent Fulgham 		eric.carlson: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Glenn Adams
Comment 1 2013-02-14 07:03:06 PST
no spec, no extension; propose something in the W3C or WHATWG CG, get it written into a spec, then reopen this bug
Eric Carlson
Comment 2 2013-02-14 07:47:39 PST
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
Comment 3 2014-03-12 14:08:12 PDT
This work was landed in Chromium under commit 0cdebc1d76d41bfcb9c4b022e54a4fbff132225d, and should be merged.
Brent Fulgham
Comment 4 2014-03-13 11:35:46 PDT
Created attachment 226603 [details] WIP Patch Not Tested Yet
Brent Fulgham
Comment 5 2014-03-14 12:57:05 PDT
Created attachment 226754 [details] Patch
Eric Carlson
Comment 6 2014-03-14 13:13:57 PDT
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 7 2014-03-14 13:16:48 PDT
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 8 2014-03-17 13:58:16 PDT
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
Comment 9 2014-03-17 15:15:53 PDT
Committed r165765: <http://trac.webkit.org/changeset/165765>
Brent Fulgham
Comment 10 2014-03-18 10:38:35 PDT
*** Bug 117469 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.