-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add level suppression logic #1536
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,8 +15,9 @@ import ID3TrackController from './controller/id3-track-controller'; | |
import {isSupported} from './helper/is-supported'; | ||
import {logger, enableLogs} from './utils/logger'; | ||
import EventEmitter from 'events'; | ||
import {hlsDefaultConfig} from './config'; | ||
import {FragmentTracker} from './helper/fragment-tracker'; | ||
import { hlsDefaultConfig } from './config'; | ||
import { FragmentTracker } from './helper/fragment-tracker'; | ||
import LevelSuppression from './utils/level-suppression'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like us to use the word which is more widely used in this context, rather than suppression. What we really also would want would that this manages general constraints, like min/maximum resolution or bitrate that could be set as well. Thinking a bit ahead, but I think this all belongs together and should be put under a more wider-thought umbrella. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Btw, as I had forgotten, we already have the whole min/max resolution stuff covered in cap-controller. So this is where all your additional features here should be implemented. See my last comment on this PR ;) |
||
|
||
// polyfill for IE11 | ||
require('string.prototype.endswith'); | ||
|
@@ -76,6 +77,7 @@ export default class Hls { | |
enableLogs(config.debug); | ||
this.config = config; | ||
this._autoLevelCapping = -1; | ||
this.levelSuppression = new LevelSuppression(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Simplest solution here would be, make this a member of our ABR-controller instance rather than a member of Hls itself. you can then still easily access it to set your stuff. |
||
// observer setup | ||
var observer = this.observer = new EventEmitter(); | ||
observer.trigger = function trigger (event, ...data) { | ||
|
@@ -154,6 +156,7 @@ export default class Hls { | |
this.url = null; | ||
this.observer.removeAllListeners(); | ||
this._autoLevelCapping = -1; | ||
this.levelSuppression = null; | ||
} | ||
|
||
attachMedia(media) { | ||
|
@@ -334,15 +337,51 @@ export default class Hls { | |
get nextAutoLevel() { | ||
const hls = this; | ||
// ensure next auto level is between min and max auto level | ||
return Math.min(Math.max(hls.abrController.nextAutoLevel,hls.minAutoLevel),hls.maxAutoLevel); | ||
let level = Math.min(Math.max(hls.abrController.nextAutoLevel, hls.minAutoLevel), hls.maxAutoLevel); | ||
|
||
if (hls.config.enableLevelSuppression && (hls.currentLevel !== level)) { | ||
return this.getAppropriateLevel(level); | ||
} else { | ||
return level; | ||
} | ||
|
||
} | ||
|
||
// this setter is used to force next auto level | ||
// this is useful to force a switch down in auto mode : in case of load error on level N, hls.js can set nextAutoLevel to N-1 for example) | ||
// forced value is valid for one fragment. upon succesful frag loading at forced level, this value will be resetted to -1 by ABR controller | ||
set nextAutoLevel(nextLevel) { | ||
const hls = this; | ||
hls.abrController.nextAutoLevel = Math.max(hls.minAutoLevel,nextLevel); | ||
|
||
let level = Math.max(hls.minAutoLevel, nextLevel); | ||
|
||
if (hls.config.enableLevelSuppression) { | ||
hls.abrController.nextAutoLevel = this.getAppropriateLevel(level); | ||
} else { | ||
hls.abrController.nextAutoLevel = level; | ||
} | ||
} | ||
|
||
getAppropriateLevel(level) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instead of adding a method on our lib API, this logic should be localized to ABR controller's interface There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's confusing that this method is a getter by name, but seems to do much more ... |
||
|
||
//to avoid extra checks, return level if not suppressed | ||
if (!this.levelSuppression.isSuppressed(level)) return level; | ||
|
||
//reset if all levels have been suppressed, continute to suppress last problematic level | ||
if (this.levelSuppression.isAllSuppressed(this.minAutoLevel, this.maxAutoLevel)) { | ||
//reset | ||
this.levelSuppression = new LevelSuppression(); | ||
//suppress problematic level | ||
this.levelSuppression.set(this.streamController.levelLastLoaded, this.config.levelLoadingMaxRetryTimeout); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it feels like this shouldn't be done on-access, but at some other moment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. once we moved Then the logic here that does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. update: CapController has already |
||
} | ||
|
||
|
||
//find non-suppressed level | ||
while (this.levelSuppression.isSuppressed(level)) { | ||
level = (level === 0) ? this.levels.length - 1 : level - 1; | ||
} | ||
|
||
return level; | ||
} | ||
|
||
/** get alternate audio tracks list from playlist **/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
class LevelSuppression { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's call it put this class into the ABR-controller module/file. that's the only place you should need it (no need to even export it for now, ABR-controller file is still small) |
||
constructor() { | ||
this._levelsSuppressed = {}; | ||
} | ||
|
||
isSuppressed(level) { | ||
|
||
let expiration = this._levelsSuppressed[level]; | ||
|
||
if (Date.now() < expiration) { | ||
return true; | ||
} | ||
|
||
if (this._levelsSuppressed[level]) { | ||
delete this._levelsSuppressed[level]; | ||
} | ||
|
||
return false; | ||
} | ||
|
||
isAllSuppressed(min, max) { | ||
for (var i = min; i <= max; i++) { | ||
if (!this.isSuppressed(i)) { | ||
return false; | ||
} | ||
} | ||
return true; | ||
} | ||
|
||
set(level, ttl) { | ||
this._levelsSuppressed[level] = ttl + Date.now(); | ||
} | ||
|
||
} | ||
|
||
export default LevelSuppression; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,125 @@ | ||
/* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for writing a unit test for this 👍 |
||
* | ||
* | ||
* | ||
*/ | ||
|
||
const assert = require('assert'); | ||
const sinon = require('sinon'); | ||
import LevelSuppression from '../../../src/utils/level-suppression'; | ||
import Hls from '../../../src/hls'; | ||
|
||
describe('Level suppression logic', function () { | ||
|
||
|
||
let levelSuppressionTimeout = 10000; | ||
|
||
|
||
describe('level-suppression', function () { | ||
|
||
let levelSuppression; | ||
let clock; | ||
|
||
beforeEach(function () { | ||
clock = sinon.useFakeTimers(); | ||
}); | ||
|
||
afterEach(function () { | ||
clock.restore(); | ||
}); | ||
|
||
it('suppresses some levels', function () { | ||
levelSuppression = new LevelSuppression(); | ||
levelSuppression.set(2, levelSuppressionTimeout); | ||
levelSuppression.set(3, levelSuppressionTimeout); | ||
levelSuppression.set(4, levelSuppressionTimeout); | ||
|
||
assert.strictEqual(levelSuppression.isSuppressed(0), false); | ||
assert.strictEqual(levelSuppression.isSuppressed(1), false); | ||
assert.strictEqual(levelSuppression.isSuppressed(2), true); | ||
assert.strictEqual(levelSuppression.isSuppressed(3), true); | ||
assert.strictEqual(levelSuppression.isSuppressed(4), true); | ||
}); | ||
|
||
it('suppresses all levels', function () { | ||
let levels = [0, 1, 2, 3, 4]; | ||
levelSuppression = new LevelSuppression(); | ||
|
||
levels.forEach((level) => { | ||
levelSuppression.set(level, levelSuppressionTimeout) | ||
}); | ||
|
||
assert.equal(levelSuppression.isAllSuppressed(0, levels.length - 1), true); | ||
}); | ||
|
||
it('is not suppressed when timeout is exceeded', function () { | ||
|
||
levelSuppression = new LevelSuppression(); | ||
|
||
levelSuppression.set(2, levelSuppressionTimeout); | ||
levelSuppression.set(3, levelSuppressionTimeout); | ||
levelSuppression.set(4, levelSuppressionTimeout); | ||
|
||
clock.tick(levelSuppressionTimeout); //advance clock by length of timeout | ||
|
||
assert.strictEqual(levelSuppression.isSuppressed(2), false); | ||
assert.strictEqual(levelSuppression.isSuppressed(3), false); | ||
assert.strictEqual(levelSuppression.isSuppressed(4), false); | ||
}); | ||
}); | ||
|
||
describe('getAppropriateLevel', function () { | ||
let hls; | ||
|
||
beforeEach(function () { | ||
hls = new Hls(); | ||
hls.levelController._levels = [ | ||
{ bitrate: 105000, name: "144", details: { totalduration: 4, fragments: [{}] } }, | ||
{ bitrate: 246440, name: "240", details: { totalduration: 10, fragments: [{}] } }, | ||
{ bitrate: 460560, name: "380", details: { totalduration: 10, fragments: [{}] } }, | ||
{ bitrate: 836280, name: "480", details: { totalduration: 10, fragments: [{}] } }, | ||
{ bitrate: 2149280, name: "720", details: { totalduration: 10, fragments: [{}] } }, | ||
{ bitrate: 6221600, name: "1080", details: { totalduration: 10, fragments: [{}] } } | ||
]; | ||
}); | ||
|
||
afterEach(function () { | ||
hls = null; | ||
}); | ||
|
||
it('returns the next non-suppressed level when one level is suppressed', function () { | ||
|
||
let currentLevel = 4; | ||
|
||
// suppress the fourth level | ||
hls.levelSuppression.set(currentLevel, hls.config.levelLoadingMaxRetryTimeout); | ||
|
||
assert.strictEqual(hls.getAppropriateLevel(currentLevel), 3); | ||
}); | ||
|
||
it('returns the next non-suppressed level when multiple levels are suppressed', function () { | ||
|
||
hls.levelSuppression.set(4, hls.config.levelLoadingMaxRetryTimeout); | ||
hls.levelSuppression.set(5, hls.config.levelLoadingMaxRetryTimeout); | ||
|
||
let nonSuppressedLevel = hls.getAppropriateLevel(5); | ||
|
||
assert.strictEqual(nonSuppressedLevel, 3); | ||
}); | ||
|
||
it('returns the last level if all levels are suppressed', function () { | ||
|
||
//last level hls loaded | ||
hls.streamController.levelLastLoaded = 0; | ||
|
||
//suppress all levels | ||
hls.levelController._levels.forEach((level, levelIndex) => { | ||
hls.levelSuppression.set(levelIndex, hls.config.levelLoadingMaxRetryTimeout) | ||
}); | ||
|
||
assert.strictEqual(hls.getAppropriateLevel(hls.abrController.nextAutoLevel), hls.levelController._levels.length - 1); | ||
}); | ||
|
||
}) | ||
|
||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, thanks for the lint fixes ;)