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

Disable force_layer and re-enable auto padding #903

Merged
merged 9 commits into from
Jul 4, 2017
Merged

Disable force_layer and re-enable auto padding #903

merged 9 commits into from
Jul 4, 2017

Conversation

kekkokk
Copy link
Contributor

@kekkokk kekkokk commented May 26, 2017

Description
It disable the previously forced layers of subscribed simulcast and re-enable the auto padding algo.

[] It needs and includes Unit Tests

Changes in Client or Server public APIs
if subscribedStream._setQualityLayer('auto') is passed, it switch to auto.

[] It includes documentation for these changes in /doc.

Copy link
Contributor

@jcague jcague left a comment

Choose a reason for hiding this comment

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

Really nice! I really like the idea of disabling forced layers, and I like this proposal.

But I don't like the change in the client API though, it gives developers the next wrong ideas IMO:

  • _setQualityLayer('auto', 2) it seems like we're setting automating spatial layer switching while forcing the temporal layer.
  • _setQualityLayer(0, 'auto') not sure whether it will work
  • `_setQualityLayer('auto', 'auto') not sure whether it will work or not

Just a proposal: should we change the API to have two methods to something like this?
_setStaticQualityLayer(1,2)
_setDynamicQualityLayer()
I haven't thought them a lot, but it's just to give you an idea.

@kekkokk
Copy link
Contributor Author

kekkokk commented May 29, 2017

Ok, I like your observation.
I split in two separate functions as you suggested but I didn't change nothing in the doc since neither _setQualityLayer was mentioned. I think you would like to keep it private since it is well tested.

@kekkokk
Copy link
Contributor Author

kekkokk commented May 29, 2017

It also would be nice to have a preference to set the preferred switch.
For example I'd prefer to have always an higher temporal layer instead of spatial.
Id prefer to see smooth than neat.
You think it would be possible with low effort?

@jcague
Copy link
Contributor

jcague commented May 29, 2017

agree, but that's a different API, that would be also nice to have limits on the max resolution and/or framerates each subscriber can receive.

@kekkokk
Copy link
Contributor Author

kekkokk commented Jun 30, 2017

updated with new Stream.js

@kekkokk
Copy link
Contributor Author

kekkokk commented Jul 4, 2017

!up please merge

Copy link
Contributor

@jcague jcague left a comment

Choose a reason for hiding this comment

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

👍 thanks!

@jcague jcague merged commit 3dff5b5 into lynckia:master Jul 4, 2017
@kekkokk kekkokk deleted the add/re_enable_padding branch June 4, 2018 10:54
Arri98 pushed a commit to Arri98/licode that referenced this pull request Apr 6, 2021
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.

2 participants