1432918 - Support Fmtp parsing in Rust SDP Parser

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, enhancement, P3)

Comment 1

[Johannes Willbold]

Attachment: Bug 1432918: Added fmtp parsing in rust.

Added C++/Rust glue code
Added a u8 vector view in the C++/Rust glue code
Added fmtp attribute parsing in rust
Added the option to check for warnings that appeared in the rust parsing
Changed gtest CheckRedEmptyFmtp to check for errors in case of sipcc and check for warnings in case of rust
Extended rust fmtp testcases in rust

Review commit: https://reviewboard.mozilla.org/r/248968/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248968/

Comment 2

[Dan Minor [:dminor]]

Comment on attachment 8983135 [details]
Bug 1432918: Added fmtp parsing in rust.

https://reviewboard.mozilla.org/r/248968/#review255414

I'd like to see the big Fmtp parameter structures cleaned up so that we only see parameters for each codec type instead of having them all mixed together if at all possible. I've only done a bit of Rust, so please let me know if my suggestion to use an enum is not practical.

::: media/webrtc/signaling/gtest/sdp_unittests.cpp:2518
(Diff revision 1)
>    if (::testing::get<0>(GetParam())) {
>      ParseSdp(kVideoWithRedAndUlpfecSdp + "a=fmtp:122" CRLF);
>    } else {
>      ParseSdp(kVideoWithRedAndUlpfecSdp + "a=fmtp:122" CRLF, false);
> +
> +    if (::testing::get<1>(GetParam())) {

Please add a function e.g. IsParsingWithSipccParser that does this test, so we don't have to rely upon comments to remember which branch is which in the future.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:728
(Diff revision 1)
> +      h264Parameters.max_dpb = rustFmtpParameters.max_dpb;
> +      h264Parameters.max_br = rustFmtpParameters.max_br;
> +
> +      fmtpParameters.reset(new SdpFmtpAttributeList::H264Parameters(Move(h264Parameters)));
> +    }
> +    else if(codecName == "OPUS"){

nit: here and below please place else if on the same line as the close }.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:734
(Diff revision 1)
> +      SdpFmtpAttributeList::OpusParameters opusParameters;
> +
> +      opusParameters.maxplaybackrate = rustFmtpParameters.maxplaybackrate;
> +      opusParameters.stereo = rustFmtpParameters.stereo;
> +      opusParameters.useInBandFec = rustFmtpParameters.useinbandfec;
> +

nit: please remove extra whitespace

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:740
(Diff revision 1)
> +
> +      fmtpParameters.reset(new SdpFmtpAttributeList::OpusParameters(Move(opusParameters)));
> +    }
> +    else if((codecName == "VP8") || (codecName == "VP9")){
> +      SdpFmtpAttributeList::VP8Parameters vp8Parameters(codecName == "VP8" ?
> +                                                          SdpRtpmapAttributeList::kVP8 :

nit: please align this line and the following line with codecName

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:98
(Diff revision 1)
>    StringView codecName;
>    uint32_t frequency;
>    uint32_t channels;
>  };
>  
> +struct RustSdpAttributeFmtpParameters {

If you use an enum with struct variants like I suggest for the Rust code, then perhaps you could use inheritance for parameters like here [1] or a C union here to avoid having all of the parameters jumbled together in one struct.

[1] https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/sdp/SdpAttribute.h#1242

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:102
(Diff revision 1)
>  
> +struct RustSdpAttributeFmtpParameters {
> +  // H264
> +  // pub static const max_sprop_len: u32 = 128,
> +  // pub sprop_parameter_sets: [u8, max_sprop_len],
> +  // pub sprop_parameter_sets

Please either use or remove the commented out members here and elsewhere in the struct.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:132
(Diff revision 1)
> +
> +  // telephone-event
> +  StringView dtmf_tones;
> +
> +  // Red codecs
> +  // This is acutally not a string, the StringView just points on an array of u8 elements

typo: s/acutally/actually/

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:210
(Diff revision 1)
>      pub extension_attributes: Option<String>,
>  }
>  
> +
> +#[derive(Clone)]
> +pub struct SdpAttributeFmtpParameters {

I think this would be cleaner if you used a enum with struct variants like the "Click" variant at [1] for each codec type.

[1] https://rustbyexample.com/custom_types/enum.html

::: media/webrtc/signaling/src/sdp/rsdparsa/src/error.rs:7
(Diff revision 1)
>  use std::net::AddrParseError;
>  use std::fmt;
>  use std::error;
>  use std::error::Error;
>  
> -#[derive(Debug)]
> +#[derive(Debug,Clone)]

Please remover Debug if possible.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/error.rs:99
(Diff revision 1)
>                 "IP address parsing error: invalid IP address syntax");
>      assert_eq!(addr_err.description(), "invalid IP address syntax");
>      assert!(!addr_err.cause().is_none());
>  }
>  
> -#[derive(Debug)]
> +#[derive(Debug,Clone)]

Please remove Debug if possible.

Comment 3

[Michael Froman [:mjf]]

Comment on attachment 8983135 [details]
Bug 1432918: Added fmtp parsing in rust.

https://reviewboard.mozilla.org/r/248968/#review255464

r+ after fixing nits.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:718
(Diff revision 1)
> +
> +    if(codecName == "H264"){
> +      SdpFmtpAttributeList::H264Parameters h264Parameters;
> +
> +      h264Parameters.packetization_mode = rustFmtpParameters.packetization_mode;
> +      h264Parameters.level_asymmetry_allowed = rustFmtpParameters.level_asymmetry_allowed;

Mozilla c++ coding standard is 80 chars max.  Please make sure to fix all your new code or lines you touched.  For reference: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#C.2FC.2B.2B_practices

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:766
(Diff revision 1)
> +    using std::to_string;
> +
> +    fmtpList->PushEntry(to_string(payloadType), Move(fmtpParameters));

If this is the only usage of std::to_string, drop the using and use std::to_string.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpParser.cpp:51
(Diff revision 1)
>    sdp::AddrType addrType = convertAddressType(rustOrigin.addr.addrType);
>    SdpOrigin origin(convertStringView(rustOrigin.username),
>                     rustOrigin.sessionId, rustOrigin.sessionVersion,
>                     addrType, std::string(rustOrigin.addr.unicastAddr));
> +
> +  if(err != nullptr) {

if (err) instead of if(err!= nullptr)

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs:5
(Diff revision 1)
>  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, SdpAttributeFingerprint, SdpAttributeSetup, SdpAttributeSsrc, SdpAttributeRtpmap, SdpAttributeFmtpParameters, SdpAttributeMsid, SdpAttributeMsidSemantic, SdpAttributeGroupSemantic, SdpAttributeGroup, SdpAttributeRtcp, SdpAttributeSctpmap, SdpAttributeRemoteCandidate, SdpAttributeExtmap, SdpAttributeDirection};

I think our rust coding comes from here: https://github.com/rust-lang-nursery/fmt-rfcs/blob/master/guide/guide.md
According to that link our max line length is 100 chars.

Comment 4

[Johannes Willbold]

Comment on attachment 8983135 [details]
Bug 1432918: Added fmtp parsing in rust.

https://reviewboard.mozilla.org/r/248968/#review255512

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:210
(Diff revision 1)
>      pub extension_attributes: Option<String>,
>  }
>  
> +
> +#[derive(Clone)]
> +pub struct SdpAttributeFmtpParameters {

I agree that this would be clear but this would either require a massive rewrite of the parser, as all functions in the parser are designed to be absolutly stateless, or the usage of "unsafe" to make use of mutable static/global variables.

The Problem is that the parser only has the payload_type in that function and it would require the informations from the rtpmap attributes to find out to which codec these fmtp parameters actually belong to.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/error.rs:7
(Diff revision 1)
>  use std::net::AddrParseError;
>  use std::fmt;
>  use std::error;
>  use std::error::Error;
>  
> -#[derive(Debug)]
> +#[derive(Debug,Clone)]

It is required.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/error.rs:99
(Diff revision 1)
>                 "IP address parsing error: invalid IP address syntax");
>      assert_eq!(addr_err.description(), "invalid IP address syntax");
>      assert!(!addr_err.cause().is_none());
>  }
>  
> -#[derive(Debug)]
> +#[derive(Debug,Clone)]

