Closed Bug 1341995 Opened 3 years ago Closed 3 years ago

WebRTC: payload type of RED in stream is fixed as 122, which is not match the one in SDP.

Categories

(Core :: WebRTC: Networking, defect, P2)

54 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: rzhang, Assigned: dminor)

Details

Attachments

(3 files)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36

Steps to reproduce:

1. Chrome canary 58 as caller, Firefox nightly 54 as callee. 
2. RED+ulpFEC enabled by default. 
3. The negotiated payload type of RED was 102.
4. Firefox sent RED stream with 122 payload type.
5. Chrome failed to decode the incoming stream.


Actual results:

Firefox always use the default payload type 122 as RED's payload type.

It works fine if I hardcode the payload type of RED as 122 in caller side ( Chrome ).

I tested Firefox 51 / Firefox DEV 53 / Firefox nightly 54. All have this issue.



Expected results:

The payload type of RED sent from Firefox should follow the negotiated one.
Component: Untriaged → WebRTC: Networking
Product: Firefox → Core
Dan, any thoughts here?
Flags: needinfo?(dminor)
Yes, we set it here [1] to kRedPayloadType, which is defined as 122.

It looks like we would have to add a red payload field to VideoCodecConfig (and I guess ulpfec while we are at it) and then populate those fields properly in the JsepCodecDescToCodecConfig function. We could then avoid using the constants.

[1] http://searchfox.org/mozilla-central/rev/b1044cf7c2000c3e75e8181e893236a940c8b6d2/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp#667
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(dminor)
Assignee: nobody → dminor
Rank: 25
Priority: -- → P2
I checked this fix by manually testing with Chrome Canary.
Status: NEW → ASSIGNED
Comment on attachment 8841621 [details]
Bug 1341995 - Use negotiated values for RED and ULPFEC payload types;

https://reviewboard.mozilla.org/r/115768/#review117154

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h:276
(Diff revision 1)
> +    int redPt = (uint8_t)strtoul(redPayloadType.c_str(), nullptr, 10);
> +    if (redPt == 0 && redPayloadType != "0") {
> +      return;
> +    }
> +
> +    int ulpfecPt = (uint8_t)strtoul(ulpfecPayloadType.c_str(), nullptr, 10);

Let's use SdpHelper::GetPtAsInt here.

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h:748
(Diff revision 1)
> +  int mREDPayloadType;
> +  int mULPFECPayloadType;

Let's use uint16_t or uint8_t here.
Attachment #8841621 - Flags: review?(docfaraday)
Comment on attachment 8841620 [details]
Bug 1341995 - Make ULPFEC and RED payload types configurable in VideoCodecConfig;

https://reviewboard.mozilla.org/r/115766/#review117274
Attachment #8841620 - Flags: review?(rjesup) → review+
Comment on attachment 8841621 [details]
Bug 1341995 - Use negotiated values for RED and ULPFEC payload types;

https://reviewboard.mozilla.org/r/115768/#review117522
Attachment #8841621 - Flags: review?(docfaraday) → review+
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/706398e37dbb
Make ULPFEC and RED payload types configurable in VideoCodecConfig; r=jesup
https://hg.mozilla.org/integration/autoland/rev/47c6c4caad99
Use negotiated values for RED and ULPFEC payload types; r=bwc
https://hg.mozilla.org/mozilla-central/rev/706398e37dbb
https://hg.mozilla.org/mozilla-central/rev/47c6c4caad99
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.