Initial implementation of AudioTrack, AudioTrackList, VideoTrack, and VideoTrackList.

Source/core/html/HTMLMediaElement.cpp

Index: Source/core/html/HTMLMediaElement.cpp
diff --git a/Source/core/html/HTMLMediaElement.cpp b/Source/core/html/HTMLMediaElement.cpp
index c4466249ce09de68c150f4aaa5728f0972416d3e..3a588fc9daa5c3811090aed22ccb68a7c013f9c4 100644
--- a/Source/core/html/HTMLMediaElement.cpp
+++ b/Source/core/html/HTMLMediaElement.cpp
@@ -52,9 +52,13 @@
 #include "core/html/MediaFragmentURIParser.h"
 #include "core/html/TimeRanges.h"
 #include "core/html/shadow/MediaControls.h"
+#include "core/html/track/AudioTrack.h"
+#include "core/html/track/AudioTrackList.h"
 #include "core/html/track/InbandTextTrack.h"
 #include "core/html/track/TextTrackCueList.h"
 #include "core/html/track/TextTrackList.h"
+#include "core/html/track/VideoTrack.h"
+#include "core/html/track/VideoTrackList.h"
 #include "core/loader/FrameLoader.h"
 #include "core/rendering/RenderVideo.h"
 #include "core/rendering/RenderView.h"
@@ -85,6 +89,7 @@
 using namespace std;
 using blink::WebInbandTextTrack;
 using blink::WebMimeRegistry;
+using blink::WebMediaPlayerClient;
 
 namespace WebCore {
 
@@ -163,6 +168,52 @@ private:
     HTMLMediaElement* m_mediaElement;
 };
 
+static const AtomicString& VideoKindToString(WebMediaPlayerClient::VideoTrackKind kind)

[philipj_slow, 2014/03/09 09:04:14]

Switch the order of this and the AudioKindToString for consistency?

[acolwell GONE FROM CHROMIUM, 2014/03/18 22:02:15]

Done.

+{
+    switch (kind) {
+    case WebMediaPlayerClient::VideoTrackKindNone:
+        return emptyAtom;
+    case WebMediaPlayerClient::VideoTrackKindAlternative:
+        return VideoTrack::alternativeKeyword();
+    case WebMediaPlayerClient::VideoTrackKindCaptions:
+        return VideoTrack::captionsKeyword();
+    case WebMediaPlayerClient::VideoTrackKindMain:
+        return VideoTrack::mainKeyword();
+    case WebMediaPlayerClient::VideoTrackKindSign:
+        return VideoTrack::signKeyword();
+    case WebMediaPlayerClient::VideoTrackKindSubtitles:
+        return VideoTrack::subtitlesKeyword();
+    case WebMediaPlayerClient::VideoTrackKindCommentary:
+        return VideoTrack::commentaryKeyword();
+    }
+
+    ASSERT_NOT_REACHED();
+    return emptyAtom;
+}
+
+static const AtomicString& AudioKindToString(WebMediaPlayerClient::AudioTrackKind kind)
+{
+    switch (kind) {
+    case WebMediaPlayerClient::AudioTrackKindNone:
+        return emptyAtom;
+    case WebMediaPlayerClient::AudioTrackKindAlternative:
+        return AudioTrack::alternativeKeyword();
+    case WebMediaPlayerClient::AudioTrackKindDescriptions:
+        return AudioTrack::descriptionsKeyword();
+    case WebMediaPlayerClient::AudioTrackKindMain:
+        return AudioTrack::mainKeyword();
+    case WebMediaPlayerClient::AudioTrackKindMainDescriptions:
+        return AudioTrack::mainDescriptionsKeyword();
+    case WebMediaPlayerClient::AudioTrackKindTranslation:
+        return AudioTrack::translationKeyword();
+    case WebMediaPlayerClient::AudioTrackKindCommentary:
+        return AudioTrack::commentaryKeyword();
+    }
+
+    ASSERT_NOT_REACHED();
+    return emptyAtom;
+}
+
 static bool canLoadURL(const KURL& url, const ContentType& contentType, const String& keySystem)
 {
     DEFINE_STATIC_LOCAL(const String, codecs, ("codecs"));
@@ -309,6 +360,12 @@ HTMLMediaElement::~HTMLMediaElement()
     if (m_textTracks)
         m_textTracks->clearOwner();
 
+    if (m_audioTracks)
+        m_audioTracks->shutdown();

[philipj_slow, 2014/03/09 09:04:14]

This will cancel pending events, and I think it's going to be observable: Load a
resource with a single video track and then disable that track. This schedules a
change event. Add an event handler, remove the src attribute, call load(),
remove all references to the media element and trigger a GC. This code will run
and the event will never fire.

The root cause is that "The task source for the tasks listed in this section is
the DOM manipulation task source." but we're using an m_asyncEventQueue per
track list. In additional to missing events, it makes it pretty easy to create a
test case where the event order is wrong.

Again, since you're reusing existing code it's up to you how much to fix in this
review. I can file bugs for what remains when the review is done.

[acolwell GONE FROM CHROMIUM, 2014/03/18 22:02:15]

I've removed the m_asyncEventQueue->close() from shutdown() so the events won't
be cancelled. That was a mistake on my part.
I missed the part about all this being from the DOM manipulation task source so
I figured it would be ok to have different queues. After reading up on this I
see that using different queues is the wrong thing to do.
In general, I'd like to avoid introducing known bugs with new code. Filing bugs
for the existing code sounds good to me. 

[philipj_slow, 2014/03/20 16:17:39]

Bug for TextTrackList:
https://code.google.com/p/chromium/issues/detail?id=354414

+
+    if (m_videoTracks)
+        m_videoTracks->shutdown();
+
     if (m_mediaController) {
         m_mediaController->removeMediaElement(this);
         m_mediaController = nullptr;
@@ -368,6 +425,15 @@ void HTMLMediaElement::didMoveToNewDocument(Document& oldDocument)
     oldDocument.decrementLoadEventDelayCount();
 
     ActiveDOMObject::didMoveToNewExecutionContext(&document());
+
+    // Notify track lists so they don't stop firing events when moved to a new document

[philipj_slow, 2014/03/09 09:04:14]

Can you add a test case for this? Seems like something other implementors might
get wrong as well.

+    // and the original document gets destroyed.
+    if (m_audioTracks)
+        m_audioTracks->didMoveToNewExecutionContext(&document());
+
+    if (m_videoTracks)
+        m_videoTracks->didMoveToNewExecutionContext(&document());
+
     HTMLElement::didMoveToNewDocument(oldDocument);
 }
 
@@ -1580,7 +1646,12 @@ void HTMLMediaElement::setReadyState(MediaPlayer::ReadyState state)
     }
 
     if (m_readyState >= HAVE_METADATA && oldState < HAVE_METADATA) {
+        createPlaceholderTracksIfNecessary();
+
         prepareMediaFragmentURI();
+
+        selectInitialTracksIfNecessary();
+
         scheduleEvent(EventTypeNames::durationchange);
         if (isVideo())
             scheduleEvent(EventTypeNames::resize);
@@ -2201,6 +2272,116 @@ bool HTMLMediaElement::canPlay() const
     return paused() || ended() || m_readyState < HAVE_METADATA;
 }
 
