Skip to content
This repository was archived by the owner on May 10, 2024. It is now read-only.

[RTC-32] Fetch actual bitrate of a variant #21

Merged
merged 11 commits into from
Mar 23, 2023
4 changes: 2 additions & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

68 changes: 45 additions & 23 deletions src/membraneWebRTC.ts
Original file line number Diff line number Diff line change
Expand Up @@ -636,6 +636,8 @@ export class MembraneWebRTC {
simulcastConfig: SimulcastConfig = { enabled: false, active_encodings: [] },
maxBandwidth: TrackBandwidthLimit = 0 // unlimited bandwidth
): string {
if (!simulcastConfig.enabled && !(typeof maxBandwidth === "number"))
throw "Invalid type of `maxBandwidth` argument for a non-simulcast track, expected: number";
if (this.getPeerId() === "")
throw "Cannot add tracks before being accepted by the server";
const trackId = this.getTrackId(uuidv4());
Expand Down Expand Up @@ -882,6 +884,7 @@ export class MembraneWebRTC {
trackId: string,
bandwidth: BandwidthLimit
): Promise<boolean> {
// FIXME: maxBandwidth in TrackContext is not updated
const trackContext = this.localTrackIdToTrack.get(trackId);

if (!trackContext) {
Expand All @@ -894,6 +897,7 @@ export class MembraneWebRTC {
if (parameters.encodings.length === 0) {
parameters.encodings = [{}];
} else {
// TODO: sent MediaEvent with updated variant bitrates
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Acctually, would it be hard to address this issue in this PR?

Copy link
Contributor Author

@LVala LVala Jan 24, 2023

Choose a reason for hiding this comment

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

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).

this.applyBandwidthLimitation(parameters.encodings, bandwidth);
}

Expand Down Expand Up @@ -934,6 +938,14 @@ export class MembraneWebRTC {
delete encoding.maxBitrate;
} else {
encoding.maxBitrate = bandwidth * 1024;
let mediaEvent = generateCustomEvent({
type: "setTrackVariantBitrates",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would favor trackVariantBitrates. It will fit more into our convention

data: {
trackId: trackId,
variantBitrates: { [rid]: bandwidth },
},
});
this.sendMediaEvent(mediaEvent);
}

return sender
Expand Down Expand Up @@ -1150,23 +1162,6 @@ export class MembraneWebRTC {
this.sendMediaEvent(mediaEvent);
};

private getMidToTrackId = () => {
const localTrackMidToTrackId = {} as any;

if (!this.connection) return null;
this.connection.getTransceivers().forEach((transceiver) => {
const localTrackId = transceiver.sender.track?.id;
const mid = transceiver.mid;
if (localTrackId && mid) {
const trackContext = Array.from(this.localTrackIdToTrack.values()).find(
(trackContext) => trackContext!.track!!.id === localTrackId
)!;
localTrackMidToTrackId[mid] = trackContext.trackId;
}
});
return localTrackMidToTrackId;
};

/**
* Leaves the room. This function should be called when user leaves the room
* in a clean way e.g. by clicking a dedicated, custom button `disconnect`.
Expand Down Expand Up @@ -1252,8 +1247,7 @@ export class MembraneWebRTC {
type: "sdpOffer",
data: {
sdpOffer: offer,
trackIdToTrackMetadata: this.getTrackIdToMetadata(),
midToTrackId: this.getMidToTrackId(),
trackIdToTrackInfo: this.getTrackIdToTrackInfo(),
},
});
this.sendMediaEvent(mediaEvent);
Expand All @@ -1262,14 +1256,42 @@ export class MembraneWebRTC {
}
}

private getTrackIdToMetadata = () => {
const trackIdToMetadata = {} as any;
private getTrackIdToTrackInfo = () => {
const trackIdToMid = this.getTrackIdToMid();

const tracksInfo = {} as any;
Array.from(this.localPeer.trackIdToMetadata.entries()).forEach(
([trackId, metadata]) => {
trackIdToMetadata[trackId] = metadata;
const maxBandwidth =
this.localTrackIdToTrack.get(trackId)!.maxBandwidth;
tracksInfo[trackId] = {
trackMetadata: metadata,
maxBandwidth:
maxBandwidth instanceof Map
? Object.fromEntries(maxBandwidth)
: maxBandwidth,
mid: trackIdToMid ? trackIdToMid.get(trackId!) : null,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

};
}
);
return trackIdToMetadata;
return tracksInfo;
};

private getTrackIdToMid = () => {
const localTrackMidToTrackId = new Map<string, string>();

if (!this.connection) return null;
this.connection.getTransceivers().forEach((transceiver) => {
const localTrackId = transceiver.sender.track?.id;
const mid = transceiver.mid;
if (localTrackId && mid) {
const trackContext = Array.from(this.localTrackIdToTrack.values()).find(
(trackContext) => trackContext!.track!!.id === localTrackId
)!;
localTrackMidToTrackId.set(trackContext.trackId, mid);
}
});
return localTrackMidToTrackId;
};

private checkIfTrackBelongToPeer = (trackId: string, peer: Peer) =>
Expand Down
3 changes: 2 additions & 1 deletion tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
"moduleResolution": "node",
"outDir": "dist/",
"strict": true,
"target": "es2018"
"target": "es2018",
"lib": ["dom", "es6", "es2019"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added that to be able to use Object.fromEntries()

Copy link
Contributor

@mickel8 mickel8 Jan 23, 2023

Choose a reason for hiding this comment

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

Object.fromEntries was added in ECMAScript 2019. Do we need to include dom and es6 (ECMAScript 2015) too?

Copy link
Contributor Author

@LVala LVala Jan 23, 2023

Choose a reason for hiding this comment

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

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.

},
"include": ["src/**/*"]
}