1432932 - Rust SDP Parser fails to produce an error on NewSdpTest.ParseInvalidSimulcastNoSuchRecvRid

Status: RESOLVED FIXED

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

Description

[Dan Minor [:dminor]]

[ RUN      ] RoundTripSerialize/NewSdpTest.ParseInvalidSimulcastNoSuchRecvRid/0
version: 0
origin: -, 4294967296, 2, 127.0.0.1
session: SIP Call
connection: 198.51.100.7
bandwidth: CT, 5000
timing: 0, 0
media: Video, 56436, Rtp/Savpf, [120]
/home/dminor/src/firefox/media/webrtc/signaling/gtest/sdp_unittests.cpp:3753: Failure
Expected: ("") != (GetParseErrors()), actual: "" vs ""
[  FAILED  ] RoundTripSerialize/NewSdpTest.ParseInvalidSimulcastNoSuchRecvRid/0, where GetParam() = (false, false) (0 ms)

Comment 1

[Johannes Willbold]

Attachment: Bug 1432932: Sanity check for simulcast recv rids.

Added Rid attribute parsing
Added C++/Rust glue code for the rid attribute
Added SdpInternalError for ParseFloatError
Added Rust unit test for parse_rid in Rust
Added U16Vec in C++/Rust glue code.
The C++ glue code no longer parses rid attributes on his own if the rust parser is used.
Reworked Simulcast parsing in rust
Added C++/rust glue code for Simulcast
Enabled CheckSimulcast c++ unit test
Added sanity check, that checks for simulcast rids that have nt been defined
Enabled ParseInvalidSimulcastNoSuchRecvRid C++ unit test
Added comments with bug number to remove redundant parsing functions.

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

Comment 2

[Dan Minor [:dminor]]

Comment on attachment 8986310 [details]
Bug 1432932: Sanity check for simulcast recv rids.

https://reviewboard.mozilla.org/r/251680/#review258214

Looks good to me. I didn't review the details of the sdp parsing itself as Byron is the expert there.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:967
(Diff revision 1)
> +    auto retrieveVersions = [] (const RustSdpAttributeSimulcastAlternativesList&
> +                                                                      altsList)
> +                                            -> SdpSimulcastAttribute::Versions {
> +      size_t  altsCount = sdp_simulcast_get_alternatives_count(altsList.alts);
> +      auto sc_altsList = MakeUnique<RustSdpAttributeSimulcastAlternatives[]>
> +                                                                    (altsCount);

The indent seems off here and above, but you're fighting against long names and an 80 column limit, so I don't know if there a better way of formating this.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:968
(Diff revision 1)
> +                                                                      altsList)
> +                                            -> SdpSimulcastAttribute::Versions {
> +      size_t  altsCount = sdp_simulcast_get_alternatives_count(altsList.alts);
> +      auto sc_altsList = MakeUnique<RustSdpAttributeSimulcastAlternatives[]>
> +                                                                    (altsCount);
> +      sdp_simulcast_get_alternatives(altsList.alts, altsCount,sc_altsList.get());

nit: please add a space after the second comma

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:988
(Diff revision 1)
> +        sdp_simulcast_get_ids(alts.ids,altCount,sc_alts.get());
> +
> +        for(size_t j = 0; j < altCount; j++){
> +          const RustSdpAttributeSimulcastId& alt = sc_alts[j];
> +          std::string id = convertStringView(alt.id);
> +          if(!alt.paused)

nit: please add { and } to this if statement

::: media/webrtc/signaling/src/sdp/SdpAttribute.cpp:1104
(Diff revision 1)
> +  rid.constraints = constraints;
> +  rid.dependIds = dependIds;
> +
> +  mRids.push_back(std::move(rid));
> +}
> +

nit: remove extra newline

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1219
(Diff revision 1)
> +        max_dpb: 0,
> +        scale_down_by: 1.0,
> +        unknown: Vec::new(),
> +    };
> +    let mut formats: Vec<u16> = Vec::new();
> +    let mut depeneds: Vec<String> = Vec::new();