+AudioTrackList& HTMLMediaElement::audioTracks()
+{
+    ASSERT(RuntimeEnabledFeatures::audioVideoTracksEnabled());
+
+    if (!m_audioTracks)
+        m_audioTracks = AudioTrackList::create(this);
+
+    return *m_audioTracks;
+}
+
+void HTMLMediaElement::audioTrackChanged(const AtomicString& audioTrackID, bool enabled)
+{
+    WTF_LOG(Media, "HTMLMediaElement::audioTrackChanged('%s', %d)", audioTrackID.ascii().data(), enabled);
+    ASSERT(RuntimeEnabledFeatures::audioVideoTracksEnabled());
+    ASSERT(!audioTrackID.isEmpty());
+
+    audioTracks().scheduleChangeEvent();
+
+    // FIXME: Add call on m_mediaSource to notify it of track changes once the SourceBuffer.audioTracks attribute is added.
+
+    if (webMediaPlayer())
+        webMediaPlayer()->enabledAudioTrackChange(audioTrackID, enabled);

[philipj_slow, 2014/03/09 09:04:14]

This API seems to make gapless track switching hard. I think that awaiting a
stable state and then notifying the backend about which tracks should be enabled
and disabled would make it easier. A timeout in the backend wouldn't work,
because the script can disable a track and then take time (without returning to
the event loop) to decide which new track to enable, and the backend has no idea
how long to wait.

+}
+
+VideoTrackList& HTMLMediaElement::videoTracks()
+{
+    ASSERT(RuntimeEnabledFeatures::audioVideoTracksEnabled());
+
+    if (!m_videoTracks)
+        m_videoTracks = VideoTrackList::create(this);
+
+    return *m_videoTracks;
+}
+
+void HTMLMediaElement::selectedVideoTrackChanged(const AtomicString& selectedTrackID)
+{
+    WTF_LOG(Media, "HTMLMediaElement::selectedVideoTrackChanged('%s')", selectedTrackID.ascii().data());
+    ASSERT(RuntimeEnabledFeatures::audioVideoTracksEnabled());
+
+    AtomicString unselectedTrackID;
+
+    int oldSelectedIndex = videoTracks().selectedIndex();
+    if (oldSelectedIndex != -1)
+        unselectedTrackID = videoTracks().anonymousIndexedGetter(oldSelectedIndex)->id();
+
+    ASSERT(unselectedTrackID != selectedTrackID);
+
+    videoTracks().trackSelected(selectedTrackID);
+
+    // FIXME: Add call on m_mediaSource to notify it of track changes once the SourceBuffer.videoTracks attribute is added.
+
+    ASSERT(!unselectedTrackID.isEmpty() || !selectedTrackID.isEmpty());
+
+    if (webMediaPlayer())
+        webMediaPlayer()->selectedVideoTrackChange(unselectedTrackID, selectedTrackID);
+}
+
+void HTMLMediaElement::addVideoTrack(const AtomicString& id, blink::WebMediaPlayerClient::VideoTrackKind kind, const AtomicString& label, const AtomicString& language, bool selected)
+{
+    AtomicString kindString = VideoKindToString(kind);
+    WTF_LOG(Media, "HTMLMediaElement::addVideoTrack('%s', '%s', '%s', '%s', %d)",
+        id.ascii().data(), kindString.ascii().data(), label.ascii().data(), language.ascii().data(), selected);
+    ASSERT(!id.isEmpty());

[philipj_slow, 2014/03/09 09:04:14]

Also assert that the id is unique? selectedVideoTrackChanged already ASSERTS it
in an indirect manner.

+
+    RefPtr<VideoTrack> videoTrack = VideoTrack::create(this, id, kindString, label, language);
+    videoTracks().add(videoTrack.get());
+
+    if (selected)
+        videoTrack->setSelected(true);

[philipj_slow, 2014/03/09 09:04:14]

This will fire a change event for any new track... "Whenever a track in a
VideoTrackList that was previously not selected is selected, the user agent must
queue a task to fire a simple event named change at the VideoTrackList object."
Since the VideoTrack previously didn't exist, "previously not selected" becomes
non-sensical, I've filed a spec bug for clarification:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24977

+}
+
+void HTMLMediaElement::removeVideoTrack(const AtomicString& id)
+{
+    WTF_LOG(Media, "HTMLMediaElement::removeVideoTrack('%s')", id.ascii().data());
+    ASSERT(!id.isEmpty());
+
+    if (!RuntimeEnabledFeatures::audioVideoTracksEnabled())
+        return;
+
+    videoTracks().remove(id);
+}
+
+void HTMLMediaElement::addAudioTrack(const AtomicString& id, blink::WebMediaPlayerClient::AudioTrackKind kind, const AtomicString& label, const AtomicString& language, bool enabled)

[philipj_slow, 2014/03/09 09:04:14]

Move these up together with the other *AudioTrack functions?

[acolwell GONE FROM CHROMIUM, 2014/03/18 22:02:15]

Done.

