[webkit-reviews] review granted: [Bug 122043] [Mac] Add AudioTrack support to MediaPlayerPrivateAVFObC. : [Attachment 212859] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 122043: [Mac] Add AudioTrack support to MediaPlayerPrivateAVFObC.
https://bugs.webkit.org/show_bug.cgi?id=122043

Attachment 212859: Patch
https://bugs.webkit.org/attachment.cgi?id=212859&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=212859&action=review


>
Source/WebCore/platform/graphics/avfoundation/objc/AudioTrackPrivateAVFObjC.mm:
93
> +    for (AVMetadataItem* item in [assetTrack commonMetadata]) {
> +	   if ([item.key isEqual:AVMetadataCommonKeyTitle]) {
> +		setLabel((NSString*)item.value);
> +	       break;
> +	   }
> +    }

It would be good to return the label in the user's preferred language if
possible. See InbandTextTrackPrivateLegacyAVFObjC::label.

>
Source/WebCore/platform/graphics/avfoundation/objc/AudioTrackPrivateAVFObjC.mm:
95
> +    setLanguage([assetTrack extendedLanguageTag]);

This will miss a language stored as a QuickTime 5-bit packed code. See
MediaPlayerPrivateAVFoundationObjC::languageOfPrimaryAudioTrack.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.h:220
> +    Vector<RefPtr<AudioTrackPrivateAVFObjC> > m_audioTracks;

We don't need a space in "> >" any more.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:1060
> +	   typedef Vector<RefPtr<AudioTrackPrivateAVFObjC> > AudioTrackVector;

Ditto.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:1073
> +		   player()->removeAudioTrack(i->get());

It would probably be safer to so this after the m_audioTracks.swap, so the new
track configuration is already set up.

>
Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundati
onObjC.mm:1081
> +	       player()->addAudioTrack(newTrack.get());

Ditto.