Closed Bug 1432922 Opened 6 years ago Closed 6 years ago

Fix parsing of a=rtcp-fb:* with Rust SDP Parser

Categories

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

Product:
Core
Component:
WebRTC: Signaling
Version:
58 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: dminor, Assigned: johannes.willbold)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1432922: Implemented parsing support for rtcpfb-wildcard.
59 bytes, text/x-review-board-request
bwc
: review+
dminor
: review+
Details 
[ RUN      ] RoundTripSerialize/NewSdpTest.BasicAudioVideoDataSdpParse/0
version: 0      
origin: Mozilla-SIPUA-35.0a1, 27987, 0, 0.0.0.0
session: SIP Call
timing: 0, 0
media: Audio, 9, Rtp/Savpf, [109, 9, 0, 8, 101]
connection: 0.0.0.0
media: Video, 9, Rtp/Savpf, [120, 126, 97]
connection: 0.0.0.0
media: Application, 9, Dtls/Sctp, ["5000"]
connection: 0.0.0.0
Line { error: Integer(ParseIntError { kind: InvalidDigit }), line: "a=rtcp-fb:* ccm tmmbr", line_number: 53 }
Error parsing SDP in rust: invalid digit found in string
/home/dminor/src/firefox/media/webrtc/signaling/gtest/sdp_unittests.cpp:1550: Failure
Value of: !!mSdp
  Actual: false
Expected: true 
Parse failed: 53: invalid digit found in string
Summary: Fix NewSdpTest.BasicAudioVideoDataSdpParse with Rust SDP Parser → Fix parsing of a=rtcp-fb:* with Rust SDP Parser
Assignee: nobody → johannes.willbold
Comment on attachment 8981679 [details]
Bug 1432922: Implemented parsing support for rtcpfb-wildcard.

https://reviewboard.mozilla.org/r/247824/#review254078

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:867
(Diff revision 1)
> +    auto& rtcpfb = rustRtcpfbs[i];
> +    auto payloadTypeU32 = rtcpfb.payloadType;
> +
> +    std::stringstream ss;
> +    if(payloadTypeU32 == std::numeric_limits<uint32_t>::max())
> +      ss << "*";
> +    else
> +      ss << payloadTypeU32;
> +
> +    auto feedbackType = rtcpfb.feedbackType;
> +    auto parameter = convertStringView(rtcpfb.parameter);
> +    auto extra = convertStringView(rtcpfb.extra);

I'm not so comfortable with the amount of auto we're using in here.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:871
(Diff revision 1)
> +    if(payloadTypeU32 == std::numeric_limits<uint32_t>::max())
> +      ss << "*";
> +    else
> +      ss << payloadTypeU32;

Don't omit braces in this codebase.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:9
(Diff revision 1)
> +
> +#[derive(Clone)]
> +pub enum SdpAttributePayloadType {
> +    PayloadType(u32),
> +    Wildcard, // Wildcard means "*",
> +}
> +

I am not yet proficient in Rust, so you may want to get a review from someone who is.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:12
(Diff revision 1)
>  use network::{parse_nettype, parse_addrtype, parse_unicast_addr};
>  
> +
> +#[derive(Clone)]
> +pub enum SdpAttributePayloadType {
> +    PayloadType(u32),

This should be a u8.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:982
(Diff revision 1)
> +                                        _ => {
> +                                            return Err(SdpParserInternalError::Unsupported(
> +                                                format!("Unknown rtcpfb feedback type: {:?}",x).to_string()
> +                                            ))
> +                                        }

We shouldn't be failing to parse in this case; unknown feedback types should be ignored by the application. It probably makes sense to skip every a=rtcp-fb with a feedback type that is not recognized.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:996
(Diff revision 1)
> +                                    Some(x) => match x.as_ref() {
> +                                        "pli" | "sli" | "rpsi" | "app" |
> +                                        "fir" | "tmmbr" | "tstr" | "vbcm" =>  x.to_string(),
> +                                        _ if x.parse::<u32>().is_ok() => x.to_string(),
> +                                        _ => {
> +                                            return Err(SdpParserInternalError::Unsupported(
> +                                                format!("Unknown rtcpfb parameter: {:?}",x).to_string()
> -                                                   ))
> +                                            ))
> -                                    }
> +                                        }

Not all of these make sense with all of the feedback types. For instance, the "ack" feedback type only works with "rpsi" and "app". We have logic in SipccSdpAttributeList that does some of this checking:

https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/sdp/SipccSdpAttributeList.cpp#923