+{
+    AtomicString kindString = AudioKindToString(kind);
+    WTF_LOG(Media, "HTMLMediaElement::addAudioTrack('%s', '%s', '%s', '%s', %d)",
+        id.ascii().data(), kindString.ascii().data(), label.ascii().data(), language.ascii().data(), enabled);
+    ASSERT(!id.isEmpty());
+
+    if (!RuntimeEnabledFeatures::audioVideoTracksEnabled())
+        return;
+
+    RefPtr<AudioTrack> audioTrack = AudioTrack::create(this, id, kindString, label, language);
+    audioTracks().add(audioTrack.get());
+
+    if (enabled)
+        audioTrack->setEnabled(true);
+}
+
+void HTMLMediaElement::removeAudioTrack(const AtomicString& id)
+{
+    WTF_LOG(Media, "HTMLMediaElement::removeAudioTrack('%s')", id.ascii().data());
+    ASSERT(!id.isEmpty());
+
+    if (!RuntimeEnabledFeatures::audioVideoTracksEnabled())
+        return;
+
+    audioTracks().remove(id);
+}
+
 void HTMLMediaElement::mediaPlayerDidAddTextTrack(WebInbandTextTrack* webTrack)
 {
     if (!RuntimeEnabledFeatures::videoTrackEnabled())
@@ -2276,11 +2457,20 @@ void HTMLMediaElement::removeTextTrack(TextTrack* track)
 
 void HTMLMediaElement::forgetResourceSpecificTracks()
 {
+    // Implements the "forget the media element's media-resource-specific tracks" algorithm.
+    // The order is explicitly specified as text, then audio, and finally video. Also

[philipj_slow, 2014/03/09 09:04:14]

Hmm, are there any events that fire that make it possible to tell the
difference? If there are, test please :)

[acolwell GONE FROM CHROMIUM, 2014/03/18 22:02:15]

I don't believe so. I just put the comment here to keep this code matching the
spec, just in case something comes up later.

[philipj_slow, 2014/03/20 16:17:39]

Great!

+    // 'removetrack' events should not be fired.
     if (m_textTracks) {
         TrackDisplayUpdateScope scope(this);
         m_textTracks->removeAllInbandTracks();
         closeCaptionTracksChanged();
     }
+
+    if (m_audioTracks)
+        m_audioTracks->removeAll();
+
+    if (m_videoTracks)
+        m_videoTracks->removeAll();
 }
 
 PassRefPtr<TextTrack> HTMLMediaElement::addTextTrack(const AtomicString& kind, const AtomicString& label, const AtomicString& language, ExceptionState& exceptionState)
@@ -3590,6 +3780,8 @@ void HTMLMediaElement::prepareMediaFragmentURI()
     } else
         m_fragmentEndTime = MediaPlayer::invalidTime();
 
+    // FIXME: Add support for selecting tracks by ID with the Media Fragments track dimension.
+
     if (m_fragmentStartTime != MediaPlayer::invalidTime() && m_readyState < HAVE_FUTURE_DATA)
         prepareToPlay();
 }
@@ -3648,4 +3840,32 @@ bool HTMLMediaElement::isInteractiveContent() const
     return fastHasAttribute(controlsAttr);
 }
 
+void HTMLMediaElement::createPlaceholderTracksIfNecessary()
+{
+    if (!RuntimeEnabledFeatures::audioVideoTracksEnabled())
+        return;
+
+    // Create a placeholder audio track if |m_player| says it has audio but it didn't explicitly announce the tracks.
+    if (m_player->hasAudio() && !audioTracks().length())
+        addAudioTrack("audio", WebMediaPlayerClient::AudioTrackKindMain, "Audio Track", "", true);
+
+    // Create a placeholder video track if |m_player| says it has video but it didn't explicitly announce the tracks.
+    if (m_player->hasVideo() && !videoTracks().length())
+        addVideoTrack("video", WebMediaPlayerClient::VideoTrackKindMain, "Video Track", "", true);
+}
+
+void HTMLMediaElement::selectInitialTracksIfNecessary()

[philipj_slow, 2014/03/09 09:04:14]

Why is this needed? We can assert that at least one of the added audio/video
tracks were enabled. That'll certainly hold true for
createPlaceholderTracksIfNecessary() and I think we should demand it from the
real backends as well.

[acolwell GONE FROM CHROMIUM, 2014/03/18 22:02:15]

This was initially intended to house the logic for initially selecting tracks
based on user preferences, like preferred language, but only currently provides
the fallback "if nothing was selected" behavior. I think it is better to have
default selection logic here instead of in the backend since Blink is likely to
be more aware of what the default selected tracks should be.

[philipj_slow, 2014/03/20 16:17:39]

Ah, I didn't really consider what the low-level media formats look like, is it
the case that the media resource won't itself can't always specify a default
track? If so I agree that having the logic here makes sense. If there are *no*
formats we (want to) support that can specify a default track, I think just
removing the selected/enabled arguments from the platform layer would make the
responsibilities clearer.

+{
+    if (!RuntimeEnabledFeatures::audioVideoTracksEnabled())
+        return;
+
+    // Enable the first audio track if an audio track hasn't been enabled yet.
+    if (audioTracks().length() > 0 && !audioTracks().hasEnabledTrack())
+        audioTracks().anonymousIndexedGetter(0)->setEnabled(true);
+
+    // Select the first video track if a video track hasn't been selected yet.
+    if (videoTracks().length() > 0 && videoTracks().selectedIndex() == -1)
+        videoTracks().anonymousIndexedGetter(0)->setSelected(true);
+}
+
 }