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

Allow immediate switching when top quality index changes #1667

Merged
merged 3 commits into from
Nov 30, 2016

Conversation

JontyUsborne
Copy link
Contributor

When the max representation available to ABR changes because of updateQuality or Portal Bitrate change, the Buffer Level rule prevents switching because the buffer is larger than the stable bitrate buffer target (12 or 20s). The previous "top quality" target buffer size of 60s means that any appending to the buffer is suppressed for 48s so the user sees their buffer collapse.

Any change in getTopQualityIndexFor() now allows an immediate update of the rules. This should prevent the buffer near exhausting when quality changes and therefore reduce risk, especially as this happens whenever a user goes fullscreen.

Expensive bufferLevelRule is also not called now if getNextFragment is already triggered by isReplacement or topQualityChanged.

@dsparacio
Copy link
Contributor

I would not image this is needed if you just enable Fast Switching. Then you will fall into the isReplacement if there is a higher quality and you will see the higher quality render in 1 to 2 segments lengths regardless of the buffer level. Am I missing something?

@JibberJim
Copy link

If you enable fast switching, certainly it's not needed, but neither is the
different buffer targets for top quality and not at top quality, you might
as well always build a bigger safer buffer target if you're will to take
the distribution cost and load.

Without fast switching though this is very important to us, we have portal
and user requested restrictions on quality, which if they change without
the change here results the buffer dropping to low levels without need, and
also because the rules don't run any seek done during that period starts
seeking at the lower index. Both are undesirable, fast switch would
remove the need for this enhancement, but we're not ready for fast switch,
and I'm not actually sure it's ideal for most.

Another improvement in the area is changing the buffer target at top
quality to also be the buffer target at a stable bitrate - we have a lot of
people who have connections or computers which simply cannot go up to our
top bitrates, limiting them to only the small target buffer is not ideal.
That would also mostly solve this issue in a different way, but it's a lot
more complicated and leads to questions about what a stable bitrate is.

On Tue, Nov 15, 2016 at 6:09 PM, Dan Sparacio notifications@github.com
wrote:

I would not image this is needed if you just enable Fast Switching. Then
you will fall into the isReplacement if there is a higher quality and you
will see the higher quality render in 1 to 2 segments lengths regardless of
the buffer level. Am I missing something?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1667 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFLujoXI3OQBK2RUR-7GDi55z1mfW-XTks5q-fU9gaJpZM4KyXgU
.

@dsparacio
Copy link
Contributor

@JibberJim Was this tested with Fast Switching enabled?

@JibberJim
Copy link

JibberJim commented Nov 29, 2016 via email

@dsparacio
Copy link
Contributor

Sounds good ill pull it in and we can all test during the test phase. Thanks @JontyUsborne and @JibberJim

function hasTopQualityChanged(type, id) {

topQualityIndex[id] = topQualityIndex[id] || {};
let newTopQualityIndex = abrController.getTopQualityIndexFor(type,id);
Copy link
Contributor

Choose a reason for hiding this comment

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

we should use const here since we do not reassign newTopQualityIndex.

const isReplacement = replaceRequestArray.length > 0;
const readyToLoad = bufferLevelRule.execute(streamProcessor, type, streamController.isVideoTrackPresent());
const topQualityChanged = hasTopQualityChanged(currentRepresentationInfo.mediaInfo.type, streamProcessor.getStreamInfo().id);
let isReplacement, topQualityChanged, readyToLoad;
Copy link
Contributor

Choose a reason for hiding this comment

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

This i hard to read. Are you assigning the var in the if statement. We should keep code standards. We do not list vars in one line. Use const if not reassigned. Used keyword per var being assigned for break point access etc.

Copy link
Contributor

@dsparacio dsparacio left a comment

Choose a reason for hiding this comment

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

Please format code to be consistent with other ES6 code line. Ignore code standards when old ES5 code is used.

Copy link
Contributor

@dsparacio dsparacio left a comment

Choose a reason for hiding this comment

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

Thank you very much. Ill merge right away.

@dsparacio dsparacio merged commit 3b6bf34 into Dash-Industry-Forum:development Nov 30, 2016
@dsparacio dsparacio added this to the v2.4.0 milestone Dec 19, 2016
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.

None yet

3 participants