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

Do not downgrade layers too early #61

Merged
merged 4 commits into from
Oct 25, 2021
Merged

Do not downgrade layers too early #61

merged 4 commits into from
Oct 25, 2021

Conversation

boks1971
Copy link
Contributor

Problem

On my slow machine, about half the time the "f" layer does not even start before the
monitor code swoops in and downgrades quality.

It is puzzling as to why it is not started. Activity Monitor shows 75% idle even when all
layers are encoding. And the server is running on the same machine. So, bandwidth
should not be an issue.

Anyhow, once downgraded, we wait the 60 seconds before enabling the "f" layer
which is quite a while for high quality video to be sent out.

Fix

Check if the best encoder has been active (by checking framesSent !== 0) before
downgrading. With this change, the "f" layer starts about 15 - 20 seconds into the
session rather the 60+ seconds because of downgrading too early. 15 - 20 seconds is
not great either, but at least it is not as bad.

To think about

What if even "h" was too much and we need to downgrade to "q"? By waiting for "f"
to start, we will never get to that. Maybe, we need to check the currently active (or
at some point active) best encoding and see if that is suffering. Or maybe, letting the
browser handle it is okay as "h" suffering is probably a sign that the machine is going
to perform poorly even for the most basic cases.

Testing

Ensure that "f" layer is not clobbered before it has a chance to start.

@boks1971 boks1971 requested a review from davidzhao October 25, 2021 13:13
@boks1971
Copy link
Contributor Author

@davidzhao Realized that this could be due to the RTP change I made to parse the padding packet properly. Chrome sends a bunch of padding packets initially to probe available bandwidth. Because of the RTP change, they were not submitted to the TWCC.

I made a change in ion-sfu to submit to TWCC before dropping any padding only packets. Will make a PR for that.

In spite of that, I think this is a decent change to not downgrade prematurely. What do you think?

Copy link
Member

@davidzhao davidzhao left a comment

Choose a reason for hiding this comment

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

this is a good fix. 👍

@boks1971 boks1971 merged commit 3ee9b10 into main Oct 25, 2021
@boks1971 boks1971 deleted the raja_delay_downgrade branch October 25, 2021 15:56
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