Bug 1432922: Implemented parsing support for rtcpfb-wildcard. r=bwc,dminor draft
authorJohannes Willbold <j.willbold@mozilla.com>
Tue, 29 May 2018 16:32:52 -0700
changeset 802318 97fdfda9134197381d16e0a61dda5357bba9e9da
parent 800874 f01bb6245db1ea2a87e5360104a4110571265137
push id111862
push userbmo:johannes.willbold@rub.de
push dateThu, 31 May 2018 18:01:50 +0000
reviewersbwc, dminor
bugs1432922
milestone62.0a1
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 {