It is required.

Comment 5

[Johannes Willbold]

Comment on attachment 8983135 [details]
Bug 1432918: Added fmtp parsing in rust.

https://reviewboard.mozilla.org/r/248968/#review255414

> If you use an enum with struct variants like I suggest for the Rust code, then perhaps you could use inheritance for parameters like here [1] or a C union here to avoid having all of the parameters jumbled together in one struct.
> 
> [1] https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/sdp/SdpAttribute.h#1242

I really tried to do that, please see my comment on the rust struct about that.

> I think this would be cleaner if you used a enum with struct variants like the "Click" variant at [1] for each codec type.
> 
> [1] https://rustbyexample.com/custom_types/enum.html

I agree that this would be cleaner but this would either require a massive rewrite of the parser, as all functions in the parser are designed to be absolutly stateless, or the usage of "unsafe" to make use of mutable static/global variables.

The Problem is that the parser has only the payload_type in that function and it would require the informations from the rtpmap attributes to find out to which codec these fmtp parameters belong to.

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8983135 [details]
Bug 1432918: Added fmtp parsing in rust.

https://reviewboard.mozilla.org/r/248968/#review255476

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:706
(Diff revision 1)
> -  size_t numValidFmtp = sdp_get_fmtp(attributeList, numFmtp,
> -                                        rustFmtps.get());
> +  size_t numValidFmtp = sdp_get_fmtp(attributeList, numFmtp,rustFmtps.get());
> +  auto fmtpList = MakeUnique<SdpFmtpAttributeList>();
>    for(size_t i = 0; i < numValidFmtp; i++) {
> -    RustSdpAttributeFmtp& fmtp = rustFmtps[i];
> +    const RustSdpAttributeFmtp& fmtp = rustFmtps[i];
> +    uint8_t payloadType = fmtp.payloadType;
> +    auto codecName = convertStringView(fmtp.codecName);

Let's not use auto with convertStringView.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:715
(Diff revision 1)
> +      SdpFmtpAttributeList::H264Parameters h264Parameters;
> +
> +      h264Parameters.packetization_mode = rustFmtpParameters.packetization_mode;
> +      h264Parameters.level_asymmetry_allowed = rustFmtpParameters.level_asymmetry_allowed;
> +      h264Parameters.profile_level_id = rustFmtpParameters.profile_level_id;
> +      h264Parameters.max_mbps = rustFmtpParameters.max_mbps;
> +      h264Parameters.max_fs = rustFmtpParameters.max_fs;
> +      h264Parameters.max_cpb = rustFmtpParameters.max_cpb;
> +      h264Parameters.max_dpb = rustFmtpParameters.max_dpb;
> +      h264Parameters.max_br = rustFmtpParameters.max_br;
> +
> +      fmtpParameters.reset(new SdpFmtpAttributeList::H264Parameters(Move(h264Parameters)));

I don't see sprop-parameter-sets in here, but upon closer inspection I'm not sure we even use those deeper down. I _think_ this is the TODO for that:

https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/CodecConfig.h#113


I've filed a bug for this, if you could put a "// TODO(bug 1466859): Support sprop-parameter-sets" comment here that would be good enough for me.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:752
(Diff revision 1)
> +    else if(codecName == "TELEPHONE-EVENT"){
> +      SdpFmtpAttributeList::TelephoneEventParameters telephoneEventParameters;
> +
> +      telephoneEventParameters.dtmfTones = convertStringView(rustFmtpParameters.dtmf_tones);
> +
> +      fmtpParameters.reset(new SdpFmtpAttributeList::TelephoneEventParameters(Move(telephoneEventParameters)));

Nit: Wrap to 80 columns, here and anywhere else necessary.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:111
(Diff revision 1)
> +  uint32_t max_fs;
> +  uint32_t max_cpb;
> +  uint32_t max_dpb;
> +  uint32_t max_br;
> +  uint32_t max_mbps;
> +  uint32_t paramter_add;

We don't use this, and it is non-standard anyhow. Let's remove it and any corresponding code.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:112
(Diff revision 1)
> +  uint32_t usedtx;
> +  uint32_t stereo;
> +  uint32_t useinbandfec;
> +  uint32_t cbr;

These are all opus, not h264. We don't support usedtx or cbr right now, but we should probably leave them in the parse code.

These are also all booleans.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:113
(Diff revision 1)
> +  uint32_t stereo;
> +  uint32_t useinbandfec;

It seems I broke reviewboard; I cannot remove this comment. Please ignore.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:116
(Diff revision 1)
> +  uint32_t profile;
> +  uint32_t level;

We don't use these, and they are non-standard. Let's remove them and any corresponding code.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpParser.cpp:51
(Diff revision 1)
> +  if(err != nullptr) {
> +    size_t line = sdp_get_error_line_num(err);
> +    std::string warningMsg = convertStringView(sdp_get_error_message(err));
> +    sdp_free_error(err);
> +    AddParseWarnings(line, warningMsg);
> +  }

Let's put this right after the error handling, so we're using |err| all in one place.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:803
(Diff revision 1)
> +        max_mbps: 0,
> +        paramter_add: 0,
> +        usedtx: 0,
> +        stereo: 0,
> +        useinbandfec: 0,
> +        cbr: 1,

The spec (https://tools.ietf.org/html/rfc7587) says this is 0 by default.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:812
(Diff revision 1)
> +        maxplaybackrate: 48000,
> +        encodings: Vec::new(),
> +        dtmf_tones: "".to_string(),
> +        unknown_tokens: Vec::new(),
> +    };
> +

I would prefer that this code know from the start what codec is it dealing with, to minimize these inferences, and to allow for stricter parsing. For instance, you never know when some key/value pair that we don't support (and should ignore) will contain a '/' in the value. How hard would this be? (we would need to parse all the rtpmaps, and also know about the default payload types, and _then_ parse fmtp)

If it is not practical to do this, let's first look for an '=', and if we find one, assume we're dealing with the ';' delimited list of key=value pairs. If we do not find '=', then look for RED or telephone-event.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:821
(Diff revision 1)
> +                    _ => ()
> +                },
> +                Err(_) => ()

These should probably be errors.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:831
(Diff revision 1)
> +        let validate_digits = |digit_option: Option<u8> | -> Option<u8> {
> +            match digit_option{
> +                Some(x) => match x {
> +                    0...70 => Some(x),
> +                    _ => None,
> +                },
> +                None => None,
> +            }
> +        };

Let's not range-check this (except to the extent that we verify it fits in a u8). This parameter is how the other side tells us which DTMF events it can receive, which may include events beyond 70.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:848
(Diff revision 1)
> +            let dtmf_tone_range: Vec<&str> = dtmf_tone.splitn(2,"-").collect();
> +
> +            dtmf_tone_is_ok = match dtmf_tone_range.len() {
> +                // In this case the dtmf tone is a range
> +                2 => {
> +                    match validate_digits(dtmf_tone_range[0].parse::<u8>().ok()) {

A question: Will parse::<u8> leave trailing bytes? For instance, what happens when we use parse::<u8> on something like "10foo" or "1024"?

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:878
(Diff revision 1)
> +        }
> +    } else  { // This is the case for regular fmtp parameters
> +        let paramter_tokens: Vec<&str> = parameter_token.split(";").collect();
> +
> +        for parameter_token in paramter_tokens.iter() {
> +            let splitted_parameter_token: Vec<&str> = parameter_token.splitn(2,"=").collect();

Minor nit: "splitted" is not a word (in modern english, anyhow). Maybe call this variable "key_value_pair" or "name_value_pair"?

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:882
(Diff revision 1)
> +        for parameter_token in paramter_tokens.iter() {
> +            let splitted_parameter_token: Vec<&str> = parameter_token.splitn(2,"=").collect();
> +
> +            if splitted_parameter_token.len() != 2 {
> +                return Err(SdpParserInternalError::Generic(
> +                    "A fmtp parameter needs to have to be either a telephone event dtmf tone or a parameter list or a red codec list".to_string()

Too many words here, I suggest s/needs to have to/must/

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:886
(Diff revision 1)
> +                return Err(SdpParserInternalError::Generic(
> +                    "A fmtp parameter needs to have to be either a telephone event dtmf tone or a parameter list or a red codec list".to_string()
> +                ))
> +            }
> +
> +            let parameter_name = splitted_parameter_token[0];

Hmm. To be on the safe side, let's make this logic case-insensitive.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:891
(Diff revision 1)
> +            let parameter_name = splitted_parameter_token[0];
> +            let parameter_val = splitted_parameter_token[1];
> +
> +            match parameter_name {
> +                // H264
> +                "profile-level-id" => parameters.profile_level_id = u32::from_str_radix(parameter_val,16)?,

Let's check that this is at most 3 bytes (< 16777216).

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:894
(Diff revision 1)
> +            match parameter_name {
> +                // H264
> +                "profile-level-id" => parameters.profile_level_id = u32::from_str_radix(parameter_val,16)?,
> +                "PROFILE" => parameters.profile = parameter_val.parse::<u32>()?,
> +                "LEVEL" => parameters.level = parameter_val.parse::<u32>()?,
> +                "packetization-mode" => parameters.packetization_mode = parameter_val.parse::<u32>()?,

We should probably verify this is 0, 1, or 2.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:899
(Diff revision 1)
> +                "packetization-mode" => parameters.packetization_mode = parameter_val.parse::<u32>()?,
> +                "level-asymmetry-allowed" => parameters.level_asymmetry_allowed = match parameter_val.parse::<u8>()? {
> +                    0 => false,
> +                    1 => true,
> +                    _ => return Err(SdpParserInternalError::Generic(
> +                        "The fmtp paremter 'level-asymmetry-allowed' needs to be 0 or 1".to_string()

Nit: s/paremter/parameter/

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:908
(Diff revision 1)
> +                "usedtx" => parameters.usedtx = parameter_val.parse::<u32>()?,
> +                "stereo" => parameters.stereo = parameter_val.parse::<u32>()?,
> +                "useinbandfec" => parameters.useinbandfec = parameter_val.parse::<u32>()?,
> +                "cbr" => parameters.cbr = parameter_val.parse::<u32>()?,

These are all 0 or 1, and we should probably check.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1356
(Diff revision 1)
>  #[test]
>  fn test_parse_attribute_fmtp() {
> -    assert!(parse_attribute("fmtp:109 maxplaybackrate=48000;stereo=1;useinbandfec=1").is_ok())
> +    assert!(parse_attribute("fmtp:66 0-15").is_ok());
> +    assert!(parse_attribute("fmtp:109 0-15,66").is_ok());
> +    assert!(parse_attribute("fmtp:66 111/115").is_ok());
> +    assert!(parse_attribute("fmtp:109 maxplaybackrate=48000;stereo=1;useinbandfec=1").is_ok());

Can we test that we are tolerant of stuff with whitespace like "maxplaybackrate=48000; stereo=1"? Some implementations do this, due to whitespace in examples in the specifications.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1360
(Diff revision 1)
> +    assert!(parse_attribute("fmtp:66 111/115").is_ok());
> +    assert!(parse_attribute("fmtp:109 maxplaybackrate=48000;stereo=1;useinbandfec=1").is_ok());
> +    assert!(parse_attribute("fmtp:8 maxplaybackrate=48000").is_ok());
> +
> +    assert!(parse_attribute("fmtp:77 ").is_err());
> +    assert!(parse_attribute("fmtp:109 111").is_ok());

Hmm. I would expect the parse code above to interpret this as a telephone-event fmtp, see the too-large value 111, and then default to "0-15" with a successful parse. Is something else happening here?

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/types.rs:74
(Diff revision 1)
> +pub unsafe extern "C" fn u8_vec_get(vec: *const Vec<u8>,
> +                                     index: size_t,
> +                                     ret: *mut uint8_t) -> nsresult {

Nit: indent needs fixup

Comment 7

[Johannes Willbold]

Comment on attachment 8983135 [details]
Bug 1432918: Added fmtp parsing in rust.

https://reviewboard.mozilla.org/r/248968/#review255476

> We don't use this, and it is non-standard anyhow. Let's remove it and any corresponding code.

Removed it from the testcase data kH264AudioVideoOffer.

> We don't use these, and they are non-standard. Let's remove them and any corresponding code.

Removed it from the testcase data kH264AudioVideoOffer.

> I would prefer that this code know from the start what codec is it dealing with, to minimize these inferences, and to allow for stricter parsing. For instance, you never know when some key/value pair that we don't support (and should ignore) will contain a '/' in the value. How hard would this be? (we would need to parse all the rtpmaps, and also know about the default payload types, and _then_ parse fmtp)
> 
> If it is not practical to do this, let's first look for an '=', and if we find one, assume we're dealing with the ';' delimited list of key=value pairs. If we do not find '=', then look for RED or telephone-event.

I would also prefer this solution, but as stated in another review comment already, this would invoke a massive rewrite of the rust parser. Currently all functions are designed to be completly stateless.

Fixed the second part of that comment.

> Let's not range-check this (except to the extent that we verify it fits in a u8). This parameter is how the other side tells us which DTMF events it can receive, which may include events beyond 70.

The following test fail for an unlimited range (0..256):

[  FAILED  ] RoundTripSerialize/NewSdpTest.CheckTelephoneEventIncludingCommas/0, where GetParam() = (false, false)
[  FAILED  ] RoundTripSerialize/NewSdpTest.CheckTelephoneEventIncludingCommas/2, where GetParam() = (true, false)
[  FAILED  ] RoundTripSerialize/NewSdpTest.CheckTelephoneEventComplexEvents/0, where GetParam() = (false, false)
[  FAILED  ] RoundTripSerialize/NewSdpTest.CheckTelephoneEventComplexEvents/2, where GetParam() = (true, false)
[  FAILED  ] RoundTripSerialize/NewSdpTest.CheckTelephoneEventNoHyphen/0, where GetParam() = (false, false)
[  FAILED  ] RoundTripSerialize/NewSdpTest.CheckTelephoneEventNoHyphen/2, where GetParam() = (true, false)
[  FAILED  ] RoundTripSerialize/NewSdpTest.CheckTelephoneEventOnlyOne/0, where GetParam() = (false, false)
[  FAILED  ] RoundTripSerialize/NewSdpTest.CheckTelephoneEventOnlyOne/2, where GetParam() = (true, false)
[  FAILED  ] RoundTripSerialize/NewSdpTest.CheckFormatParameters/0, where GetParam() = (false, false)
[  FAILED  ] RoundTripSerialize/NewSdpTest.CheckFormatParameters/2, where GetParam() = (true, false)

Limiting the range to 0..100 is fine.
What shall I do here?

> A question: Will parse::<u8> leave trailing bytes? For instance, what happens when we use parse::<u8> on something like "10foo" or "1024"?

No parse::<u8> will not leave them, it will output a ParseIntError of kind: InvalidDigit (or of kind: Overflow for 1024)

> Minor nit: "splitted" is not a word (in modern english, anyhow). Maybe call this variable "key_value_pair" or "name_value_pair"?

nice to know :D Fixed

> Hmm. I would expect the parse code above to interpret this as a telephone-event fmtp, see the too-large value 111, and then default to "0-15" with a successful parse. Is something else happening here?

No you are right, this should be .is_err()

Comment 8

[Johannes Willbold]

Comment on attachment 8983135 [details]
Bug 1432918: Added fmtp parsing in rust.

https://reviewboard.mozilla.org/r/248968/#review255414

Please check my comment on this strcuture below. I guess it would be practical but the fmtp parsing function would need to know to which codec these parameters belong to, which is defined in the rtpmap attributes. So not only would the fmtp parsing function be required to know things rtpmap defines, but also the entire paser would need to parse many parts two times, since the order in which attributes appear is undefined for media attributes. So the sdp may define all fmtp fields before any rtpmap is defined.

Comment 9

[Johannes Willbold]

Comment on attachment 8983135 [details]
Bug 1432918: Added fmtp parsing in rust.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248968/diff/1-2/

Comment 10

[Byron Campen [:bwc]]

Comment on attachment 8983135 [details]
Bug 1432918: Added fmtp parsing in rust.

https://reviewboard.mozilla.org/r/248968/#review255802

Just minor issues now.

::: media/webrtc/signaling/gtest/sdp_unittests.cpp:2643
(Diff revisions 1 - 2)
>  "c=IN IP6 ::1" CRLF
>  "a=mid:second" CRLF
>  "a=rtpmap:97 H264/90000" CRLF
>  "a=fmtp:97 profile-level-id=42a01e" CRLF
>  "a=rtpmap:98 H264/90000" CRLF
> -"a=fmtp:98 PROFILE=0;LEVEL=0;profile-level-id=42a00d;packetization-mode=1;level-asymmetry-allowed=1;max-mbps=42000;max-fs=1400;max-cpb=1000;max-dpb=1000;max-br=180000;parameter-add=1;usedtx=0;stereo=0;useinbandfec=0;cbr=0" CRLF
> +"a=fmtp:98 profile-level-id=42a00d;packetization-mode=1;level-asymmetry-allowed=1;max-mbps=42000;max-fs=1400;max-cpb=1000;max-dpb=1000;max-br=180000;usedtx=0;stereo=0;useinbandfec=0;cbr=0" CRLF

Let's leave these in to verify that we don't barf when we encounter a parameter we don't support.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:727
(Diff revisions 1 - 2)
>        h264Parameters.max_mbps = rustFmtpParameters.max_mbps;
>        h264Parameters.max_fs = rustFmtpParameters.max_fs;
>        h264Parameters.max_cpb = rustFmtpParameters.max_cpb;
>        h264Parameters.max_dpb = rustFmtpParameters.max_dpb;
>        h264Parameters.max_br = rustFmtpParameters.max_br;
>  

Let's put a TODO for sprop-parameter-sets here also.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:115
(Diff revisions 1 - 2)
> -  // pub max_fs: u32,  // already defined in H264
> +  // max_fs, already defined in H264
>    uint32_t max_fr;
>  
>    // Opus
>    uint32_t maxplaybackrate;
> -  // uint32_t stereo, // already defined in H264
> +  // stereo, already defined in H264

Remove this comment.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:230
(Diff revisions 1 - 2)
> -    // pub max_fs: u32,  // already defined in H264
> +    // max_fs, already defined in H264
>      pub max_fr: u32,
>  
>      // Opus
>      pub maxplaybackrate: u32,
> -    // pub stereo: u32, // already defined in H264
> +    // stereo, already defined in H264

Remove this comment.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:853
(Diff revisions 1 - 2)
> +                "USEDTX" => parameters.usedtx = match parameter_val.parse::<u8>()? {
> +                    0 => false,
> +                    1 => true,
> +                    _ => return Err(SdpParserInternalError::Generic(
> +                        "The fmtp parameter 'usedtx' needs to be 0 or 1".to_string()
> +                    ))

It might be nice to have a function that we can reuse for these.

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs:344
(Diff revisions 1 - 2)
> -    // pub max_fs: u32,  // already defined in H264
> +    // max_fs, already defined in H264
>      pub max_fr: uint32_t,
>  
>      // Opus
>      pub maxplaybackrate: uint32_t,
> -    // pub stereo: uint32_t, // already defined in H264
> +    // stereo, already defined in H264

Remove this comment.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:806
(Diff revision 2)
> +        dtmf_tones: "".to_string(),
> +        unknown_tokens: Vec::new(),
> +    };
> +
> +    if parameter_token.contains("=") {
> +        let paramter_tokens: Vec<&str> = parameter_token.split(";").collect();

Nit: s/paramter/parameter/

Comment 11

[Johannes Willbold]

Comment on attachment 8983135 [details]
Bug 1432918: Added fmtp parsing in rust.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248968/diff/2-3/

Comment 12

[Dan Minor [:dminor]]

Comment on attachment 8983135 [details]
Bug 1432918: Added fmtp parsing in rust.

https://reviewboard.mozilla.org/r/248968/#review255968

Comment 13

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 5b8f205b99ecc1183d29f3f24f512048a3df5d76 -d 674d58c3a8f3: rebasing 467556:5b8f205b99ec "Bug 1432918: Added fmtp parsing in rust. r=bwc,dminor,mjf" (tip)
merging media/webrtc/signaling/gtest/sdp_unittests.cpp
merging media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp
merging media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h
merging media/webrtc/signaling/src/sdp/RsdparsaSdpParser.cpp
merging media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs
merging media/webrtc/signaling/src/sdp/rsdparsa/src/lib.rs
merging media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs
warning: conflicts while merging media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h! (edit, then use 'hg resolve --mark')
warning: conflicts while merging media/webrtc/signaling/src/sdp/RsdparsaSdpParser.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs! (edit, then use 'hg resolve --mark')
warning: conflicts while merging media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 14

[Johannes Willbold]

Comment on attachment 8983135 [details]
Bug 1432918: Added fmtp parsing in rust.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248968/diff/3-4/

Comment 15

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s cc05d56525504fc5d0bc34089db1e3ead9e315b6 -d 8d32de537989: rebasing 467570:cc05d5652550 "Bug 1432918: Added fmtp parsing in rust. r=bwc,dminor,mjf" (tip)
merging media/webrtc/signaling/gtest/sdp_unittests.cpp
merging media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h
merging media/webrtc/signaling/src/sdp/rsdparsa/src/lib.rs
merging media/webrtc/signaling/src/sdp/rsdparsa_capi/src/types.rs
warning: conflicts while merging media/webrtc/signaling/src/sdp/rsdparsa_capi/src/types.rs! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 16

[Johannes Willbold]

Comment on attachment 8983135 [details]
Bug 1432918: Added fmtp parsing in rust.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248968/diff/4-5/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b80f6d8ea65
Added fmtp parsing in rust. r=bwc,dminor,mjf

Comment 18

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/1b80f6d8ea65