1279049 - Add sdp for audio FEC

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P1)

Description

[Michael Froman [:mjf]]

Covers the audio sdp portion of FEC signaling.

Comment 1

[Michael Froman [:mjf]]

Attachment: Bug 1279049 - add sdp handling for audio FEC,

Review commit: https://reviewboard.mozilla.org/r/58598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58598/

Comment 2

[Michael Froman [:mjf]]

Comment on attachment 8761381 [details]
Bug 1279049 - add sdp handling for audio FEC,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58598/diff/1-2/

Comment 3

[Michael Froman [:mjf]]

Comment on attachment 8761381 [details]
Bug 1279049 - add sdp handling for audio FEC,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58598/diff/2-3/

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8761381 [details]
Bug 1279049 - add sdp handling for audio FEC,

https://reviewboard.mozilla.org/r/58598/#review55662

Have you tested it, both in Opus and in G722?  And verified the SDP offered/answered, and checked interop with Chrome in Opus and G722?

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h:175
(Diff revision 3)
>        }
>        if (mChannels == 2 && !mForceMono) {
>          // We prefer to receive stereo, if available.
>          opusParams.stereo = 1;
>        }
> +      opusParams.useInBandFec = mFECEnabled?1:0;

nit: spaces around ?/:

::: media/webrtc/signaling/src/sdp/SdpAttribute.h:1245
(Diff revision 3)
>      enum { kDefaultMaxPlaybackRate = 48000,
> -           kDefaultStereo = 0 };
> +           kDefaultStereo = 0,
> +           kDefaultUseInBandFec = 0 };

I find it freaky to use an enum in this manner in place of a static const, but it was already doing it.  (and they're not the same)

Comment 5

[Michael Froman [:mjf]]

https://reviewboard.mozilla.org/r/58598/#review55662

I have tested with Opus between Fx/Fx and Fx/Chrome.  draft-ietf-rtcweb-fec-03.txt section 4.2 seems to say that the useinbandfec param only applies to Opus and not G722.  Chrome doesn't offer a "red" payload type for doing redundant encoding on audio.  It only sends the useinbandfec param for Opus.

I think it would be easy enough to add the additional RED audio payload to the offer later if and when we have something that will use it.  Is that acceptable?

Comment 6

[Michael Froman [:mjf]]

https://reviewboard.mozilla.org/r/58598/#review55662

> I find it freaky to use an enum in this manner in place of a static const, but it was already doing it.  (and they're not the same)

I didn't love this either, but as you said "already doing it."

Comment 7

[Michael Froman [:mjf]]

Comment on attachment 8761381 [details]
Bug 1279049 - add sdp handling for audio FEC,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58598/diff/3-4/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/653ddd57a84e
add sdp handling for audio FEC, r=jesup

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/653ddd57a84e