Title: [206127] trunk
Revision
206127
Author
jer.no...@apple.com
Date
2016-09-19 16:24:15 -0700 (Mon, 19 Sep 2016)

Log Message

[media-source] Fix imported/w3c/web-platform-tests/media-source/mediasource-avtracks.html
https://bugs.webkit.org/show_bug.cgi?id=162104

Reviewed by Eric Carlson.

Source/WebCore:

Fixes test: imported/w3c/web-platform-tests/media-source/mediasource-avtracks.html

Multiple overlapping issues are causing this test to fail:

- When a MediaSource object is not attached from a HTMLMediaElement, it's SourceBuffer
  objects will return `null` from .videoTracks and .audioTracks, foiling the tests ability
  to assert that sourceBuffer.videoTracks.length == 0.

- When a MediaSource object is detached from a HTMLMediaElement, it's tracks are removed
  but do not generate 'removedtrack' events.

When these bugs were fixed, a few more popped up:

- The HTMLMediaElement removes its tracks before it closes the MediaSource, which causes an
  assertion when the MediaSource tells the HTMLMediaElement to remove its copy of the
  source's tracks (which have already been removed).

- When the HTMLMediaElement is stop()-ed due to its ScriptExecutionContext being destroyed,
  it tries to close its MediaSource, which has itself already been stop()-ed and thus asserts.

To eliminate all these bugs and make the code more self explanatory, we will rename the
HTMLMediaElement's closeMediaSource() method to detachMediaSource(), and the MediaSource's
close() method to detachFromElement(). The only way to close a MediaSource is now by calling
detachMediaSource() from the HTMLMediaElement.  The parts of the "Detaching from a media
element" algorithm which were previously spread across setReadyState() and onReadyStateChange()
are now unified in the newly renamed detachFromElement() method. The HTMLMediaElement will
first detach its MediaSource, and only after that remove all its tracks.

* Modules/mediasource/MediaSource.cpp:
(WebCore::MediaSource::setReadyState): Move steps into detachFromElement().
(WebCore::MediaSource::onReadyStateChange): Ditto.
(WebCore::MediaSource::detachFromElement): Perform the steps as specified.
(WebCore::MediaSource::attachToElement): Takes a reference rather than a bare pointer.
(WebCore::MediaSource::stop): Ask the media elemnet to detach.
(WebCore::MediaSource::close): Renamed to detachFromElement().
* Modules/mediasource/MediaSource.h:
* Modules/mediasource/SourceBuffer.cpp:
(WebCore::SourceBuffer::videoTracks): Always return a valid TrackList object.
(WebCore::SourceBuffer::audioTracks): Ditto.
(WebCore::SourceBuffer::textTracks): Ditto.
* html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::~HTMLMediaElement): Renamed closeMediaSource() -> detachMediaSource().
(WebCore::HTMLMediaElement::prepareForLoad): Ditto.
(WebCore::HTMLMediaElement::loadResource): Ditto.
(WebCore::HTMLMediaElement::noneSupported): Ditto.
(WebCore::HTMLMediaElement::mediaLoadingFailedFatally): Ditto.
(WebCore::HTMLMediaElement::detachMediaSource): Ditto.
(WebCore::HTMLMediaElement::userCancelledLoad): Ditto.
(WebCore::HTMLMediaElement::createMediaPlayer): Ditto.
(WebCore::HTMLMediaElement::clearMediaPlayer): Ditto. Also, detach from the MediaSource before
    removing tracks.
(WebCore::HTMLMediaElement::closeMediaSource): Deleted.
* html/HTMLMediaElement.h:
* html/track/TrackListBase.cpp:
(TrackListBase::remove): Only try to clear the media element from Tracks that have one.

LayoutTests:

* imported/w3c/web-platform-tests/media-source/mediasource-avtracks-expected.txt
* platform/mac/TestExpectations:

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (206126 => 206127)


--- trunk/LayoutTests/ChangeLog	2016-09-19 23:05:11 UTC (rev 206126)
+++ trunk/LayoutTests/ChangeLog	2016-09-19 23:24:15 UTC (rev 206127)
@@ -1,3 +1,13 @@
+2016-09-16  Jer Noble  <jer.no...@apple.com>
+
+        [media-source] Fix imported/w3c/web-platform-tests/media-source/mediasource-avtracks.html
+        https://bugs.webkit.org/show_bug.cgi?id=162104
+
+        Reviewed by Eric Carlson.
+
+        * imported/w3c/web-platform-tests/media-source/mediasource-avtracks-expected.txt
+        * platform/mac/TestExpectations:
+
 2016-09-19  Daniel Bates  <daba...@apple.com>
 
         Remove ENABLE(TEXT_AUTOSIZING) automatic text size adjustment code

