Implement HTMLMediaElement::srcObject.

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 91ef6708e6f5e78594ef9b36ad751aec9955cc39..7091070161a06ee81ebe6d13c4a33a10d2ec2d6c 100644
--- a/Source/core/html/HTMLMediaElement.cpp
+++ b/Source/core/html/HTMLMediaElement.cpp
@@ -48,6 +48,7 @@
 #include "core/html/MediaController.h"
 #include "core/html/MediaError.h"
 #include "core/html/MediaFragmentURIParser.h"
+#include "core/html/MediaProvider.h"
 #include "core/html/TimeRanges.h"
 #include "core/html/shadow/MediaControls.h"
 #include "core/html/track/AudioTrack.h"
@@ -325,6 +326,7 @@ HTMLMediaElement::HTMLMediaElement(const QualifiedName& tagName, Document& docum
     , m_lastTimeUpdateEventWallTime(0)
     , m_lastTimeUpdateEventMovieTime(std::numeric_limits<double>::max())
     , m_loadState(WaitingForSource)
+    , m_srcObject(std::nullptr_t())

[philipj_slow, 2014/09/16 13:39:50]

You don't need to explicitly init these smart pointer types.

[perkj_chrome, 2014/09/17 19:26:13]

Well, WTF::RawPtr does not allow you to not set a pointer value and later check
for null. But there is an explicit constructur that seems to be allowed
RawPtr(std::nullptr_t).

     , m_deferredLoadState(NotDeferred)
     , m_deferredLoadTimer(this, &HTMLMediaElement::deferredLoadTimerFired)
     , m_webLayer(0)
@@ -682,6 +684,19 @@ PassRefPtrWillBeRawPtr<MediaError> HTMLMediaElement::error() const
     return m_error;
 }
 
+void HTMLMediaElement::setSrcObject(RawPtr<MediaProvider> mediaProvider)
+{
+    WTF_LOG(Media, "HTMLMediaElement::setSrcObject()");

[philipj_slow, 2014/09/16 13:39:50]

Please include the %p this pointer as well.

[perkj_chrome, 2014/09/17 19:26:13]

Done.

+    m_srcObject = mediaProvider;
+    clearMediaPlayer(LoadMediaResource);
+    scheduleDelayedAction(LoadMediaResource);

[philipj_slow, 2014/09/16 13:39:50]

The spec says "On setting, it must set the element's assigned media provider
object to the new value, and then invoke the element's media element load
algorithm."

That means simply calling load() here, or at least it would if load() were
implemented per spec. It should be closer to spec than this though, please add a
test that verifies that some of the reset steps of load() have run immediately
after setting srcObject, e.g.

test(function()
{
    var v = document.createElement('video');
    v.playbackRate = 2;
    v.srcObject = mediaStreamOrSomething;
    assert_equals(v.playbackRate, 1);
});

[perkj_chrome, 2014/09/17 19:26:13]

Now calls load() and test is added to mediastream-srcobject.html

+}
+
+MediaProvider* HTMLMediaElement::srcObject() const

[philipj_slow, 2014/09/16 13:39:50]

Please invert order here too.

[perkj_chrome, 2014/09/17 19:26:13]

Done.

+{
+    return m_srcObject.get();
+}
+
 void HTMLMediaElement::setSrc(const AtomicString& url)
 {
     setAttribute(srcAttr, url);
@@ -855,29 +870,32 @@ void HTMLMediaElement::selectMediaResource()
 {
     WTF_LOG(Media, "HTMLMediaElement::selectMediaResource");
 
-    enum Mode { attribute, children };
-
-    // 3 - If the media element has a src attribute, then let mode be attribute.
-    Mode mode = attribute;
-    if (!fastHasAttribute(srcAttr)) {
-        // Otherwise, if the media element does not have a src attribute but has a source
-        // element child, then let mode be children and let candidate be the first such
-        // source element child in tree order.
-        if (HTMLSourceElement* element = Traversal<HTMLSourceElement>::firstChild(*this)) {
-            mode = children;
-            m_nextChildNodeToConsider = element;
-            m_currentSourceNode = nullptr;
-        } else {
-            // Otherwise the media element has neither a src attribute nor a source element
-            // child: set the networkState to NETWORK_EMPTY, and abort these steps; the
-            // synchronous section ends.
-            m_loadState = WaitingForSource;
-            setShouldDelayLoadEvent(false);
-            m_networkState = NETWORK_EMPTY;
+    enum Mode {Object, Attribute, Children};
+
+    // 3 - Decide which source to load.

[philipj_slow, 2014/09/16 13:39:51]

Please copy the steps from the spec. This should begin with "If the media
element has an assigned media provider object, then let mode be object."

[perkj_chrome, 2014/09/17 19:26:13]

Done.

+    // If the media element has a srcObject let the mode be object.
+    // If it has a src attribute, let the mode be attribute.
+    // If it has a source element, let the mode be Children.
+    Mode mode = Object;

[philipj_slow, 2014/09/16 13:39:51]

Remove the default init unless it's needed to squash some compiler warning? It
shouldn't be unless the compiler is very short-sighted.

[perkj_chrome, 2014/09/17 19:26:13]

I can try. I am afraid it will hit me on one platform.

+    if (srcObject()) {
+        mode = Object;
+    } else if (fastHasAttribute(srcAttr)) {
+        mode = Attribute;
+    } else if (Traversal<HTMLSourceElement>::firstChild(*this)) {
+        mode = Children;
+        HTMLSourceElement* element = Traversal<HTMLSourceElement>::firstChild(*this);
+        m_nextChildNodeToConsider = element;
+        m_currentSourceNode = nullptr;
+    } else {
+        // Otherwise the media element has no source.
+        // set the networkState to NETWORK_EMPTY, and abort these steps; the
+        // synchronous section ends.
+        m_loadState = WaitingForSource;
+        setShouldDelayLoadEvent(false);
+        m_networkState = NETWORK_EMPTY;
 
-            WTF_LOG(Media, "HTMLMediaElement::selectMediaResource, nothing to load");
-            return;
-        }
+        WTF_LOG(Media, "HTMLMediaElement::selectMediaResource, nothing to load");
+        return;
     }
 
     // 4 - Set the media element's delaying-the-load-event flag to true (this delays the load event),
@@ -888,34 +906,64 @@ void HTMLMediaElement::selectMediaResource()
     // 5 - Queue a task to fire a simple event named loadstart at the media element.
     scheduleEvent(EventTypeNames::loadstart);
 
-    // 6 - If mode is attribute, then run these substeps
-    if (mode == attribute) {
-        m_loadState = LoadingFromSrcAttr;
+    // 6 - Load the correct source.

[philipj_slow, 2014/09/16 13:39:50]

Should say "Run the appropriate steps from the following list:"

[perkj_chrome, 2014/09/17 19:26:13]

Done.

+    switch (mode) {
+    case Object:
+        loadSourceFromObject();

[philipj_slow, 2014/09/16 13:39:50]

I think just loadFromObject() and loadFromAttribute() would be better, it's not
a source that's being loaded, but rather one may end up in the resource fetch
algorithm, or abort early.

[perkj_chrome, 2014/09/17 19:26:13]

Done.

+        break;
+    case Attribute:
+        loadSourceFromAttribute();
+        break;
+    case Children:
+        loadNextSourceChild();
+        break;
+    }
+}
+
+void HTMLMediaElement::loadSourceFromObject()
+{
+    m_loadState = LoadingFromSrcObject;

[philipj_slow, 2014/09/16 13:39:50]

Can you annotate this function with comments from the spec? In particular I see
that "Set the currentSrc attribute to the empty string." does not seem to be
implemented. That would also need a test. First get a video element into a state
where currentSrc!="", then set srcObject and verify that immediately after
currentSrc is "".

[perkj_chrome, 2014/09/17 19:26:13]

Added test of currentSrc to mediastream-srcobject.html.
The implementation is a bit hacky due to how loadResource and both MediaStreams
and MediaSource require the use of URLs.

 
-        // If the src attribute's value is the empty string ... jump down to the failed step below
-        KURL mediaURL = getNonEmptyURLAttribute(srcAttr);
-        if (mediaURL.isEmpty()) {
-            mediaLoadingFailed(WebMediaPlayer::NetworkStateFormatError);
-            WTF_LOG(Media, "HTMLMediaElement::selectMediaResource, empty 'src'");
-            return;
-        }
+    KURL mediaURL = document().completeURL(m_srcObject->getObjectUrl());

[philipj_slow, 2014/09/16 13:39:51]

This is a little bit peculiar, since the point of srcObject is to avoid these
magic URLs. I don't have a concrete proposal, but eventually I think it would be
nice to unify things in the other direction, so that a magic URL is internally
handled like a MediaProvider object, and that any special load works by creating
a WebMediaPlayer from a MediaProver in some way.

Anyway, what you have here will get things started, and I don't believe it
really creates any additional complexity, so no action required.

[perkj_chrome, 2014/09/17 19:26:13]

Agree, this cl adds the interface MediaProvider so it should be possible. But
switching all cases to use a new interface sounds like a huge task since there
are many objects that today uses the src url. There are also two different media
player implementations in Chrome. 

 
-        if (!isSafeToLoadURL(mediaURL, Complain)) {
-            mediaLoadingFailed(WebMediaPlayer::NetworkStateFormatError);
-            return;
-        }
+    if (!isSafeToLoadURL(mediaURL, Complain)) {
+        mediaLoadingFailed(WebMediaPlayer::NetworkStateFormatError);

[philipj_slow, 2014/09/16 13:39:51]

This bit needs a test. It can cause HTMLMediaElement::noneSupported() to run
which will set m_error synchronously. Per spec the synchronous section has
already ended here, so there's no way that the error could be modified
synchronously in the srcObject setter. In order to pass that test, some code
change would be needed too.

Alternatively, if there actually is no case where isSafeToLoadURL() would return
false here, assert that and save some trouble.

[perkj_chrome, 2014/09/17 19:26:13]

I don't see how it would ever return anything but true since the url is
generated internally by blink. But I can't say that I understand exactly what it
it that it checks. 

+        return;
+    }
+
+    // No type or key system information is available when the url comes

[philipj_slow, 2014/09/16 13:39:51]

This is repeated, can you add a shared void loadResource(const KURL&) override
that provides the empty ContentType and keySystem instead?

[perkj_chrome, 2014/09/17 19:26:13]

Done.

+    // from the 'srcObject' so MediaPlayer
+    // will have to pick a media engine based on the file extension.
+    ContentType contentType((String()));
+    loadResource(mediaURL, contentType, String());
+    WTF_LOG(Media, "HTMLMediaElement::selectMediaResource, using 'srcObject' attribute.");
+    return;
+}
+
+void HTMLMediaElement::loadSourceFromAttribute()
+{
+    m_loadState = LoadingFromSrcAttr;
+
+    // If the src attribute's value is the empty string ... jump down to the failed step below
+    KURL mediaURL = getNonEmptyURLAttribute(srcAttr);
+    if (mediaURL.isEmpty()) {
+        mediaLoadingFailed(WebMediaPlayer::NetworkStateFormatError);
+        WTF_LOG(Media, "HTMLMediaElement::selectMediaResource, empty 'src'");
+        return;
+    }
 
-        // No type or key system information is available when the url comes
-        // from the 'src' attribute so MediaPlayer
-        // will have to pick a media engine based on the file extension.
-        ContentType contentType((String()));
-        loadResource(mediaURL, contentType, String());
-        WTF_LOG(Media, "HTMLMediaElement::selectMediaResource, using 'src' attribute url");
+    if (!isSafeToLoadURL(mediaURL, Complain)) {
+        mediaLoadingFailed(WebMediaPlayer::NetworkStateFormatError);
         return;
     }
 
-    // Otherwise, the source elements will be used
-    loadNextSourceChild();
+    // No type or key system information is available when the url comes
+    // from the 'src' attribute so MediaPlayer
+    // will have to pick a media engine based on the file extension.
+    ContentType contentType((String()));
+    loadResource(mediaURL, contentType, String());
+    WTF_LOG(Media, "HTMLMediaElement::selectMediaResource, using 'src' attribute url");
+    return;
 }
 
 void HTMLMediaElement::loadNextSourceChild()