Closed Bug 1274340 Opened 3 years ago Closed 3 years ago

Enable FEC during codec setup when negotiated

Categories

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

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: dminor, Assigned: dminor)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

This bug covers enabling FEC at the codec level if it has been negotiated by sdp.
Rank: 15
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.
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Attachment #8754496 - Flags: feedback?(rjesup)
Attached patch WIP to enabled FEC for video (obsolete) — Splinter Review
WIP, compiles, otherwise untested.
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.
Attachment #8754496 - Flags: feedback?(rjesup) → review+
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
Attachment #8757361 - Flags: feedback+
Keywords: leave-open
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13bcb1c7d3b3
enable FEC during audio codec setup when negotiated; r=jesup
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.
Attachment #8757361 - Attachment is obsolete: true
Attachment #8759805 - Flags: review?(rjesup)
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
Attachment #8759805 - Flags: review?(rjesup) → review+
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/581cba07c379
enable FEC during video codec setup when negotiated; r=jesup
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.
Attachment #8766823 - Flags: review?(rjesup)
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.
Attachment #8766824 - Flags: review?(rjesup)
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!
Flags: needinfo?(mfroman)
Attachment #8766823 - Flags: review?(rjesup) → review+
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
Attachment #8766824 - Flags: review?(rjesup) → review+
Pushed by dminor@mozilla.com:
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
(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.
(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.
Flags: needinfo?(mfroman)
I think it's safe to close this now and open new bugs for any problems that show up.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.