Bug 129156 - Extend media support for WebVTT sources
Summary: Extend media support for WebVTT sources
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-02-21 10:08 PST by Brent Fulgham
Modified: 2014-02-21 21:00 PST (History)
14 users (show)

See Also:


Attachments
WIP1 (23.13 KB, patch)
2014-02-21 10:24 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (749.54 KB, application/zip)
2014-02-21 12:13 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (804.10 KB, application/zip)
2014-02-21 12:50 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (498.48 KB, application/zip)
2014-02-21 13:49 PST, Build Bot
no flags Details
Patch (48.69 KB, patch)
2014-02-21 17:27 PST, Brent Fulgham
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2014-02-21 10:08:38 PST
Update the Cocoa media infrastructure to support out-of-band/WebVTT track sources.
Comment 1 Brent Fulgham 2014-02-21 10:12:18 PST
<rdar://problem/13066687>
Comment 2 Brent Fulgham 2014-02-21 10:24:49 PST
Created attachment 224882 [details]
WIP1
Comment 3 Brent Fulgham 2014-02-21 10:25:10 PST
Comment on attachment 224882 [details]
WIP1

Initial patch; a few questions outstanding.
Comment 4 Eric Carlson 2014-02-21 11:05:27 PST
Comment on attachment 224882 [details]
WIP1

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

r=me with the suggested changes.

> Source/WTF/wtf/Platform.h:1010
> +#define WTF_USE_AVF_CAPTIONS 1

Support for this depends on the system version, so it should be defined conditionally in the various FeatureDefines.xcconfig files.

> Source/WebCore/html/HTMLMediaElement.cpp:5555
> +        if (!trackElement.canLoadURL(url))
> +            continue;

You should modify canLoadURL because it always fires a 'beforeload' event if the url is valid, and we don't want to do that in this case.

> Source/WebCore/html/HTMLMediaElement.cpp:5563
> +        // FIXME: Do we need to do any other filtering to avoid InBand or Script-based content?

I don't think so, "childrenOfType<HTMLTrackElement>" will only return <track> elements.

> Source/WebCore/html/HTMLMediaElement.cpp:5564
> +        // FIXME: Where would we get the extended language tag e.g., "English" -> "en_US"?

Nothing more should be necessary, the srclan attribute is supposed to be a BCP 47 language.

> Source/WebCore/platform/graphics/PlatformOutOfBandTextTrackInfo.h:36
> +class PlatformOutOfBandTextTrackInfo : public RefCounted<PlatformOutOfBandTextTrackInfo> {

Can you update and use PlatformTextTrack instead of creating this new, almost identical, class?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:201
> +

Nit: extra blank line

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:568
> +                    reinterpret_cast<const NSString*>(language.get()), AVOutOfBandAlternateTrackDisplayNameKey,

language -> track display name? Don't you want label instead?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:569
> +                    // FIXME: Where would we get the extended language?, AVOutOfBandAlternateTrackExtendedLanguageTagKey,

I believe <track srclang> is the same as "extended language". AVF "language" is an ISO 639-2/T code, srclang is supposed to be a BCP 47 language.
Comment 5 Eric Carlson 2014-02-21 11:07:55 PST
Comment on attachment 224882 [details]
WIP1

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

> Source/WebCore/platform/graphics/MediaPlayer.h:533
> +    Vector<RefPtr<PlatformOutOfBandTextTrackInfo>> outOfBandTrackSources();

Does this need a "#if USE(AVF_CAPTIONS)" guard?
Comment 6 Build Bot 2014-02-21 12:13:21 PST
Comment on attachment 224882 [details]
WIP1

Attachment 224882 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5607538762448896

New failing tests:
media/track/track-css-user-override.html
media/track/track-cues-pause-on-exit.html
media/track/track-cue-mutable-fragment.html
media/track/track-css-all-cues.html
media/track/track-css-matching-timestamps.html
media/track/track-kind.html
media/track/track-cue-rendering-horizontal.html
media/track/track-css-matching-lang.html
media/track/track-css-cue-lifetime.html
media/track/track-cue-rendering-snap-to-lines-not-set.html
media/track/track-legacyapi-with-automatic-mode.html
media/track/track-cues-cuechange.html
media/track/track-cues-missed.html
fast/repaint/obscured-background-no-repaint.html
media/track/track-active-cues.html
media/track/track-cue-rendering-with-padding.html
media/track/track-cue-rendering-rtl.html
media/track/track-delete-during-setup.html
media/track/track-cue-rendering-on-resize.html
media/track/track-cue-nothing-to-render.html
media/track/track-css-matching.html
media/track/track-css-matching-default.html
media/track/track-in-band-duplicate-tracks-when-source-changes.html
media/track/track-cues-sorted-before-dispatch.html
media/track/track-cues-seeking.html
media/track/track-disabled.html
media/track/track-cue-container-rendering-position.html
media/track/track-cues-enter-exit.html
media/track/track-css-property-whitelist.html
media/track/track-cue-rendering-tree-is-removed-properly.html
Comment 7 Build Bot 2014-02-21 12:13:25 PST
Created attachment 224887 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 8 Build Bot 2014-02-21 12:50:45 PST
Comment on attachment 224882 [details]
WIP1

Attachment 224882 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5695388291956736

New failing tests:
media/track/track-css-user-override.html
media/track/track-cues-pause-on-exit.html
media/track/track-cue-mutable-fragment.html
media/track/track-css-all-cues.html
media/track/track-css-matching-timestamps.html
media/track/track-kind.html
media/track/track-cue-rendering-horizontal.html
media/track/track-css-matching-lang.html
media/track/track-css-cue-lifetime.html
media/track/track-cue-rendering-snap-to-lines-not-set.html
media/track/track-legacyapi-with-automatic-mode.html
media/track/track-cues-cuechange.html
media/track/track-cues-missed.html
media/track/track-active-cues.html
media/track/track-load-error-readyState.html
media/track/track-cue-rendering-rtl.html
media/track/track-delete-during-setup.html
media/track/track-cue-rendering-on-resize.html
media/track/track-cue-nothing-to-render.html
media/track/track-css-matching.html
media/track/track-css-matching-default.html
media/track/track-in-band-duplicate-tracks-when-source-changes.html
media/track/track-cues-sorted-before-dispatch.html
media/track/track-cues-seeking.html
media/track/track-disabled.html
media/track/track-cue-container-rendering-position.html
media/track/track-cue-rendering-with-padding.html
media/track/track-cues-enter-exit.html
media/track/track-css-property-whitelist.html
media/track/track-cue-rendering-tree-is-removed-properly.html
Comment 9 Build Bot 2014-02-21 12:50:48 PST
Created attachment 224893 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 10 Brent Fulgham 2014-02-21 13:16:18 PST
(In reply to comment #5)
> (From update of attachment 224882 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224882&action=review
> 
> > Source/WebCore/platform/graphics/MediaPlayer.h:533
> > +    Vector<RefPtr<PlatformOutOfBandTextTrackInfo>> outOfBandTrackSources();
> 
> Does this need a "#if USE(AVF_CAPTIONS)" guard?

Yes we do!
Comment 11 Brent Fulgham 2014-02-21 13:29:19 PST
(In reply to comment #4)
> (From update of attachment 224882 [details])
> > Source/WebCore/html/HTMLMediaElement.cpp:5555
> > +        if (!trackElement.canLoadURL(url))
> > +            continue;
> 
> You should modify canLoadURL because it always fires a 'beforeload' event if the url is valid, and we don't want to do that in this case.

There are only two tests we need to do, and both are available outside of HTMLTrackElement, so I'll just inline them here.
 
> > Source/WebCore/platform/graphics/PlatformOutOfBandTextTrackInfo.h:36
> > +class PlatformOutOfBandTextTrackInfo : public RefCounted<PlatformOutOfBandTextTrackInfo> {
> 
> Can you update and use PlatformTextTrack instead of creating this new, almost identical, class?

Done.
 
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:568
> > +                    reinterpret_cast<const NSString*>(language.get()), AVOutOfBandAlternateTrackDisplayNameKey,
> 
> language -> track display name? Don't you want label instead?

Yikes! I munged two lines together. Thanks for catching that.
Comment 12 Build Bot 2014-02-21 13:49:42 PST
Comment on attachment 224882 [details]
WIP1

Attachment 224882 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5519577832226816

New failing tests:
media/track/track-css-cue-lifetime.html
media/track/track-css-matching-lang.html
media/track/track-css-matching.html
media/track/track-css-all-cues.html
media/track/track-css-matching-timestamps.html
media/track/track-css-matching-default.html
media/track/track-active-cues.html
Comment 13 Build Bot 2014-02-21 13:49:44 PST
Created attachment 224898 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 14 Brent Fulgham 2014-02-21 17:27:01 PST
Created attachment 224931 [details]
Patch
Comment 15 Eric Carlson 2014-02-21 17:55:27 PST
Comment on attachment 224931 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:577
> +        [options.get() setObject: outOfBandTracks forKey:@"AVURLAssetOutOfBandAlternateTracksKey"];

Nit: no need for the @"", you soft linked this constant.
Comment 16 Brent Fulgham 2014-02-21 18:07:25 PST
I'll wait until the mac bots pass, then I'll land the change. Thanks for the review!
Comment 17 Brent Fulgham 2014-02-21 21:00:36 PST
Committed r164527: <http://trac.webkit.org/changeset/164527>