Log Message
[Gstreamer] cleanup buffering and state updates https://bugs.webkit.org/show_bug.cgi?id=116642
Reviewed by Philippe Normand. Source/WebCore: Covered by existing media tests, especially the ones that have been unskipped. Always do download buffering. It should not differ if preload is "none" because it's about when to start buffering, not about how. Fix a bunch other subtle issues to make this change go smoothly with the tests. As a bonus some tests that were failing so far started to pass. It is possible to further tweak the buffering in a way that will enable us to support a different and appropriate behavior for preload="metadata" but it should be managed a bit differently. I will implement it in a follow-up. * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp: (WebCore::MediaPlayerPrivateGStreamer::MediaPlayerPrivateGStreamer): (WebCore::MediaPlayerPrivateGStreamer::load): Don't call setDownloadBuffering yet. Now it also responsible for starting the fill timer and it turned out that setting the download flag before the pipeline is set to a state has negative effect on some tests. Most probably GStreamer starts buffering too early if we set the download flag. (WebCore::MediaPlayerPrivateGStreamer::commitLoad): Call setDownloadBuffering now. (WebCore::MediaPlayerPrivateGStreamer::play): Ditto, plus set m_delayingLoad to false because otherwise setPreload will wrongly assume that the load has not been committed yet if prepareToPlay is not called before play. (WebCore::MediaPlayerPrivateGStreamer::pause): Make sure we don't commit a load here if it was not committed yet by looking in the pipeline state. (WebCore::MediaPlayerPrivateGStreamer::processBufferingStats): No need to observe the type of buffering. (WebCore::MediaPlayerPrivateGStreamer::fillTimerFired): Introduced m_downloadFinished to update ready and network states in updateStates based on it. The previously used condition - (maxTimeLoaded() == duration()) - was not correct in the case when duration is not known. (WebCore::MediaPlayerPrivateGStreamer::maxTimeLoaded): Removed a lie. If the fill timer is not active it does not necesarily means that download has been finished. This lie was actually ensuring a lot of tests to pass because it made the condition (maxTimeLoaded() == duration()) true. Now that state updates has been cleaned up we don't need to lie. (WebCore::MediaPlayerPrivateGStreamer::updateStates): Merge all the parts that takes part in updating ready and network states. It was untrackable. Unset download buffering for for live streams early so we don't need to restart them. * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h: (MediaPlayerPrivateGStreamer): Removed m_startedBuffering because there is no need for it now. LayoutTests: Remove failure expectations from tests that started to pass. * platform/efl/TestExpectations: * platform/gtk/TestExpectations:
Modified Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (151796 => 151797)
--- trunk/LayoutTests/ChangeLog 2013-06-20 20:14:03 UTC (rev 151796)
+++ trunk/LayoutTests/ChangeLog 2013-06-20 20:19:36 UTC (rev 151797)
@@ -1,3 +1,15 @@
+2013-05-24 Balazs Kelemen <[email protected]>
+
+ [Gstreamer] cleanup buffering and state updates
+ https://bugs.webkit.org/show_bug.cgi?id=116642
+
+ Reviewed by Philippe Normand.
+
+ Remove failure expectations from tests that started to pass.
+
+ * platform/efl/TestExpectations:
+ * platform/gtk/TestExpectations:
+
2013-06-20 Eric Carlson <[email protected]>
ASSERT removing then adding a <track> element
Modified: trunk/LayoutTests/platform/efl/TestExpectations (151796 => 151797)
--- trunk/LayoutTests/platform/efl/TestExpectations 2013-06-20 20:14:03 UTC (rev 151796)
+++ trunk/LayoutTests/platform/efl/TestExpectations 2013-06-20 20:19:36 UTC (rev 151797)
@@ -1751,16 +1751,7 @@
webkit.org/b/114612 editing/style/block-style-005.html [ Failure ]
# Unidentified failing media tests
-webkit.org/b/115419 http/tests/media/reload-after-dialog.html [ Failure ]
-webkit.org/b/115419 http/tests/media/remove-while-loading.html [ Failure ]
-webkit.org/b/115419 http/tests/media/video-cancel-load.html [ Failure ]
-webkit.org/b/115419 http/tests/media/video-error-abort.html [ Failure ]
-webkit.org/b/115419 http/tests/media/video-load-suspend.html [ Failure ]
-webkit.org/b/115419 http/tests/media/video-load-twice.html [ Failure ]
-webkit.org/b/115419 http/tests/media/video-play-progress.html [ Failure ]
webkit.org/b/115419 http/tests/media/video-redirect.html [ Failure ]
-webkit.org/b/115419 http/tests/media/video-served-as-text.html [ Failure ]
-webkit.org/b/115419 http/tests/media/video-throttled-load-metadata.html [ Failure ]
webkit.org/b/115419 media/media-can-play-ogg.html [ Failure ]
webkit.org/b/115419 media/track/track-automatic-subtitles.html [ Failure ]
webkit.org/b/115419 media/video-poster-background.html [ ImageOnlyFailure ]
Modified: trunk/Source/WebCore/ChangeLog (151796 => 151797)
--- trunk/Source/WebCore/ChangeLog 2013-06-20 20:14:03 UTC (rev 151796)
+++ trunk/Source/WebCore/ChangeLog 2013-06-20 20:19:36 UTC (rev 151797)
@@ -1,3 +1,48 @@
+2013-05-24 Balazs Kelemen <[email protected]>
+
+ [Gstreamer] cleanup buffering and state updates
+ https://bugs.webkit.org/show_bug.cgi?id=116642
+
+ Reviewed by Philippe Normand.
+
+ Covered by existing media tests, especially the ones that have been unskipped.
+
+ Always do download buffering. It should not differ if preload is "none" because
+ it's about when to start buffering, not about how. Fix a bunch other subtle issues
+ to make this change go smoothly with the tests. As a bonus some tests that were
+ failing so far started to pass.
+ It is possible to further tweak the buffering in a way that will enable us to support
+ a different and appropriate behavior for preload="metadata" but it should be managed
+ a bit differently. I will implement it in a follow-up.
+
+ * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
+ (WebCore::MediaPlayerPrivateGStreamer::MediaPlayerPrivateGStreamer):
+ (WebCore::MediaPlayerPrivateGStreamer::load): Don't call setDownloadBuffering
+ yet. Now it also responsible for starting the fill timer and it turned out that
+ setting the download flag before the pipeline is set to a state has negative
+ effect on some tests. Most probably GStreamer starts buffering too early if
+ we set the download flag.
+ (WebCore::MediaPlayerPrivateGStreamer::commitLoad): Call setDownloadBuffering now.
+ (WebCore::MediaPlayerPrivateGStreamer::play): Ditto, plus set m_delayingLoad to false
+ because otherwise setPreload will wrongly assume that the load has not been committed
+ yet if prepareToPlay is not called before play.
+ (WebCore::MediaPlayerPrivateGStreamer::pause): Make sure we don't commit a load here
+ if it was not committed yet by looking in the pipeline state.
+ (WebCore::MediaPlayerPrivateGStreamer::processBufferingStats): No need to observe the
+ type of buffering.
+ (WebCore::MediaPlayerPrivateGStreamer::fillTimerFired): Introduced m_downloadFinished to update
+ ready and network states in updateStates based on it. The previously used condition
+ - (maxTimeLoaded() == duration()) - was not correct in the case when duration is not known.
+ (WebCore::MediaPlayerPrivateGStreamer::maxTimeLoaded): Removed a lie. If the fill timer is not active
+ it does not necesarily means that download has been finished. This lie was actually ensuring
+ a lot of tests to pass because it made the condition (maxTimeLoaded() == duration()) true.
+ Now that state updates has been cleaned up we don't need to lie.
+ (WebCore::MediaPlayerPrivateGStreamer::updateStates): Merge all the parts that takes part in updating
+ ready and network states. It was untrackable. Unset download buffering for for live streams early so
+ we don't need to restart them.
+ * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:
+ (MediaPlayerPrivateGStreamer): Removed m_startedBuffering because there is no need for it now.
+
2013-06-20 Eric Carlson <[email protected]>
ASSERT removing then adding a <track> element
Modified: trunk/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp (151796 => 151797)
--- trunk/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp 2013-06-20 20:14:03 UTC (rev 151796)
+++ trunk/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp 2013-06-20 20:19:36 UTC (rev 151797)
@@ -217,7 +217,7 @@
, m_playbackRate(1)
, m_errorOccured(false)
, m_mediaDuration(0)
- , m_startedBuffering(false)
+ , m_downloadFinished(false)
, m_fillTimer(this, &MediaPlayerPrivateGStreamer::fillTimerFired)
, m_maxTimeLoaded(0)
, m_bufferingPercentage(0)
@@ -288,10 +288,8 @@
if (kurl.isLocalFile())
cleanUrl = cleanUrl.substring(0, kurl.pathEnd());
- if (!m_playBin) {
+ if (!m_playBin)
createGSTPlayBin();
- setDownloadBuffering();
- }
ASSERT(m_playBin);
@@ -333,6 +331,7 @@
// start providing anything useful.
gst_element_set_state(m_playBin.get(), GST_STATE_PAUSED);
+ setDownloadBuffering();
updateStates();
}
@@ -407,13 +406,17 @@
{
if (changePipelineState(GST_STATE_PLAYING)) {
m_isEndReached = false;
+ m_delayingLoad = false;
+ setDownloadBuffering();
LOG_MEDIA_MESSAGE("Play");
}
}
void MediaPlayerPrivateGStreamer::pause()
{
- if (m_isEndReached)
+ GstState currentState, pendingState;
+ gst_element_get_state(m_playBin.get(), ¤tState, &pendingState, 0);
+ if (currentState < GST_STATE_PAUSED && pendingState <= GST_STATE_PAUSED)
return;
if (changePipelineState(GST_STATE_PAUSED))
@@ -827,38 +830,13 @@
void MediaPlayerPrivateGStreamer::processBufferingStats(GstMessage* message)
{
- // This is the immediate buffering that needs to happen so we have
- // enough to play right now.
m_buffering = true;
const GstStructure *structure = gst_message_get_structure(message);
gst_structure_get_int(structure, "buffer-percent", &m_bufferingPercentage);
LOG_MEDIA_MESSAGE("[Buffering] Buffering: %d%%.", m_bufferingPercentage);
- GstBufferingMode mode;
- gst_message_parse_buffering_stats(message, &mode, 0, 0, 0);
- if (mode != GST_BUFFERING_DOWNLOAD) {
- updateStates();
- return;
- }
-
- // This is on-disk buffering, that allows us to download much more
- // than needed for right now.
- if (!m_startedBuffering) {
- LOG_MEDIA_MESSAGE("[Buffering] Starting on-disk buffering.");
-
- m_startedBuffering = true;
-
- if (m_fillTimer.isActive())
- m_fillTimer.stop();
-
- m_fillTimer.startRepeating(0.2);
- }
-
- if (!m_paused && m_bufferingPercentage < 100) {
- LOG_MEDIA_MESSAGE("[Buffering] Download in progress, pausing pipeline.");
- gst_element_set_state(m_playBin.get(), GST_STATE_PAUSED);
- }
+ updateStates();
}
void MediaPlayerPrivateGStreamer::fillTimerFired(Timer<MediaPlayerPrivateGStreamer>*)
@@ -894,7 +872,8 @@
LOG_MEDIA_MESSAGE("[Buffering] Updated maxTimeLoaded: %f", m_maxTimeLoaded);
}
- if (fillStatus != 100.0) {
+ m_downloadFinished = fillStatus == 100.0;
+ if (!m_downloadFinished) {
updateStates();
return;
}
@@ -903,7 +882,6 @@
// connection is cut. Buffering is done, remove the fill source
// from the main loop.
m_fillTimer.stop();
- m_startedBuffering = false;
updateStates();
}
@@ -926,8 +904,8 @@
return 0.0f;
float loaded = m_maxTimeLoaded;
- if (!loaded && !m_fillTimer.isActive())
- loaded = duration();
+ if (m_isEndReached && m_mediaDuration)
+ loaded = m_mediaDuration;
LOG_MEDIA_MESSAGE("maxTimeLoaded: %f", loaded);
return loaded;
}
@@ -1097,7 +1075,7 @@
bool shouldUpdatePlaybackState = false;
switch (getStateResult) {
- case GST_STATE_CHANGE_SUCCESS:
+ case GST_STATE_CHANGE_SUCCESS: {
LOG_MEDIA_MESSAGE("State: %s, pending: %s", gst_element_state_get_name(state), gst_element_state_get_name(pending));
if (state <= GST_STATE_READY) {
@@ -1106,26 +1084,45 @@
} else
cacheDuration();
- // Try to figure out ready and network states.
- if (state == GST_STATE_READY) {
+ bool didBuffering = m_buffering;
+
+ // Update ready and network states.
+ switch (state) {
+ case GST_STATE_NULL:
+ m_readyState = MediaPlayer::HaveNothing;
+ m_networkState = MediaPlayer::Empty;
+ break;
+ case GST_STATE_READY:
m_readyState = MediaPlayer::HaveMetadata;
m_networkState = MediaPlayer::Empty;
- } else if ((state == GST_STATE_NULL) || (maxTimeLoaded() == duration())) {
- m_networkState = MediaPlayer::Loaded;
- m_readyState = MediaPlayer::HaveEnoughData;
- } else {
- m_readyState = currentTime() < maxTimeLoaded() ? MediaPlayer::HaveFutureData : MediaPlayer::HaveCurrentData;
- m_networkState = MediaPlayer::Loading;
- }
+ break;
+ case GST_STATE_PAUSED:
+ case GST_STATE_PLAYING:
+ if (m_buffering) {
+ if (m_bufferingPercentage == 100) {
+ LOG_MEDIA_MESSAGE("[Buffering] Complete.");
+ m_buffering = false;
+ m_readyState = MediaPlayer::HaveEnoughData;
+ m_networkState = m_downloadFinished ? MediaPlayer::Idle : MediaPlayer::Loading;
+ } else {
+ m_readyState = MediaPlayer::HaveCurrentData;
+ m_networkState = MediaPlayer::Loading;
+ }
+ } else if (m_downloadFinished) {
+ m_readyState = MediaPlayer::HaveEnoughData;
+ m_networkState = MediaPlayer::Loaded;
+ } else {
+ m_readyState = MediaPlayer::HaveFutureData;
+ m_networkState = MediaPlayer::Loading;
+ }
- if (m_buffering && state != GST_STATE_READY) {
- m_readyState = MediaPlayer::HaveCurrentData;
- m_networkState = MediaPlayer::Loading;
+ break;
+ default:
+ ASSERT_NOT_REACHED();
+ break;
}
- // Now let's try to get the states in more detail using
- // information from GStreamer, while we sync states where
- // needed.
+ // Sync states where needed.
if (state == GST_STATE_PAUSED) {
if (!m_webkitAudioSink)
updateAudioSink();
@@ -1136,39 +1133,20 @@
m_volumeAndMuteInitialized = true;
}
- if (m_buffering && m_bufferingPercentage == 100) {
- m_buffering = false;
- m_bufferingPercentage = 0;
- m_readyState = MediaPlayer::HaveEnoughData;
-
- LOG_MEDIA_MESSAGE("[Buffering] Complete.");
-
- if (!m_paused) {
- LOG_MEDIA_MESSAGE("[Buffering] Restarting playback.");
- gst_element_set_state(m_playBin.get(), GST_STATE_PLAYING);
- }
- } else if (!m_buffering && (currentTime() < duration())) {
- m_paused = true;
+ if (didBuffering && !m_buffering && !m_paused) {
+ LOG_MEDIA_MESSAGE("[Buffering] Restarting playback.");
+ gst_element_set_state(m_playBin.get(), GST_STATE_PLAYING);
}
} else if (state == GST_STATE_PLAYING) {
- m_readyState = MediaPlayer::HaveEnoughData;
m_paused = false;
if (m_buffering && !isLiveStream()) {
- m_readyState = MediaPlayer::HaveCurrentData;
- m_networkState = MediaPlayer::Loading;
-
LOG_MEDIA_MESSAGE("[Buffering] Pausing stream for buffering.");
-
gst_element_set_state(m_playBin.get(), GST_STATE_PAUSED);
}
} else
m_paused = true;
- // Is on-disk buffering in progress?
- if (m_fillTimer.isActive())
- m_networkState = MediaPlayer::Loading;
-
if (m_changingRate) {
m_player->rateChanged();
m_changingRate = false;
@@ -1180,19 +1158,11 @@
}
break;
+ }
case GST_STATE_CHANGE_ASYNC:
LOG_MEDIA_MESSAGE("Async: State: %s, pending: %s", gst_element_state_get_name(state), gst_element_state_get_name(pending));
- // Change in progress
+ // Change in progress.
- // On-disk buffering was attempted but the media is live. This
- // can't work so disable on-disk buffering and reset the
- // pipeline.
- if (state == GST_STATE_READY && isLiveStream() && m_preload == MediaPlayer::Auto) {
- setPreload(MediaPlayer::None);
- gst_element_set_state(m_playBin.get(), GST_STATE_NULL);
- gst_element_set_state(m_playBin.get(), GST_STATE_PAUSED);
- }
-
// A live stream was paused, reset the pipeline.
if (state == GST_STATE_PAUSED && pending == GST_STATE_PLAYING && isLiveStream()) {
gst_element_set_state(m_playBin.get(), GST_STATE_NULL);
@@ -1207,13 +1177,15 @@
case GST_STATE_CHANGE_NO_PREROLL:
LOG_MEDIA_MESSAGE("No preroll: State: %s, pending: %s", gst_element_state_get_name(state), gst_element_state_get_name(pending));
+ // Live pipelines go in PAUSED without prerolling.
+ m_isStreaming = true;
+ setDownloadBuffering();
+
if (state == GST_STATE_READY)
m_readyState = MediaPlayer::HaveNothing;
else if (state == GST_STATE_PAUSED) {
m_readyState = MediaPlayer::HaveEnoughData;
m_paused = true;
- // Live pipelines go in PAUSED without prerolling.
- m_isStreaming = true;
} else if (state == GST_STATE_PLAYING)
m_paused = false;
@@ -1339,7 +1311,6 @@
}
m_mediaLocationCurrentIndex--;
return false;
-
}
void MediaPlayerPrivateGStreamer::loadStateChanged()
@@ -1371,6 +1342,7 @@
if (!m_player->mediaPlayerClient()->mediaPlayerIsLooping()) {
m_paused = true;
gst_element_set_state(m_playBin.get(), GST_STATE_NULL);
+ m_downloadFinished = false;
}
}
@@ -1545,12 +1517,15 @@
GstPlayFlags flags;
g_object_get(m_playBin.get(), "flags", &flags, NULL);
- if (m_preload == MediaPlayer::Auto) {
+ bool shouldDownload = !isLiveStream();
+ if (shouldDownload) {
LOG_MEDIA_MESSAGE("Enabling on-disk buffering");
g_object_set(m_playBin.get(), "flags", flags | GST_PLAY_FLAG_DOWNLOAD, NULL);
+ m_fillTimer.startRepeating(0.2);
} else {
LOG_MEDIA_MESSAGE("Disabling on-disk buffering");
g_object_set(m_playBin.get(), "flags", flags & ~GST_PLAY_FLAG_DOWNLOAD, NULL);
+ m_fillTimer.stop();
}
}
@@ -1561,8 +1536,6 @@
m_preload = preload;
- setDownloadBuffering();
-
if (m_delayingLoad && m_preload != MediaPlayer::None) {
m_delayingLoad = false;
commitLoad();
Modified: trunk/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h (151796 => 151797)
--- trunk/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h 2013-06-20 20:14:03 UTC (rev 151796)
+++ trunk/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h 2013-06-20 20:19:36 UTC (rev 151797)
@@ -148,7 +148,7 @@
float m_playbackRate;
bool m_errorOccured;
mutable gfloat m_mediaDuration;
- bool m_startedBuffering;
+ bool m_downloadFinished;
Timer<MediaPlayerPrivateGStreamer> m_fillTimer;
float m_maxTimeLoaded;
int m_bufferingPercentage;
_______________________________________________ webkit-changes mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-changes