is this supposed to be depends?

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1242
(Diff revision 1)
> +
> +        for param in parameters {
> +            let param_value_pair: Vec<&str> = param.splitn(2,"=").collect();
> +            if param_value_pair.len() != 2{
> +                return Err(SdpParserInternalError::Generic(
> +                    "A rid parameters needs to be of form 'param=value'".to_string()

typo: s/parameters/parameter/

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1256
(Diff revision 1)
> +                "max-br" => params.max_br = param_value_pair[1].parse::<u32>()?,
> +                "max-pps" => params.max_pps = param_value_pair[1].parse::<u32>()?,
> +                "max-mbps" => params.max_mbps = param_value_pair[1].parse::<u32>()?,
> +                "max-cpb" => params.max_cpb = param_value_pair[1].parse::<u32>()?,
> +                "max-dpb" => params.max_dpb = param_value_pair[1].parse::<u32>()?,
> +                "scale_down-by" => params.scale_down_by = param_value_pair[1].parse::<f64>()?,

is this supposed to be scale-down-by?

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1927
(Diff revision 1)
>  
>      assert!(parse_attribute("rid:").is_err());
> +    assert!(parse_attribute("rid:120 send pt=").is_err());
> +    assert!(parse_attribute("rid:120 send pt=;max-width=10").is_err());
> +    assert!(parse_attribute("rid:120 send pt=9;max-width=").is_err());
> +    assert!(parse_attribute("rid:120 send pt=9;max-width=;max-width=10").is_err());

Please add a test for each parameter that we support parsing. Just in case.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/media_type.rs:178
(Diff revision 1)
>               SdpFormatList::Strings(ref mut x) => x.push(rtpmap.payload_type.to_string()),
>           }
>  
>          self.add_attribute(&SdpAttribute::Rtpmap(rtpmap))?;
>          Ok(())
>      }    

nit: please remove trailing whitespace

Comment 3

[Johannes Willbold]

Comment on attachment 8986310 [details]
Bug 1432932: Sanity check for simulcast recv rids.

https://reviewboard.mozilla.org/r/251680/#review258214

> is this supposed to be depends?

the parameter in the actual attribute is called like this, so I found this name suitable.
Example" rid:110 send pt=9,10;max-width=10;depends=1,2,3

> is this supposed to be scale-down-by?

I think so.

Comment 4

[Byron Campen [:bwc]]

Comment on attachment 8986310 [details]
Bug 1432932: Sanity check for simulcast recv rids.

https://reviewboard.mozilla.org/r/251680/#review258538

Partial review, will return to this later today or tomorrow.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:962
(Diff revision 1)
> +    auto retrieveVersions = [] (const RustSdpAttributeSimulcastAlternativesList&
> +                                                                      altsList)
> +                                            -> SdpSimulcastAttribute::Versions {

This function is big enough that it probably merits being a named function.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:965
(Diff revision 1)
> +    auto simulcast = MakeUnique<SdpSimulcastAttribute>();
> +
> +    auto retrieveVersions = [] (const RustSdpAttributeSimulcastAlternativesList&
> +                                                                      altsList)
> +                                            -> SdpSimulcastAttribute::Versions {
> +      size_t  altsCount = sdp_simulcast_get_alternatives_count(altsList.alts);

Minor nit: whitespace

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:965
(Diff revision 1)
> +    auto simulcast = MakeUnique<SdpSimulcastAttribute>();
> +
> +    auto retrieveVersions = [] (const RustSdpAttributeSimulcastAlternativesList&
> +                                                                      altsList)
> +                                            -> SdpSimulcastAttribute::Versions {
> +      size_t  altsCount = sdp_simulcast_get_alternatives_count(altsList.alts);

Ugh, so many naming inconsistencies. It would be great if we could call a list of alternatives a "version" everywhere. But, in the interest of expediency, let's just s/alts/version/ in the variable names defined here.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:966
(Diff revision 1)
> +
> +    auto retrieveVersions = [] (const RustSdpAttributeSimulcastAlternativesList&
> +                                                                      altsList)
> +                                            -> SdpSimulcastAttribute::Versions {
> +      size_t  altsCount = sdp_simulcast_get_alternatives_count(altsList.alts);
> +      auto sc_altsList = MakeUnique<RustSdpAttributeSimulcastAlternatives[]>

versionArray maybe?

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:982
(Diff revision 1)
> +        if(!altCount) {
> +          continue;
> +        }
> +
> +        SdpSimulcastAttribute::Version version;
> +        auto sc_alts = MakeUnique<RustSdpAttributeSimulcastId[]>(altCount);

altArray?

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:983
(Diff revision 1)
> +          continue;
> +        }
> +
> +        SdpSimulcastAttribute::Version version;
> +        auto sc_alts = MakeUnique<RustSdpAttributeSimulcastId[]>(altCount);
> +        sdp_simulcast_get_ids(alts.ids,altCount,sc_alts.get());

Nit: whitespace after the commas, please.

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8986310 [details]
Bug 1432932: Sanity check for simulcast recv rids.

https://reviewboard.mozilla.org/r/251680/#review258866

More partial review.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:988
(Diff revision 1)
> +          if(!alt.paused)
> +            version.choices.push_back(id);
> +        }

So, I don't think we should omit stuff that is paused. From what I can tell, pausing/unpausing an RTP stream is handled with RTCP, and the '~' just tells the initial state of the stream. The expectation (I think) would be that the receiver could unpause the stream at will by sending the appropriate RTCP packet, but it will not be able to do that if it does not know about it.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:1118
(Diff revision 1)
> +    parameters.maxMbps = rid.params.max_mbps;
> +    parameters.maxCpb = rid.params.max_cpb;
> +    parameters.maxDpb = rid.params.max_dpb;
> +    parameters.scaleDownBy = rid.params.scale_down_by;

Nobody has specified these for rid. max-mbps, max-cpb, and max-dpb are only defined for h264 a=fmtp attributes, and scaleDownBy only comes from JS constraints. You can just remove all 4 of these, here and everywhere else.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:1125
(Diff revision 1)
> +    parameters.maxDpb = rid.params.max_dpb;
> +    parameters.scaleDownBy = rid.params.scale_down_by;
> +
> +    std::vector<std::string> depends = convertStringVec(rid.depends);
> +
> +    ridList->PushEntry(id,direction,formats,parameters,depends);

Nit: Whitespace after commas.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:218
(Diff revision 1)
> +struct RustSdpAttributeSimulcastAlternativesVec;
> +struct RustSdpAttributeSimulcastAlternativesList {
> +  uint8_t type;
> +  RustSdpAttributeSimulcastAlternativesVec* alts;
> +};
> +
> +struct RustSdpAttributeSimulcast {
> +  RustSdpAttributeSimulcastAlternativesList send;
> +  RustSdpAttributeSimulcastAlternativesList recv;
> +};

Oh, so this is all new code. Let's change AlternativesList/AlternativesVec to VersionList/VersionVec (because each element in these describes a different version of the same stream; one might be the thumbnail version, another might be the full version, for example).

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:374
(Diff revision 1)
> +size_t sdp_simulcast_get_alternatives_count(
> +                    const RustSdpAttributeSimulcastAlternativesVec* aAltsList);
> +void sdp_simulcast_get_alternatives(
> +                    const RustSdpAttributeSimulcastAlternativesVec* aAltsList,
> +                    size_t listSize,
> +                    RustSdpAttributeSimulcastAlternatives* ret);

get_version_count, get_version

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:168
(Diff revision 1)
> -pub struct SdpAttributeSimulcast {
> -    pub send: Vec<SdpAttributeSimulcastAlternatives>,
> -    pub receive: Vec<SdpAttributeSimulcastAlternatives>,
> +pub enum SdpAttributeSimulcastAlternativesList {
> +    Rid(Vec<SdpAttributeSimulcastAlternatives>),
> +    Pt(Vec<SdpAttributeSimulcastAlternatives>)
>  }
>  
> -impl SdpAttributeSimulcast {
> -    fn parse_ids(&mut self, direction: SdpAttributeDirection, idlist: String) {
> -        let list = idlist
> -            .split(';')
> -            .map(|x| x.to_string())
> -            .map(SdpAttributeSimulcastAlternatives::new)
> -            .collect();
> -        // TODO prevent over-writing existing values
> +impl SdpAttributeSimulcastAlternativesList {
> +    pub fn has_alternatives(&self) -> bool {
> +        match *self {
> +            SdpAttributeSimulcastAlternativesList::Rid(ref x) =>
> +                return x.len() > 0,
> +            SdpAttributeSimulcastAlternativesList::Pt(ref x) =>
> +                return x.len() > 0,
> +        }
> -        match direction {
> -            SdpAttributeDirection::Recvonly => self.receive = list,
> -            SdpAttributeDirection::Sendonly => self.send = list,
> -            _ => (),
> -        }
> +    }
> -    }
> +}

Actually, don't bother with this. The pt stuff was removed from the draft a while back, and at this point I'm sure it won't be coming back. If there are some tests for that simulcast pt stuff still, remove the tests.

Note however that the pt stuff is still present in rid.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1200
(Diff revision 1)
> +fn parse_rid(to_parse: &str) -> Result<SdpAttribute, SdpParserInternalError> {
> +    let tokens: Vec<&str> = to_parse.splitn(3, " ").collect();
> +
> +    if tokens.len() < 2 {
> +        return Err(SdpParserInternalError::Generic(
> +                "TODO".to_string()

Don't forget to fill this in.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1219
(Diff revision 1)
> +        max_dpb: 0,
> +        scale_down_by: 1.0,
> +        unknown: Vec::new(),
> +    };
> +    let mut formats: Vec<u16> = Vec::new();
> +    let mut depeneds: Vec<String> = Vec::new();

s/depeneds/depends/

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1227
(Diff revision 1)
> +    if let Some(param_token) = tokens.get(2) {
> +        let mut parameters = param_token.split(";").peekable();
> +
> +        // The 'pt' parameter must be the first parameter if present, so it
> +        // cannot be checked along with the other parameters below
> +        if let Some(maybe_fmt_parameter) = parameters.clone().peek() {

Why are we cloning the iterator here?

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1239
(Diff revision 1)
> +            let param_value_pair: Vec<&str> = param.splitn(2,"=").collect();
> +            if param_value_pair.len() != 2{
> +                return Err(SdpParserInternalError::Generic(
> +                    "A rid parameters needs to be of form 'param=value'".to_string()
> +                ));
> +            }

Let's put a todo for bug 1225877 here; we need to make this parse code work with value-less params.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1240
(Diff revision 1)
> +            }
> +        }
> +
> +        for param in parameters {
> +            let param_value_pair: Vec<&str> = param.splitn(2,"=").collect();
> +            if param_value_pair.len() != 2{

Nit: Whitespace before the '{', right?

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1551
(Diff revision 1)
>                                 }
>                             }))
>  }
>  
>  fn parse_simulcast(to_parse: &str) -> Result<SdpAttribute, SdpParserInternalError> {
> -    let mut tokens = to_parse.split_whitespace();
> +    let mut tokens = to_parse.trim().split_whitespace();

Only a plain space is allowed to delimit this stuff now, let's put a TODO here pointing at bug 1225877

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1912
(Diff revision 1)
> -    assert!(parse_attribute("rid:foo").is_ok());
> +    assert!(parse_attribute("rid:110 send pt=9").is_ok());
> +    assert!(parse_attribute("rid:foo send pt=10").is_ok());
> +    assert!(parse_attribute("rid:110 send pt=9,10").is_ok());
> +    assert!(parse_attribute("rid:110 send pt=9,10;max-fs=10").is_ok());
> +    assert!(parse_attribute("rid:110 send pt=9,10;max-width=10;depends=1,2,3").is_ok());
> +    assert!(parse_attribute("rid:110 send pt=9,10;max-fs=10;UNKNOWN=100;depends=1,2,3").is_ok());
> +    assert!(parse_attribute("rid:110 send pt=9, 10;max-fs=10;UNKNOWN=100; depends=1, 2, 3").is_ok());
> +    assert!(parse_attribute("rid:110 send max-fs=10").is_ok());
> +    assert!(parse_attribute("rid:110 send max-width=10;depends=1,2,3").is_ok());
> +    assert!(parse_attribute("rid:110 send max-fs=10;UNKNOWN=100;depends=1,2,3").is_ok());
>  
>      assert!(parse_attribute("rid:").is_err());
> +    assert!(parse_attribute("rid:120 send pt=").is_err());
> +    assert!(parse_attribute("rid:120 send pt=;max-width=10").is_err());
> +    assert!(parse_attribute("rid:120 send pt=9;max-width=").is_err());
> +    assert!(parse_attribute("rid:120 send pt=9;max-width=;max-width=10").is_err());

Hmm. Most of the testing for our existing rid and simulcast parse code calls the parse functions defined on those attribute types. As a result, we will not have very good testing coverage for the simulcast/rid rust parser in sdp_unittests. It would be nice to check whether the parsed version is what we expect it to be on these.

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8986310 [details]
Bug 1432932: Sanity check for simulcast recv rids.

https://reviewboard.mozilla.org/r/251680/#review258946

Yet more partial review. That's all the time I have for this today.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1561
(Diff revision 1)
> -        send: Vec::new(),
> -        receive: Vec::new(),
> -    };
> +    let parse_alts_list = |to_parse: &str|
> +            -> Result<SdpAttributeSimulcastAlternativesList, SdpParserInternalError> {
> +        let make_alts_list = |to_parse: &str| {

s/alts_list/version_list/ here and below.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1561
(Diff revision 1)
>                         ))
>          }
> -        Some(x) => x,
>      };
> -    let mut sc = SdpAttributeSimulcast {
> -        send: Vec::new(),
> +
> +    let parse_alts_list = |to_parse: &str|

Let's not define this function in between parsing the first direction, and the corresponding list of versions.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1569
(Diff revision 1)
> +                "pt" => Ok(SdpAttributeSimulcastAlternativesList::Pt(
> +                    make_alts_list(descriptor_altlist_pair.next().unwrap())
> +                )),

We can remove the pt stuff, but we still need to handle the old-fashioned "rid=" syntax, because we still serialize it. So let's put a TODO pointing at bug 1470568 for removing the "rid=" parse code.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1588
(Diff revision 1)
> +        } else {
> +            Ok(SdpAttributeSimulcastAlternativesList::Rid(make_alts_list(to_parse)))
> +        }
> -        };
> +    };
> -        match tokens.next() {
> +
> +    let first_alt_list = match tokens.next() {

s/alt_list/version_list/ here and below.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1594
(Diff revision 1)
> +        Some(x) => {
> +            parse_alts_list(x)?
> +        },
> -            None => {
> +        None => {
> -                return Err(SdpParserInternalError::Generic("Simulcast attribute is missing id list"
> -                                                               .to_string()))
> +            return Err(SdpParserInternalError::Generic(
> +                    "Simulcast attribute must have an alternatives list after the direction token"

Let's s/an alternatives list/a list of simulcast stream versions/ here.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1614
(Diff revision 1)
> +            Some(x) => {
> +                parse_alts_list(x)?
> +            },
>              None => {
> -                break;
> +                return Err(SdpParserInternalError::Generic(
> +                    "Simulcast has defined a second direction but no second alternatives list"

Let's s/alternatives list/list of simulcast stream versions/ here.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/lib.rs:679
(Diff revision 1)
> +                    }
> +                }
> +                Ok(())
> +            };
> +
> +            println!("{:?}",recv_rids);

Make sure to remove this.

Comment 7

[Johannes Willbold]

Comment on attachment 8986310 [details]
Bug 1432932: Sanity check for simulcast recv rids.

https://reviewboard.mozilla.org/r/251680/#review258866

> So, I don't think we should omit stuff that is paused. From what I can tell, pausing/unpausing an RTP stream is handled with RTCP, and the '~' just tells the initial state of the stream. The expectation (I think) would be that the receiver could unpause the stream at will by sending the appropriate RTCP packet, but it will not be able to do that if it does not know about it.

Ok, so how am I supposed to handle the paused state? The SdpSimulcastAttribute::Version does not have a field for this.

> Nobody has specified these for rid. max-mbps, max-cpb, and max-dpb are only defined for h264 a=fmtp attributes, and scaleDownBy only comes from JS constraints. You can just remove all 4 of these, here and everywhere else.

I cannot remove these fields from https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/common/EncodingConstraints.h#14 as they are used. Also, this tructure was reason I implemented these fields initially. Shall I still remove these fields from the rust parsing?

> Actually, don't bother with this. The pt stuff was removed from the draft a while back, and at this point I'm sure it won't be coming back. If there are some tests for that simulcast pt stuff still, remove the tests.
> 
> Note however that the pt stuff is still present in rid.

I think the problem is, that Firefox ESR still uses this old format and also sends offers with this old format. Nils told me it is required because of this reason.

> Why are we cloning the iterator here?

The if let-line borrwos the iterator in this line, that would prevent the next borrow 7 lines below. So in order to make it work 7 lines below, I have to clone here. But I dont think that is a problem as I only clone the iterator and peek on it.

> Hmm. Most of the testing for our existing rid and simulcast parse code calls the parse functions defined on those attribute types. As a result, we will not have very good testing coverage for the simulcast/rid rust parser in sdp_unittests. It would be nice to check whether the parsed version is what we expect it to be on these.

Isn't https://searchfox.org/mozilla-central/source/media/webrtc/signaling/gtest/sdp_unittests.cpp#2898 andhttps://searchfox.org/mozilla-central/source/media/webrtc/signaling/gtest/sdp_unittests.cpp#3220 sufficient?

Comment 8

[Johannes Willbold]

Comment on attachment 8986310 [details]
Bug 1432932: Sanity check for simulcast recv rids.

https://reviewboard.mozilla.org/r/251680/#review258946

> We can remove the pt stuff, but we still need to handle the old-fashioned "rid=" syntax, because we still serialize it. So let's put a TODO pointing at bug 1470568 for removing the "rid=" parse code.

Same for this as for the comment above. This is required for ESR 52 support. I added the bug nummber though.

Comment 9

[Johannes Willbold]

Comment on attachment 8986310 [details]
Bug 1432932: Sanity check for simulcast recv rids.

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

Comment 10

[Byron Campen [:bwc]]

Comment on attachment 8986310 [details]
Bug 1432932: Sanity check for simulcast recv rids.

https://reviewboard.mozilla.org/r/251680/#review258866

> Ok, so how am I supposed to handle the paused state? The SdpSimulcastAttribute::Version does not have a field for this.

Just ignore that it is marked paused for now, and put a TODO pointing at bug 1225877.

> I cannot remove these fields from https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/common/EncodingConstraints.h#14 as they are used. Also, this tructure was reason I implemented these fields initially. Shall I still remove these fields from the rust parsing?

Yeah, remove them from the code you're working on here.

> Isn't https://searchfox.org/mozilla-central/source/media/webrtc/signaling/gtest/sdp_unittests.cpp#2898 andhttps://searchfox.org/mozilla-central/source/media/webrtc/signaling/gtest/sdp_unittests.cpp#3220 sufficient?

Those are not very thorough tests, no. You don't need to go quite as in-depth as we did before, because Rust is better at catching errors, but I would at least like to see a little more verification.

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 8986310 [details]
Bug 1432932: Sanity check for simulcast recv rids.

https://reviewboard.mozilla.org/r/251680/#review258946

> Same for this as for the comment above. This is required for ESR 52 support. I added the bug nummber though.

Firefox has never emitted pt=; that was only there in case some other implementation used it.

Comment 12

[Byron Campen [:bwc]]

Comment on attachment 8986310 [details]
Bug 1432932: Sanity check for simulcast recv rids.

https://reviewboard.mozilla.org/r/251680/#review259238

::: media/webrtc/signaling/src/sdp/rsdparsa/src/lib.rs:663
(Diff revision 2)
> +                if let &SdpAttributeSimulcastVersionList::Rid(ref x) = simulcast_version_list {
> +                    for simulcast_rids in x.iter() {
> +                        for simulcast_rid in simulcast_rids.ids.iter() {
> +                            if !rid_ids.contains(&simulcast_rid.id.as_str()) {

This could probably be made more readable by using map and flatten (something like "for rid in versions.iter().map(|rids| rids.ids.iter()).flatten()" I think)

Comment 13

[Byron Campen [:bwc]]

Comment on attachment 8986310 [details]
Bug 1432932: Sanity check for simulcast recv rids.

https://reviewboard.mozilla.org/r/251680/#review259250

Ok, you've fixed almost all the issues already, but I want to give this one last look once you're done with all of them.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:969
(Diff revisions 1 - 2)
> -      sdp_simulcast_get_alternatives(altsList.alts, altsCount,sc_altsList.get());
>  
> -      SdpSimulcastAttribute::Versions versions;
> +  SdpSimulcastAttribute::Versions versions;
> -      versions.type = static_cast<SdpSimulcastAttribute::Versions::Type>
> +  versions.type = static_cast<SdpSimulcastAttribute::Versions::Type>
> -                                                        (altsList.type);
> -      for(size_t i = 0; i < altsCount; i++){
> +                                                    (rustVersionList.type);
> +  for(size_t i = 0; i < rustVersionCount; i++){

Nit: Whitespace before '{'

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:972
(Diff revisions 1 - 2)
> -      versions.type = static_cast<SdpSimulcastAttribute::Versions::Type>
> +  versions.type = static_cast<SdpSimulcastAttribute::Versions::Type>
> -                                                        (altsList.type);
> -      for(size_t i = 0; i < altsCount; i++){
> -        const RustSdpAttributeSimulcastAlternatives& alts = sc_altsList[i];
> -
> -        size_t altCount = sdp_simulcast_get_ids_count(alts.ids);
> +                                                    (rustVersionList.type);
> +  for(size_t i = 0; i < rustVersionCount; i++){
> +    const RustSdpAttributeSimulcastVersion& rustVersion = rustVersionArray[i];
> +    size_t rustIdCount = sdp_simulcast_get_ids_count(rustVersion.ids);
> +    if(!rustIdCount) {

Nit: Whitespace after "if"

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:982
(Diff revisions 1 - 2)
> -          std::string id = convertStringView(alt.id);
> -          if(!alt.paused)
> -            version.choices.push_back(id);
> +      std::string id = convertStringView(rustId.id);
> +      std::stringstream ss;
> +      ss << id;
> +      // ss << (rustId.paused ? "~" : "");
> +      version.choices.push_back(ss.str());

We don't need this stringstream stuff anymore, right?

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1239
(Diff revisions 1 - 2)
>                  parameters.next();
>              }
>          }
>  
>          for param in parameters {
> +            // TODO: Bug 1225877

Let's note that the TODO here is to accept params without '='

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1580
(Diff revisions 1 - 2)
> -            Ok(SdpAttributeSimulcastAlternativesList::Rid(make_alts_list(to_parse)))
> +       Ok(SdpAttributeSimulcastVersionList::Rid(make_version_list(to_parse)))
> +    }
> +}
> +
> +fn parse_simulcast(to_parse: &str) -> Result<SdpAttribute, SdpParserInternalError> {
> +    // TODO: Bug 1225877

Specifically, let's note that we want to stop accepting all kinds of whitespace here, and only accept SP.

Comment 14

[Johannes Willbold]

Comment on attachment 8986310 [details]
Bug 1432932: Sanity check for simulcast recv rids.

https://reviewboard.mozilla.org/r/251680/#review258946

> Firefox has never emitted pt=; that was only there in case some other implementation used it.

This breaks a lot of testcases. Including every testcase using kBasicAudioVideoOffer (~27 times used).
In addition: 
ParseInvalidSimulcastNoSuchPt becomes obsolete 
ParseInvalidSimulcastNotSending must to altered
ParseInvalidSimulcastNotReceiving must be altered
CheckSimulcastSerialize must be altered

Comment 15

[Johannes Willbold]

Comment on attachment 8986310 [details]
Bug 1432932: Sanity check for simulcast recv rids.

https://reviewboard.mozilla.org/r/251680/#review259250

> We don't need this stringstream stuff anymore, right?

Right

Comment 16

[Byron Campen [:bwc]]

Comment on attachment 8986310 [details]
Bug 1432932: Sanity check for simulcast recv rids.

https://reviewboard.mozilla.org/r/251680/#review258946

> This breaks a lot of testcases. Including every testcase using kBasicAudioVideoOffer (~27 times used).
> In addition: 
> ParseInvalidSimulcastNoSuchPt becomes obsolete 
> ParseInvalidSimulcastNotSending must to altered
> ParseInvalidSimulcastNotReceiving must be altered
> CheckSimulcastSerialize must be altered

Yeah, any test-case for pt= can be removed, honestly.

Comment 17

[Johannes Willbold]

Comment on attachment 8986310 [details]
Bug 1432932: Sanity check for simulcast recv rids.

https://reviewboard.mozilla.org/r/251680/#review258946

> Yeah, any test-case for pt= can be removed, honestly.

Please check that I have altered kBasicAudioVideoOffer correctly

Comment 18

[Johannes Willbold]

Comment on attachment 8986310 [details]
Bug 1432932: Sanity check for simulcast recv rids.

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

Comment 19

[Byron Campen [:bwc]]

Comment on attachment 8986310 [details]
Bug 1432932: Sanity check for simulcast recv rids.

https://reviewboard.mozilla.org/r/251680/#review259648

Ok, r+ with a few test fixes.

::: media/webrtc/signaling/gtest/sdp_unittests.cpp:1908
(Diff revisions 2 - 3)
>  "a=rid:bar recv pt=96;max-width=800;max-height=600" CRLF
> +"a=rid:bar123 recv pt=97;max-width=1080;max-height=1920" CRLF
> +"a=simulcast:recv rid=bar;bar123" CRLF

Since we're using two rids here, let's have one of them not use pt= (eg; a=rid:bar123 recv max-width=1080;max-height=1920)

::: media/webrtc/signaling/gtest/sdp_unittests.cpp:2916
(Diff revisions 2 - 3)
> -  ASSERT_EQ(1U, rids.mRids.size());
> +  ASSERT_EQ(2U, rids.mRids.size());
>    ASSERT_EQ("bar", rids.mRids[0].id);
>    ASSERT_EQ(sdp::kRecv, rids.mRids[0].direction);
>    ASSERT_EQ(1U, rids.mRids[0].formats.size());
>    ASSERT_EQ(96U, rids.mRids[0].formats[0]);
>    ASSERT_EQ(800U, rids.mRids[0].constraints.maxWidth);
>    ASSERT_EQ(600U, rids.mRids[0].constraints.maxHeight);

Let's check the values for both rids here.

::: media/webrtc/signaling/gtest/sdp_unittests.cpp
(Diff revisions 2 - 3)
> -TEST_P(NewSdpTest, ParseInvalidSimulcastNoSuchPt) {
> -  SKIP_TEST_WITH_RUST_PARSER; // See Bug 1432933
> -  ParseSdp("v=0" CRLF
> -           "o=- 4294967296 2 IN IP4 127.0.0.1" CRLF
> -           "s=SIP Call" CRLF
> -           "c=IN IP4 198.51.100.7" CRLF
> -           "b=CT:5000" CRLF
> -           "t=0 0" CRLF
> -           "m=video 56436 RTP/SAVPF 120" CRLF
> -           "a=rtpmap:120 VP8/90000" CRLF
> -           "a=sendrecv" CRLF
> -           "a=simulcast: send pt=9" CRLF,
> -           false);
> -  ASSERT_NE("", GetParseErrors());
> -}
> -

Can we check that we error out if a rid contains a pt that is not present?

::: media/webrtc/signaling/gtest/sdp_unittests.cpp:3785
(Diff revisions 2 - 3)
>  TEST_P(NewSdpTest, ParseInvalidSimulcastNotSending) {
>    ParseSdp("v=0" CRLF
>             "o=- 4294967296 2 IN IP4 127.0.0.1" CRLF
>             "s=SIP Call" CRLF
>             "c=IN IP4 198.51.100.7" CRLF
>             "b=CT:5000" CRLF
>             "t=0 0" CRLF
>             "m=video 56436 RTP/SAVPF 120" CRLF
>             "a=rtpmap:120 VP8/90000" CRLF
>             "a=recvonly" CRLF
> -           "a=simulcast: send pt=120" CRLF,
> +           "a=simulcast: send rid=120" CRLF,
>             false);
>    ASSERT_NE("", GetParseErrors());
>  }
>  
>  TEST_P(NewSdpTest, ParseInvalidSimulcastNotReceiving) {
>    ParseSdp("v=0" CRLF
>             "o=- 4294967296 2 IN IP4 127.0.0.1" CRLF
>             "s=SIP Call" CRLF
>             "c=IN IP4 198.51.100.7" CRLF
>             "b=CT:5000" CRLF
>             "t=0 0" CRLF
>             "m=video 56436 RTP/SAVPF 120" CRLF
>             "a=rtpmap:120 VP8/90000" CRLF
>             "a=sendonly" CRLF
> -           "a=simulcast: recv pt=120" CRLF,
> +           "a=simulcast: recv rid=120" CRLF,

I guess we should put a rid attribute in these, so we're only testing one error.

::: media/webrtc/signaling/gtest/sdp_unittests.cpp
(Diff revisions 2 - 3)
> -  os.str("");
> -
> -  simulcast.sendVersions.push_back(SdpSimulcastAttribute::Version());
> -  simulcast.sendVersions.back().choices.push_back("9");
> -  simulcast.Serialize(os);
> -  ASSERT_EQ("a=simulcast: send rid=9 recv pt=8" CRLF, os.str());

Let's keep this chunk, so we can test both send and recv at the same time.

Comment 20

[Johannes Willbold]

Comment on attachment 8986310 [details]
Bug 1432932: Sanity check for simulcast recv rids.

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

Comment 21

[Johannes Willbold]

Comment on attachment 8986310 [details]
Bug 1432932: Sanity check for simulcast recv rids.

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

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b23bf217d88f
Sanity check for simulcast recv rids. r=bwc,dminor

Comment 23

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/b23bf217d88f