Bug 129749 – Revise Out-of-band VTT support for better integration with AVFoundation engine

• Component: Media
• Status: RESOLVED FIXED

[Brent Fulgham]

AVFoundation can do a better job if it knows about the Out-of-band tracks we have selected, even though WebKit is doing the actual rendering.

Revise the implementation so that we tell the AVFoundation backend when we select out-of-band tracks.

[Brent Fulgham]

<rdar://problem/16215701>

[Brent Fulgham]

Created attachment 225896 [details]
Patch

[Eric Carlson]

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?

[Brent Fulgham]

Created attachment 225918 [details]
Patch

[Brent Fulgham]

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

[Brent Fulgham]

Created attachment 225931 [details]
Patch

[Eric Carlson]

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?

[Brent Fulgham]

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

[Brent Fulgham]

Created attachment 226023 [details]
Patch

[Eric Carlson]

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.

[Brent Fulgham]

Created attachment 226042 [details]
Patch

[Brent Fulgham]

Created attachment 226043 [details]
Patch

[Brent Fulgham]

Committed r165227: <http://trac.webkit.org/changeset/165227>