We'll either need something similar here in the rust code, or in the c++ wrapper code.

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs:578
(Diff revision 1)
>  
> +
> +#[repr(C)]
> +#[derive(Clone, Copy)]
> +pub struct RustSdpAttributeRtcpFb {
> +    pub payload_type: u32,

This can stay a u32 (or u16) so we can indicate wildcards.
Attachment #8981679 - Flags: review?(docfaraday) → review-
Comment on attachment 8981679 [details]
Bug 1432922: Implemented parsing support for rtcpfb-wildcard.

https://reviewboard.mozilla.org/r/247824/#review254078

> I am not yet proficient in Rust, so you may want to get a review from someone who is.

Reviwed by Nils.

> We shouldn't be failing to parse in this case; unknown feedback types should be ignored by the application. It probably makes sense to skip every a=rtcp-fb with a feedback type that is not recognized.

That is exactly what is happening there. SdpParserInternalError::Unsupported gives a warning and skips the current line.
Comment on attachment 8981679 [details]
Bug 1432922: Implemented parsing support for rtcpfb-wildcard.

https://reviewboard.mozilla.org/r/247824/#review254358

This looks good to me, just some formatting issues. I didn't look at the correctness of the parsing in detail, Byron is much more familiar with SDP than I am.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:874
(Diff revisions 1 - 2)
>  
>      std::stringstream ss;
> -    if(payloadTypeU32 == std::numeric_limits<uint32_t>::max())
> +    if(payloadTypeU32 == std::numeric_limits<uint32_t>::max()){
>        ss << "*";
> -    else
> +    }
> +    else {

nit: please place else on same line as }

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:973
(Diff revisions 1 - 2)
>  fn parse_rtcp_fb(to_parse: &str) -> Result<SdpAttribute, SdpParserInternalError> {
>      let tokens: Vec<&str> = to_parse.splitn(4,' ').collect();
> -    Ok(SdpAttribute::Rtcpfb(SdpAttributeRtcpFb {
> -                                payload_type: parse_payload_type(tokens[0])?,
>  
> -                                feedback_type: match tokens.get(1) {
> +    // Parse this in adnvance to use it later in the parameter switch

nit: s/advance/adnvance/

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:998
(Diff revisions 1 - 2)
> +
> +
> +    Ok(SdpAttribute::Rtcpfb(SdpAttributeRtcpFb {
> +                                payload_type: parse_payload_type(tokens[0])?,
>  
> -                                parameter: match tokens.get(2) {
> +                                parameter: match feedback_type {

Please move this match block to just under where you define feedback_type. This ends up being a long block of very indented code and I find it difficult to read in its current location.
Attachment #8981679 - Flags: review?(dminor) → review+
Comment on attachment 8981679 [details]
Bug 1432922: Implemented parsing support for rtcpfb-wildcard.

https://reviewboard.mozilla.org/r/247824/#review254400

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1008
(Diff revisions 1 - 2)
> +                                        None => {
> +                                            return Err(SdpParserInternalError::Unsupported(
> +                                                format!("The rtcpfb ack feeback type needs a parameter:").to_string()
> +                                            ))
> +                                        }

Hmm. The spec is self-contradictory in this case. The BNF allows this to be the empty string, but right after the BNF it says that there MUST be parameters. Weird. I'm going to ping abr about this, but this is probably fine since we did not support it before.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1039
(Diff revisions 1 - 2)
> -                                        _ => {
> +                                            _ => {
> -                                            return Err(SdpParserInternalError::Unsupported(
> +                                                return Err(SdpParserInternalError::Unsupported(
> -                                                format!("Unknown rtcpfb parameter: {:?}",x).to_string()
> +                                                    format!("Unknown rtcpfb trr-int parameter: {:?}",x).to_string()
> +                                                ))
> +                                            },

We should fail the parse here, I think, since the BNF doesn't allow this and is not extensible like the others.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1045
(Diff revisions 1 - 2)
> +                                        None => {
> +                                                return Err(SdpParserInternalError::Unsupported(
> +                                                    format!("The rtcpfb trr-int feedback type needs a parameter").to_string()
> -                                            ))
> +                                                ))
>                                          }

Same here.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs:1055
(Diff revisions 1 - 2)
>                                      },
> +                                    SdpAttributeRtcpFbType::Remb => match tokens.get(2) {
> +                                        Some(x) => match x {
> +                                            _ => {
> +                                                return Err(SdpParserInternalError::Unsupported(
> +                                                    format!("Unknown rtcpfp remb parameter: {:?}",x).to_string()

Nit: s/rtcpfp/rtcpfb/
Attachment #8981679 - Flags: review?(docfaraday) → review+
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/34215e726ea8
Implemented parsing support for rtcpfb-wildcard. r=bwc,dminor
https://hg.mozilla.org/mozilla-central/rev/34215e726ea8
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: