Bug 129156 – Extend media support for WebVTT sources

• Component: Media
• Status: RESOLVED FIXED

[Brent Fulgham]

Update the Cocoa media infrastructure to support out-of-band/WebVTT track sources.

[Brent Fulgham]

<rdar://problem/13066687>

[Brent Fulgham]

Created attachment 224882 [details]
WIP1

[Brent Fulgham]

Comment on attachment 224882 [details]
WIP1

Initial patch; a few questions outstanding.

[Eric Carlson]

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.

[Eric Carlson]

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?

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Brent Fulgham]

(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!

[Brent Fulgham]

(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.

[Build Bot]

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

[Build Bot]

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

[Brent Fulgham]

Created attachment 224931 [details]
Patch

[Eric Carlson]

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.

[Brent Fulgham]

I'll wait until the mac bots pass, then I'll land the change. Thanks for the review!

[Brent Fulgham]

Committed r164527: <http://trac.webkit.org/changeset/164527>