This repository has been archived by the owner. It is now read-only.

blacklist the playlist that has stopped being updated and never blacklist the final playlist available #1039

Merged
merged 28 commits into from Mar 24, 2017

Conversation

Projects
None yet
3 participants
@zhuangs
Copy link
Contributor

zhuangs commented Mar 6, 2017

Description

Detect that playlist has stopped being updated in a timely manner and blacklist the playlist
Never blacklist the final playlist available #1026

@zhuangs zhuangs force-pushed the zhuangs:fix/blacklisting-stuck-playlist branch from 61b4072 to 3d28543 Mar 6, 2017

@zhuangs zhuangs force-pushed the zhuangs:fix/blacklisting-stuck-playlist branch from 3d28543 to b3b9598 Mar 7, 2017

@@ -361,6 +370,32 @@ export class MasterPlaylistController extends videojs.EventTarget {
} else {
addSeekableRange();
}

buffered = this.tech_.buffered();

This comment has been minimized.

@mjneil

mjneil Mar 7, 2017

Collaborator

I think it would be good to pull out this section of calculations to a separate function as this event handler is getting quite gnarly

This comment has been minimized.

@zhuangs

zhuangs Mar 10, 2017

Contributor

changed this

// and the buffered data ends at the end of the last segment in the playlist
if (bufferedTime <= Ranges.TIME_FUDGE_FACTOR &&
endTime <= Ranges.TIME_FUDGE_FACTOR &&
currentTime > seekableEnd) {

This comment has been minimized.

@mjneil

mjneil Mar 7, 2017

Collaborator

seekableEnd, lastBufferedEnd and endTime have the potential of being NaN at this point since seekableEnd and lastBufferedEnd may never be assigned a value if the conditionals for the if statements fail, and playEnd may be null from this.playlistEnd() affecting the calculation of endTime

This comment has been minimized.

@zhuangs

zhuangs Mar 8, 2017

Contributor

I thought seekableEnd, lastBufferedEnd, endTime being NaN may only happen at the very beginning of a playlist and that isn't the case we should blacklistCurrentPlaylist, is this correct? In that case, it doesn't affect the if statement. Did you mean we should check seekableEnd, lastBufferedEnd, endTime isNaN() inside the if?

This comment has been minimized.

@mjneil

mjneil Mar 8, 2017

Collaborator

With lastBufferedEnd for example, you declare lastBufferedEnd at the start of this function, so it is undefined. The only location lastBufferedEnd is assigned a value is on line 378, which is inside the if (buffered.length) check. If buffered.length === 0 then lastBufferedEnd will still be undefined and any calculation involving lastBufferedEnd e.g. endTime = playEnd - lastBufferedEnd will result in NaN.

With that said, looking at this again, it may be ok for this to happen. Since any comparison with NaN will return false, seekable.length === 0 or buffered.length === 0 probably means we don't have enough information to know if the playlist stopped updating and probably shouldn't blacklist it. This will require testing to see if we can create a scenario in which those values result in NaN but we still want to blacklist

This comment has been minimized.

@mjneil

mjneil Mar 14, 2017

Collaborator

So I was giving this more thought and I think we can avoid worrying about it all together by calling this method and blacklisting the playlist outside of the loadedplaylist listener. Playlist Loader already knows when a playlist it has loaded didn't actually update the live window. The isPlaylistOutdated_ method more so checks if the player is stuck at the end despite loading "fresh" playlists. If instead playlist loader triggered an event at that moment it knows the playlist isnt a fresh update, then MasterPlaylistController can listen to that and run the checks for playlistOutdated and if those checks pass, then we blacklist. This way we wont run these checks every time the playlist is refreshing correctly and run the risk of accidentally blacklisting a playlist we don't want to

This comment has been minimized.

@zhuangs

zhuangs Mar 15, 2017

Contributor

changed as suggested

@@ -233,6 +234,7 @@ export class MasterPlaylistController extends videojs.EventTarget {

this.seekable_ = videojs.createTimeRanges();
this.hasPlayed_ = () => false;
this.playlistEnd = playlistEnd;

This comment has been minimized.

@mjneil

mjneil Mar 7, 2017

Collaborator

It isn't necessary to attach this function to MasterPlaylistController, you can just call playlistEnd directly where you need it. On top of that, if you attach playlistEnd to the default export of playlist.js, the default export is already included in the Hls object available in this file. If you go that route, you do not need to import this function in a seperate import call, and instead could just call Hls.Playlist.playlistEnd()

This comment has been minimized.

@zhuangs

zhuangs Mar 8, 2017

Contributor

I attached this function to MasterPlaylistController because I was trying to call this function in the videojs-contrib-hls.test.js file. but maybe there is better way to do this

This comment has been minimized.

@mjneil

mjneil Mar 8, 2017

Collaborator

Ah didn't notice that. Hmmmm, its not a big deal to attach the function, but if you come up with another solution that doesnt require it, thatd be great

This comment has been minimized.

@zhuangs

zhuangs Mar 8, 2017

Contributor

yes, I found directly override Hls.Playlist.playlistEnd() in the test file is better :)

* @function playlistEnd
*/

export const playlistEnd = function(playlist) {

This comment has been minimized.

@mjneil

mjneil Mar 7, 2017

Collaborator

this function is very similar to seekable defined just below. Is there any way we can reduce the amount of repeat code here?

This comment has been minimized.

@zhuangs

zhuangs Mar 10, 2017

Contributor

changed this

@mjneil

This comment has been minimized.

Copy link
Collaborator

mjneil commented Mar 7, 2017

Would be good to have a some tests for playlistEnd

@mjneil mjneil self-assigned this Mar 7, 2017

@@ -305,6 +307,43 @@ const calculateExpiredTime = function(playlist, expiredSync, segmentSync) {

return segmentSync.time - sumDurations(playlist, syncIndex, 0);
}
return 0;

This comment has been minimized.

@mjneil

mjneil Mar 13, 2017

Collaborator

This should return null by default isntead of 0. If there isn't enough information to determine expired time, returning 0 would be sending false information if there actually is expired time.

This comment has been minimized.

@zhuangs

zhuangs Mar 13, 2017

Contributor

Did this in the first place but then I found there is a case that the playlist contains the endList, so the seekableEnd will return the duration of playlist but the seekableStart returns null. I think that's why I changed here to return 0 and check for null on the sync points instead of checking expired.

This comment has been minimized.

@mjneil

mjneil Mar 13, 2017

Collaborator

That is a good point, but I don't think returning 0 is the right call here since there are live scenarios where we may not have any sync points, but there has been expired time, we just can't calculate it. Since endList indicates a VOD asset, we actually already have a known sync point then just off knowing endList alone, which would be { time: 0, mediaSequence: 0 } since VOD always starts at time 0 segment 0. I'd say it would make most sense to update getPlaylistSyncPoints to set expiredSync to { time: 0, mediaSequence: 0 } if playlist.endList, otherwise set to playlist.syncInfo, and finally resorting to null. That way calculateExpiredTime can properly calculate for VOD case, while still returning null in the live case where we don't have enough information

This comment has been minimized.

@zhuangs

zhuangs Mar 13, 2017

Contributor

fixed this

if (playlist.endList) {
return duration(playlist);
}
let { expiredSync, segmentSync } = getPlaylistSyncPoints(playlist);

This comment has been minimized.

@mjneil

mjneil Mar 13, 2017

Collaborator

Since you moved getPlaylistSyncPoints to calculateExpiredTime calling it again here just to check for null on expiredSync segmentSync is unnecessary work. If calculateExpiredTime starts returning null when it can't calculate expired, you can just check for null on expired instead of checking the sync points

This comment has been minimized.

@zhuangs

zhuangs Mar 13, 2017

Contributor

fixed this

* @param {Object} playlist the media playlist object
* @return {boolean} whether the playlist has stopped being updated or not
*/
stopUpdateCheck(playlist) {

This comment has been minimized.

@mjneil

mjneil Mar 13, 2017

Collaborator

minor, but I think we can come up with a better name for the method. Maybe isPlaylistOutdated_, isPlaylistStagnant_, isPlaylistUpdating or something along these lines that is a bit more descriptive of what the method does. (note the return value should reflect the name logically). This method should also be private, indicated by the _ appended to the end of the name

This comment has been minimized.

@zhuangs

zhuangs Mar 13, 2017

Contributor

fixed this

Shan Zhuang added some commits Mar 10, 2017

Shan Zhuang
pull out the playlistEnd calculation to a separate function, rewrite …
…seekable and playlistEnd to reduce the amount of repeat code and add tests for playlistEnd
Shan Zhuang
handle the expired for VOD case, check for null on expired instead of…
… checking the sync points and rename playlist update check function

@zhuangs zhuangs force-pushed the zhuangs:fix/blacklisting-stuck-playlist branch from 978bef5 to 5fe7303 Mar 13, 2017

@zhuangs zhuangs force-pushed the zhuangs:fix/blacklisting-stuck-playlist branch from 400b990 to fa12f4f Mar 15, 2017

@@ -408,6 +408,15 @@ export class MasterPlaylistController extends videojs.EventTarget {
bubbles: true
});
});

