Bug 129749 - Revise Out-of-band VTT support for better integration with AVFoundation engine
Summary: Revise Out-of-band VTT support for better integration with AVFoundation engine
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: 129900 130247
  Show dependency treegraph
 
Reported: 2014-03-05 11:23 PST by Brent Fulgham
Modified: 2014-03-14 10:39 PDT (History)
10 users (show)

See Also:


Attachments
Patch (31.18 KB, patch)
2014-03-05 11:44 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (39.58 KB, patch)
2014-03-05 14:52 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (41.94 KB, patch)
2014-03-05 17:44 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (36.97 KB, patch)
2014-03-06 12:01 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (39.64 KB, patch)
2014-03-06 14:41 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (39.64 KB, patch)
2014-03-06 14:45 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-03-05 11:23:05 PST
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.
Comment 1 Brent Fulgham 2014-03-05 11:23:38 PST
<rdar://problem/16215701>
Comment 2 Brent Fulgham 2014-03-05 11:44:07 PST
Created attachment 225896 [details]
Patch
Comment 3 Eric Carlson 2014-03-05 12:16:12 PST
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?
Comment 4 Brent Fulgham 2014-03-05 14:52:06 PST
Created attachment 225918 [details]
Patch
Comment 5 Brent Fulgham 2014-03-05 15:45:41 PST
(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.
Comment 6 Brent Fulgham 2014-03-05 17:44:31 PST
Created attachment 225931 [details]
Patch
Comment 7 Eric Carlson 2014-03-05 19:03:59 PST
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?
Comment 8 Brent Fulgham 2014-03-06 10:26:47 PST
(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.
Comment 9 Brent Fulgham 2014-03-06 12:01:34 PST
Created attachment 226023 [details]
Patch
Comment 10 Eric Carlson 2014-03-06 14:32:20 PST
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.
Comment 11 Brent Fulgham 2014-03-06 14:41:36 PST
Created attachment 226042 [details]
Patch
Comment 12 Brent Fulgham 2014-03-06 14:45:52 PST
Created attachment 226043 [details]
Patch
Comment 13 Brent Fulgham 2014-03-06 15:45:00 PST
Committed r165227: <http://trac.webkit.org/changeset/165227>