mozilla / webrtc-sdp

Rust SDP parser for WebRTC
Mozilla Public License 2.0
155 stars 30 forks source link

Makes changes to SdpBandwidthType and SdpBandwidth #41

Closed Paul-E closed 7 years ago

Paul-E commented 7 years ago

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},
}
nils-ohlmeier commented 7 years ago

Makes sense to combine them into one. Probably result from implementing this in phases.

I would go with the tuples. I would only do Unknown(String, u64) as I assume in case of unknown you first want to look at the type.

Paul-E commented 7 years ago

One thing I find is that if the bandwidth comes first, then I can always grab the first element to get the bandwidth value, regardless of variant.

nils-ohlmeier commented 7 years ago

But the bandwidth numbers is completely meaningless without the type, because you don't even know if it's in bits, bytes, kilo-bytes or whatever other unit.