Support Fmtp parsing in Rust SDP Parser

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
Rank:
25
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: dminor, Assigned: johannes.willbold)

Tracking

(Blocks 1 bug)

58 Branch
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Reporter)

Updated

a year ago
Rank: 25

Updated

11 months ago
Assignee: nobody → johannes.willbold
Comment hidden (mozreview-request)
(Reporter)

Comment 2

11 months ago
mozreview-review
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.
Attachment #8983135 - Flags: review?(dminor)

Comment 3

11 months ago
mozreview-review
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.
Attachment #8983135 - Flags: review?(mfroman) → review+
(Assignee)

Comment 4

11 months ago
mozreview-review
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.
(Assignee)

Comment 5

11 months ago
mozreview-review-reply
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

11 months ago
mozreview-review
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
Attachment #8983135 - Flags: review?(docfaraday) → review-
(Assignee)

Comment 7

11 months ago
mozreview-review-reply
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()
(Assignee)

Comment 8

11 months ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 10

11 months ago
mozreview-review
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/
Attachment #8983135 - Flags: review?(docfaraday) → review+
Comment hidden (mozreview-request)
(Reporter)

Comment 12

11 months ago
mozreview-review
Comment on attachment 8983135 [details]
Bug 1432918: Added fmtp parsing in rust.

https://reviewboard.mozilla.org/r/248968/#review255968
Attachment #8983135 - Flags: review?(dminor) → review+

Comment 13

11 months ago
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 hidden (mozreview-request)

Comment 15

11 months ago
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 hidden (mozreview-request)

Comment 17

10 months ago
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/1b80f6d8ea65
Added fmtp parsing in rust. r=bwc,dminor,mjf

Comment 18

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1b80f6d8ea65
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.