Skip to content
This repository was archived by the owner on Jan 12, 2019. It is now read-only.

use specified mediasequence for VOD expired sync instead of assuming 0 #1097

Merged
merged 5 commits into from
May 4, 2017

Conversation

mjneil
Copy link
Contributor

@mjneil mjneil commented Apr 24, 2017

Description

Seekable calculations assumed that VOD playlists have an EXT-X-MEDIA-SEQUENCE of 0, however this is not always the case. This fixes the calculations to use the media sequence specified in the playlist This has been updated to now use the SyncController to pick a sync point and calculate expired time

@gesinger
Copy link
Contributor

A test we should check for: if a live stream ends, and we switch to a playlist that we haven't seen before (don't have syncInfo), or, we tune into a live stream towards its end and rendition switch, then it is possible this may change that behavior.

@mjneil
Copy link
Contributor Author

mjneil commented Apr 26, 2017

That scenario is definitely a problem, but it is not a very common case. I think it will be ok to move forward with this and make an issue for that case to address in another PR

Copy link
Contributor

@gesinger gesinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to have some tests for getExpiredTime for VOD.

// If the sync-point is beyond the start of the playlist, we want to subtract the
// duration from index 0 to syncPoint.segmentIndex instead of adding.
if (syncPoint.segmentIndex > 0) {
syncPoint.time = -1 * syncPoint.time;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can just do syncPoint.time *= -1


/**
* Runs each sync-point strategy and returns a list of sync-points returned by the
* stratigies
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strategies

* @param {Playlist} playlist
* Playlist object to calculate expired from
* @param {Number} duration
* Duration of the MediaSource (Infinite if playling a live source)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use Infinity?

return null;
}

const syncPoints = this.runStrategies_(playlist, duration, playlist.discontinuitySequence, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long line

*
* @private
* @param {Playlist} playlist - The playlist that needs a sync-point
* @param {Number} duration - Duration of the MediaSource (Infinite if playing a live source)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also use Infinity here?

* @param {Number} target.value
* The value to target for the specified key.
* @returns {Object}
* The sync-poiont nearest the target
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

point


let expired = this.syncController.getExpiredTime(playlist, Infinity);

assert.equal(expired, 100, 'seekable range calculated');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assertion would probably be clearer using your below format. Something like estimated expired time using sync info


expired = this.syncController.getExpiredTime(playlist, Infinity);

assert.equal(expired, 98.5, 'estimated start time using segmentSync');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably better to use estimated expired time... instead of start for these and others

@zhuangs
Copy link
Contributor

zhuangs commented Apr 28, 2017

Tested and LGTM

@dmlap dmlap merged commit 016d82f into videojs:master May 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants