[RTC-32] Fetch actual bitrate of a variant #21
Conversation
tsconfig.json
Outdated
| @@ -9,7 +9,8 @@ | |||
| "moduleResolution": "node", | |||
| "outDir": "dist/", | |||
| "strict": true, | |||
| "target": "es2018" | |||
| "target": "es2018", | |||
| "lib": ["dom", "es6", "es2019"] | |||
There was a problem hiding this comment.
Added that to be able to use Object.fromEntries()
There was a problem hiding this comment.
Object.fromEntries was added in ECMAScript 2019. Do we need to include dom and es6 (ECMAScript 2015) too?
There was a problem hiding this comment.
This SO question misled me a bit (the "nothing imports es6" part) but looking at Typescript source it seems that es2019 also import previous libs so it is not necessary. DOM on the other hand contains all of the browser api definitions and without it Typescript throws errors on MediaStream type etc.
src/membraneWebRTC.ts
Outdated
| @@ -894,6 +897,7 @@ export class MembraneWebRTC { | |||
| if (parameters.encodings.length === 0) { | |||
| parameters.encodings = [{}]; | |||
| } else { | |||
| // TODO: sent MediaEvent with updated variant bitrates | |||
There was a problem hiding this comment.
Sending TrackVariantBitrates MediaEvent here would be quite troublesome (need to split the bandwidth before sending bitrates for each of the tracks), so I decided to leave it to the bigger refactor that we mentioned.
There was a problem hiding this comment.
Acctually, would it be hard to address this issue in this PR?
There was a problem hiding this comment.
Ok, I did, but I would't call the code clean, the ambiguity of how track variant bandwidth is kept (sometimes as an aggregated value, sometimes bitrate per variant) and that the function responsible for splitting bitrate is very deep on the call stack make it hard to do it nicely without bigger refactor (which, as far as I understand, we don't want to do in this PR).
src/membraneWebRTC.ts
Outdated
| @@ -934,6 +938,14 @@ export class MembraneWebRTC { | |||
| delete encoding.maxBitrate; | |||
| } else { | |||
| encoding.maxBitrate = bandwidth * 1024; | |||
| let mediaEvent = generateCustomEvent({ | |||
| type: "setTrackVariantBitrates", | |||
There was a problem hiding this comment.
I would favor trackVariantBitrates. It will fit more into our convention
src/membraneWebRTC.ts
Outdated
| maxBandwidth instanceof Map | ||
| ? Object.fromEntries(maxBandwidth) | ||
| : maxBandwidth, | ||
| mid: trackIdToMid ? trackIdToMid.get(trackId!) : null, |
There was a problem hiding this comment.
I don't think we should allow for sending null here. Server won't be able to handle it so we should assert that trackIdtoMid is always present and contains trackId.
tsconfig.json
Outdated
| @@ -9,7 +9,8 @@ | |||
| "moduleResolution": "node", | |||
| "outDir": "dist/", | |||
| "strict": true, | |||
| "target": "es2018" | |||
| "target": "es2018", | |||
| "lib": ["dom", "es6", "es2019"] | |||
There was a problem hiding this comment.
Object.fromEntries was added in ECMAScript 2019. Do we need to include dom and es6 (ECMAScript 2015) too?
src/membraneWebRTC.ts
Outdated
| @@ -894,6 +897,7 @@ export class MembraneWebRTC { | |||
| if (parameters.encodings.length === 0) { | |||
| parameters.encodings = [{}]; | |||
| } else { | |||
| // TODO: sent MediaEvent with updated variant bitrates | |||
There was a problem hiding this comment.
Acctually, would it be hard to address this issue in this PR?
LVala
force-pushed
the
fetch-bitrate-of-simulcast-variant
branch
from
acae0e5 to
ed3079e
Compare
LVala
changed the title
|
I'm removing my review request, until the backwards compatibility is resolved in the other Pull Request |
|
Adjusted to match latest commit in this RTC Engine PR. Not the cleanest (passing media event object down 3 functions), but I believe that otherwise it would require greater refactor and I'm not sure if that's what we want to do now. |
| if (mediaEvent) | ||
| mediaEvent.data.data.variantBitrates[encoding.rid!] = | ||
| encoding.maxBitrate; | ||
| } else delete encoding.maxBitrate; |
There was a problem hiding this comment.
This case will not be adequately handled on the server. I think what will happen is that the encoding will be missing from the media event and the server will still consider the previous limit valid, which is not true because the limit has been removed, so we're back to Chrome's default limit for the variant.
Maybe we should just force the limit values to be set for all present variants in the Media Event? 🤔 This way we won't have to hard code the default Chrome limits in the RTC Engine except for backward compatibility reasons. This approach may also be better when you consider other clients, which may have different default limits.
|
I changed the media event to always contain all of the variant bitrates. Also, I'm not very sure about the default audio bitrate as I couldn't find anything about it in chromium source in a reasonable time. |
No description provided.