mozilla / webrtc-sdp Goto Github PK

Rust 1.13 introduced the ? operator, which is the same as the try! macro, but more succinct. There is also the promise of better compiler errors with ? in the future.

SdpOrigin has SdpAddrType which is an enum that tells us if the address is IPv4 or IPv6, but SdpOrigin also has an IpAddr which contains this information as well. This information should be de-duplicated.

Replace all the stuct init calls with duplicate in it.
See https://blog.rust-lang.org/2017/04/27/Rust-1.17.html

https://github.com/nils-ohlmeier/rsdparsa/blob/master/src/attribute_type.rs#L368

IpAddr::from_str can return AddrParseError. Add a new case to SdpParserError in error.rs for this.

A number of Result<> convenience methods require this.

https://tools.ietf.org/html/rfc4585#section-4.2

The dtls-message attribute is specified in Piggybacked DTLS Handshakes in SDP. We may want to support this.

dyn appeared in rust 1.27, now that we use this, we need to bump the minimum version accordingly.

Eg here the first field would be Str(String)

Port over the SDP unit tests from mozilla-central to rsdparsa to ensure the same coverage.

It looks like sipcc does some format specific parsing of the Fmtp attribute. We probably want to do as much parsing in rust as possible, which means parsing Fmtp attributes in rsdparsa.

Eg SdpAttribute's members are private, and there is no way to access them, making the struct useless. Marking them as public causes the compiler to complain about leaking private types. Is the right solution to start marking things public until it compiles?

get_session() returns a &String. It would be more idiomatic to return a &str.

The Unknown value in SdpBandwidthType should carry a String if SdpBandwidthType it is only ever used in SdpBandwidth with the unknown_type option set to the corresponding String. Then unknown_type in SdpBandwidth can be eliminated.

Alternatively, we can simplify down to one struct. Eg:

pub enum SdpBandwidth {
    As(u64),
    Ct(u64),
    Tias(u64),
    Unknown(u64, String),
}

or

pub enum SdpBandwidth {
    As {bandwidth: u64},
    Ct {bandwidth: u64},
    Tias {bandwidth: u64},
    Unknown {bandwidth: u64, bandwidth_type: String},
}

None of SdpConnection's fields are public. Make them public.

https://github.com/nils-ohlmeier/rsdparsa/blob/master/src/attribute_type.rs#L226

It would be more idomatic, but might make it harder to filter based on the enum.

rsdparsa splits on any arbitrary amount of any kind of whitespace, whereas the RFCs usually specify tokens are separted by SP, which is defined by the RBNF RFC(https://tools.ietf.org/html/rfc5234#appendix-B.1) as only 0x20. Do we want to be more strict in parsing SDP? I would guess being strict would break a lot of things if SIPCC already splits on any kind of whitespace pattern.

A media line might have m=<media> <port>/<number of ports> <proto> <fmt> .... Currently we cannot parse the <number of ports> portion.

We have SdpAttributeDirection with Recvonly, Sendonly, and Sendrecv. We also have SdpAttribute whicih has Recvonly, Sendonly, and Sendrecv. If these are the same, we should consolidate them.

In the Travis configuration we currently have: 'cargo clippy --all-targets --all-features -- -D warnings -A clippy::filter_next;' https://github.com/nils-ohlmeier/rsdparsa/blob/master/.travis.yml#L52

It would be great to be able to remove the clippy::filter_next exception. But I haven't found a way to remove the .next() call without making the code more ugly and complex.

'cargo clippy' always returns 0 even if it finds problems. But travis continues as long as the return code is zero.

One option might be to use 'tee' to get the clippy output into a file and then grep through that file after the clippy run.

The C++ code has ValidateSimulcast. We want a similar function in rsdparsa.

The Sipcc C++ glue has a check to ensure that there isn't an extmap attribute defined at both the media and session level. We should check this in the parser as well.

Any plans on that?

Currently SdpParserError does not implement the Error trait. It might be nice to have, especially if we were to start statically allocating some error strings.

https://tools.ietf.org/html/rfc5285#section-5 describes extmaps as

a=extmap:<value>["/"<direction>] <URI> <extensionattributes>

giving the example

a=extmap:2/sendrecv http://example.com/082005/ext.htm#xmeta short

We do not currently support the extensionattributes

Instead of using split_whitespace().collect() to get a temporary vector just use split_whitespace() and then the iterator with .next() for parsing.

Right now MsidSemantic is just a string with the entire line, but the parser should split out the msid and the following tokens.

#106 (comment) raised the idea to remove the parsers for unsupported types and thus make the over all code base quite a bit smaller.

cc @na-g

SdpAttributeRtpmap frequency. RFC 4566 section 6.

Looks like travis-cargo could simplify the travis.yaml file quite a bit https://github.com/huonw/travis-cargo

If SdpAttribute implements PartialEq, then we can do things such as attributes.contains(&SdpAttribute::IceLite), rather than custom for loops.

• SdpAttributeRtcp
• SdpConnection

SdpMedia needs an accessor for its bandwidth field.

I'm trying to parse simple INVITE sdp from client, but I got unsupported protocol, here is the sdp string:

let sdp_str = "v=0\r\n
        o=- 4232193541 678199583 IN IP4 192.168.5.33\r\n
        s=-\r\n
        c=IN IP4 192.168.5.33\r\n
        t=0 0\r\n
        m=audio 42498 RTP/AVP 96 0 8 101\r\n
        a=rtpmap:96 opus/48000/2\r\n
        a=fmtp:96 stereo=1;sprop-stereo=1;maxaveragebitrate=28000\r\n
        a=rtpmap:0 PCMU/8000\r\n
        a=rtpmap:8 PCMA/8000\r\n
        a=rtpmap:101 telephone-event/8000\r\n
        a=fmtp:101 0-15\r\n
        a=sendrecv\r\n
        a=label:1\r\n
        a=rtcp-rsize\r\n
        a=ssrc:1526903191 cname:sip:[email protected]:8080\r\n
        a=ptime:20\r\n";

And the error is:

error occured: Unsupported { error: Unsupported("unsupported protocol value: RTP/AVP"),
line: "m=audio 42498 RTP/AVP 96 0 8 101", line_number: 10 }

Apparently https://doc.rust-lang.org/std/error/trait.Error.html#method.cause has been deprecated in Rust 1.33.0. This results in compiler warnings for Beta right now, but soon on on Release as well.

We are suppose to replace the .cause() calls with .source() calls, but since Error::source() only got added in Rust version 1.30.0 https://doc.rust-lang.org/std/error/trait.Error.html#method.source that would mean we would have to bump the minimum Rust version to 1.30.0 only to support that transition. That looks like a pretty high price to me.

It would be nice to stay backward compatible, but still be able to pay attention to compiler warnings as errors.

SdpFormatList should be made into a tuple struct.