Changeset 206127 in webkit


Ignore:
Timestamp:
Sep 19, 2016 4:24:15 PM (8 years ago)
Author:
jer.noble@apple.com
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:
Location:
trunk
Files:
1 added
9 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r206119 r206127  
     12016-09-16  Jer Noble  <jer.noble@apple.com>
     2
     3        [media-source] Fix imported/w3c/web-platform-tests/media-source/mediasource-avtracks.html
     4        https://bugs.webkit.org/show_bug.cgi?id=162104
     5
     6        Reviewed by Eric Carlson.
     7
     8        * imported/w3c/web-platform-tests/media-source/mediasource-avtracks-expected.txt
     9        * platform/mac/TestExpectations:
     10
    1112016-09-19  Daniel Bates  <dabates@apple.com>
    212

  • trunk/LayoutTests/platform/mac/TestExpectations

    r206119 r206127  
    10461046[ Yosemite+ ] imported/w3c/web-platform-tests/media-source/mediasource-addsourcebuffer-mode.html [ Pass ]
    10471047[ Yosemite+ ] imported/w3c/web-platform-tests/media-source/mediasource-addsourcebuffer.html [ Pass ]
     1048[ Yosemite+ ] imported/w3c/web-platform-tests/media-source/mediasource-avtracks.html [ Pass ]
    10481049[ Yosemite+ ] imported/w3c/web-platform-tests/media-source/mediasource-buffered.html [ Pass ]
    10491050[ Yosemite+ ] imported/w3c/web-platform-tests/media-source/mediasource-closed.html [ Pass ]

  • trunk/Source/WebCore/ChangeLog

    r206126 r206127  
     12016-09-16  Jer Noble  <jer.noble@apple.com>
     2
     3        [media-source] Fix imported/w3c/web-platform-tests/media-source/mediasource-avtracks.html
     4        https://bugs.webkit.org/show_bug.cgi?id=162104
     5
     6        Reviewed by Eric Carlson.
     7
     8        Fixes test: imported/w3c/web-platform-tests/media-source/mediasource-avtracks.html
     9
     10        Multiple overlapping issues are causing this test to fail:
     11
     12        - When a MediaSource object is not attached from a HTMLMediaElement, it's SourceBuffer
     13          objects will return `null` from .videoTracks and .audioTracks, foiling the tests ability
     14          to assert that sourceBuffer.videoTracks.length == 0.
     15
     16        - When a MediaSource object is detached from a HTMLMediaElement, it's tracks are removed
     17          but do not generate 'removedtrack' events.
     18
     19        When these bugs were fixed, a few more popped up:
     20
     21        - The HTMLMediaElement removes its tracks before it closes the MediaSource, which causes an
     22          assertion when the MediaSource tells the HTMLMediaElement to remove its copy of the
     23          source's tracks (which have already been removed).
     24
     25        - When the HTMLMediaElement is stop()-ed due to its ScriptExecutionContext being destroyed,
     26          it tries to close its MediaSource, which has itself already been stop()-ed and thus asserts.
     27
     28        To eliminate all these bugs and make the code more self explanatory, we will rename the
     29        HTMLMediaElement's closeMediaSource() method to detachMediaSource(), and the MediaSource's
     30        close() method to detachFromElement(). The only way to close a MediaSource is now by calling
     31        detachMediaSource() from the HTMLMediaElement.  The parts of the "Detaching from a media
     32        element" algorithm which were previously spread across setReadyState() and onReadyStateChange()
     33        are now unified in the newly renamed detachFromElement() method. The HTMLMediaElement will
     34        first detach its MediaSource, and only after that remove all its tracks.
     35
     36        * Modules/mediasource/MediaSource.cpp:
     37        (WebCore::MediaSource::setReadyState): Move steps into detachFromElement().
     38        (WebCore::MediaSource::onReadyStateChange): Ditto.
     39        (WebCore::MediaSource::detachFromElement): Perform the steps as specified.
     40        (WebCore::MediaSource::attachToElement): Takes a reference rather than a bare pointer.
     41        (WebCore::MediaSource::stop): Ask the media elemnet to detach.
     42        (WebCore::MediaSource::close): Renamed to detachFromElement().
     43        * Modules/mediasource/MediaSource.h:
     44        * Modules/mediasource/SourceBuffer.cpp:
     45        (WebCore::SourceBuffer::videoTracks): Always return a valid TrackList object.
     46        (WebCore::SourceBuffer::audioTracks): Ditto.
     47        (WebCore::SourceBuffer::textTracks): Ditto.
     48        * html/HTMLMediaElement.cpp:
     49        (WebCore::HTMLMediaElement::~HTMLMediaElement): Renamed closeMediaSource() -> detachMediaSource().
     50        (WebCore::HTMLMediaElement::prepareForLoad): Ditto.
     51        (WebCore::HTMLMediaElement::loadResource): Ditto.
     52        (WebCore::HTMLMediaElement::noneSupported): Ditto.
     53        (WebCore::HTMLMediaElement::mediaLoadingFailedFatally): Ditto.
     54        (WebCore::HTMLMediaElement::detachMediaSource): Ditto.
     55        (WebCore::HTMLMediaElement::userCancelledLoad): Ditto.
     56        (WebCore::HTMLMediaElement::createMediaPlayer): Ditto.
     57        (WebCore::HTMLMediaElement::clearMediaPlayer): Ditto. Also, detach from the MediaSource before
     58            removing tracks.
     59        (WebCore::HTMLMediaElement::closeMediaSource): Deleted.
     60        * html/HTMLMediaElement.h:
     61        * html/track/TrackListBase.cpp:
     62        (TrackListBase::remove): Only try to clear the media element from Tracks that have one.
     63
    1642016-09-19  Alex Christensen  <achristensen@webkit.org>
    265

  • trunk/Source/WebCore/Modules/mediasource/MediaSource.cpp

    r206001 r206127  
    466466    AtomicString oldState = readyState();
    467467    LOG(MediaSource, "MediaSource::setReadyState(%p) : %s -> %s", this, oldState.string().ascii().data(), state.string().ascii().data());
    468 
    469     if (state == closedKeyword()) {
    470         m_private = nullptr;
    471         m_mediaElement = nullptr;
    472         m_duration = MediaTime::invalidTime();
    473     }
    474468
    475469    if (oldState == state)
    … …  
    820814}
    821815
    822 void MediaSource::close()
    823 {
     816void MediaSource::detachFromElement(HTMLMediaElement& element)
     817{
     818    ASSERT_UNUSED(element, m_mediaElement == &element);
     819    ASSERT(!isClosed());
     820
     821    // 2.4.2 Detaching from a media element
     822    // https://rawgit.com/w3c/media-source/45627646344eea0170dd1cbc5a3d508ca751abb8/media-source-respec.html#mediasource-detach
     823
     824    // 1. Set the readyState attribute to "closed".
     825    // 7. Queue a task to fire a simple event named sourceclose at the MediaSource.
    824826    setReadyState(closedKeyword());
     827
     828    // 2. Update duration to NaN.
     829    m_duration = MediaTime::invalidTime();
     830
     831    // 3. Remove all the SourceBuffer objects from activeSourceBuffers.
     832    // 4. Queue a task to fire a simple event named removesourcebuffer at activeSourceBuffers.
     833    while (m_activeSourceBuffers->length())
     834        removeSourceBuffer(*m_activeSourceBuffers->item(0), IGNORE_EXCEPTION);
     835
     836    // 5. Remove all the SourceBuffer objects from sourceBuffers.
     837    // 6. Queue a task to fire a simple event named removesourcebuffer at sourceBuffers.
     838    while (m_sourceBuffers->length())
     839        removeSourceBuffer(*m_sourceBuffers->item(0), IGNORE_EXCEPTION);
     840
     841    m_private = nullptr;
     842    m_mediaElement = nullptr;
    825843}
    826844
    … …  
    830848}
    831849
    832 bool MediaSource::attachToElement(HTMLMediaElement* element)
     850bool MediaSource::attachToElement(HTMLMediaElement& element)
    833851{
    834852    if (m_mediaElement)
    … …  
    837855    ASSERT(isClosed());
    838856
    839     m_mediaElement = element;
     857    m_mediaElement = &element;
    840858    return true;
    841859}
    … …  
    859877{
    860878    m_asyncEventQueue.close();
    861     if (!isClosed())
    862         setReadyState(closedKeyword());
     879    if (m_mediaElement)
     880        m_mediaElement->detachMediaSource();
    863881    m_private = nullptr;
    864882}
    … …  
    890908
    891909    ASSERT(isClosed());
    892 
    893     m_activeSourceBuffers->clear();
    894 
    895     // Clear SourceBuffer references to this object.
    896     for (auto& buffer : *m_sourceBuffers)
    897         buffer->removedFromMediaSource();
    898     m_sourceBuffers->clear();
    899    
    900910    scheduleEvent(eventNames().sourcecloseEvent);
    901911}

  • trunk/Source/WebCore/Modules/mediasource/MediaSource.h

    r206001 r206127  
    7878    void seekToTime(const MediaTime&) override;
    7979
    80     bool attachToElement(HTMLMediaElement*);
    81     void close();
     80    bool attachToElement(HTMLMediaElement&);
     81    void detachFromElement(HTMLMediaElement&);
    8282    void monitorSourceBuffers();
    8383    bool isSeeking() const { return m_pendingSeekTime.isValid(); }

  • trunk/Source/WebCore/Modules/mediasource/SourceBuffer.cpp

    r206037 r206127  
    970970VideoTrackList* SourceBuffer::videoTracks()
    971971{
    972     if (!m_source || !m_source->mediaElement())
    973         return nullptr;
    974 
    975972    if (!m_videoTracks)
    976973        m_videoTracks = VideoTrackList::create(m_source->mediaElement(), ActiveDOMObject::scriptExecutionContext());
    … …  
    981978AudioTrackList* SourceBuffer::audioTracks()
    982979{
    983     if (!m_source || !m_source->mediaElement())
    984         return nullptr;
    985 
    986980    if (!m_audioTracks)
    987981        m_audioTracks = AudioTrackList::create(m_source->mediaElement(), ActiveDOMObject::scriptExecutionContext());
    … …  
    992986TextTrackList* SourceBuffer::textTracks()
    993987{
    994     if (!m_source || !m_source->mediaElement())
    995         return nullptr;
    996 
    997988    if (!m_textTracks)
    998989        m_textTracks = TextTrackList::create(m_source->mediaElement(), ActiveDOMObject::scriptExecutionContext());

  • trunk/Source/WebCore/html/HTMLMediaElement.cpp

    r205938 r206127  
    611611
    612612#if ENABLE(MEDIA_SOURCE)
    613     closeMediaSource();
     613    detachMediaSource();
    614614#endif
    615615
    … …  
    12071207
    12081208#if ENABLE(MEDIA_SOURCE)
    1209     closeMediaSource();
     1209    detachMediaSource();
    12101210#endif
    12111211
    … …  
    15231523
    15241524    if (m_mediaSource) {
    1525         if (m_mediaSource->attachToElement(this))
     1525        if (m_mediaSource->attachToElement(*this))
    15261526            m_player->load(url, contentType, m_mediaSource.get());
    15271527        else {
    … …  
    20682068
    20692069#if ENABLE(MEDIA_SOURCE)
    2070     closeMediaSource();
     2070    detachMediaSource();
    20712071#endif
    20722072
    … …  
    21022102
    21032103#if ENABLE(MEDIA_SOURCE)
    2104     closeMediaSource();
     2104    detachMediaSource();
    21052105#endif
    21062106
    … …  
    32873287
    32883288#if ENABLE(MEDIA_SOURCE)
    3289 void HTMLMediaElement::closeMediaSource()
     3289void HTMLMediaElement::detachMediaSource()
    32903290{
    32913291    if (!m_mediaSource)
    32923292        return;
    32933293
    3294     m_mediaSource->close();
     3294    m_mediaSource->detachFromElement(*this);
    32953295    m_mediaSource = nullptr;
    32963296}
    … …  
    51115111
    51125112#if ENABLE(MEDIA_SOURCE)
    5113     closeMediaSource();
     5113    detachMediaSource();
    51145114#endif
    51155115
    … …  
    51435143    LOG(Media, "HTMLMediaElement::clearMediaPlayer(%p) - flags = %s", this, actionName(flags).utf8().data());
    51445144
     5145#if ENABLE(MEDIA_SOURCE)
     5146    detachMediaSource();
     5147#endif
     5148
    51455149#if ENABLE(VIDEO_TRACK)
    51465150    forgetResourceSpecificTracks();
    5147 #endif
    5148 
    5149 #if ENABLE(MEDIA_SOURCE)
    5150     closeMediaSource();
    51515151#endif
    51525152
    … …  
    60976097#if ENABLE(MEDIA_SOURCE)
    60986098    if (m_mediaSource)
    6099         m_mediaSource->close();
     6099        m_mediaSource->detachFromElement(*this);
    61006100#endif
    61016101

  • trunk/Source/WebCore/html/HTMLMediaElement.h

    r205938 r206127  
    242242#if ENABLE(MEDIA_SOURCE)
    243243//  Media Source.
    244     void closeMediaSource();
     244    void detachMediaSource();
    245245    void incrementDroppedFrameCount() { ++m_droppedVideoFrames; }
    246246    size_t maximumSourceBufferSize(const SourceBuffer&) const;

  • trunk/Source/WebCore/html/track/TrackListBase.cpp

    r204050 r206127  
    7474    ASSERT(index != notFound);
    7575
    76     ASSERT(track.mediaElement() == m_element);
    77     track.setMediaElement(nullptr);
     76    if (track.mediaElement()) {
     77        ASSERT(track.mediaElement() == m_element);
     78        track.setMediaElement(nullptr);
     79    }
    7880
    7981    Ref<TrackBase> trackRef = *m_inbandTracks[index];
Note: See TracChangeset for help on using the changeset viewer.