Closed Bug 1275360 Opened 4 years ago Closed 3 years ago

Add sdp for video FEC

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: mjf, Assigned: mjf)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

This bug covers the SDP and negotiation for FEC
Assignee: nobody → mfroman
Rank: 15
Priority: -- → P1
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.
Flags: needinfo?(rjesup)
(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.
(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
Flags: needinfo?(rjesup)
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?
Please checkout my previous comment.
Flags: needinfo?(rjesup)
Summary: Add FEC SDP negotiation → Add sdp for video FEC
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/
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.
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 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 on attachment 8766969 [details]
Bug 1275360 - add sdp handling for video FEC (red/ulpfec),

https://reviewboard.mozilla.org/r/61672/#review58648
Attachment #8766969 - Flags: review?(drno) → review+
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
Attachment #8766969 - Flags: review-
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 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/
Attachment #8766969 - Flags: review-
I've created a new bug (Bug 1285280) for making the changes around creating codec descriptions using defines from a new header file.
Flags: needinfo?(rjesup)
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
Attachment #8766969 - Flags: review+
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/
Attachment #8766969 - Flags: review+
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 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/
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7e10e7ea1b2
Add sdp handling for video FEC (red/ulpfec). r=drno
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e7e10e7ea1b2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1295690
Blocks: 1313527
Blocks: 1364618
You need to log in before you can comment on or make changes to this bug.