1436080 - Implement RsdparsaSdp::AddMediaSection method

Status: RESOLVED FIXED

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

Comment 1

[Johannes Willbold]

Attachment: Bug 1436080: Implemented RsdparsaSdp::AddMediaSection,

Implemented RsdparsaSdp::AddMediaSection
Added sdp_add_media_section to Rust/C++ glue code
Added SdpSession::add_media in Rust
Added C++ unit test CheckAddMediaSection

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

Comment 2

[Dan Minor [:dminor]]

Comment on attachment 8986517 [details]
Bug 1436080: Implemented RsdparsaSdp::AddMediaSection,

https://reviewboard.mozilla.org/r/251842/#review258296

lgtm!

::: media/webrtc/signaling/gtest/sdp_unittests.cpp:3995
(Diff revision 1)
> +  ASSERT_EQ(3U, mSdp->GetMediaSectionCount())
> +    << "Wrong number of media sections";
> +
> +  mSdp->AddMediaSection(SdpMediaSection::kVideo,
> +                        SdpDirectionAttribute::Direction::kSendrecv,
> +                        58000,SdpMediaSection::kUdpDtlsSctp,sdp::kIPv4,

nit: here and below please make sure you have a space after each comma in an argument list.

::: media/webrtc/signaling/gtest/sdp_unittests.cpp:4029
(Diff revision 1)
> +            nextNewMediaSection.GetDirectionAttribute().mValue);
> +  ASSERT_EQ(14006U,nextNewMediaSection.GetPort());
> +  ASSERT_EQ(SdpMediaSection::kTcpTlsRtpSavpf,nextNewMediaSection.GetProtocol());
> +  ASSERT_EQ(sdp::kIPv6,nextNewMediaSection.GetConnection().GetAddrType());
> +  ASSERT_EQ("2607:f8b0:4004:801::2013",
> +            nextNewMediaSection.GetConnection().GetAddress());

Please add some tests for invalid values to make sure the error is handled properly.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/lib.rs:21
(Diff revision 1)
>  pub mod network;
>  pub mod unsupported_types;
>  
>  use attribute_type::{SdpAttribute, SdpAttributeType, parse_attribute};
>  use error::{SdpParserInternalError, SdpParserError};
> -use media_type::{SdpMedia, SdpMediaLine, parse_media, parse_media_vector};
> +use media_type::*;

Please list the types you use rather than using *.

::: media/webrtc/signaling/src/sdp/rsdparsa/src/lib.rs:188
(Diff revision 1)
>      pub fn has_media(&self) -> bool {
>          !self.media.is_empty()
>      }
> +
> +    pub fn add_media(&mut self,media_type: SdpMediaValue, direction: SdpAttribute, port: u32,
> +                              protocol: SdpProtocolValue, addr: String)

nits: add whitespace after "self,", fix up indent

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/lib.rs:150
(Diff revision 1)
> +pub unsafe extern "C" fn sdp_add_media_section(session: *mut SdpSession,
> +                                               media_type: u32, direction: u32,
> +                                               port: u16, protocol: u32,
> +                                               addr_type: u32, addr: StringView) -> nsresult {
> +
> +    let addr_str:String = match addr.into(){

nit: space before {

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/lib.rs:158
(Diff revision 1)
> +           println!("Error while pasing string, description: {:?}", (*boxed_error).description());
> +           return NS_ERROR_INVALID_ARG;
> +       }
> +    };
> +
> +    let media_type = match media_type{

nit: space before {

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/lib.rs:190
(Diff revision 1)
> +
> +    // Check that the provided address type is valid, the rust parser will find out
> +    // on his own which address type was provided
> +    match addr_type {
> +      // enum AddrType { kAddrTypeNone, kIPv4, kIPv6 };
> +      1...2 => (),

Please add a comment explaining why you exclude kAddrTypeNone here. My first thought is that if it is a valid enumerated value, we should accept it.

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/lib.rs:196
(Diff revision 1)
> +      _ => {
> +          return NS_ERROR_INVALID_ARG;
> +      }
> +    }
> +
> +    match (*session).add_media(media_type,direction,port as u32,protocol,addr_str) {

nit: please add spaces after commas

Comment 3

[Johannes Willbold]

Comment on attachment 8986517 [details]
Bug 1436080: Implemented RsdparsaSdp::AddMediaSection,

https://reviewboard.mozilla.org/r/251842/#review258296

> Please list the types you use rather than using *.

Fixed it. Although I think that this is very time consuming, as these lines have high merge conflict potential.

> Please add a comment explaining why you exclude kAddrTypeNone here. My first thought is that if it is a valid enumerated value, we should accept it.

I assumed that kAddrTypeNone is basically an 'invalid' flag. My asumption is based on this line: https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/sdp/SdpEnum.h#39

Comment 4

[Johannes Willbold]

Comment on attachment 8986517 [details]
Bug 1436080: Implemented RsdparsaSdp::AddMediaSection,

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/301f7d8a5ba2
Implemented RsdparsaSdp::AddMediaSection, r=dminor

Comment 6

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/301f7d8a5ba2