1274340 - Enable FEC during codec setup when negotiated

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[Dan Minor [:dminor]]

This bug covers enabling FEC at the codec level if it has been negotiated by sdp.

Comment 1

[Dan Minor [:dminor]]

Attachment: Allow enabling FEC for audio

This adds support for enabling FEC for audio only. I wanted to see if I was on the right path before starting in on video.

Comment 2

[Dan Minor [:dminor]]

Attachment: WIP to enabled FEC for video

WIP, compiles, otherwise untested.

Comment 3

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

Comment on attachment 8754496 [details] [diff] [review]
Allow enabling FEC for audio

Review of attachment 8754496 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
@@ +395,5 @@
>                                           mPtrVoEBase->LastError());
>      return kMediaConduitUnknownError;
>    }
>  
> +  if (mPtrVoECodec->SetFECStatus(mChannel, codecConfig->mFECEnabled) == -1) {

Add a comment that this must be after SetSendCodec()

::: media/webrtc/signaling/test/mediaconduit_unittests.cpp
@@ -572,5 @@
>  
>      //configure send and recv codecs on the audio-conduit
> -    //mozilla::AudioCodecConfig cinst1(124,"PCMU",8000,80,1,64000);
> -    mozilla::AudioCodecConfig cinst1(124,"opus",48000,960,1,64000);
> -    mozilla::AudioCodecConfig cinst2(125,"L16",16000,320,1,256000);

while you're here, add spaces after each comma.  Also, using L16 here seems odd, since we support PCMU and don't support L16, but that's a separate issue.

Comment 4

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

Comment on attachment 8757361 [details] [diff] [review]
WIP to enabled FEC for video

Review of attachment 8757361 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +746,5 @@
>    mSendingHeight = 0;
>    mSendingFramerate = video_codec.maxFramerate;
>  
> +  // TODO: determine whether or not to use FEC from config
> +  if (use_fec) {

Unfortunately, this file uses {'s on their own lines.

@@ +890,5 @@
>        {
>          if(mPtrViECodec->GetCodec(idx, video_codec) == 0)
>          {
>            payloadName = video_codec.plName;
> +

don't need to add blank line here I think

@@ +2110,5 @@
> +WebrtcVideoConduit::DetermineREDAndULPFECPayloadTypes(uint8_t &payload_type_red, uint8_t &payload_type_ulpfec)
> +{
> +    webrtc::VideoCodec video_codec;
> +    payload_type_red = 0;
> +    payload_type_ulpfec = 0;

0 is a valid payload value (it's pre-assigned to PCMU, but can be re-assigned to other payloads/codecs with a=rtpmap

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13bcb1c7d3b3
enable FEC during audio codec setup when negotiated; r=jesup

Comment 6

[Dan Minor [:dminor]]

Attachment: Allow enabling FEC for video

I've switched the default value for the payload types to 255. I noticed that the webrtc.org code used -1, but I wanted to avoid mixing signed and unsigned types.

Comment 7

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

Comment on attachment 8759805 [details] [diff] [review]
Allow enabling FEC for video

Review of attachment 8759805 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +747,5 @@
>  
> +  if (codecConfig->RtcpFbFECIsSet())
> +  {
> +    uint8_t payload_type_red = 255;
> +    uint8_t payload_type_ulpfec = 255;

Comment what 255 means, and #define INVALID_RTP_PAYLOAD 255
A comment that RTP payloads are 0-127 would help.

@@ +990,3 @@
>    {
> +    uint8_t payload_type_red = 255;
> +    uint8_t payload_type_ulpfec = 255;

Ditto - though they don't really need to be initialized, it's safer (very slightly)

@@ +2121,5 @@
> +WebrtcVideoConduit::DetermineREDAndULPFECPayloadTypes(uint8_t &payload_type_red, uint8_t &payload_type_ulpfec)
> +{
> +    webrtc::VideoCodec video_codec;
> +    payload_type_red = 255;
> +    payload_type_ulpfec = 255;

ditto

@@ +2140,5 @@
> +        }
> +      }
> +    }
> +
> +    return payload_type_red != 255 && payload_type_ulpfec != 255;

ditto

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/13bcb1c7d3b3

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/581cba07c379
enable FEC during video codec setup when negotiated; r=jesup

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/581cba07c379

Comment 11

[Dan Minor [:dminor]]

Attachment: Make RED and ULPFEC payloads match sdp values

Comment 12

[Dan Minor [:dminor]]

Attachment: Call SetReceiveCodec for RED and ULPFEC codecs

Comment 13

[Dan Minor [:dminor]]

The attachments are sufficient for video to work on call to chrome with FEC enabled. I'll request review once I've had a chance to do further testing with simulated packet loss.

Comment 14

[Dan Minor [:dminor]]

Comment on attachment 8766824 [details] [diff] [review]
Call SetReceiveCodec for RED and ULPFEC codecs

I've rebased and tested the attachments again today. I'm not sure if we'll be doing any packet loss tests for video FEC soon, but since the feature is preffed off anyway, I don't think there is any harm in getting these reviewed and landed to avoid bitrot.

Comment 15

[Dan Minor [:dminor]]

In VideoConduit.cpp I gate enabling FEC for codecs on codecConfig->RtcpFbFECIsSet() returning true, but it is not currently being set (I test by hard coding it to true). Is this the proper thing to check to see if FEC has been negotiated or should I be looking elsewhere? Thanks!

Comment 16

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

Comment on attachment 8766824 [details] [diff] [review]
Call SetReceiveCodec for RED and ULPFEC codecs

Review of attachment 8766824 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with some style cleanup to match the file

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +999,5 @@
>          return kMediaConduitFECStatusError;
>      }
>  
> +    // We also need to call SetReceiveCodec for RED and ULPFEC codecs
> +    for(int idx=0; idx < mPtrViECodec->NumberOfCodecs(); idx++)

nit: for () {
(similar for the if()'s)
Is NumberOfCodecs int?  (It may be; just checking)

@@ +1013,5 @@
> +          {
> +            CSFLogError(logTag, "%s Invalid Receive Codec %d ", __FUNCTION__,
> +                        mPtrViEBase->LastError());
> +          } else {
> +            CSFLogError(logTag, "%s Successfully Set the codec %s", __FUNCTION__,

Perhaps not LogError for the success case

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9b04662c81b
Make RED and ULPFEC payload type match sdp values; r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac0708fe8f74
Call SetReceiveCodec for RED and ULPFEC when FEC is enabled; r=jesup

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c9b04662c81b
https://hg.mozilla.org/mozilla-central/rev/ac0708fe8f74

Comment 19

[Michael Froman [:mjf]]

(In reply to Dan Minor [:dminor] from comment #15)
> In VideoConduit.cpp I gate enabling FEC for codecs on
> codecConfig->RtcpFbFECIsSet() returning true, but it is not currently being
> set (I test by hard coding it to true). Is this the proper thing to check to
> see if FEC has been negotiated or should I be looking elsewhere? Thanks!

It is the proper thing to check, but now that we are without Hello, I need to make sure I can cause exercise it.  I need to get my second machine setup tonight to do that more easily.  I'll check back with you tomorrow.

Comment 20

[Michael Froman [:mjf]]

(In reply to Michael Froman [:mjf] from comment #19)
> (In reply to Dan Minor [:dminor] from comment #15)
I've opened bug 1295690 to fix this.  I outsmarted myself with a premature optimization in a for loop.

Comment 21

[Dan Minor [:dminor]]

I think it's safe to close this now and open new bugs for any problems that show up.