Summary: | Revise Out-of-band VTT support for better integration with AVFoundation engine | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||||||
Component: | Media | Assignee: | Brent Fulgham <bfulgham> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | bfulgham, calvaris, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, philipj, sergio | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 129900, 130247 | ||||||||||||||||
Attachments: |
|
Description
Brent Fulgham
2014-03-05 11:23:05 PST
Created attachment 225896 [details]
Patch
Comment on attachment 225896 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225896&action=review > Source/WebCore/html/HTMLMediaElement.cpp:1603 > + if (track->trackType() == TextTrack::TrackElement) { > + if (m_player) Nit: this would be slightly simpler with only one "if": if (track->trackType() == TextTrack::TrackElement && m_player) > Source/WebCore/html/HTMLMediaElement.cpp:5574 > + int uniqueId = track ? track->uniqueId() : 0; HTMLTrackElement::track will never return NULL (yes, it should return a reference instead of a pointer), so both the local comparison and variable are unnecessary. > Source/WebCore/html/HTMLMediaElement.cpp:5577 > + if (trackElement.track()) { Not needed. > Source/WebCore/platform/graphics/MediaPlayer.cpp:1265 > +void MediaPlayer::outOfBandTrackModeChanged(TextTrack* track) Nit: I don't think this method should have "out of band" in its name. A much bigger problem is that this is a layering violation - TextTrack is in /html. Luckily I don't think you need to pass the track, the media engine can just call outOfBandTrackSources to get current track configurations. > Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.h:66 > virtual bool isLegacyClosedCaptionsTrack() const = 0; > - > + virtual bool isOutOfBandTextTrack() const = 0; Nit: these are mutually exclusive, so you could use a type enum instead. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:566 > + RetainPtr<CFStringRef> uniqueID = String::number(track->uniqueId()).createCFString(); > + const AtomicString& mode = track->mode(); > + > + for (auto& textTrack : m_textTracks) { > + if (!textTrack->isOutOfBandTextTrack()) > + continue; > + > + RefPtr<OutOfBandTextTrackPrivateAVF> track = static_cast<OutOfBandTextTrackPrivateAVF*>(textTrack.get()); > + RetainPtr<AVMediaSelectionOptionType> currentOption = track->mediaSelectionOption(); > + > + if ([[currentOption.get() outOfBandIdentifier] isEqual: reinterpret_cast<const NSString*>(uniqueID.get())]) { > + if (mode == TextTrack::disabledKeyword()) > + textTrack->setMode(InbandTextTrackPrivate::Disabled); > + else if (mode == TextTrack::hiddenKeyword()) > + textTrack->setMode(InbandTextTrackPrivate::Hidden); > + else if (mode == TextTrack::showingKeyword()) > + textTrack->setMode(InbandTextTrackPrivate::Showing); > + else > + ASSERT_NOT_REACHED(); > + > + break; > + } > + } This logic needs to be reworked because you don't have access to the TextTrack at this level. > Source/WebCore/platform/graphics/avfoundation/objc/OutOfBandTextTrackPrivateAVF.h:45 > + void processCue(CFArrayRef, double) { } > + void resetCueValues() { } Shouldn't you make these virtual in the base class and "override" them here? > Source/WebCore/platform/graphics/avfoundation/objc/OutOfBandTextTrackPrivateAVF.h:59 > + void processCueAttributes(CFAttributedStringRef, GenericCueData*) { } Why is this necessary? Created attachment 225918 [details]
Patch
(In reply to comment #3) > (From update of attachment 225896 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=225896&action=review > > > Source/WebCore/html/HTMLMediaElement.cpp:1603 > > + if (track->trackType() == TextTrack::TrackElement) { > > + if (m_player) > > Nit: this would be slightly simpler with only one "if": if (track->trackType() == TextTrack::TrackElement && m_player) Done. > > Source/WebCore/html/HTMLMediaElement.cpp:5574 > > + int uniqueId = track ? track->uniqueId() : 0; > > HTMLTrackElement::track will never return NULL (yes, it should return a reference instead of a pointer), so both the local comparison and variable are unnecessary. > > > Source/WebCore/html/HTMLMediaElement.cpp:5577 > > + if (trackElement.track()) { > > Not needed. Done. > > Source/WebCore/platform/graphics/MediaPlayer.cpp:1265 > > +void MediaPlayer::outOfBandTrackModeChanged(TextTrack* track) > > Nit: I don't think this method should have "out of band" in its name. I changed this to "notifyTrackModeChanged" > A much bigger problem is that this is a layering violation - TextTrack is in /html. Luckily I don't think you need to pass the track, the media engine can just call outOfBandTrackSources to get current track configurations. Revised as discussed on IRC. > > Source/WebCore/platform/graphics/avfoundation/InbandTextTrackPrivateAVF.h:66 > > virtual bool isLegacyClosedCaptionsTrack() const = 0; > > - > > + virtual bool isOutOfBandTextTrack() const = 0; > > Nit: these are mutually exclusive, so you could use a type enum instead. Done. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:566 > > + RetainPtr<CFStringRef> uniqueID = String::number(track->uniqueId()).createCFString(); [...] > This logic needs to be reworked because you don't have access to the TextTrack at this level. Done. > > Source/WebCore/platform/graphics/avfoundation/objc/OutOfBandTextTrackPrivateAVF.h:45 > > + void processCue(CFArrayRef, double) { } > > + void resetCueValues() { } > > Shouldn't you make these virtual in the base class and "override" them here? You are right! Changed. > > Source/WebCore/platform/graphics/avfoundation/objc/OutOfBandTextTrackPrivateAVF.h:59 > > + void processCueAttributes(CFAttributedStringRef, GenericCueData*) { } > > Why is this necessary? You are right -- it's not needed. I blindly overloaded everything, but of course this is only called by the already-overridden "processCue" method. Created attachment 225931 [details]
Patch
Comment on attachment 225931 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225931&action=review > Source/WebCore/html/HTMLMediaElement.cpp:5560 > - PlatformTextTrack::TrackKind platformKind = PlatformTextTrack::Caption; > + InbandTextTrackPrivate::Kind platformKind = InbandTextTrackPrivate::Captions; > if (trackElement.kind() == TextTrack::captionsKeyword()) What prompted the change from PlatformTextTrack::TrackKind to InbandTextTrackPrivate::Kind? A "platform" text track can be either in-band or out-of-band, so why does it have an InbandTextTrackPrivate::Kind and an InbandTextTrackPrivate::Mode? This isn't a huge deal, but I don't think it is a useful change. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:558 > + RetainPtr<CFStringRef> uniqueID = String::number(track->uniqueId()).createCFString(); > + > + if (![[currentOption.get() outOfBandIdentifier] isEqual: reinterpret_cast<const NSString*>(uniqueID.get())]) > + continue; Why not use createNSString() and avoid the cast? (In reply to comment #7) > (From update of attachment 225931 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=225931&action=review > > > Source/WebCore/html/HTMLMediaElement.cpp:5560 > > - PlatformTextTrack::TrackKind platformKind = PlatformTextTrack::Caption; > > + InbandTextTrackPrivate::Kind platformKind = InbandTextTrackPrivate::Captions; > > if (trackElement.kind() == TextTrack::captionsKeyword()) > > What prompted the change from PlatformTextTrack::TrackKind to InbandTextTrackPrivate::Kind? A "platform" text track can be either in-band or out-of-band, so why does it have an InbandTextTrackPrivate::Kind and an InbandTextTrackPrivate::Mode? > > This isn't a huge deal, but I don't think it is a useful change. I'll revert it. I was hoping to avoid having to create adaptor functions to convert from InbandTextTrackPrivate::Hidden to PlatformTextTrack::Hidden and so forth. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:558 > > + RetainPtr<CFStringRef> uniqueID = String::number(track->uniqueId()).createCFString(); > > + > > + if (![[currentOption.get() outOfBandIdentifier] isEqual: reinterpret_cast<const NSString*>(uniqueID.get())]) > > + continue; > > Why not use createNSString() and avoid the cast? I'd have to go String->StringView->NSString. It would look prettier, but I'm not sure if it's worth changing for this simple use. Created attachment 226023 [details]
Patch
Comment on attachment 226023 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226023&action=review > Source/WebCore/platform/graphics/avfoundation/objc/InbandTextTrackPrivateAVFObjC.h:60 > + Spaces :-O > Source/WebCore/platform/graphics/avfoundation/objc/InbandTextTrackPrivateLegacyAVFObjC.h:60 > + Ditto. Created attachment 226042 [details]
Patch
Created attachment 226043 [details]
Patch
Committed r165227: <http://trac.webkit.org/changeset/165227> |