| Summary: | Extend media support for WebVTT sources | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||||
| Component: | Media | Assignee: | Brent Fulgham <bfulgham> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | benjamin, bfulgham, buildbot, cmarcelo, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, mkwst, philipj, rniwa, sergio | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Brent Fulgham
2014-02-21 10:08:38 PST
Created attachment 224882 [details]
WIP1
Comment on attachment 224882 [details]
WIP1
Initial patch; a few questions outstanding.
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 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 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 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 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 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
(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! (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 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 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
Created attachment 224931 [details]
Patch
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. I'll wait until the mac bots pass, then I'll land the change. Thanks for the review! Committed r164527: <http://trac.webkit.org/changeset/164527> |