this.masterPlaylistLoader_.on('playlistnotupdate', () => {

This comment has been minimized.

@mjneil

mjneil Mar 15, 2017

Collaborator

maybe name the event playlistunchanged ?

This comment has been minimized.

@zhuangs

zhuangs Mar 16, 2017

Contributor

renamed!

* @param {Object} playlist the media playlist object
* @return {boolean} whether the playlist has stopped being updated or not
*/
isPlaylistOutdated_(playlist) {

This comment has been minimized.

@mjneil

mjneil Mar 15, 2017

Collaborator

I know I've suggested renaming this already, but now that we have an event that signifies the playlist is outdated/not refreshing, and this method more checks if we are stuck at the end of the playlist, so maybe somthing like stuckAtPlaylistEnd_or isAtPlaylistEnd_. Then our logic sort of semantically reads like if the refreshed playlist is unchanged and the player is at the end of the playlist, blacklist this playlist.

This comment has been minimized.

@zhuangs

zhuangs Mar 16, 2017

Contributor

makes sense! renamed!

Shan Zhuang
'16.ts\n');
// trigger a refresh
this.clock.tick(10 * 1000);

This comment has been minimized.

@mjneil

mjneil Mar 16, 2017

Collaborator

I think it would be good to add a check between the two refreshes to make sure the playlist wasnt blacklisted early. (you may have to move the play and playing triggers up, not sure though)

@gesinger
Copy link
Collaborator

gesinger left a comment

Would be good to have some tests for stuckAtPlaylistEnd_ directly.

bufferedTime = lastBufferedEnd - currentTime;

// the end of the current playlist
// playEnd = this.playlistEnd(playlist);

This comment has been minimized.

@gesinger

gesinger Mar 17, 2017

Collaborator

extra comment

let playEnd;
let endTime;

buffered = this.tech_.buffered();

This comment has been minimized.

@gesinger

gesinger Mar 17, 2017

Collaborator

Can remove a lot of lines up top if we create the variables where they are first used.

// the time between end of the playlist and the end of the buffered
endTime = playEnd - lastBufferedEnd;

if (this.seekable().length) {

This comment has been minimized.

@gesinger

gesinger Mar 17, 2017

Collaborator

If there is no seekable range, can we be technically stuck at playlist end?


// the end of the current playlist
// playEnd = this.playlistEnd(playlist);
playEnd = Hls.Playlist.playlistEnd(playlist);

This comment has been minimized.

@gesinger

gesinger Mar 17, 2017

Collaborator

Since this is only used in one place, it may be clearer to just use it in the calculation directly: endTime = Hls.Playlist.playlistEnd(playlist) - lastBufferedEnd; . We should also change the comment to say it is using the absolute end of the playlist, and not the safe live end.

if (buffered.length) {
lastBufferedEnd = buffered.end(buffered.length - 1);
}
bufferedTime = lastBufferedEnd - currentTime;

This comment has been minimized.

@gesinger

gesinger Mar 17, 2017

Collaborator

While NaN can sometimes be OK, it is usually good to try to avoid using it. In this case, where we need the amount of time buffered, this should be 0 instead of NaN.

*/

export const playlistEnd = function(playlist) {
return calculatePlaylistEnd_(playlist);

This comment has been minimized.

@gesinger

gesinger Mar 17, 2017

Collaborator

If this function just calls the other function, do we need both?

return null;
};

const calculatePlaylistEnd_ = function(playlist, useSafeLiveEnd) {

This comment has been minimized.

@gesinger

gesinger Mar 17, 2017

Collaborator

Would be good to keep some documentation about the method, particularly the definition of useSafeLiveEnd (i.e., "live playlists should not expose three segment durations worth...")

return null;
}
endSequence = useSafeLiveEnd ? Math.max(0, playlist.segments.length - Playlist.UNSAFE_LIVE_SEGMENTS) : Math.max(0, playlist.segments.length);
let end = intervalDuration(playlist,

This comment has been minimized.

@gesinger

gesinger Mar 17, 2017

Collaborator

Can just return directly instead of creating a variable for it.

openMediaSource(this.player, this.clock);
this.player.tech_.triggerReady();

this.requests[0].respond(200, null,

This comment has been minimized.

@gesinger

gesinger Mar 17, 2017

Collaborator

We might be able to use standardXHRResponse to save some lines. We can also use this.requests.shift to make it so that the latest outstanding request is always at the beginning of the array (no need to keep track of them via indexing).

url = this.requests[1].url.slice(this.requests[1].url.lastIndexOf('/') + 1);
media = this.player.tech_.hls.playlists.master.playlists[url];

assert.ok(!media.excludeUntil, 'playlist didnt be blacklisted');

This comment has been minimized.

@gesinger

gesinger Mar 17, 2017

Collaborator

playlist wasn't blacklisted

@zhuangs zhuangs force-pushed the zhuangs:fix/blacklisting-stuck-playlist branch from 73af052 to 68f4687 Mar 20, 2017

videojs.log.warn('Problem encountered with the current ' +
'HLS playlist. Switching to another playlist.');
videojs.log.warn('Problem encountered with the current HLS playlist.' +
(error.message ? ' ' + error.message : '') +

This comment has been minimized.

@gesinger

gesinger Mar 23, 2017

Collaborator

Indentation

This comment has been minimized.

@zhuangs

zhuangs Mar 23, 2017

Contributor

changed! sorry for keeping making this mistake

Shan Zhuang

Shan Zhuang and others added some commits Mar 23, 2017

Shan Zhuang
Shan Zhuang
Shan Zhuang
Shan Zhuang
Shan Zhuang
this.player.tech_.setCurrentTime(50);
this.player.tech_.buffered = () => videojs.createTimeRange();
Hls.Playlist.playlistEnd = () => 130;
assert.ok(!this.masterPlaylistController.stuckAtPlaylistEnd_(playlist), 'not stuck at playlist end');

This comment has been minimized.

@mjneil

mjneil Mar 24, 2017

Collaborator

Since stuckAtPlaylistEnd_ only uses seekable to check if we have a seekable range, but doesn't use seekable.end(0) for any of its checks, is there even a point to checking not stuck at playlist end when currentTime not at seekable end even if the buffer is empty?

If we still want to keep this part in, I'm not sure having playlistEnd and seekable.end(0) both be the same (130) is representative of an actual scenario since seekable.end is the time since playback started to minus 3 segment durations from the end of the window, where playlistEnd is time since playback started to minus 0 segment durations from the end of the window.

This comment has been minimized.

@gesinger

gesinger Mar 24, 2017

Collaborator

Good point. I think that leaving in tests that make use of seekable values is good, even if stuckAtPlaylistEnd_ doesn't use them, since the tests shouldn't know too much about the internal workings of the function. But you're right that the scenario of playlistEnd equaling seekable's end shouldn't be a common case. However, in the event that a playlist changes from LIVE to VOD (at the end of a stream), and the only update was the addition of #EXT-X-ENDLIST, seekable's end should equal playlistEnd, and I think it's possible that this case is a valid one.

@mjneil mjneil added tested and removed needs:testing labels Mar 24, 2017

@zhuangs zhuangs force-pushed the zhuangs:fix/blacklisting-stuck-playlist branch from bab9852 to d5dbf4c Mar 24, 2017


let expiredSync = playlist.syncInfo || null;

let expiredSync = playlist.endList ? { time: 0, mediaSequence: 0} : playlist.syncInfo || null;

This comment has been minimized.

@mjneil

mjneil Mar 24, 2017

Collaborator

When a live stream is finished, usually an EXT-X-ENDLIST tag is applied to the end of the playlist even though it wasn't originally a VOD. This would set expiredSync to { time: 0, mediaSequence: 0 } even if that isn't an actual sync point. We should prioritize playlist.syncInfo if it exists, otherwise { time: 0, mediaSequence: 0 } if playlist.endList is true, otherwise null.

Shan Zhuang
@mjneil

mjneil approved these changes Mar 24, 2017

Shan Zhuang added some commits Mar 24, 2017

Shan Zhuang
Shan Zhuang

@zhuangs zhuangs force-pushed the zhuangs:fix/blacklisting-stuck-playlist branch from f62c7c9 to a79bdf7 Mar 24, 2017

@mjneil mjneil added needs:testing and removed tested labels Mar 24, 2017

@zhuangs zhuangs changed the title blacklist the playlist that has stopped being updated blacklist the playlist that has stopped being updated and never blacklist the final playlist available Mar 24, 2017

@mjneil

mjneil approved these changes Mar 24, 2017

@mjneil mjneil added tested and removed needs:testing labels Mar 24, 2017

@gesinger gesinger merged commit aeceda3 into videojs:master Mar 24, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gesinger gesinger referenced this pull request Apr 26, 2017

Closed

Fix for #550 and #555 #561

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.