Added: trunk/LayoutTests/imported/w3c/web-platform-tests/media-source/mediasource-avtracks-expected.txt (0 => 206127)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/media-source/mediasource-avtracks-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/media-source/mediasource-avtracks-expected.txt	2016-09-19 23:24:15 UTC (rev 206127)
@@ -0,0 +1,6 @@
+
+PASS Check that media tracks and their properties are populated properly 
+PASS Media tracks must be removed when the SourceBuffer is removed from the MediaSource 
+PASS Media tracks must be removed when the HTMLMediaElement.src is changed 
+PASS Media tracks must be removed when HTMLMediaElement.load() is called 
+

Modified: trunk/LayoutTests/platform/mac/TestExpectations (206126 => 206127)


--- trunk/LayoutTests/platform/mac/TestExpectations	2016-09-19 23:05:11 UTC (rev 206126)
+++ trunk/LayoutTests/platform/mac/TestExpectations	2016-09-19 23:24:15 UTC (rev 206127)
@@ -1045,6 +1045,7 @@
 [ Yosemite+ ] imported/w3c/web-platform-tests/media-source/URL-createObjectURL.html [ Pass ]
 [ Yosemite+ ] imported/w3c/web-platform-tests/media-source/mediasource-addsourcebuffer-mode.html [ Pass ]
 [ Yosemite+ ] imported/w3c/web-platform-tests/media-source/mediasource-addsourcebuffer.html [ Pass ]
+[ Yosemite+ ] imported/w3c/web-platform-tests/media-source/mediasource-avtracks.html [ Pass ]
 [ Yosemite+ ] imported/w3c/web-platform-tests/media-source/mediasource-buffered.html [ Pass ]
 [ Yosemite+ ] imported/w3c/web-platform-tests/media-source/mediasource-closed.html [ Pass ]
 [ Yosemite+ ] imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-a-bitrate.html [ Pass ]

Modified: trunk/Source/WebCore/ChangeLog (206126 => 206127)


--- trunk/Source/WebCore/ChangeLog	2016-09-19 23:05:11 UTC (rev 206126)
+++ trunk/Source/WebCore/ChangeLog	2016-09-19 23:24:15 UTC (rev 206127)
@@ -1,3 +1,66 @@
+2016-09-16  Jer Noble  <jer.no...@apple.com>
+
+        [media-source] Fix imported/w3c/web-platform-tests/media-source/mediasource-avtracks.html
+        https://bugs.webkit.org/show_bug.cgi?id=162104
+
+        Reviewed by Eric Carlson.
+
+        Fixes test: imported/w3c/web-platform-tests/media-source/mediasource-avtracks.html
+
+        Multiple overlapping issues are causing this test to fail:
+
+        - When a MediaSource object is not attached from a HTMLMediaElement, it's SourceBuffer
+          objects will return `null` from .videoTracks and .audioTracks, foiling the tests ability
+          to assert that sourceBuffer.videoTracks.length == 0.
+
+        - When a MediaSource object is detached from a HTMLMediaElement, it's tracks are removed
+          but do not generate 'removedtrack' events.
+
+        When these bugs were fixed, a few more popped up:
+
+        - The HTMLMediaElement removes its tracks before it closes the MediaSource, which causes an
+          assertion when the MediaSource tells the HTMLMediaElement to remove its copy of the 
+          source's tracks (which have already been removed).
+
+        - When the HTMLMediaElement is stop()-ed due to its ScriptExecutionContext being destroyed,
+          it tries to close its MediaSource, which has itself already been stop()-ed and thus asserts.
+
+        To eliminate all these bugs and make the code more self explanatory, we will rename the 
+        HTMLMediaElement's closeMediaSource() method to detachMediaSource(), and the MediaSource's
+        close() method to detachFromElement(). The only way to close a MediaSource is now by calling
+        detachMediaSource() from the HTMLMediaElement.  The parts of the "Detaching from a media
+        element" algorithm which were previously spread across setReadyState() and onReadyStateChange()
+        are now unified in the newly renamed detachFromElement() method. The HTMLMediaElement will
+        first detach its MediaSource, and only after that remove all its tracks.
+
+        * Modules/mediasource/MediaSource.cpp:
+        (WebCore::MediaSource::setReadyState): Move steps into detachFromElement().
+        (WebCore::MediaSource::onReadyStateChange): Ditto.
+        (WebCore::MediaSource::detachFromElement): Perform the steps as specified.
+        (WebCore::MediaSource::attachToElement): Takes a reference rather than a bare pointer.
+        (WebCore::MediaSource::stop): Ask the media elemnet to detach.
+        (WebCore::MediaSource::close): Renamed to detachFromElement().
+        * Modules/mediasource/MediaSource.h:
+        * Modules/mediasource/SourceBuffer.cpp:
+        (WebCore::SourceBuffer::videoTracks): Always return a valid TrackList object.
+        (WebCore::SourceBuffer::audioTracks): Ditto.
+        (WebCore::SourceBuffer::textTracks): Ditto.
+        * html/HTMLMediaElement.cpp:
+        (WebCore::HTMLMediaElement::~HTMLMediaElement): Renamed closeMediaSource() -> detachMediaSource().
+        (WebCore::HTMLMediaElement::prepareForLoad): Ditto.
+        (WebCore::HTMLMediaElement::loadResource): Ditto.
+        (WebCore::HTMLMediaElement::noneSupported): Ditto.
+        (WebCore::HTMLMediaElement::mediaLoadingFailedFatally): Ditto.
+        (WebCore::HTMLMediaElement::detachMediaSource): Ditto.
+        (WebCore::HTMLMediaElement::userCancelledLoad): Ditto.
+        (WebCore::HTMLMediaElement::createMediaPlayer): Ditto.
+        (WebCore::HTMLMediaElement::clearMediaPlayer): Ditto. Also, detach from the MediaSource before
+            removing tracks.
+        (WebCore::HTMLMediaElement::closeMediaSource): Deleted.
+        * html/HTMLMediaElement.h:
+        * html/track/TrackListBase.cpp:
+        (TrackListBase::remove): Only try to clear the media element from Tracks that have one.
+
 2016-09-19  Alex Christensen  <achristen...@webkit.org>
 
         URLParser can read memory out of bounds

