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

specific rebufferingGoal override manifest one #1547 #1581

Merged
merged 6 commits into from Sep 21, 2018

Conversation

Projects
None yet
4 participants
@fadomire
Contributor

fadomire commented Sep 12, 2018

EDIT: pull request updated to add manifest.dash.ignoreMinBufferTime

i'm not really satisfied of adding a defaultRebufferingGoal to config but at least changes in code are very light

see #1547

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Sep 12, 2018

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

googlebot commented Sep 12, 2018

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers
@fadomire

This comment has been minimized.

Show comment
Hide comment
@fadomire

fadomire Sep 12, 2018

Contributor

I signed it!

Contributor

fadomire commented Sep 12, 2018

I signed it!

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Sep 12, 2018

CLAs look good, thanks!

googlebot commented Sep 12, 2018

CLAs look good, thanks!

add the config field manifest.dash.ignoreMinBufferTime
add the config field manifest.dash.ignoreMinBufferTime, which will default to false. If true, the DASH parser will ignore minBufferTime in the manifest, such that streaming.rebufferingGoal is the only factor in play.
Show outdated Hide outdated lib/dash/dash_parser.js Outdated
@@ -101,6 +101,7 @@ shaka.util.PlayerConfiguration = class {
ignoreDrmInfo: false,
xlinkFailGracefully: false,
defaultPresentationDelay: 10,
ignoreMinBufferTime: false,

This comment has been minimized.

@joeyparrish

joeyparrish Sep 19, 2018

Member

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!

@joeyparrish

joeyparrish Sep 19, 2018

Member

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!

This comment has been minimized.

@fadomire

fadomire Sep 19, 2018

Contributor

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

@fadomire

fadomire Sep 19, 2018

Contributor

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

This comment has been minimized.

@joeyparrish

joeyparrish Sep 19, 2018

Member

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?

@joeyparrish

joeyparrish Sep 19, 2018

Member

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?

This comment has been minimized.

@fadomire

fadomire Sep 19, 2018

Contributor

definitly help, thanks

@fadomire

fadomire Sep 19, 2018

Contributor

definitly help, thanks

@joeyparrish

This comment has been minimized.

Show comment
Hide comment
@joeyparrish

joeyparrish Sep 19, 2018

Member

Thank you for contributing! You should also add yourself to CONTRIBUTORS and AUTHORS files (alphabetically).

Member

joeyparrish commented Sep 19, 2018

Thank you for contributing! You should also add yourself to CONTRIBUTORS and AUTHORS files (alphabetically).

Show outdated Hide outdated externs/shaka/player.js Outdated
@fadomire

This comment has been minimized.

Show comment
Hide comment
@fadomire

fadomire Sep 19, 2018

Contributor

i added 2 tests for ignoreMinBufferTime

  • one to check that when ignoreMinBufferTime is true minBufferTime is 0
  • another to verify when it is false it should respect manifest minBufferTime

dont know if the second one is necessary or if it is already tested somehow

Contributor

fadomire commented Sep 19, 2018

i added 2 tests for ignoreMinBufferTime

  • one to check that when ignoreMinBufferTime is true minBufferTime is 0
  • another to verify when it is false it should respect manifest minBufferTime

dont know if the second one is necessary or if it is already tested somehow

@joeyparrish

Looks great! Thanks!

@shaka-bot

This comment has been minimized.

Show comment
Hide comment
@shaka-bot

shaka-bot Sep 21, 2018

Collaborator

All tests passed!

Collaborator

shaka-bot commented Sep 21, 2018

All tests passed!

@joeyparrish joeyparrish merged commit 99ebc69 into google:master Sep 21, 2018

2 checks passed

cla/google All necessary CLAs are signed
shaka-build-bot All tests passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment