1275360 - Add sdp for video FEC

Status: RESOLVED FIXED

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

Description

[Michael Froman [:mjf]]

This bug covers the SDP and negotiation for FEC

Comment 1

[Michael Froman [:mjf]]

Do we care about enabling FEC for codecs other than Opus?

Section 4.1 of https://www.ietf.org/id/draft-ietf-rtcweb-fec-03.txt lists several other potential options:
- built-in FEC for AMR/AMR-WB
  - we don't yet deal with the max-red parameter in SDP, but it seems that FEC can happen without it
    explicitly specified
- redundant encoding for variable-bitrate codecs without an internal FEC
  - do we have any of these?  Seems like no.
- redundant encoding for constant-bitrate codecs
  - since this is a MAY, also seems like a no.

Comment 2

[Michael Froman [:mjf]]

(In reply to Michael Froman [:mjf] from comment #1)
> Do we care about enabling FEC for codecs other than Opus?
That should say _audio_ codecs.  I'm just getting started on video.

Comment 3

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

(In reply to Michael Froman [:mjf] from comment #1)
> Do we care about enabling FEC for codecs other than Opus?
> 
> Section 4.1 of https://www.ietf.org/id/draft-ietf-rtcweb-fec-03.txt lists
> several other potential options:
> - built-in FEC for AMR/AMR-WB
>   - we don't yet deal with the max-red parameter in SDP, but it seems that

We don't do AMR

> - redundant encoding for variable-bitrate codecs without an internal FEC
>   - do we have any of these?  Seems like no.

No: Opus (variable), G722 (fixed), G711a/u (fixed)

> - redundant encoding for constant-bitrate codecs
>   - since this is a MAY, also seems like a no.

We can start here, though let's check what chrome does for them

Comment 4

[Michael Froman [:mjf]]

Now for the video questions..
https://www.ietf.org/id/draft-ietf-rtcweb-fec-03.txt
points to:
https://www.ietf.org/id/draft-ietf-payload-flexible-fec-scheme-02.txt

Looking at the offer Chrome sends to appear.in, I see:
a=rtpmap:116 red/90000
a=rtpmap:117 ulpfec/90000
This appears to be RFC 5109.  Is this what we're aiming for as opposed to flexible-fec?  If so, section 14.2 also mentions an additional a=fmtp line that Chrome isn't sending that is required for the sender to know (now quoting from RFC 2198) "which codecs are recommended for the primary and secondary (and tertiary, etc) encodings."
An example from the rfc is:
      m=audio 12345 RTP/AVP 121 0 5 100
      a=rtpmap:121 red/8000/1
      a=rtpmap:100 ulpfec/8000
      a=fmtp:121 0/5/100             <==== Chrome is missing this line

The first step is just signaling that we accept FEC, which we'll be doing without that missing line according to RFC 2198.  If neither side provides that line it seems unclear what, if any, redundant stream will be sent.

Any thoughts?

Comment 5

[Michael Froman [:mjf]]

Please checkout my previous comment.

Comment 6

[Michael Froman [:mjf]]

Attachment: Bug 1275360 - add sdp handling for video FEC (red/ulpfec),

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

Comment 7

[Michael Froman [:mjf]]

Comment on attachment 8766969 [details]
Bug 1275360 - add sdp handling for video FEC (red/ulpfec),

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

Comment 8

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/61672/#review58538

In general good, but I would like to get some answer before r+ing.

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h:249
(Diff revision 1)
> +    // FEC for video works a little different since support for it is
> +    // indicated by the presence of particular codes (red and ulpfec)
> +    // instead of using rtcpfb attributes on a given codec

I find this comment a little confusing.
It sounds like you added this after first working on audio and then switching to video ('different' compared to what - I guess audio).

It's nice introductory into how to do FEC signaling, but why here? Wouldn't it make more sense to give this description when you check for "red" etc?

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h:295
(Diff revision 1)
>        h264Params.packetization_mode = mPacketizationMode;
>        // Hard-coded, may need to change someday?
>        h264Params.level_asymmetry_allowed = true;
>  
>        msection.SetFmtp(SdpFmtpAttributeList::Fmtp(mDefaultPt, h264Params));
> +    } else if (mName == "red") {

What about 'ulpfec' here? Not needed?

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h:460
(Diff revision 1)
>                             &mProfileLevelId);
>          }
>        } else {
>          // TODO(bug 1143709): max-recv-level support
>        }
> -
> +    } else if (mName == "red") {

'ulpfec' also not needed here?

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:2169
(Diff revision 1)
> +      "122",
> +      "red",
> +      90000

Brief comments for these initial magic values might be helpful for non-experts.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:2176
(Diff revision 1)
> +  red->mRedundantEncodings.push_back(121);
> +  red->mRedundantEncodings.push_back(120);
> +  red->mRedundantEncodings.push_back(126);
> +  red->mRedundantEncodings.push_back(97);
> +  red->mRedundantEncodings.push_back(123);

What are these magic numbers?
I'm guessing codec PT numbers. If so what are the assumed values here (in case someone ever needs to change the default PT's for one of the codecs)?
Even better then comments would be some kind of codec name -> codec PT lookup function/registry. The webrtc.org has something like that. Not sure if it's worth replicating that.

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:393
(Diff revision 1)
> -  // answer
> -  if (!codecs->empty()) {
> +  // answer.  For now, remove all but the first codec unless the first codec
> +  // is red, and then we include the others per RFC 5109, section 14.2.
> +  if (!codecs->empty() && (*codecs)[0]->mName != "red") {

I don't see that the comment actually reflects the code.
The code appears to skip over the whole thing if the first codec is 'red'. And I don't see any code which includes other codecs.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1055
(Diff revision 1)
> +        uint8_t pt = (uint8_t)strtoul(codec->mDefaultPt.c_str(), nullptr, 10);
> +        // don't search for the codec payload type unless we have a valid
> +        // conversion (non-zero)
> +        if (pt != 0) {

I'm guessing strtoul for PCMU returning 0 is okay, as there is no RED for PCMU :-)

::: media/webrtc/signaling/test/jsep_track_unittest.cpp:44
(Diff revision 1)
> +        JsepVideoCodecDescription* red = new JsepVideoCodecDescription(
> +            "122",
> +            "red",
> +            90000
> +            );
> +        red->mRedundantEncodings.push_back(120);
> +        red->mRedundantEncodings.push_back(126);
> +        red->mRedundantEncodings.push_back(123);
> +        results.push_back(red);

As this appears to be redundant with the red generation further down, can we move this into a little helper function to avoid the duplication?

::: media/webrtc/signaling/test/jsep_track_unittest.cpp:415
(Diff revision 1)
> +  ASSERT_NE(mOffer->ToString().find("a=rtpmap:122 red"), std::string::npos);
> +  ASSERT_EQ(mAnswer->ToString().find("a=rtpmap:122 red"), std::string::npos);
> +  ASSERT_NE(mOffer->ToString().find("a=rtpmap:123 ulpfec"), std::string::npos);
> +  ASSERT_EQ(mAnswer->ToString().find("a=rtpmap:123 ulpfec"), std::string::npos);

I would find it easier to understand this if you check the offer first for red and ulpfec and then check the answer. But that might be just a personal preference.

::: media/webrtc/signaling/test/jsep_track_unittest.cpp:467
(Diff revision 1)
> +  mOffCodecs.values = MakeCodecs(true, true);
> +  mAnsCodecs.values = MakeCodecs(true);

I'm missing the test for:
 ... MakeCodecs(true);
 ... MakeCodecs(true);
So without preferring Red.

Comment 9

[Michael Froman [:mjf]]

https://reviewboard.mozilla.org/r/61672/#review58538

> I find this comment a little confusing.
> It sounds like you added this after first working on audio and then switching to video ('different' compared to what - I guess audio).
> 
> It's nice introductory into how to do FEC signaling, but why here? Wouldn't it make more sense to give this description when you check for "red" etc?

I will add an additional comment when I check for red.  Specifically, this comment is here because in every other case, turning on some "feature" of a codec is done with attributes like rtcpfb.  Since the other cases directly above (for REMB and TMMBR) both have to push rtcpfb attributes onto a vector, and this case is different, I wanted to call out the reason for that difference here.

> What about 'ulpfec' here? Not needed?

Nope.  Redundant encodes don't require ulpfec.  You could use a completely different codec than ulpfec to supply the redundant info.  We're just taking care of the list of redundant encodings that are possible here.

> 'ulpfec' also not needed here?

Same as above - redundant doesn't require ulpfec.  We're just taking care of the list of redundant encodings that are possible here.

> Brief comments for these initial magic values might be helpful for non-experts.

Sorry - I was matching the other codecs' definitions.  I'll comment on the new ones I've added.

> What are these magic numbers?
> I'm guessing codec PT numbers. If so what are the assumed values here (in case someone ever needs to change the default PT's for one of the codecs)?
> Even better then comments would be some kind of codec name -> codec PT lookup function/registry. The webrtc.org has something like that. Not sure if it's worth replicating that.

I added a method in JsepVideoCodecDescription that builds the redundant encodings list from a provided vector of JsepCodecDescription.  If someone changes default PT for one of codecs, it will be handled automatically.

> I don't see that the comment actually reflects the code.
> The code appears to skip over the whole thing if the first codec is 'red'. And I don't see any code which includes other codecs.

Before this if statement, all the codecs that are enabled exist in the codecs vector.  Previously, the code _always_ removed everything but the first codec.  This meant answers from Fx never contained more than one codec.  Now, if the first codec is "red", we leave them in the vector.

You have raised a different question in my mind though, and that is whether we want to leave the other codecs in the vector if "red" is anywhere in the list, not just the first (therefore preferred) codec.  I've made that change, and will run some additional tests.

> I'm guessing strtoul for PCMU returning 0 is okay, as there is no RED for PCMU :-)

Since we're only checking video codecs, we're safe here.

> I would find it easier to understand this if you check the offer first for red and ulpfec and then check the answer. But that might be just a personal preference.

I changed it!

Comment 10

[Michael Froman [:mjf]]

Comment on attachment 8766969 [details]
Bug 1275360 - add sdp handling for video FEC (red/ulpfec),

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

Comment 11

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/61670/#review58646

LGTM

Comment 12

[Nils Ohlmeier [:drno]]

Comment on attachment 8766969 [details]
Bug 1275360 - add sdp handling for video FEC (red/ulpfec),

https://reviewboard.mozilla.org/r/61672/#review58648

Comment 13

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

Comment on attachment 8766969 [details]
Bug 1275360 - add sdp handling for video FEC (red/ulpfec),

https://reviewboard.mozilla.org/r/61672/#review59166

r- only for the pt=0 thing.  Note: I've only reviewed the final update to the patch so far, not the entire patch

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h:694
(Diff revisions 2 - 3)
> +        uint8_t pt = (uint8_t)strtoul(codec->mDefaultPt.c_str(), nullptr, 10);
> +        // returns 0 if failed to convert - this is safe because we won't
> +        // see video codecs with pt of 0
> +        if (pt != 0) {
> +          mRedundantEncodings.push_back(pt);
> +        }

Unfortunately not quite correct... 0 by default is PCMU, but a=rtpmap:0 video/VP8 90000 is totally valid SDP, making 0 be VP8 and overriding the default mapping.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:2168
(Diff revisions 2 - 3)
>    JsepVideoCodecDescription* red = new JsepVideoCodecDescription(
> -      "122",
> -      "red",
> -      90000
> +      "122", // payload type
> +      "red", // codec name
> +      90000  // clock rate (match other video codecs)
>        );

the added comments are unneeded given the other code here with raw values and no comments; if anything the comment should be on the first usage not the last.  Pulling them all out into a .h file as defines/etc would be even more preferable but not required in this go-round.

::: media/webrtc/signaling/src/jsep/JsepTrack.cpp:391
(Diff revisions 2 - 3)
>    // Make sure strongly preferred codecs are up front, overriding the remote
>    // side's preference.
>    std::stable_sort(codecs->begin(), codecs->end(), CompareCodec);

not part of this patch, but this comment concerns me

::: media/webrtc/signaling/test/jsep_track_unittest.cpp:67
(Diff revisions 2 - 3)
> -          JsepVideoCodecDescription* red = new JsepVideoCodecDescription(
> +          red = new JsepVideoCodecDescription(
>                "122",
>                "red",
>                90000
>                );

And this is part of why I'd like it in a .h file; it's in as multiple cases of hard constants

Comment 14

[Michael Froman [:mjf]]

https://reviewboard.mozilla.org/r/61672/#review59166

> Unfortunately not quite correct... 0 by default is PCMU, but a=rtpmap:0 video/VP8 90000 is totally valid SDP, making 0 be VP8 and overriding the default mapping.

Ahh!  I never stopped to think about it being default and not reserved.  Sorry about that misunderstanding!  Also added a test in jsep_track_unittest to check this case.

> the added comments are unneeded given the other code here with raw values and no comments; if anything the comment should be on the first usage not the last.  Pulling them all out into a .h file as defines/etc would be even more preferable but not required in this go-round.

Should I create a bug for doing this work separately?

> not part of this patch, but this comment concerns me

It did me too when I first saw it.  In practice, I'm not sure we ever set the mStronglyPreferred flag on the codec descriptions.  I added some printf logging when I started this bug to see what that was actually doing (which seems to be mostly nothing).  I didn't want to make any changes here since it wasn't specific to adding support for red/ulpfec.

> And this is part of why I'd like it in a .h file; it's in as multiple cases of hard constants

Please see my previous question about adding a bug for this work.

Comment 15

[Michael Froman [:mjf]]

Comment on attachment 8766969 [details]
Bug 1275360 - add sdp handling for video FEC (red/ulpfec),

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

Comment 16

[Michael Froman [:mjf]]

I've created a new bug (Bug 1285280) for making the changes around creating codec descriptions using defines from a new header file.

Comment 17

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

Comment on attachment 8766969 [details]
Bug 1275360 - add sdp handling for video FEC (red/ulpfec),

https://reviewboard.mozilla.org/r/61672/#review61058

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h:51
(Diff revision 3)
>    Matches(const std::string& fmt, const SdpMediaSection& remoteMsection) const
>    {
> +    // note: fmt here is remote fmt (to go with remoteMsection)

remoteFmt perhaps?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1108
(Diff revision 3)
> +      redCodec = static_cast<JsepVideoCodecDescription*>(codec);
> +      break;
> +    }
> +  }
> +  // if red codec was found, configure it for the other enabled codecs
> +  if (redCodec != nullptr) {

Generally style is "if (ptr) {"

::: media/webrtc/signaling/src/sdp/sipcc/sdp_attr.c:1788
(Diff revision 3)
> +        } else if (strchr(tmp, '/')) {
> +            // XXX Note that because RFC 5109 so conveniently specified
> +            // this fmtp with no param names, we hope that nothing else
> +            // has a slash in the string because otherwise we won't know
> +            // how to differentiate.
> +            temp=PL_strtok_r(tmp, "/", &strtok_state);

Style is "var = value", not "var=value" (nit, in a number of spots

Comment 18

[Michael Froman [:mjf]]

Comment on attachment 8766969 [details]
Bug 1275360 - add sdp handling for video FEC (red/ulpfec),

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

Comment 19

[Michael Froman [:mjf]]

https://reviewboard.mozilla.org/r/61672/#review61058

> remoteFmt perhaps?

So what is the stance on changing code that isn't directly related to the current bug?  You're correct about that variable name making more sense, but I didn't think it was appopriate to make that sort of change here.  If it is, I'll gladly start making those sorts of changes.

Comment 20

[Michael Froman [:mjf]]

Comment on attachment 8766969 [details]
Bug 1275360 - add sdp handling for video FEC (red/ulpfec),

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61672/diff/5-6/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7e10e7ea1b2
Add sdp handling for video FEC (red/ulpfec). r=drno

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e7e10e7ea1b2