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.

Sign up for GitHub

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

Jump to bottom

specific rebufferingGoal override manifest one #1547 #1581

Merged
merged 6 commits into from
Sep 21, 2018
Merged

specific rebufferingGoal override manifest one #1547 #1581

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
74c4060
add the config field manifest.dash.ignoreMinBufferTime
fadomire Sep 19, 2018
0285c45
fix parseAttr() eventually returning null
fadomire Sep 19, 2018
3be8be2
add ignoreMinBufferTime to doc and fix tests
fadomire Sep 19, 2018
f483d59
fix typo
fadomire Sep 19, 2018
4f4436d
add name to authors and contributors
fadomire Sep 19, 2018
7091f3f
add tests for ignoreMinBufferTime
fadomire Sep 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion externs/shaka/player.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,8 @@ shaka.extern.DrmConfiguration;
* clockSyncUri: string,
* ignoreDrmInfo: boolean,
* xlinkFailGracefully: boolean,
* defaultPresentationDelay: number
* defaultPresentationDelay: number,
* ignoreMinBufferTime: boolean
* }}
*
* @property {shaka.extern.DashContentProtectionCallback} customScheme
Expand All @@ -536,6 +537,10 @@ shaka.extern.DrmConfiguration;
* @property {number} defaultPresentationDelay
* A default presentationDelay if suggestedPresentationDelay is missing
* in the MPEG DASH manifest. This has to be bigger than minBufferTime * 1.5.
* @property {boolean} ignoreMinBufferTime
* If true will cause DASH parser to ignore minBufferTime from manifest
Copy link
Member

Choose a reason for hiding this comment

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

Minor English nit: This sentence needs a period at the end, and the next line should begin with a capital letter.

* it allows player config to take precedence over manifest for
* rebufferingGoal. Defaults to false if not provided.
*
* @exportDoc
*/
Expand Down
7 changes: 6 additions & 1 deletion lib/dash/dash_parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,8 +486,13 @@ shaka.dash.DashParser.prototype.processManifest_ =
let baseUris = shaka.util.ManifestParserUtils.resolveUris(
manifestBaseUris, uris);

let minBufferTime =
let ignoreMinBufferTime = this.config_.dash.ignoreMinBufferTime;
let minBufferTime;
if (!ignoreMinBufferTime) {
minBufferTime =
XmlUtils.parseAttr(mpd, 'minBufferTime', XmlUtils.parseDuration);
Copy link
Member

Choose a reason for hiding this comment

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

parseAttr() can return null if there's no such attribute, so please add || 0 to the end of this statement.

}

this.updatePeriod_ = /** @type {number} */ (XmlUtils.parseAttr(
mpd, 'minimumUpdatePeriod', XmlUtils.parseDuration, -1));

Expand Down
1 change: 1 addition & 0 deletions lib/util/player_configuration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ shaka.util.PlayerConfiguration = class {
ignoreDrmInfo: false,
xlinkFailGracefully: false,
defaultPresentationDelay: 10,
ignoreMinBufferTime: false,
Copy link
Member

Choose a reason for hiding this comment

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

This should also be added to externs/shaka/player.js, in the shaka.extern.DashManifestConfiguration type. Please add @property to describe the property, and also add it to the @typedef block.

This will probably produce a few build errors, as the DASH config object in several of our tests will need to be updated to match the updated type. Be sure to run python build/all.py and fix any errors that come up.

After that, please add a new test in test/dash/dash_parser_manifest_unit.js that shows the effect of ignoreMinBufferTime. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm not yet sure on how to write a test properly but will check it later on

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest looking at the very last test in that file ('exposes Representation IDs') as a template.

The important parts of the test are:

  1. Fake manifest text. This can be very simple for your case, with only one Representation.
  2. A call to fakeNetEngine.setResponseText() so the fake manifest text gets returned from the networking engine.
  3. Any necessary configuration passed to parser.configure(). For you, this should set the new flag.
  4. The (async) call to parser.start() to parse the manifest and get the results.
  5. Expectations on the output. You would expect that minBufferTime is 0, in spite of the attribute in the manifest text.

Does that help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

definitly help, thanks

},
};

Expand Down
1 change: 1 addition & 0 deletions test/dash/dash_parser_content_protection_unit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ describe('DashParser ContentProtection', function() {
ignoreDrmInfo: ignoreDrmInfo,
xlinkFailGracefully: false,
defaultPresentationDelay: 10,
ignoreMinBufferTime: false,
},
});
let playerEvents = {
Expand Down
2 changes: 2 additions & 0 deletions test/dash/dash_parser_live_unit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ describe('DashParser Live', function() {
ignoreDrmInfo: false,
xlinkFailGracefully: false,
defaultPresentationDelay: 10,
ignoreMinBufferTime: false,
},
});
playerInterface = {
Expand Down Expand Up @@ -713,6 +714,7 @@ describe('DashParser Live', function() {
ignoreDrmInfo: false,
xlinkFailGracefully: false,
defaultPresentationDelay: 10,
ignoreMinBufferTime: false,
},
});

Expand Down
2 changes: 2 additions & 0 deletions test/hls/hls_live_unit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ describe('HlsParser live', function() {
ignoreDrmInfo: false,
xlinkFailGracefully: false,
defaultPresentationDelay: 10,
ignoreMinBufferTime: false,
},
};

Expand Down Expand Up @@ -498,6 +499,7 @@ describe('HlsParser live', function() {
ignoreDrmInfo: false,
xlinkFailGracefully: false,
defaultPresentationDelay: 10,
ignoreMinBufferTime: false,
},
});

Expand Down
1 change: 1 addition & 0 deletions test/hls/hls_parser_unit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ describe('HlsParser', function() {
ignoreDrmInfo: false,
xlinkFailGracefully: false,
defaultPresentationDelay: 10,
ignoreMinBufferTime: false,
},
};

Expand Down
1 change: 1 addition & 0 deletions test/test/util/dash_parser_util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ shaka.test.Dash.makeDashParser = function() {
ignoreDrmInfo: false,
xlinkFailGracefully: false,
defaultPresentationDelay: 10,
ignoreMinBufferTime: false,
},
});
return parser;
Expand Down