Modified: trunk/Source/WebCore/Modules/mediasource/MediaSource.cpp (206126 => 206127)


--- trunk/Source/WebCore/Modules/mediasource/MediaSource.cpp	2016-09-19 23:05:11 UTC (rev 206126)
+++ trunk/Source/WebCore/Modules/mediasource/MediaSource.cpp	2016-09-19 23:24:15 UTC (rev 206127)
@@ -466,12 +466,6 @@
     AtomicString oldState = readyState();
     LOG(MediaSource, "MediaSource::setReadyState(%p) : %s -> %s", this, oldState.string().ascii().data(), state.string().ascii().data());
 
-    if (state == closedKeyword()) {
-        m_private = nullptr;
-        m_mediaElement = nullptr;
-        m_duration = MediaTime::invalidTime();
-    }
-
     if (oldState == state)
         return;
 
@@ -819,9 +813,33 @@
     return readyState() == endedKeyword();
 }
 
-void MediaSource::close()
+void MediaSource::detachFromElement(HTMLMediaElement& element)
 {
+    ASSERT_UNUSED(element, m_mediaElement == &element);
+    ASSERT(!isClosed());
+
+    // 2.4.2 Detaching from a media element
+    // https://rawgit.com/w3c/media-source/45627646344eea0170dd1cbc5a3d508ca751abb8/media-source-respec.html#mediasource-detach
+
+    // 1. Set the readyState attribute to "closed".
+    // 7. Queue a task to fire a simple event named sourceclose at the MediaSource.
     setReadyState(closedKeyword());
+
+    // 2. Update duration to NaN.
+    m_duration = MediaTime::invalidTime();
+
+    // 3. Remove all the SourceBuffer objects from activeSourceBuffers.
+    // 4. Queue a task to fire a simple event named removesourcebuffer at activeSourceBuffers.
+    while (m_activeSourceBuffers->length())
+        removeSourceBuffer(*m_activeSourceBuffers->item(0), IGNORE_EXCEPTION);
+
+    // 5. Remove all the SourceBuffer objects from sourceBuffers.
+    // 6. Queue a task to fire a simple event named removesourcebuffer at sourceBuffers.
+    while (m_sourceBuffers->length())
+        removeSourceBuffer(*m_sourceBuffers->item(0), IGNORE_EXCEPTION);
+
+    m_private = nullptr;
+    m_mediaElement = nullptr;
 }
 
 void MediaSource::sourceBufferDidChangeActiveState(SourceBuffer&, bool)
