Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use dead time latency flag #1990

Merged
merged 4 commits into from
Jun 19, 2017

Conversation

robertbryer
Copy link
Contributor

#1955 Requests a method to switch between the dead time method of adjusting bandwidth and simply using the total time for bandwidth. This adds a flag useDeadTimeLatencyForAbr so implementors can choose what behaviour they want and A/B test between the two choices.

*/
function setUseDeadTimeLatencyForAbr(useDeadTimeLatency) {
abrController.setUseDeadTimeLatency(useDeadTimeLatency);
}
Copy link

@AxelDelmas AxelDelmas Jun 6, 2017

Choose a reason for hiding this comment

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

The isssue with setting this property on abrController is that whenever resetAndInitialize is called in MediaPlayer, this setting will be reset to default. This can happen when calling attachView, which is a very unexpected side effect.

This is a more general design issue with AbrController (there is already the same issue with all the AbrController settings), maybe it would deserve its own issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is a problem for all the things that are set on AbrController - Perhaps a better move is to store a json config object on MediaPlayer.js; this way we can be sure config has the same lifetime as the player. It would also make the API on MediaPlayer look a bit nicer without the huge number of functions for turning flags on and off.

Copy link

@AxelDelmas AxelDelmas Jun 7, 2017

Choose a reason for hiding this comment

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

There's a lot of config that is stored in MediaPlayerModel and not reset during the player lifetime (see https://github.com/Dash-Industry-Forum/dash.js/blob/development/src/streaming/models/MediaPlayerModel.js#L432-L433).
It might make sense to move everything there (and maybe rename it to MediaPlayerConfig).
I find it generally a good idea to have such a store used only to store configuration, and usable anytime during the player's lifetime (other issue with abrController config params is that trying to set them before calling initialize throws an exception, which does not make a lot of sense when all you do is changing configuration settings)

Choose a reason for hiding this comment

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

And yeah, moving everything to a config object accessible on the MediaPlayer's API and whose properties can be accessed and changed directly would be a good idea too ! I agree about the getter/setter mess.
This said, this is a breaking change in the API, so it should be rolled out with care. Maybe in a v3 with API overhaul if other needs for API changes have been expressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding API breaking, we could just proxy those calls to set the relevant config and mark the functions as deprecated for now; but if anyone is relying on this reset behaviour, we would be breaking that (hopefully nobody, perhaps regard that as an acceptable risk).

@dsparacio
Copy link
Contributor

@robertbryer sorry Bob can you resolve when you have time.

@dsparacio dsparacio merged commit 93d5edb into Dash-Industry-Forum:development Jun 19, 2017
@robertbryer robertbryer deleted the useDeadTimeLatencyFlag branch June 27, 2017 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants