Remove settings methods from MediaPlayer #2954
Conversation
|
I'm reviewing your PR @epiclabsDASH. First, according to me, the main problem is that config type is not checked now. I think maybe an update of mixin function can be done to test if the default type of a specific config parameter is the same than the new value....do you agree? |
| @@ -566,7 +566,11 @@ var ControlBar = function (dashjsMediaPlayer, displayUTCTimeCodes) { | |||
| return idx; | |||
|
|
|||
| } else if (menuType === "bitrate") { | |||
| return player.getAutoSwitchQualityFor(mediaType) ? 0 : player.getQualityFor(mediaType); | |||
| var cfg = player.getSettings(); | |||
There was a problem hiding this comment.
no need to get autoswitch settings now, @jeoliva ?
There was a problem hiding this comment.
Why not? this method is to retrieve the current configured InitialBitrate.
| if (player.getAutoSwitchQualityFor(self.mediaType)) { | ||
| player.setAutoSwitchQualityFor(self.mediaType, false); | ||
| var cfg = player.getSettings(); | ||
| if (cfg && cfg.streaming && cfg.streaming.abr && cfg.streaming.abr.initialBitrate && cfg.streaming.abr.autoSwitchBitrate[self.mediaType]) { |
There was a problem hiding this comment.
same style of question, why do you check initialBitrate @epiclabsDASH ? Maybe, I missed something....
There was a problem hiding this comment.
You are right. It was a copy&paste issue.
| @@ -16,7 +16,7 @@ | |||
|
|
|||
| function init() { | |||
| player = dashjs.MediaPlayer().create(); | |||
| player.getDebug().setLogToBrowserConsole(false); | |||
| player.updateSettings({ 'debug': { 'logLevel': dashjs.Debug.LOG_LEVEL_NONE }}); | |||
There was a problem hiding this comment.
maybe the function setLogToBrowserConsole can also be removed in Debug.js....
There was a problem hiding this comment.
Yep, agree with you.
There was a problem hiding this comment.
setLogTimestampVisible and setCalleeNameVisible should be also part of the configuration but will take care of them with reviewing new config parameters to be added (texttrack, trackswitchmode, etc)
There was a problem hiding this comment.
I will take care of all these changes in the third PR. It requires to move some unit tests that are already done in Debug module to the unit tests focused on Settings module. In the mean time I am going to ensure no one call these methods but from Debug unit tests.
| @@ -115,23 +113,16 @@ function MediaPlayerModel() { | |||
| } | |||
| } | |||
|
|
|||
| function getStableBufferTime() { | |||
There was a problem hiding this comment.
it means that stableBufferTime will be the same if low latency is enabled or fast switch...
There was a problem hiding this comment.
You are right.
| @@ -169,10 +169,6 @@ class MediaPlayerModelMock { | |||
| } | |||
| } | |||
|
|
|||
| getStableBufferTime() { | |||
There was a problem hiding this comment.
some attributes can be removed from MediaPlayerModelMock like retryAttempts, stableBufferTime, etc....
There was a problem hiding this comment.
It was already cleaned up. I left retryAttempts methods because they add some extra logic that that depends on low latency mode (same for stableBufferTime).
| @@ -260,297 +259,6 @@ describe('MediaPlayer', function () { | |||
| expect(player.setAutoPlay.bind(player, 12)).to.throw(Constants.BAD_ARGUMENT_ERROR); | |||
| }); | |||
|
|
|||
| it('should not set setMaxAllowedBitrateFor value if it\'s not a number type or NaN or if type is not Video or Audio', function () { | |||
There was a problem hiding this comment.
a lot of this unit tests have to be saved to be sure that settings type is correct.
There was a problem hiding this comment.
Yep. That's part of the pending work I mentioned and that will be included in a third PR.
| } | ||
| settings.update(s); |
There was a problem hiding this comment.
By removing those lines, datas will not be saved between different streaming.....no?
There was a problem hiding this comment.
The problem I see with using settings to persist an initialBitrate that is not really the one set by the user but a "calculated one" (ex: coming from DOMStorage) is just that. When playing a second stream, even when local storage is disabled, AbrController will set as initial bitrate the one that was set for the first played stream which for most of cases is the value coming from DOMStorage.
If user set initialBitrate through settings, with this approach it will be saved across consecutive playbacks.
Second phase of changes related with new configuration mechanism for dash.js. We are removing all configuration methods from MediaPlayer, reducing its size and number of methods, and making simpler adding new setting properties (less chained functions).
In this second phase I am removing deprecated methods from MediaPlayer, fixing unit tests, control bar and updating sample source code, included the Dash.js reference player. Typings have also been updated. dash.js package size reduced by 7Kb.
Pending work: