Bug fix on ended event not triggered

[nicosang]

Hi,

this PR has to fix the bug #2129. The problem was that scheduleController pushed initialize segment after the signalEndOfStream was called.

Nicolas

[spiterikevin]

I tried running the "steps to reproduce" in #2129 on this patch and unfortunately removing the #2130 workaround reintroduces the problem.

This patch also introduces a new problem in multi-period - see sample "VOD (Static MPD) / Multiperiod". There are a few seconds of playback dropped between periods (just before 3:08).

[nicosang]

@spiterikevin, take a look, please, at my latest commit about the new issue you've detected.

[epiclabsDASH]

@nicosang, what do you think if we replace this line with this other one?

setTimeout(function () {
    eventBus.trigger(Events.PLAYBACK_ENDED);
}, e.timeToEnd > 0  ? e.timeToEnd : 0)

Another question. Could we ensure that in all the cases onPlaybackTimeUpdated is going to be called at least one time after onStreamBufferingCompleted?

[nicosang]

@epiclabsDASH , do you prefer the last commit with the use of a timer in onStreamBufferingCompleted function rather than onPlaybackTimeUpdated?

[epiclabsDASH]

I think your last change makes more sense, using the timeout in onStreamBufferingCompleted and avoiding the possibility of having race conditions between onPlaybackTimeUpdated and onStreamBufferingCompleted.

Let's wait for @spiterikevin opinion.

Thanks!

[sandersaares]

On Firefox 56, the player from PR now sometimes skips the 2nd period of https://media.axprod.net/TestVectors/v8-MultiContent/Clear/Manifest.mpd playing from the 1st straight into the 3rd period.

Does not happen 100% of the time but seems to happen most of the time. Have not noticed on other browsers.

Log:
Firefox_skip_period2.txt

[nicosang]

@sandersaares , I can't help you. As I said, I have an issue with firefox 56 when I want to play your stream :

Video Element Error: MEDIA_ERR_DECODE (NS_ERROR_DOM_MEDIA_FATAL_ERR (0x806e0005) - error creating audio decoder)

[epiclabsDASH]

I couldn't reproduce any issue after testing with many different type of streams. @spiterikevin, could you please take a last look to this?

[spiterikevin]

@epiclabsDASH I will. I don't have any time this coming week, but on the week of December 18 I'll have plenty. Sorry for the delay.

[spiterikevin]

I tested this on MacOS X Sierra 10.12.6 with Firefox 57.0.1, Safari 11.0.2 and Chrome 63.0.3239.108.

I'm no longer seeing the issues I saw earlier with this PR 👍
I also tested the mpd in #1901 and it works with this PR.

I saw an issue (created #2344) with multiperiod, but that new issue is not related to this PR and manifests itself with and without it. Multiperiod still works fine on Firefox and Safari with this PR.

[epiclabsDASH]

Many thanks!

[fvalleeHbbTV]

FYI, I've a regression after "remove workaround about not triggered ended event" commit, in some cases the period transition is broken. Unfortunately my tests are involving dynamic mpd update using a private server (HbbTV OTS) so I can't provide a way to reproduce it.
Typical use case is a MPD loaded with 1 period, then updated with a 2nd Period inserted.
Period transition is not working in 2.6.5 (or development branch HEAD), it was fine in 2.6.4.
In the failing cases, PLAYBACK_ENDED is not dispatched at the end of the first Period
(seems onStreamBufferingCompleted is never called).
I don't know if the failures are related to dynamic MPD update or anything else (but I didn't reproduce the issue with static MPD).

Following patch seems to fix/workaround the issue (it's basically restoring the 'workaround about not triggered ended event'):

diff --git a/src/streaming/controllers/StreamController.js b/src/streaming/controllers/StreamController.js
index 8b589ca1..add314da 100644
--- a/src/streaming/controllers/StreamController.js
+++ b/src/streaming/controllers/StreamController.js
@@ -140,7 +140,7 @@ function StreamController() {
      * Called when current playback position is changed.
      * Used to determine the time current stream is finished and we should switch to the next stream.
      */
-    function onPlaybackTimeUpdated(/*e*/) {
+    function onPlaybackTimeUpdated(e) {
         if (isVideoTrackPresent()) {
             const playbackQuality = videoModel.getPlaybackQuality();
             if (playbackQuality) {
@@ -151,6 +151,15 @@ function StreamController() {
         // Sometimes after seeking timeUpdateHandler is called before seekingHandler and a new stream starts
         // from beginning instead of from a chosen position. So we do nothing if the player is in the seeking state
         if (playbackController.isSeeking()) return;
+
+        const STREAM_END_THRESHOLD = 0.5;
+        if (e.timeToEnd <= STREAM_END_THRESHOLD && playbackEndedTimerId === undefined) {
+            // In some cases the ended event is not triggered at the end of the stream, do it artificially here.
+            // This should only be a fallback, put an extra STREAM_END_TIMEOUT_DELAY to give the real ended event time to trigger.
+            log('[StreamController][onPlaybackTimeUpdated] timeToEnd = ' + e.timeToEnd + ' PLAYBACK_ENDED need to be triggered');
+            const delayPlaybackEnded =  e.timeToEnd > 0 ? e.timeToEnd * 1000 : 0;
+            playbackEndedTimerId = setTimeout(function () {eventBus.trigger(Events.PLAYBACK_ENDED);}, delayPlaybackEnded);
+        }
     }

(I don't expect the patch to be merged, but I guess somebody else may have similar issue. Any feedback is welcome).
Thanks a lot for dash.js anyway!

[nicosang]

could you, please, at least provide us your logs without the workaround?

Thanks,
Nico

[fvalleeHbbTV]

I managed to play the dash content using the Reference client (http://reference.dashif.org/dash.js/nightly/samples/dash-if-reference-player/, firefox) and the period transition was OK using the nightly, 2.6.4 or 2.6.5 so I guess there is something wrong on my side and the regression I observed is not coming from dash.js. Sorry for the inconvenience.
Thanks,
Fabien