Bug 1432922: Implemented parsing support for rtcpfb-wildcard. r=bwc,dminor
authorJohannes Willbold <j.willbold@mozilla.com>
Tue, 29 May 2018 16:32:52 -0700
changeset 420825 34215e726ea891066d8f7f1404c709027ec28711
parent 420824 7b1bd397ecadf03d546b496b2541eef1f9e3419e
child 420826 137a69fa8f692c53e028ea480ad9e1dc798056f9
push id103894
push usercsabou@mozilla.com
push dateFri, 01 Jun 2018 09:46:36 +0000
treeherdermozilla-inbound@e99ff79303ea [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbwc, dminor
bugs1432922
milestone62.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1432922: Implemented parsing support for rtcpfb-wildcard. r=bwc,dminor Implemented Rust/C++ glue code for rtcp-fb Implemented parsing support for rtcpfb-wildcard in rust Activated c++ unit tests MozReview-Commit-ID: 5xRSQz7pucZ
media/webrtc/signaling/gtest/sdp_unittests.cpp
media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp
media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.h
media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h
media/webrtc/signaling/src/sdp/SdpAttribute.cpp
media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs
media/webrtc/signaling/src/sdp/rsdparsa/src/media_type.rs
media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs
--- a/media/webrtc/signaling/gtest/sdp_unittests.cpp
+++ b/media/webrtc/signaling/gtest/sdp_unittests.cpp
@@ -3044,24 +3044,22 @@ const std::string kBasicAudioVideoDataOf
 "a=setup:actpass" CRLF
 "a=rtcp-mux" CRLF
 "m=application 9 DTLS/SCTP 5000" CRLF
 "c=IN IP4 0.0.0.0" CRLF
 "a=sctpmap:5000 webrtc-datachannel 16" CRLF
 "a=setup:actpass" CRLF;
 
 TEST_P(NewSdpTest, BasicAudioVideoDataSdpParse) {
-  SKIP_TEST_WITH_RUST_PARSER; // See Bug 1432922
   ParseSdp(kBasicAudioVideoDataOffer);
   ASSERT_EQ(0U, mSdpErrorHolder->GetParseErrors().size()) <<
     "Got parse errors: " << GetParseErrors();
 }
 
 TEST_P(NewSdpTest, CheckApplicationParameters) {
-  SKIP_TEST_WITH_RUST_PARSER; // See Bug 1432922
   ParseSdp(kBasicAudioVideoDataOffer);
   ASSERT_TRUE(!!mSdp);
   ASSERT_EQ(3U, mSdp->GetMediaSectionCount()) << "Wrong number of media sections";
   ASSERT_EQ(SdpMediaSection::kAudio, mSdp->GetMediaSection(0).GetMediaType())
     << "Wrong type for first media section";
   ASSERT_EQ(SdpMediaSection::kVideo, mSdp->GetMediaSection(1).GetMediaType())
     << "Wrong type for second media section";
   ASSERT_EQ(SdpMediaSection::kApplication, mSdp->GetMediaSection(2).GetMediaType())
@@ -3082,17 +3080,16 @@ TEST_P(NewSdpTest, CheckApplicationParam
 
   ASSERT_TRUE(mSdp->GetMediaSection(2).GetAttributeList().HasAttribute(
       SdpAttribute::kSetupAttribute));
   ASSERT_EQ(SdpSetupAttribute::kActpass,
       mSdp->GetMediaSection(2).GetAttributeList().GetSetup().mRole);
 }
 
 TEST_P(NewSdpTest, CheckExtmap) {
-  SKIP_TEST_WITH_RUST_PARSER; // See Bug 1432922
   ParseSdp(kBasicAudioVideoDataOffer);
   ASSERT_TRUE(!!mSdp);
   ASSERT_EQ(3U, mSdp->GetMediaSectionCount()) << "Wrong number of media sections";
 
   ASSERT_TRUE(mSdp->GetMediaSection(0).GetAttributeList().HasAttribute(
         SdpAttribute::kExtmapAttribute));
 
   auto extmaps =
@@ -3118,17 +3115,16 @@ TEST_P(NewSdpTest, CheckExtmap) {
   ASSERT_FALSE(extmaps[2].direction_specified);
   ASSERT_EQ("some_other_extension",
       extmaps[2].extensionname);
   ASSERT_EQ("some_params some more params",
       extmaps[2].extensionattributes);
 }
 
 TEST_P(NewSdpTest, CheckRtcpFb) {
-  SKIP_TEST_WITH_RUST_PARSER; // See Bug 1432922
   ParseSdp(kBasicAudioVideoDataOffer);
   ASSERT_TRUE(!!mSdp);
   ASSERT_EQ(3U, mSdp->GetMediaSectionCount()) << "Wrong number of media sections";
 
   auto& video_attrs = mSdp->GetMediaSection(1).GetAttributeList();
   ASSERT_TRUE(video_attrs.HasAttribute(SdpAttribute::kRtcpFbAttribute));
   auto& rtcpfbs = video_attrs.GetRtcpFb().mFeedbacks;
   ASSERT_EQ(20U, rtcpfbs.size());
--- a/media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp
+++ b/media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp
@@ -8,16 +8,18 @@
 
 #include "signaling/src/sdp/RsdparsaSdpAttributeList.h"
 #include "signaling/src/sdp/RsdparsaSdpInc.h"
 #include "signaling/src/sdp/RsdparsaSdpGlue.h"
 
 #include <ostream>
 #include "mozilla/Assertions.h"
 
+#include <limits>
+
 namespace mozilla
 {
 
 const std::string RsdparsaSdpAttributeList::kEmptyString = "";
 
 RsdparsaSdpAttributeList::~RsdparsaSdpAttributeList()
 {
   for (size_t i = 0; i < kNumAttributeTypes; ++i) {
@@ -450,16 +452,19 @@ RsdparsaSdpAttributeList::LoadAttribute(
         LoadMsidSemantics(attributeList);
         return;
       case SdpAttribute::kGroupAttribute:
         LoadGroup(attributeList);
         return;
       case SdpAttribute::kRtcpAttribute:
         LoadRtcp(attributeList);
         return;
+      case SdpAttribute::kRtcpFbAttribute:
+        LoadRtcpFb(attributeList);
+        return;
       case SdpAttribute::kImageattrAttribute:
         LoadImageattr(attributeList);
         return;
       case SdpAttribute::kSctpmapAttribute:
         LoadSctpmaps(attributeList);
         return;
       case SdpAttribute::kDirectionAttribute:
         LoadDirection(attributeList);
@@ -468,22 +473,22 @@ RsdparsaSdpAttributeList::LoadAttribute(
         LoadRemoteCandidates(attributeList);
         return;
       case SdpAttribute::kRidAttribute:
         LoadRids(attributeList);
         return;
       case SdpAttribute::kExtmapAttribute:
         LoadExtmap(attributeList);
         return;
+
       case SdpAttribute::kDtlsMessageAttribute:
       case SdpAttribute::kLabelAttribute:
       case SdpAttribute::kMaxptimeAttribute:
       case SdpAttribute::kSsrcGroupAttribute:
       case SdpAttribute::kMaxMessageSizeAttribute:
-      case SdpAttribute::kRtcpFbAttribute:
       case SdpAttribute::kRtcpRsizeAttribute:
       case SdpAttribute::kSctpPortAttribute:
       case SdpAttribute::kCandidateAttribute:
       case SdpAttribute::kSimulcastAttribute:
       case SdpAttribute::kConnectionAttribute:
       case SdpAttribute::kIceMismatchAttribute:
         // TODO: Not implemented, or not applicable.
         // Sort this out in Bug 1437165.
@@ -842,16 +847,49 @@ RsdparsaSdpAttributeList::LoadRtcp(RustA
       std::string addr(rtcp.unicastAddr.unicastAddr);
       SetAttribute(new SdpRtcpAttribute(rtcp.port, sdp::kInternet,
                                         addrType, addr));
     }
   }
 }
 
 void
+RsdparsaSdpAttributeList::LoadRtcpFb(RustAttributeList* attributeList)
+{
+  auto rtcpfbsCount = sdp_get_rtcpfb_count(attributeList);
+  if (!rtcpfbsCount) {
+    return;
+  }
+
+  auto rustRtcpfbs = MakeUnique<RustSdpAttributeRtcpFb[]>(rtcpfbsCount);
+  sdp_get_rtcpfbs(attributeList, rtcpfbsCount, rustRtcpfbs.get());
+
+  auto rtcpfbList = MakeUnique<SdpRtcpFbAttributeList>();
+  for(size_t i = 0; i < rtcpfbsCount; i++) {
+    RustSdpAttributeRtcpFb& rtcpfb = rustRtcpfbs[i];
+    uint32_t payloadTypeU32 = rtcpfb.payloadType;
+
+    std::stringstream ss;
+    if(payloadTypeU32 == std::numeric_limits<uint32_t>::max()){
+      ss << "*";
+    } else {
+      ss << payloadTypeU32;
+    }
+
+    uint32_t feedbackType = rtcpfb.feedbackType;
+    std::string parameter = convertStringView(rtcpfb.parameter);
+    std::string extra = convertStringView(rtcpfb.extra);
+
+    rtcpfbList->PushEntry(ss.str(), static_cast<SdpRtcpFbAttributeList::Type>(feedbackType), parameter, extra);
+  }
+
+  SetAttribute(rtcpfbList.release());
+}
+
+void
 RsdparsaSdpAttributeList::LoadImageattr(RustAttributeList* attributeList)
 {
   size_t numImageattrs = sdp_get_imageattr_count(attributeList);
   if (numImageattrs == 0) {
     return;
   }
   auto rustImageattrs = MakeUnique<StringView[]>(numImageattrs);
   sdp_get_imageattrs(attributeList, numImageattrs, rustImageattrs.get());
--- a/media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.h
+++ b/media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.h
@@ -130,16 +130,17 @@ private:
   void LoadFmtp(RustAttributeList* attributeList);
   void LoadPtime(RustAttributeList* attributeList);
   void LoadFlags(RustAttributeList* attributeList);
   void LoadMid(RustAttributeList* attributeList);
   void LoadMsid(RustAttributeList* attributeList);
   void LoadMsidSemantics(RustAttributeList* attributeList);
   void LoadGroup(RustAttributeList* attributeList);
   void LoadRtcp(RustAttributeList* attributeList);
+  void LoadRtcpFb(RustAttributeList* attributeList);
   void LoadImageattr(RustAttributeList* attributeList);
   void LoadSctpmaps(RustAttributeList* attributeList);
   void LoadDirection(RustAttributeList* attributeList);
   void LoadRemoteCandidates(RustAttributeList* attributeList);
   void LoadRids(RustAttributeList* attributeList);
   void LoadExtmap(RustAttributeList* attributeList);
 
   void WarnAboutMisplacedAttribute(SdpAttribute::AttributeType type,
--- a/media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h
+++ b/media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h
@@ -89,16 +89,23 @@ struct RustSdpAttributeSsrc {
 
 struct RustSdpAttributeRtpmap {
   uint8_t payloadType;
   StringView codecName;
   uint32_t frequency;
   uint32_t channels;
 };
 
+struct RustSdpAttributeRtcpFb {
+  uint32_t payloadType;
+  uint32_t feedbackType;
+  StringView parameter;
+  StringView extra;
+};
+
 struct RustSdpAttributeFmtp {
   uint8_t payloadType;
   StringView codecName;
   StringVec* tokens;
 };
 
 struct RustSdpAttributeFlags {
   bool iceLite;
@@ -254,16 +261,21 @@ void sdp_get_msid_semantics(const RustAt
 
 size_t sdp_get_group_count(const RustAttributeList* aList);
 nsresult sdp_get_groups(const RustAttributeList* aList, size_t listSize,
                         RustSdpAttributeGroup* ret);
 
 nsresult sdp_get_rtcp(const RustAttributeList* aList,
                       RustSdpAttributeRtcp* ret);
 
+
+size_t sdp_get_rtcpfb_count(const RustAttributeList* aList);
+void sdp_get_rtcpfbs(const RustAttributeList* aList, size_t listSize,
+                     RustSdpAttributeRtcpFb* ret);
+
 size_t sdp_get_imageattr_count(const RustAttributeList* aList);
 void sdp_get_imageattrs(const RustAttributeList* aList, size_t listSize,
                         StringView* ret);
 
 size_t sdp_get_sctpmap_count(const RustAttributeList* aList);
 void sdp_get_sctpmaps(const RustAttributeList* aList, size_t listSize,
                       RustSdpAttributeSctpmap* ret);
 
--- a/media/webrtc/signaling/src/sdp/SdpAttribute.cpp
+++ b/media/webrtc/signaling/src/sdp/SdpAttribute.cpp
@@ -1095,16 +1095,17 @@ const char* SdpRtcpFbAttributeList::sli 
 const char* SdpRtcpFbAttributeList::rpsi = "rpsi";
 const char* SdpRtcpFbAttributeList::app = "app";
 
 const char* SdpRtcpFbAttributeList::fir = "fir";
 const char* SdpRtcpFbAttributeList::tmmbr = "tmmbr";
 const char* SdpRtcpFbAttributeList::tstr = "tstr";
 const char* SdpRtcpFbAttributeList::vbcm = "vbcm";
 
+
 void
 SdpRtcpFbAttributeList::Serialize(std::ostream& os) const
 {
   for (auto i = mFeedbacks.begin(); i != mFeedbacks.end(); ++i) {
     os << "a=" << mType << ":" << i->pt << " " << i->type;
     if (i->parameter.length()) {
       os << " " << i->parameter;
       if (i->extra.length()) {
--- a/media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs
+++ b/media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs
@@ -1,16 +1,23 @@
 use std::net::IpAddr;
 use std::str::FromStr;
 use std::fmt;
 
 use SdpType;
 use error::SdpParserInternalError;
 use network::{parse_nettype, parse_addrtype, parse_unicast_addr};
 
+
+#[derive(Clone)]
+pub enum SdpAttributePayloadType {
+    PayloadType(u8),
+    Wildcard, // Wildcard means "*",
+}
+
 #[derive(Clone)]
 pub enum SdpAttributeCandidateTransport {
     Udp,
     Tcp,
 }
 
 #[derive(Clone)]
 pub enum SdpAttributeCandidateType {
@@ -178,20 +185,32 @@ impl SdpAttributeRtcp {
     }
 
     fn set_addr(&mut self, addr: IpAddr) {
         self.unicast_addr = Some(addr)
     }
 }
 
 #[derive(Clone)]
+pub enum SdpAttributeRtcpFbType {
+    Ack = 0,
+    Ccm = 2, // This is explicitly 2 to make the conversion to the
+             // enum used in the glue-code possible. The glue code has "app"
+             // in the place of 1
+    Nack,
+    TrrInt,
+    Remb
+}
+
+#[derive(Clone)]
 pub struct SdpAttributeRtcpFb {
-    pub payload_type: u32,
-    // TODO parse this and use an enum instead?
-    pub feedback_type: String,
+    pub payload_type: SdpAttributePayloadType,
+    pub feedback_type: SdpAttributeRtcpFbType,
+    pub parameter: String,
+    pub extra: String,
 }
 
 #[derive(Clone)]
 pub enum SdpAttributeDirection {
     Recvonly,
     Sendonly,
     Sendrecv,
 }
@@ -565,16 +584,24 @@ fn string_or_empty(to_parse: &str) -> Re
     if to_parse.is_empty() {
         Err(SdpParserInternalError::Generic("This attribute is required to have a value"
                                                 .to_string()))
     } else {
         Ok(to_parse.to_string())
     }
 }
 
+fn parse_payload_type(to_parse: &str) -> Result<SdpAttributePayloadType, SdpParserInternalError>
+{
+    Ok(match to_parse {
+             "*" => SdpAttributePayloadType::Wildcard,
+             _ => SdpAttributePayloadType::PayloadType(to_parse.parse::<u8>()?)
+         })
+}
+
 fn parse_sctp_port(to_parse: &str) -> Result<SdpAttribute, SdpParserInternalError> {
     let port = to_parse.parse()?;
     if port > 65535 {
         return Err(SdpParserInternalError::Generic(format!("Sctpport port {} can only be a bit 16bit number",
                                                            port)));
     }
     Ok(SdpAttribute::SctpPort(port))
 }
@@ -936,27 +963,116 @@ fn parse_rtcp(to_parse: &str) -> Result<
                 }
             };
         }
     };
     Ok(SdpAttribute::Rtcp(rtcp))
 }
 
 fn parse_rtcp_fb(to_parse: &str) -> Result<SdpAttribute, SdpParserInternalError> {
-    let tokens: Vec<&str> = to_parse.splitn(2, ' ').collect();
+    let tokens: Vec<&str> = to_parse.splitn(4,' ').collect();
+
+    // Parse this in advance to use it later in the parameter switch
+    let feedback_type = match tokens.get(1) {
+        Some(x) => match x.as_ref(){
+            "ack" => SdpAttributeRtcpFbType::Ack,
+            "ccm" => SdpAttributeRtcpFbType::Ccm,
+            "nack" => SdpAttributeRtcpFbType::Nack,
+            "trr-int" => SdpAttributeRtcpFbType::TrrInt,
+            "goog-remb" => SdpAttributeRtcpFbType::Remb,
+            _ => {
+                return Err(SdpParserInternalError::Unsupported(
+                    format!("Unknown rtcpfb feedback type: {:?}",x).to_string()
+                ))
+            }
+        },
+        None => {
+            return Err(SdpParserInternalError::Generic(
+                           "Error parsing rtcpfb: no feedback type".to_string(),
+                       ))
+        }
+    };
+
+    // Parse this in advance to make the initilization block below better readable
+    let parameter = match &feedback_type {
+        &SdpAttributeRtcpFbType::Ack => match tokens.get(2) {
+            Some(x) => match x.as_ref() {
+                "rpsi" | "app"  => x.to_string(),
+                _ => {
+                    return Err(SdpParserInternalError::Unsupported(
+                        format!("Unknown rtcpfb ack parameter: {:?}",x).to_string()
+                    ))
+                },
+            }
+            None => {
+                return Err(SdpParserInternalError::Unsupported(
+                    format!("The rtcpfb ack feeback type needs a parameter:").to_string()
+                ))
+            }
+        },
+        &SdpAttributeRtcpFbType::Ccm => match tokens.get(2) {
+            Some(x) => match x.as_ref() {
+                "fir" | "tmmbr" | "tstr" | "vbcm"  => x.to_string(),
+                _ => {
+                    return Err(SdpParserInternalError::Unsupported(
+                        format!("Unknown rtcpfb ccm parameter: {:?}",x).to_string()
+                    ))
+                },
+            }
+            None => "".to_string(),
+        },
+        &SdpAttributeRtcpFbType::Nack => match tokens.get(2) {
+            Some(x) => match x.as_ref() {
+                "sli" | "pli" | "rpsi" | "app"  => x.to_string(),
+                _ => {
+                    return Err(SdpParserInternalError::Unsupported(
+                        format!("Unknown rtcpfb nack parameter: {:?}",x).to_string()
+                    ))
+                },
+            }
+            None => "".to_string(),
+        },
+        &SdpAttributeRtcpFbType::TrrInt => match tokens.get(2) {
+            Some(x) => match x {
+                _ if x.parse::<u32>().is_ok() => x.to_string(),
+                _ => {
+                    return Err(SdpParserInternalError::Generic(
+                        format!("Unknown rtcpfb trr-int parameter: {:?}",x).to_string()
+                    ))
+                },
+            }
+            None => {
+                    return Err(SdpParserInternalError::Generic(
+                        format!("The rtcpfb trr-int feedback type needs a parameter").to_string()
+                    ))
+            }
+        },
+        &SdpAttributeRtcpFbType::Remb => match tokens.get(2) {
+            Some(x) => match x {
+                _ => {
+                    return Err(SdpParserInternalError::Unsupported(
+                        format!("Unknown rtcpfb remb parameter: {:?}",x).to_string()
+                    ))
+                },
+            }
+            None => "".to_string(),
+        }
+    };
+
+
     Ok(SdpAttribute::Rtcpfb(SdpAttributeRtcpFb {
-                                // TODO limit this to dymaic PTs
-                                payload_type: tokens[0].parse::<u32>()?,
-                                feedback_type: match tokens.get(1) {
+                                payload_type: parse_payload_type(tokens[0])?,
+
+                                feedback_type: feedback_type,
+
+                                parameter: parameter,
+
+                                extra: match tokens.get(3) {
                                     Some(x) => x.to_string(),
-                                    None => {
-                                        return Err(SdpParserInternalError::Generic(
-                                                       "Error parsing rtcpfb".to_string(),
-                                                   ))
-                                    }
+                                    None => "".to_string(),
                                 },
                             }))
 }
 
 fn parse_sctpmap(to_parse: &str) -> Result<SdpAttribute, SdpParserInternalError> {
     let tokens: Vec<&str> = to_parse.split_whitespace().collect();
     if tokens.len() != 3 {
         return Err(SdpParserInternalError::Generic("Sctpmap needs to have three tokens"
--- a/media/webrtc/signaling/src/sdp/rsdparsa/src/media_type.rs
+++ b/media/webrtc/signaling/src/sdp/rsdparsa/src/media_type.rs
@@ -355,16 +355,18 @@ pub fn parse_media_vector(lines: &[SdpLi
         _ => {
             return Err(SdpParserError::Sequence {
                            message: "first line in media section needs to be a media line"
                                .to_string(),
                            line_number: lines[0].line_number,
                        })
         }
     };
+
+
     for line in lines.iter().skip(1) {
         match line.sdp_type {
             SdpType::Connection(ref c) => {
                 sdp_media
                     .set_connection(c)
                     .map_err(|e: SdpParserInternalError| {
                                  SdpParserError::Sequence {
                                      message: format!("{}", e),
@@ -403,12 +405,14 @@ pub fn parse_media_vector(lines: &[SdpLi
                            })
             }
 
             // the line parsers throw unsupported errors for these already
             SdpType::Information(_) |
             SdpType::Key(_) => (),
         };
     }
+
     media_sections.push(sdp_media);
+
     Ok(media_sections)
 }
 // TODO add unit tests for parse_media_vector
--- a/media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs
+++ b/media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs
@@ -1,18 +1,19 @@
 use std::slice;
 use libc::{size_t, uint8_t, uint16_t, uint32_t, int64_t};
 
 use rsdparsa::SdpSession;
-use rsdparsa::attribute_type::{SdpAttribute, SdpAttributeFingerprint, SdpAttributeSetup, SdpAttributeSsrc, SdpAttributeRtpmap, SdpAttributeMsid, SdpAttributeMsidSemantic, SdpAttributeGroupSemantic, SdpAttributeGroup, SdpAttributeRtcp, SdpAttributeSctpmap, SdpAttributeRemoteCandidate, SdpAttributeExtmap, SdpAttributeDirection};
+use rsdparsa::attribute_type::{SdpAttribute, SdpAttributePayloadType, SdpAttributeFingerprint, SdpAttributeSetup, SdpAttributeSsrc, SdpAttributeRtpmap, SdpAttributeMsid, SdpAttributeMsidSemantic, SdpAttributeGroupSemantic, SdpAttributeGroup, SdpAttributeRtcp, SdpAttributeRtcpFb, SdpAttributeSctpmap, SdpAttributeRemoteCandidate, SdpAttributeExtmap, SdpAttributeDirection};
 use nserror::{nsresult, NS_OK, NS_ERROR_INVALID_ARG};
 
 use types::StringView;
 use network::RustIpAddr;
 
+
 #[repr(C)]
 #[derive(Clone, Copy, PartialEq, Eq)]
 pub enum RustSdpAttributeType {
     BundleOnly,
     Candidate,
     EndOfCandidates,
     Extmap,
     Fingerprint,
@@ -565,16 +566,57 @@ pub unsafe extern "C" fn sdp_get_rtcp(at
     let attr = get_attribute((*attributes).as_slice(), RustSdpAttributeType::Rtcp);
     if let Some(&SdpAttribute::Rtcp(ref data)) = attr {
         *ret = RustSdpAttributeRtcp::from(data);
         return NS_OK;
     }
     NS_ERROR_INVALID_ARG
 }
 
+
+#[repr(C)]
+#[derive(Clone, Copy)]
+pub struct RustSdpAttributeRtcpFb {
+    pub payload_type: u32,
+    pub feedback_type: u32,
+    pub parameter: StringView,
+    pub extra: StringView
+}
+
+impl<'a> From<&'a SdpAttributeRtcpFb> for RustSdpAttributeRtcpFb {
+    fn from(other: &SdpAttributeRtcpFb) -> Self {
+        RustSdpAttributeRtcpFb {
+            payload_type: match other.payload_type{
+                SdpAttributePayloadType::Wildcard => u32::max_value(),
+                SdpAttributePayloadType::PayloadType(x) => x as u32,
+            },
+            feedback_type: other.feedback_type.clone() as u32,
+            parameter: StringView::from(other.parameter.as_str()),
+            extra: StringView::from(other.extra.as_str()),
+        }
+    }
+}
+
+#[no_mangle]
+pub unsafe extern "C" fn sdp_get_rtcpfb_count(attributes: *const Vec<SdpAttribute>) -> size_t {
+    count_attribute((*attributes).as_slice(), RustSdpAttributeType::Rtcpfb)
+}
+
+#[no_mangle]
+pub unsafe extern "C" fn sdp_get_rtcpfbs(attributes: *const Vec<SdpAttribute>, ret_size: size_t, ret_rtcpfbs: *mut RustSdpAttributeRtcpFb) {
+    let attrs: Vec<_> = (*attributes).iter().filter_map(|x| if let SdpAttribute::Rtcpfb(ref data) = *x {
+        Some(RustSdpAttributeRtcpFb::from(data))
+    } else {
+        None
+    }).collect();
+    let rtcpfbs = slice::from_raw_parts_mut(ret_rtcpfbs, ret_size);
+    rtcpfbs.clone_from_slice(attrs.as_slice());
+}
+
+
 #[no_mangle]
 pub unsafe extern "C" fn sdp_get_imageattr_count(attributes: *const Vec<SdpAttribute>) -> size_t {
     count_attribute((*attributes).as_slice(), RustSdpAttributeType::ImageAttr)
 }
 
 #[no_mangle]
 pub unsafe extern "C" fn sdp_get_imageattrs(attributes: *const Vec<SdpAttribute>, ret_size: size_t, ret_groups: *mut StringView) {
     let attrs: Vec<_> = (*attributes).iter().filter_map(|x| if let SdpAttribute::ImageAttr(ref string) = *x {