fix: make the fmtp params could be munged in firefox #12

x-epoch · 2022-12-07T12:43:41Z

As this code shows, we originally pushed max-mbps & max-fs into fmtParams to make homer support the 1080P video, and it finally generated the SDP by ts-sdp as below:

a=fmtp:102 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42001f
a=fmtp:102 max-mbps=244800
a=fmtp:102 max-fs=8160

Chrome CAN accept this SDP line when we set them by calling setLocalDescription API, however, Firefox do not like it . In order to achieve the same purpose in Firefox, we need to enhance the toLines() method in codec-info.ts , make it serialize things out correctly, which the final serialize string could be :

a=fmtp:102 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42001f;max-mbps=244800;max-fs=8160

bbaldino · 2022-12-07T17:43:18Z

src/model/codec-info.ts

-      lines.push(new FmtpLine(this.pt, fmt));
-    });
-
+    if (this.fmtParams.length > 0) {


I think we should do this at the FmtpLine level, not the CodecInfo level: it should still be possible to serialize multiple fmtp lines.

What we want is for the caller (wcme) to be able to grab the existing fmtp lines in the CodecInfo, and 'expand' one of them by adding extra params.

I think we should do this at the FmtpLine level, not the CodecInfo level: it should still be possible to serialize multiple fmtp lines.

Since the fmtParams in CodecInfo currently is defined as Array type, so we CAN NOT prevent upper layer adding new fmtp params through the push method, and that actually the way we used to add new fmtp params in WCME as this code shows.
So what's this PR want to solve is: when there are multiple fmtp lines with the same payload type in fmtParams, just merge it as one line.

What we want is for the caller (wcme) to be able to grab the existing fmtp lines in the CodecInfo, and 'expand' one of them by adding extra params.

Yes, of course we can do this, but if we keep the fmtParams still as Array type, we still need to consider the problem that the fmtParams may be contains multiple values(lines) with same payload type, which would result in Firefox failed.

Before the push method being called, there may be only 1 item in fmtParams , which could be:

['fmtp:102 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42001f']

After the push method being called like this:

ci.fmtParams.push(max-mbps=${defaultMaxVideoEncodeMbps});
ci.fmtParams.push(max-fs=${defaultMaxVideoEncodeFrameSize});

Then the value of fmtParams would contains 3 items, which could be:

['fmtp:102 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42001f', max-mbps=244800, max-fs=8160]

What this PR want to solve is, merge all items with same payload type in the fmtParams , and make it as one line, which final be:

['fmtp:102 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42001f';max-mbps=244800;max-fs=8160]

Ok, I re-read the spec and it does say only one fmtp line is allowed per payload type, so since CodecInfo is on the scope of a payload type, it does make sense to make sure we always serialize to a single fmtp line. But this just means that, when serializing CodecInfo, instead of writing a new fmtp line for each param, we just need to use a single line for all the params?

This doesn't protect against multiple, duplicate values, though. But if we want to protect against that, FmtpLine should parse the params into an object/map, instead of a single string. CodecInfo should then hold a map of these params, instead of a list. So then the codec can add a new param just as before, but can avoid duplicate param values.

Code updated.

…fo as map

x-epoch · 2022-12-12T14:57:18Z

src/lines/fmtp-line.ts

+  const fmtpObj = new Map<string, string | undefined>();
+
+  // compatible with RED，such as `a=fmtp:121 0/5` in chrome and `a=fmtp:121 0-5` in firefox, which can {@link https://www.rfc-editor.org/rfc/rfc2198}
+  if (/^\d+([/-]\d+)+$/.test(fmtpParams)) {


Since we further parse the fmtp params as Map here, so we have to consider the case of RED, which the sample line likes a=fmtp:121 0/5 (and it would be a=fmtp:121 0-5 in Firefox by testing).
In such case, since it is lack of equals sign(= ), we need to set its default value as undefined when put it into Map. When compose the fmtp line in toSdpLine method from Map, we should ignore the value and only need to output the Map key.

bbaldino

cool, looks good. one small comment.

bbaldino · 2022-12-12T18:19:03Z

src/lines/fmtp-line.ts

+ * @param fmtpParams -  like `key1=val1;key2=val2;key3=val3".
+ * @returns A JS key-value object.
+ */
+export function parseFmtpParams(fmtpParams: string) {


Does this need to be exported? At this point I think it's just an internal helper function, right?

I see you pass a map in down below, I think we still don't need to export this. This will be used for the 'internal' parsing (where it takes the sdp string and parses it). The map constructor will be used when someone wants to craft a line on their own, so they'll just add the params specifically to the map.

Since this function is also needed in WCME, so just make it be exported and could be shared with codec-utils.ts in WCME which the code can refer to here

Ah I see, I didn't think there was a need for parsing those after this, but I didn't realize RTCRtpCodecParameters still holds them in that format.

## [1.3.1](v1.3.0...v1.3.1) (2022-12-13) ### Bug Fixes * make the fmtp params could be munged in firefox ([#12](#12)) ([6636f37](6636f37))

fix: make the fmtp params could be munged in firefox

115ff9b

bbaldino reviewed Dec 7, 2022

View reviewed changes

fix: change the params type in FmtpLine and fmtParams type in CodecIn…

35a177c

…fo as map

x-epoch force-pushed the firefox-fmtp-compatible branch from 48371b2 to 35a177c Compare December 12, 2022 14:47

x-epoch commented Dec 12, 2022

View reviewed changes

bbaldino reviewed Dec 12, 2022

View reviewed changes

bbaldino approved these changes Dec 13, 2022

View reviewed changes

bbaldino merged commit 6636f37 into webex:main Dec 13, 2022

arun3528 pushed a commit that referenced this pull request Dec 13, 2022

chore(release): 1.3.1 [skip ci]

2599a0e

## [1.3.1](v1.3.0...v1.3.1) (2022-12-13) ### Bug Fixes * make the fmtp params could be munged in firefox ([#12](#12)) ([6636f37](6636f37))

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: make the fmtp params could be munged in firefox #12

fix: make the fmtp params could be munged in firefox #12

x-epoch commented Dec 7, 2022

bbaldino Dec 7, 2022

x-epoch Dec 8, 2022 •

edited

x-epoch Dec 8, 2022 •

edited

bbaldino Dec 8, 2022

x-epoch Dec 12, 2022 •

edited

x-epoch Dec 12, 2022 •

edited

bbaldino left a comment

bbaldino Dec 12, 2022

bbaldino Dec 12, 2022

x-epoch Dec 13, 2022 •

edited

bbaldino Dec 13, 2022

fix: make the fmtp params could be munged in firefox #12

fix: make the fmtp params could be munged in firefox #12

Conversation

x-epoch commented Dec 7, 2022

bbaldino Dec 7, 2022

Choose a reason for hiding this comment

x-epoch Dec 8, 2022 • edited

Choose a reason for hiding this comment

x-epoch Dec 8, 2022 • edited

Choose a reason for hiding this comment

bbaldino Dec 8, 2022

Choose a reason for hiding this comment

x-epoch Dec 12, 2022 • edited

Choose a reason for hiding this comment

x-epoch Dec 12, 2022 • edited

Choose a reason for hiding this comment

bbaldino left a comment

Choose a reason for hiding this comment

bbaldino Dec 12, 2022

Choose a reason for hiding this comment

bbaldino Dec 12, 2022

Choose a reason for hiding this comment

x-epoch Dec 13, 2022 • edited

Choose a reason for hiding this comment

bbaldino Dec 13, 2022

Choose a reason for hiding this comment

x-epoch Dec 8, 2022 •

edited

x-epoch Dec 8, 2022 •

edited

x-epoch Dec 12, 2022 •

edited

x-epoch Dec 12, 2022 •

edited

x-epoch Dec 13, 2022 •

edited