ref(QualityController) Add recovery mechanism and adjust the resolution check. #2546
Conversation
jallamsetty1
force-pushed
the
ref-qualityCOntroller
branch
4 times, most recently
from
05bcd23
to
43c750c
Compare
…on check. Impl a recovery mechanism for the lastN to be increased if the cpu limitation goes away and doesn't return after increasing lastN. Also, additionally improve the calculation of the expected resolution taking simulcast stream resolutions into account.
jallamsetty1
force-pushed
the
ref-qualityCOntroller
branch
from
43c750c
to
626e2b0
Compare
|
|
||
| if (lastN !== -1 && lastN <= MIN_LAST_N) { | ||
| // Do nothing if lastN is already set to zero, i.e., the endpoint is in audio-only mode. |
There was a problem hiding this comment.
We use lastN of 1 when there is some SS going on, even in audio-only mode. Should we adjust the check to be < MIN_LAST_N perhaps?
There was a problem hiding this comment.
Hmm .. it doesn't seem to be that straight forward since audio-only feature is fully implemented in the application. However, I realized that when audio-only mode is enabled, client's local video sources are muted so the stats won't be processed and when either camera or screenshare is started, the client is automatically kicked out of audio-only mode. Therefore, we don't have to make assumptions about whether client is in that mode while calculating lastN. I have removed that check all together now.
| @@ -21,6 +22,10 @@ const LIMITED_BY_CPU_TIMEOUT = 60000; | |||
| // The min. value that lastN will be set to while trying to fix video qaulity issues. | |||
| const MIN_LAST_N = 3; | |||
|
|
|||
| // Wait time to check if cpu limitation reappears after lastN rampup is attempted for recovery. If it does, all further | |||
| // attempts to bring lastN to configured channelLastN value will be blocked. | |||
| const LAST_N_RAMPUP_TIME = 60000; | |||
There was a problem hiding this comment.
Do we think we this should be configurable? Not even for our own testing?
There was a problem hiding this comment.
I guess we could. I will add a variable under the testing section.
| let newLastN = lastN; | ||
|
|
||
| // If channelLastN is not set or set to -1 in config.js, the client will ramp up lastN to only up to 25. | ||
| let { channelLastN = 25 } = this._conference.options.config; |
There was a problem hiding this comment.
This 25 should be defined somewhere as a constant and used as the derfault everywhere.
There was a problem hiding this comment.
Yes! I was being lazy, we use this value in multiple places. I will make this a constant and default video quality settings.
| channelLastN = channelLastN === -1 ? 25 : channelLastN; | ||
| if (cpuLimited) { | ||
| const videoStreamsReceived = this._conference.getForwardedSources().length; | ||
| if (lastN !== -1 && lastN <= MIN_LAST_N) { |
There was a problem hiding this comment.
Looks like we could've done this check above?
| */ | ||
| constructor(conference: JitsiConference, codecSettings: any, enableAdaptiveMode: boolean) { | ||
| constructor(conference: JitsiConference, options) { |
|
|
||
| newLastN = videoStreamsReceived / 2; |
| @@ -380,7 +377,7 @@ export class QualityController { | |||
| trackStats: [ ssrcStats ] | |||
| }; | |||
|
|
|||
| statsPerTrack.set(track.rtcId, existingStats); | |||
| statsPerTrack.set(trackId, existingStats); | |||
There was a problem hiding this comment.
Why are we setting the string as a key, which then forces us to use .toString() everywhere?
There was a problem hiding this comment.
I initially declared the key for the encode time stats to be a string and later realized that rtcId is a number. I changed it back. At that time I thought a string made more sense if its a number/string but I see that its a number always.
…on check. (jitsi#2546) * ref(QualityController) Add recovery mechanism and adjust the resolution check. Impl a recovery mechanism for the lastN to be increased if the cpu limitation goes away and doesn't return after increasing lastN. Also, additionally improve the calculation of the expected resolution taking simulcast stream resolutions into account. * squash: Address review comments and add more unit tests * squash: Address review comments
…on check. (#2546) * ref(QualityController) Add recovery mechanism and adjust the resolution check. Impl a recovery mechanism for the lastN to be increased if the cpu limitation goes away and doesn't return after increasing lastN. Also, additionally improve the calculation of the expected resolution taking simulcast stream resolutions into account. * squash: Address review comments and add more unit tests * squash: Address review comments
…on check. (#2546) * ref(QualityController) Add recovery mechanism and adjust the resolution check. Impl a recovery mechanism for the lastN to be increased if the cpu limitation goes away and doesn't return after increasing lastN. Also, additionally improve the calculation of the expected resolution taking simulcast stream resolutions into account. * squash: Address review comments and add more unit tests * squash: Address review comments
Impl a recovery mechanism for the lastN to be increased if the cpu limitation goes away and doesn't return after increasing lastN. Also, additionally improve the calculation of the expected resolution taking simulcast stream resolutions into account.