@@ -829,7 +847,7 @@
     regenerateActiveSourceBuffers();
 }
 
-bool MediaSource::attachToElement(HTMLMediaElement* element)
+bool MediaSource::attachToElement(HTMLMediaElement& element)
 {
     if (m_mediaElement)
         return false;
@@ -836,7 +854,7 @@
 
     ASSERT(isClosed());
 
-    m_mediaElement = element;
+    m_mediaElement = &element;
     return true;
 }
 
@@ -858,8 +876,8 @@
 void MediaSource::stop()
 {
     m_asyncEventQueue.close();
-    if (!isClosed())
-        setReadyState(closedKeyword());
+    if (m_mediaElement)
+        m_mediaElement->detachMediaSource();
     m_private = nullptr;
 }
 
@@ -889,14 +907,6 @@
     }
 
     ASSERT(isClosed());
-
-    m_activeSourceBuffers->clear();
-
-    // Clear SourceBuffer references to this object.
-    for (auto& buffer : *m_sourceBuffers)
-        buffer->removedFromMediaSource();
-    m_sourceBuffers->clear();
-    
     scheduleEvent(eventNames().sourcecloseEvent);
 }
 

Modified: trunk/Source/WebCore/Modules/mediasource/MediaSource.h (206126 => 206127)


--- trunk/Source/WebCore/Modules/mediasource/MediaSource.h	2016-09-19 23:05:11 UTC (rev 206126)
+++ trunk/Source/WebCore/Modules/mediasource/MediaSource.h	2016-09-19 23:24:15 UTC (rev 206127)
@@ -77,8 +77,8 @@
     std::unique_ptr<PlatformTimeRanges> buffered() const override;
     void seekToTime(const MediaTime&) override;
 
-    bool attachToElement(HTMLMediaElement*);
-    void close();
+    bool attachToElement(HTMLMediaElement&);
+    void detachFromElement(HTMLMediaElement&);
     void monitorSourceBuffers();
     bool isSeeking() const { return m_pendingSeekTime.isValid(); }
     void completeSeek();

Modified: trunk/Source/WebCore/Modules/mediasource/SourceBuffer.cpp (206126 => 206127)


--- trunk/Source/WebCore/Modules/mediasource/SourceBuffer.cpp	2016-09-19 23:05:11 UTC (rev 206126)
+++ trunk/Source/WebCore/Modules/mediasource/SourceBuffer.cpp	2016-09-19 23:24:15 UTC (rev 206127)
@@ -969,9 +969,6 @@
 
 VideoTrackList* SourceBuffer::videoTracks()
 {
-    if (!m_source || !m_source->mediaElement())
-        return nullptr;
-
     if (!m_videoTracks)
         m_videoTracks = VideoTrackList::create(m_source->mediaElement(), ActiveDOMObject::scriptExecutionContext());
 
@@ -980,9 +977,6 @@
 
 AudioTrackList* SourceBuffer::audioTracks()
 {
-    if (!m_source || !m_source->mediaElement())
-        return nullptr;
-
     if (!m_audioTracks)
         m_audioTracks = AudioTrackList::create(m_source->mediaElement(), ActiveDOMObject::scriptExecutionContext());
 
@@ -991,9 +985,6 @@
 
 TextTrackList* SourceBuffer::textTracks()
 {
-    if (!m_source || !m_source->mediaElement())
-        return nullptr;
-
     if (!m_textTracks)
         m_textTracks = TextTrackList::create(m_source->mediaElement(), ActiveDOMObject::scriptExecutionContext());
 

Modified: trunk/Source/WebCore/html/HTMLMediaElement.cpp (206126 => 206127)


--- trunk/Source/WebCore/html/HTMLMediaElement.cpp	2016-09-19 23:05:11 UTC (rev 206126)
+++ trunk/Source/WebCore/html/HTMLMediaElement.cpp	2016-09-19 23:24:15 UTC (rev 206127)
@@ -610,7 +610,7 @@
     }
 
 #if ENABLE(MEDIA_SOURCE)
-    closeMediaSource();
+    detachMediaSource();
 #endif
 
 #if ENABLE(ENCRYPTED_MEDIA_V2)
@@ -1206,7 +1206,7 @@
         scheduleEvent(eventNames().abortEvent);
 
 #if ENABLE(MEDIA_SOURCE)
-    closeMediaSource();
+    detachMediaSource();
 #endif
 
     createMediaPlayer();
@@ -1522,7 +1522,7 @@
         m_mediaSource = MediaSource::lookup(url.string());
 
     if (m_mediaSource) {
-        if (m_mediaSource->attachToElement(this))
+        if (m_mediaSource->attachToElement(*this))
             m_player->load(url, contentType, m_mediaSource.get());
         else {
             // Forget our reference to the MediaSource, so we leave it alone
@@ -2067,7 +2067,7 @@
     rejectPendingPlayPromises(DOMError::create("NotSupportedError", "The operation is not supported."));
 
 #if ENABLE(MEDIA_SOURCE)
-    closeMediaSource();
+    detachMediaSource();
 #endif
 
     // 8 - Set the element's delaying-the-load-event flag to false. This stops delaying the load event.
@@ -2101,7 +2101,7 @@
     scheduleEvent(eventNames().errorEvent);
 
 #if ENABLE(MEDIA_SOURCE)
-    closeMediaSource();
+    detachMediaSource();
 #endif
 
     // 4 - Set the element's networkState attribute to the NETWORK_EMPTY value and queue a
@@ -3286,12 +3286,12 @@
 }
 
 #if ENABLE(MEDIA_SOURCE)
-void HTMLMediaElement::closeMediaSource()
+void HTMLMediaElement::detachMediaSource()
 {
     if (!m_mediaSource)
         return;
 
-    m_mediaSource->close();
+    m_mediaSource->detachFromElement(*this);
     m_mediaSource = nullptr;
 }
 #endif
@@ -5110,7 +5110,7 @@
     scheduleEvent(eventNames().abortEvent);
 
 #if ENABLE(MEDIA_SOURCE)
-    closeMediaSource();
+    detachMediaSource();
 #endif
 
     // 4 - If the media element's readyState attribute has a value equal to HAVE_NOTHING, set the 
@@ -5142,14 +5142,14 @@
 {
     LOG(Media, "HTMLMediaElement::clearMediaPlayer(%p) - flags = %s", this, actionName(flags).utf8().data());
 
+#if ENABLE(MEDIA_SOURCE)
+    detachMediaSource();
+#endif
+
 #if ENABLE(VIDEO_TRACK)
     forgetResourceSpecificTracks();
 #endif
 
-#if ENABLE(MEDIA_SOURCE)
-    closeMediaSource();
-#endif
-
 #if ENABLE(WIRELESS_PLAYBACK_TARGET)
     if (hasEventListeners(eventNames().webkitplaybacktargetavailabilitychangedEvent)) {
         m_hasPlaybackTargetAvailabilityListeners = false;
@@ -6096,7 +6096,7 @@
 
 #if ENABLE(MEDIA_SOURCE)
     if (m_mediaSource)
-        m_mediaSource->close();
+        m_mediaSource->detachFromElement(*this);
 #endif
 
 #if ENABLE(MEDIA_STREAM)

Modified: trunk/Source/WebCore/html/HTMLMediaElement.h (206126 => 206127)


--- trunk/Source/WebCore/html/HTMLMediaElement.h	2016-09-19 23:05:11 UTC (rev 206126)
+++ trunk/Source/WebCore/html/HTMLMediaElement.h	2016-09-19 23:24:15 UTC (rev 206127)
@@ -241,7 +241,7 @@
 
 #if ENABLE(MEDIA_SOURCE)
 //  Media Source.
-    void closeMediaSource();
+    void detachMediaSource();
     void incrementDroppedFrameCount() { ++m_droppedVideoFrames; }
     size_t maximumSourceBufferSize(const SourceBuffer&) const;
 #endif

Modified: trunk/Source/WebCore/html/track/TrackListBase.cpp (206126 => 206127)


--- trunk/Source/WebCore/html/track/TrackListBase.cpp	2016-09-19 23:05:11 UTC (rev 206126)
+++ trunk/Source/WebCore/html/track/TrackListBase.cpp	2016-09-19 23:24:15 UTC (rev 206127)
@@ -73,8 +73,10 @@
     size_t index = m_inbandTracks.find(&track);
     ASSERT(index != notFound);
 
-    ASSERT(track.mediaElement() == m_element);
-    track.setMediaElement(nullptr);
+    if (track.mediaElement()) {
+        ASSERT(track.mediaElement() == m_element);
+        track.setMediaElement(nullptr);
+    }
 
     Ref<TrackBase> trackRef = *m_inbandTracks[index];
 
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to