Closed Bug 1379265 Opened 7 years ago Closed 6 years ago

Write C++ bindings to rsdparsa and integrate into existing SDP code

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: pellenbogen, Assigned: dminor)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

      No description provided.
Rank: 15
Priority: -- → P1
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
Paul, if it's ok I'm going to take this over and get it landed.
Assignee: pe5 → dminor
Sounds good. 
Try job here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bb7c6c83fba54034ae7f230a6762c780d909122

The plan, as discussed with Nils, is to land this as a work-in-progress, preffed off. This will allow multiple people to work together on getting it ready for use. I've filed dependent bugs over at Bug 1365792 for things that I've noticed that are incomplete or not yet working.
Comment on attachment 8945875 [details]
Bug 1379265 - Add RsdparsaSdpParser to sdp_unittests;

https://reviewboard.mozilla.org/r/215964/#review221786


Static analysis found 1 defect in this patch.
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: media/webrtc/signaling/gtest/sdp_unittests.cpp:1562
(Diff revision 1)
>      }
>  
>      // For streaming parse errors
>      std::string GetParseErrors() const {
>        std::stringstream output;
> -      for (auto e = mParser.GetParseErrors().begin();
> +      for (auto e = mSdpErrorHolder->GetParseErrors().begin();

Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]
Comment on attachment 8945873 [details]
Bug 1379265 - Add C++ bindings for rsdparsa;

https://reviewboard.mozilla.org/r/215960/#review221780

So this is just a partial review, but there's some pretty big issues. This is going to take multiple passes.

::: media/webrtc/signaling/src/sdp/RustSdp.cpp:27
(Diff revision 1)
> +namespace mozilla
> +{
> +
> +RustSdp::RustSdp(UniquePtr<RustSdpSession, FreeSdpSession> session,
> +		 const SdpOrigin& origin)
> +  : mSession(Move(session)), mOrigin(origin) {

Newline for the bracket.

::: media/webrtc/signaling/src/sdp/RustSdp.cpp:29
(Diff revision 1)
> +
> +RustSdp::RustSdp(UniquePtr<RustSdpSession, FreeSdpSession> session,
> +		 const SdpOrigin& origin)
> +  : mSession(Move(session)), mOrigin(origin) {
> +  RustAttributeList* attributeList = get_sdp_session_attributes(mSession.get());
> +  UniquePtr<RustSdpSession, FreeSdpSession> attributeSession(sdp_new_reference(mSession.get()));

Wait, so RustSdpSession is a reference to a reference-counted object? Or are we creating something new here? Maybe we need to typedef UniquePtr<RustSdpSession, FreeSdpSession> RustSdpSessionHandle?

::: media/webrtc/signaling/src/sdp/RustSdp.cpp:30
(Diff revision 1)
> +RustSdp::RustSdp(UniquePtr<RustSdpSession, FreeSdpSession> session,
> +		 const SdpOrigin& origin)
> +  : mSession(Move(session)), mOrigin(origin) {
> +  RustAttributeList* attributeList = get_sdp_session_attributes(mSession.get());
> +  UniquePtr<RustSdpSession, FreeSdpSession> attributeSession(sdp_new_reference(mSession.get()));
> +  mAttributeList.reset(new RustSdpAttributeList(Move(attributeSession), attributeList));

If we really want to cache stuff like the RustAttributeList\* inside RustSdpAttributeList, let's allow the c'tor for RustSdpAttributeList fish it out. That said, let's think very carefully about whether it makes sense to cache these things. It makes me worried about memory safety. Same goes for the other classes in the c++ API.

::: media/webrtc/signaling/src/sdp/RustSdp.cpp:34
(Diff revision 1)
> +  UniquePtr<RustSdpSession, FreeSdpSession> attributeSession(sdp_new_reference(mSession.get()));
> +  mAttributeList.reset(new RustSdpAttributeList(Move(attributeSession), attributeList));
> +
> +  size_t section_count = sdp_media_section_count(mSession.get());
> +  for (size_t level = 0; level < section_count; level++) {
> +    RustMediaSection* mediaSection;

The C API uses very similar naming for objects as the C++ wrapper API. This is \_really\_ hard to read and understand. We need an easily distinguishable naming convention here.

::: media/webrtc/signaling/src/sdp/RustSdp.cpp:35
(Diff revision 1)
> +  mAttributeList.reset(new RustSdpAttributeList(Move(attributeSession), attributeList));
> +
> +  size_t section_count = sdp_media_section_count(mSession.get());
> +  for (size_t level = 0; level < section_count; level++) {
> +    RustMediaSection* mediaSection;
> +    sdp_get_media_section(mSession.get(), level, &mediaSection);

We aren't checking return codes here, and on a few other similar functions.

::: media/webrtc/signaling/src/sdp/RustSdp.cpp:78
(Diff revision 1)
> +RustSdp::AddMediaSection(SdpMediaSection::MediaType mediaType,
> +                          SdpDirectionAttribute::Direction dir, uint16_t port,
> +                          SdpMediaSection::Protocol protocol,
> +                          sdp::AddrType addrType, const std::string& addr)
> +{
> +  MOZ_CRASH("Method not implemented");

Do we have a bug filed yet for switching over to using the rust parser? We need a TODO here with that bug number.

::: media/webrtc/signaling/src/sdp/RustSdp.cpp:90
(Diff revision 1)
> +  BandwidthVec* bwVec = sdp_get_session_bandwidth_vec(mSession.get());
> +  RustHeapString* bwLinesHeap;
> +  std::string bwLines = stringViewToString(sdp_serialize_bandwidth(bwVec, &bwLinesHeap));
> +  sdp_free_string(bwLinesHeap);
> +  os << bwLines;

So, given that the view being returned is always going to be the entire thing, is there a way we can just use the RustHeapString? Having this separate string view and memory is begging for memory errors.

::: media/webrtc/signaling/src/sdp/RustSdpAttributeList.h:86
(Diff revision 1)
> +  virtual ~RustSdpAttributeList();
> +
> +protected:
> +
> +private:
> +  explicit RustSdpAttributeList(RustSdpSession* session);

This probably should be removed.

::: media/webrtc/signaling/src/sdp/RustSdpAttributeList.h:87
(Diff revision 1)
> +
> +protected:
> +
> +private:
> +  explicit RustSdpAttributeList(RustSdpSession* session);
> +  RustSdpAttributeList(UniquePtr<RustSdpSession, FreeSdpSession> session,

This is a pretty big departure from the way the Sipcc-based parser does things. There we only supply a handle to the c-api while loading; once loading is done, we throw away the parser. Are we trying to change this model?

::: media/webrtc/signaling/src/sdp/RustSdpAttributeList.h:90
(Diff revision 1)
> +private:
> +  explicit RustSdpAttributeList(RustSdpSession* session);
> +  RustSdpAttributeList(UniquePtr<RustSdpSession, FreeSdpSession> session,
> +                       RustAttributeList* attributes) : RustSdpAttributeList(Move(session), attributes, nullptr, nullptr) {}
> +  RustSdpAttributeList(UniquePtr<RustSdpSession, FreeSdpSession> session,
> +                       RustAttributeList* attributes,

Let's fish this out of the session ourselves, if we want to cache it at all.

::: media/webrtc/signaling/src/sdp/RustSdpAttributeList.h:102
(Diff revision 1)
> +
> +  static const std::string kEmptyString;
> +  static const size_t kNumAttributeTypes = SdpAttribute::kLastAttribute + 1;
> +
> +  UniquePtr<RustSdpSession, FreeSdpSession> mSession;
> +  RustAttributeList* mRustAttributes;

How much overhead are we saving by cacheing this? It looks like it is just a function call, probably not that significant.

::: media/webrtc/signaling/src/sdp/RustSdpAttributeList.h:104
(Diff revision 1)
> +  static const size_t kNumAttributeTypes = SdpAttribute::kLastAttribute + 1;
> +
> +  UniquePtr<RustSdpSession, FreeSdpSession> mSession;
> +  RustAttributeList* mRustAttributes;
> +  const RustSdpAttributeList* mSessionLevel;
> +  const SdpMediaSection* mMediaSection;

We don't need this at all if we can teach the rust parser to use a channels value of 0 from the get-go for video m-sections. Even if we can't do that, we could just store a bool mIsVideo instead.

::: media/webrtc/signaling/src/sdp/RustSdpAttributeList.h:112
(Diff revision 1)
> +  void RemoveAttributeConst(AttributeType type) const;
> +  void SetAttributeConst(SdpAttribute* attr) const;
> +
> +  bool IsAllowedHere(SdpAttribute::AttributeType type) const;
> +  void LoadAll() const;
> +  void LoadAttribute(AttributeType type) const;
> +  void LoadIceUfrag() const;
> +  void LoadIcePwd() const;
> +  void LoadIdentity() const;
> +  void LoadIceOptions() const;
> +  void LoadFingerprint() const;
> +  void LoadSetup() const;
> +  void LoadSsrc() const;
> +  void LoadRtpmap() const;
> +  void LoadFmtp() const;
> +  void LoadPtime() const;
> +  void LoadFlags() const;
> +  void LoadMid() const;
> +  void LoadMsid() const;
> +  void LoadMsidSemantics() const;
> +  void LoadGroup() const;
> +  void LoadRtcp() const;
> +  void LoadImageAttr() const;
> +  void LoadSctpmaps() const;
> +  void LoadDirection() const;
> +  void LoadRemoteCandidates() const;
> +  void LoadRids() const;
> +  void LoadExtmap() const;

Ah, so we're doing lazy-loading? I guess that's ok... assuming that we aren't expecting any of these functions to detect errors.

::: media/webrtc/signaling/src/sdp/RustSdpAttributeList.cpp:26
(Diff revision 1)
> +RustSdpAttributeList::RustSdpAttributeList(RustSdpSession* session) : mSession(Move(session))
> +{
> +  mSession.reset(session);
> +  memset(&mAttributes, 0, sizeof(mAttributes));
> +}

Yeah, this looks dangerous.

::: media/webrtc/signaling/src/sdp/RustSdpAttributeList.cpp:40
(Diff revision 1)
> +RustSdpAttributeList::HasAttribute(AttributeType type,
> +                                    bool sessionFallback) const

Clean up indent, here and in other places.

::: media/webrtc/signaling/src/sdp/RustSdpAttributeList.cpp:50
(Diff revision 1)
> +
> +const SdpAttribute*
> +RustSdpAttributeList::GetAttribute(AttributeType type,
> +                                    bool sessionFallback) const
> +{
> +  LoadAttribute(type);

So, this is the wrinkle we get into with lazy loading. If we call the same getter multiple times in a row, each time we stomp (invalidating what we returned previously) and re-load. There's probably code elsewhere that assumes this is not the case...

::: media/webrtc/signaling/src/sdp/RustSdpAttributeList.cpp:1009
(Diff revision 1)
> +}
> +
> +void
> +RustSdpAttributeList::Serialize(std::ostream& os) const
> +{
> +  LoadAll();

No, don't do this. Do the load sometime before, in a non-const context.

::: media/webrtc/signaling/src/sdp/RustSdpAttributeList.cpp:1009
(Diff revision 1)
> +}
> +
> +void
> +RustSdpAttributeList::Serialize(std::ostream& os) const
> +{
> +  LoadAll();

No, don't do this. Do the load sometime before, in a non-const context.
Attachment #8945873 - Flags: review?(docfaraday)
Comment on attachment 8945871 [details]
Bug 1379265 - Import rsdparsa;

https://reviewboard.mozilla.org/r/215956/#review221846

Are you planning to maintain the crate from inside m-c? If not I'd note that it's sometimes easier to publish from github to crates.io an then use `./mach vendor` to import that into `third_party/rust`. It depends how often you have to make changes to the gecko-side code at the same time which is easier.
Attachment #8945871 - Flags: review?(giles) → review+
Comment on attachment 8945872 [details]
Bug 1379265 - Add C API for rsdparsa;

https://reviewboard.mozilla.org/r/215958/#review221848

r=me for importing the code. Even if the parser itself is vendored, I think it's probably worth maintaining the capi wrapper in the gecko tree.

I made some drive-by style comments below, but it's fine to land this and work on them from in follow-up commits. Let me know if you want more substantive suggestions in that vein.

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs:12
(Diff revision 1)
> +
> +use types::StringView;
> +use network::RustIpAddr;
> +
> +#[repr(C)]
> +#[derive(Clone, Copy, Debug, PartialEq, Eq)]

I notice you `[derive(Debug)]` on most of the structs. NB that can be a code bloat issue; the serializers can be non-obviously large. LTO will strip them out if they're not used, but please be very careful about constructing a `{:?}` interpolation in release code. Not deriving `Debug` in the first place is one way to catch and review instances of that.

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs:57
(Diff revision 1)
> +}
> +
> +impl<'a> From<&'a SdpAttribute> for RustSdpAttributeType {
> +    fn from(other: &SdpAttribute) -> Self {
> +        match *other {
> +            SdpAttribute::BundleOnly{..} => RustSdpAttributeType::BundleOnly,

I remember this code. :)

I think you could do this with a macro, if you like, since the names are all the same and there are several `impl`s like this.

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/lib.rs:42
(Diff revision 1)
> +    let sdp_c_str = match CStr::from_bytes_with_nul(sdp_slice) {
> +        Ok(string) => string,
> +        Err(_) => {
> +            *session = ptr::null();
> +            *error = ptr::null(); // TODO: Give more useful return value here
> +            println!("Error converting string");

It would be better to use the `log` crate here. We already have env_logger in the tree so this will give you better output interleaving as well as being more idiomatic, with a `log!()` or `error!()` here.

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/lib.rs:66
(Diff revision 1)
> +            NS_OK
> +        },
> +        Err(e) => {
> +            *session = ptr::null();
> +            println!("{:?}", e);
> +            println!("Error parsing SDP in rust: {}", e.description());

Aha, here's a `{:?}`. Do the `Debug` impls get used here? If you switch to standard logging you can `debug!` or `trace!` here instead to avoid bloating release builds.
Attachment #8945872 - Flags: review?(giles) → review+
:bwc I didn't have permissions to reply on review board so I'll copy my responses over here. Hopefully my response can help clarify.

> Wait, so RustSdpSession is a reference to a reference-counted object? Or are we creating something new here? Maybe we need to typedef UniquePtr<RustSdpSession, FreeSdpSession> RustSdpSessionHandle?

My memory is a little rusty (har har). Even though I am no longer working on this I will try to answer some of the questions raised to explain my thinking.

I think the reason this baroque use of `UniquePtr` came about because we have multiple references to rust structs in both C++ and Rust. To avoid leaks, we need to ensure the thing is collected when all of both the Rust references and C++ references go out of scope. I didn't know of a way to share a reference counter between languages, so I end up using layering C++ and rust smart pointers. In this case, unique pointers in C++ to rust reference counted objects. The unique pointers destructors essentially decrement the rust reference count. I agree, not ideal.

If it turns out all references into these structs come in from C++, an alternative would to use a raw pointer in rust (created with [`Box::into_raw`](https://doc.rust-lang.org/std/boxed/struct.Box.html#method.into_raw)) and reference count in C++.

Whatever happens, to avoid memory leaks we need to ensure that the struct is dropped in rust, so we always need to call back into rust when we are done with the struct in C++.


> If we really want to cache stuff like the RustAttributeList* inside RustSdpAttributeList, let's allow the c'tor for RustSdpAttributeList fish it out. That said, let's think very carefully about whether it makes sense to cache these things. It makes me worried about memory safety. Same goes for the other classes in the c++ API.

It makes sense to fish the attributes out in the constructor. I think I originally wanted `RustSdpAttributeList::mRustAttributes` const and so passed it in, but it looks like it isn't even const at the moment.


> The C API uses very similar naming for objects as the C++ wrapper API. This is _really_ hard to read and understand. We need an easily distinguishable naming convention here.


When I was writing this 4 months ago, it made sense in my head. Looking at it again, I agree.


> So, given that the view being returned is always going to be the entire thing, is there a way we can just use the RustHeapString? Having this separate string view and memory is begging for memory errors.

Them RustHeapString doesn't actually exist in the rust data structure. A string has to be constructed. In rust, the bandwidth is represented as a `Vec` of [this enum](https://github.com/nils-ohlmeier/rsdparsa/blob/a836501cff59d6a62e7cc067cb31a0682d230e63/src/lib.rs#L20).

I saw two options, serialize in rust (easy) and pass that as a string into C++. Or create a C++ interface to a vec of bandwidth structs, and an interface to the bandwidth struct, then build the string in C++ using those interface. This would add a lot of C++ code. My general rule was to do as much in rust as possible and to move things into C++ as late as possible.

Side note; it would be nice if there were a C++ library with a macro (or something along those lines) that let you access rust Vecs more naturally, rather than having to reinvent the wheel for every type. (Vec<String>, Vec<SdpBandwidth>, Vec<T>, etc...)


> This is a pretty big departure from the way the Sipcc-based parser does things. There we only supply a handle to the c-api while loading; once loading is done, we throw away the parser. Are we trying to change this model?


We chose to depart from the original model. The idea was to move more of the heavy lifting into the Rust side of things, with hopes of writing a lightweight C++ wrapper. There are some parts where the C++ is doing parsing that sipcc should be doing; we moved that to rust. The idea being that every getter would dive right into rust and return whatever rust had. That meant keeping the rust data around until every view into it from C++ went out of scope.

I had to back off from that a little bit because a lot of the getters return references to C++ objects, so we needed to keep things around in C++.

> How much overhead are we saving by cacheing this? It looks like it is just a function call, probably not that significant.

Makes sense. The function call in rust is very cheap, its just accessing a struct attribute.


> Ah, so we're doing lazy-loading? I guess that's ok... assuming that we aren't expecting any of these functions to detect errors.

I believe errors are detected and returned when parsed in rust. Errors are known well before we get to the C++ getters. Nils would know more.


>Yeah, this looks dangerous.

I think this is from the [original implementation](https://searchfox.org/mozilla-central/rev/38bddf549db0f26d4d7575cf695958ff703a3ed7/media/webrtc/signaling/src/sdp/SipccSdpAttributeList.cpp#26). All of the Rust\* files were created by copying the Sipcc version first. This is probably a good opportunity to depart from that.



> So, this is the wrinkle we get into with lazy loading. If we call the same getter multiple times in a row, each time we stomp (invalidating what we returned previously) and re-load. There's probably code elsewhere that assumes this is not the case...

`RustSdpAttributeList::LoadAttribute` checks if attribute `type` is already loaded, and does nothing if there is one loaded.


> No, don't do this. Do the load sometime before, in a non-const context.


I can't remember why I did this. Maybe I was still commited to trying to lazy loading. `LoadAll()` could be moved to the constructor, then the lazy loading can be eliminated from the getters. Then I think the mutable modifier should be removed from `mAttributes`.
Comment on attachment 8945873 [details]
Bug 1379265 - Add C++ bindings for rsdparsa;

https://reviewboard.mozilla.org/r/215960/#review221780

> Let's fish this out of the session ourselves, if we want to cache it at all.

I agree with not cacheing it, but we can get a RustAttributeList from either a session or a mediasection so I think it's easier to pass this in rather than having logic to figure out which case applies and calling the appropriate function in the constructor.
Comment on attachment 8945873 [details]
Bug 1379265 - Add C++ bindings for rsdparsa;

https://reviewboard.mozilla.org/r/215960/#review221780

> We don't need this at all if we can teach the rust parser to use a channels value of 0 from the get-go for video m-sections. Even if we can't do that, we could just store a bool mIsVideo instead.

I switched to using mIsVideo and filed Bug 1436403 to do this in the Rust parser itself.
Comment on attachment 8945872 [details]
Bug 1379265 - Add C API for rsdparsa;

https://reviewboard.mozilla.org/r/215958/#review221848

> I remember this code. :)
> 
> I think you could do this with a macro, if you like, since the names are all the same and there are several `impl`s like this.

Thank you, I'm not going to try to tackle that as part of this landing, but it sounds like a good follow up.
Comment on attachment 8945873 [details]
Bug 1379265 - Add C++ bindings for rsdparsa;

https://reviewboard.mozilla.org/r/215960/#review224530

Partial review, have to do other work now. Will continue tomorrow.

::: media/webrtc/signaling/src/sdp/RsdparsaSdp.h:36
(Diff revision 2)
> +  friend class RsdparsaSdpParser;
> +
> +public:
> +  explicit RsdparsaSdp(RsdparsaSessionHandle session, const SdpOrigin& origin);
> +
> +  virtual const SdpOrigin& GetOrigin() const override;

I know that SipccSdp.h gets this wrong, but let's just use override in this new code.

::: media/webrtc/signaling/src/sdp/RsdparsaSdp.h:80
(Diff revision 2)
> +  PtrVector<RsdparsaSdpMediaSection> mMediaSections;
> +};
> +
> +} // namespace mozilla
> +
> +#endif // _sdp_h_

Nit: fix comment

::: media/webrtc/signaling/src/sdp/RsdparsaSdp.cpp:36
(Diff revision 2)
> +  mAttributeList.reset(new RsdparsaSdpAttributeList(Move(attributeSession), attributeList));
> +
> +  size_t section_count = sdp_media_section_count(mSession.get());
> +  for (size_t level = 0; level < section_count; level++) {
> +    RustMediaSection* mediaSection;
> +    nsresult nr = sdp_get_media_section(mSession.get(), level, &mediaSection);

If there's only one reason this can fail, let's avoid returning an nsresult, and just return nullptr when that error happens.

::: media/webrtc/signaling/src/sdp/RsdparsaSdp.cpp:37
(Diff revision 2)
> +    // Failure here indicates we asked for invalid section, which seems unlikely
> +    // since we just asked for the number of sections above.
> +    if (NS_FAILED(nr)) {
> +      MOZ_ASSERT(false, "Inconsistent number of media sections");

Let's unify the comment and the error string, maybe something like "sdp_get_media_section failed because level was out of bounds, but we did a bounds check!"

::: media/webrtc/signaling/src/sdp/RsdparsaSdp.cpp:44
(Diff revision 2)
> +    if (NS_FAILED(nr)) {
> +      MOZ_ASSERT(false, "Inconsistent number of media sections");
> +      break;
> +    }
> +    RsdparsaSessionHandle newSession(sdp_new_reference(mSession.get()));
> +    RsdparsaSdpMediaSection* sdpMediaSection = new RsdparsaSdpMediaSection(level, Move(newSession), mediaSection, mAttributeList.get());

Make sure we wrap to 80 in here (and other places).

::: media/webrtc/signaling/src/sdp/RsdparsaSdp.cpp:80
(Diff revision 2)
> +RsdparsaSdp::AddMediaSection(SdpMediaSection::MediaType mediaType,
> +                          SdpDirectionAttribute::Direction dir, uint16_t port,
> +                          SdpMediaSection::Protocol protocol,
> +                          sdp::AddrType addrType, const std::string& addr)

Nit: Fixup indent.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.h:64
(Diff revision 2)
> +  uint32_t GetSctpPort() const override;
> +  uint32_t GetMaxMessageSize() const override;

This was probably messed up in the Sipcc code, but let's put these with the simple types below.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.h:83
(Diff revision 2)
> +
> +  void Serialize(std::ostream&) const override;
> +
> +  virtual ~RsdparsaSdpAttributeList();
> +
> +protected:

Remove.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.h:86
(Diff revision 2)
> +  RsdparsaSdpAttributeList(RsdparsaSessionHandle session,
> +                           RustAttributeList* attributes)
> +    : RsdparsaSdpAttributeList(Move(session), attributes, nullptr, false) {}
> +
> +  RsdparsaSdpAttributeList(RsdparsaSessionHandle session,
> +                           RustAttributeList* attributes,
> +                           const RsdparsaSdpAttributeList* sessionLevel,
> +                           bool isVideo)
> +    : mSession(Move(session))
> +    , mSessionLevel(sessionLevel)
> +    , mIsVideo(isVideo)
> +    , mAttributes()
> +  {
> +    LoadAll(attributes);
> +  }

I still feel like the code would be more readable with these two c'tors instead:

explicit RsdparsaSdpAttributeList(RsdparsaSessionHandle session);

RsdparsaSdpAttributeList(
    RsdparsaSessionHandle session,
    const RustMediaSection* const msection,
    const RsdparsaSdpAttributeList* sessionAttributes);

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.h:105
(Diff revision 2)
> +  }
> +
> +  static const std::string kEmptyString;
> +  static const size_t kNumAttributeTypes = SdpAttribute::kLastAttribute + 1;
> +
> +  RsdparsaSessionHandle mSession;

Can this be const?

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:17
(Diff revision 2)
> +extern "C" {
> +#include "signaling/src/sdp/sipcc/sdp_private.h"
> +}
> +

Remove.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:35
(Diff revision 2)
> +  }
> +}
> +
> +bool
> +RsdparsaSdpAttributeList::HasAttribute(AttributeType type,
> +                                   bool sessionFallback) const

Fixup indent, here and other places in this file.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:85
(Diff revision 2)
> +  if (!IsAllowedHere(attr->GetType())) {
> +    MOZ_ASSERT(false, "This type of attribute is not allowed here");
> +    return;

Leaks |attr|, but so does the sipcc version. Might as well fix.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:93
(Diff revision 2)
> +const std::vector<std::string>&
> +RsdparsaSdpAttributeList::GetCandidate() const
> +{
> +  if (!HasAttribute(SdpAttribute::kCandidateAttribute)) {
> +    MOZ_CRASH();
> +  }
> +
> +  return static_cast<const SdpMultiStringAttribute*>(
> +             GetAttribute(SdpAttribute::kCandidateAttribute))->mValues;
> +}

So much copy/paste in these functions... we need to move a bunch of this stuff to the base class, maybe in a separate bug.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:410
(Diff revision 2)
> +RsdparsaSdpAttributeList::LoadAttribute(RustAttributeList *attributeList,
> +                                    AttributeType type)
> +{
> +  if(!mAttributes[type]) {
> +    switch(type) {
> +    case SdpAttribute::kIceUfragAttribute:

Fixup indent in here, and the other switch statements.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:462
(Diff revision 2)
> +      return;
> +    case SdpAttribute::kRtcpAttribute:
> +      LoadRtcp(attributeList);
> +      return;
> +    case SdpAttribute::kImageattrAttribute:
> +      LoadImageAttr(attributeList);

Fix camelcase.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:479
(Diff revision 2)
> +    case SdpAttribute::kDtlsMessageAttribute:
> +    case SdpAttribute::kLabelAttribute:
> +    case SdpAttribute::kMaxptimeAttribute:
> +    case SdpAttribute::kSsrcGroupAttribute:
> +    case SdpAttribute::kMaxMessageSizeAttribute:
> +    case SdpAttribute::kRtcpFbAttribute:
> +    case SdpAttribute::kRtcpRsizeAttribute:
> +    case SdpAttribute::kSctpPortAttribute:
> +    case SdpAttribute::kCandidateAttribute:
> +    case SdpAttribute::kSimulcastAttribute:
> +    case SdpAttribute::kConnectionAttribute:
> +    case SdpAttribute::kIceMismatchAttribute:

We need to implement a bunch of these still.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:498
(Diff revision 2)
> +    }
> +  }
> +}
> +
> +void
> +RsdparsaSdpAttributeList::LoadAll(RustAttributeList *attributeList) {

Put open bracket for functions on a new line.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:511
(Diff revision 2)
> +  StringView ufragStr;
> +  nsresult nr = sdp_get_iceufrag(attributeList, &ufragStr);
> +  if (NS_SUCCEEDED(nr)) {
> +    std::string iceufrag = stringViewToString(ufragStr);
> +    SetAttribute(new SdpStringAttribute(SdpAttribute::kIceUfragAttribute,
> +                                             iceufrag));

Fixup indent, here and in other places.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:563
(Diff revision 2)
> +  }
> +  auto rustFingerprints = MakeUnique<RustSdpAttributeFingerprint[]>(numFingerprints);
> +  sdp_get_fingerprints(attributeList, numFingerprints, rustFingerprints.get());
> +  auto fingerprints = MakeUnique<SdpFingerprintAttributeList>();
> +  for(size_t i = 0; i < numFingerprints; i++) {
> +    RustSdpAttributeFingerprint fingerprint = rustFingerprints[i];

Maybe use a reference here?

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:568
(Diff revision 2)
> +    if (fingerprintBytes.size() == 0) {
> +      // TODO: Should we load fingerprint earlier to detect if it is malformed and throw a proper error?
> +      continue;
> +    }

There's a whole bunch of error checking in the sipcc version here, just want to make sure the rust parser is dealing with that now.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:611
(Diff revision 2)
> +  }
> +  auto rustSsrcs = MakeUnique<RustSdpAttributeSsrc[]>(numSsrc);
> +  sdp_get_ssrcs(attributeList, numSsrc, rustSsrcs.get());
> +  auto ssrcs = MakeUnique<SdpSsrcAttributeList>();
> +  for(size_t i = 0; i < numSsrc; i++) {
> +    RustSdpAttributeSsrc ssrc = rustSsrcs[i];

Use a reference? Here and in other places.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:626
(Diff revision 2)
> +}
> +
> +SdpRtpmapAttributeList::CodecType
> +strToCodecType(const std::string &name) {
> +  auto codec = SdpRtpmapAttributeList::kOtherCodec;
> +    if (name == "opus") {

Let's make these comparisons case-insensitive (see bug 1152093).

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:677
(Diff revision 2)
> +      // the arbitrary value for that code.
> +      // TODO: handle this in Rust parser, see Bug 1436403
> +      channels = 0;
> +    }
> +    rtpmapList->PushEntry(payloadType, codec, name,
> +                          (uint32_t) rtpmap.frequency, channels);

Let's avoid c-style casts in here.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:701
(Diff revision 2)
> +#endif
> +}
> +
> +void
> +RsdparsaSdpAttributeList::LoadPtime(RustAttributeList* attributeList) {
> +  int64_t ptime = sdp_get_ptime(attributeList);

ptime is the duration, measured in ms, of each media packet (only really makes sense in audio). So, this should be unsigned, and probably not 64-bit.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:703
(Diff revision 2)
> +
> +void
> +RsdparsaSdpAttributeList::LoadPtime(RustAttributeList* attributeList) {
> +  int64_t ptime = sdp_get_ptime(attributeList);
> +  if (ptime >= 0) {
> +    SetAttribute(new SdpNumberAttribute(SdpAttribute::kPtimeAttribute, (uint32_t) ptime));

avoid c-style casting

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:783
(Diff revision 2)
> +    return;
> +  }
> +  auto groups = MakeUnique<SdpGroupAttributeList>();
> +  for(size_t i = 0; i < numGroup; i++) {
> +    RustSdpAttributeGroup group = rustGroups[i];
> +    SdpGroupAttributeList::Semantics semantics;

Let's call this variable |semantic|.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:785
(Diff revision 2)
> +    case RustSdpAttributeGroupSemantic ::kRustLipSynchronization:
> +      semantics = SdpGroupAttributeList::kLs;
> +      break;
> +    case RustSdpAttributeGroupSemantic ::kRustFlowIdentification:
> +      semantics = SdpGroupAttributeList::kFid;
> +      break;
> +    case RustSdpAttributeGroupSemantic ::kRustSingleReservationFlow:
> +      semantics = SdpGroupAttributeList::kSrf;
> +      break;
> +    case RustSdpAttributeGroupSemantic ::kRustAlternateNetworkAddressType:
> +      semantics = SdpGroupAttributeList::kAnat;
> +      break;
> +    case RustSdpAttributeGroupSemantic ::kRustForwardErrorCorrection:
> +      semantics = SdpGroupAttributeList::kFec;
> +      break;
> +    case RustSdpAttributeGroupSemantic ::kRustDecodingDependency:
> +      semantics = SdpGroupAttributeList::kDdp; //TODO: Is this a correct mapping?
> +      break;
> +    case RustSdpAttributeGroupSemantic ::kRustBundle:
> +      semantics = SdpGroupAttributeList::kBundle;
> +      break;

I don't see an "other" in here; does the rust parser ignore group attributes with an unknown semantic, or does it fail to parse? Ignoring is fine, failing is not.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:801
(Diff revision 2)
> +      break;
> +    case RustSdpAttributeGroupSemantic ::kRustForwardErrorCorrection:
> +      semantics = SdpGroupAttributeList::kFec;
> +      break;
> +    case RustSdpAttributeGroupSemantic ::kRustDecodingDependency:
> +      semantics = SdpGroupAttributeList::kDdp; //TODO: Is this a correct mapping?

Yes, that's right.

https://tools.ietf.org/html/rfc5888#section-12

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:821
(Diff revision 2)
> +  if (NS_SUCCEEDED(sdp_get_rtcp(attributeList, &rtcp))) {
> +    sdp::AddrType addrType = convertAddressType(rtcp.unicastAddr.addrType);
> +    if (sdp::kAddrTypeNone == addrType) {
> +      SetAttribute(new SdpRtcpAttribute(rtcp.port));
> +    } else {
> +      std::string addr(reinterpret_cast<char *>(rtcp.unicastAddr.unicastAddr));

Do we really need a cast here?

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:823
(Diff revision 2)
> +    if (sdp::kAddrTypeNone == addrType) {
> +      SetAttribute(new SdpRtcpAttribute(rtcp.port));
> +    } else {
> +      std::string addr(reinterpret_cast<char *>(rtcp.unicastAddr.unicastAddr));
> +      SetAttribute(new SdpRtcpAttribute(rtcp.port, sdp::kInternet,
> +                                             addrType, addr));

Fixup indent.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:150
(Diff revision 2)
> +
> +  size_t sdp_get_ssrc_count(RustAttributeList* aList);
> +  void sdp_get_ssrcs(RustAttributeList* aList, size_t listSize, RustSdpAttributeSsrc* ret);
> +
> +  typedef struct RustSdpAttributeRtpmap {
> +    uint32_t payloadType;

RTP payload types are 7 bits.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:152
(Diff revision 2)
> +  void sdp_get_ssrcs(RustAttributeList* aList, size_t listSize, RustSdpAttributeSsrc* ret);
> +
> +  typedef struct RustSdpAttributeRtpmap {
> +    uint32_t payloadType;
> +    StringView codecName;
> +    int64_t frequency;

Unfortunately, there is no BNF for rtpmap as far as I can tell. However, frequency should definitely not be signed, and probably shouldn't be 64-bit.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpMediaSection.cpp:34
(Diff revision 2)
> +  case kRustAudio:
> +    mMediaType = kAudio;
> +    break;
> +  case kRustVideo:
> +    mMediaType = kVideo;
> +    break;
> +  case kRustApplication:
> +    mMediaType = kApplication;
> +    break;

Fix indent.
Comment on attachment 8945873 [details]
Bug 1379265 - Add C++ bindings for rsdparsa;

https://reviewboard.mozilla.org/r/215960/#review224962

More partial review.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:842
(Diff revision 2)
> +  for(size_t i = 0; i < numImageattrs; i++) {
> +    StringView imageAttr = rustImageattrs[i];
> +    std::string image = stringViewToString(imageAttr);
> +    std::string error;
> +    size_t errorPos;
> +    if (!imageattrList->PushEntry(image, &error, &errorPos)) {

I guess we also want to make sure we have a TODO/bug somewhere for rust imageattr parse code?

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:869
(Diff revision 2)
> +  SetAttribute(sctpmapList.release());
> +}
> +
> +void
> +RsdparsaSdpAttributeList::LoadDirection(RustAttributeList* attributeList) {
> +  // TODO: Should this code be moved to GetDirection?

Probably not.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:889
(Diff revision 2)
> +void
> +RsdparsaSdpAttributeList::LoadRemoteCandidates(RustAttributeList* attributeList) {
> +  size_t numCandidates = sdp_get_remote_candidate_count(attributeList);
> +  if (numCandidates == 0) {
> +    return;
> +  }
> +  auto rustCandidates = MakeUnique<RustSdpAttributeRemoteCandidate[]>(numCandidates);
> +  sdp_get_remote_candidates(attributeList, numCandidates,
> +                            rustCandidates.get());
> +  std::vector<SdpRemoteCandidatesAttribute::Candidate> candidates;
> +  for(size_t i = 0; i < numCandidates; i++) {
> +    RustSdpAttributeRemoteCandidate rustCandidate = rustCandidates[i];
> +    SdpRemoteCandidatesAttribute::Candidate candidate;
> +    candidate.port = rustCandidate.port;
> +    candidate.id = std::to_string(rustCandidate.component);
> +    candidate.address = std::string(reinterpret_cast<char *>(&rustCandidate.address.unicastAddr));
> +    candidates.push_back(candidate);
> +  }
> +  SdpRemoteCandidatesAttribute* candidatesList = new SdpRemoteCandidatesAttribute(candidates);
> +  SetAttribute(candidatesList);
> +}

We didn't load a=remote-candidates before (and don't support getting them), is this being confused with a=candidate maybe?

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:925
(Diff revision 2)
> +  auto rids = MakeUnique<SdpRidAttributeList>();
> +  for(size_t i = 0; i < numRids; i++) {
> +    std::string rid = stringViewToString(rustRids[i]);
> +    std::string error;
> +    size_t errorPos;
> +    if (!rids->PushEntry(rid, &error, &errorPos)) {

Do we have a bug/TODO for rust rid parse code?

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:958
(Diff revision 2)
> +      break;
> +    case kRustSendrecv:
> +      direction = SdpDirectionAttribute::kSendrecv;
> +      break;
> +    case kRustInactive:
> +      // TODO: Is this the right thing to do?

Hmm. Not really. "inactive" can appear in extmap. The rust parser is going to need to include a bool flag here. We also probably want to have a direction conversion function that can be used here and in other places.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpGlue.h:1
(Diff revision 2)
> +#ifndef _RUSTSDPGLUE_H_

We need the boilerplate up top.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpGlue.h:19
(Diff revision 2)
> +std::string stringViewToString(StringView str);
> +std::vector<std::string> convertStringVec(StringVec* vec);
> +sdp::AddrType convertAddressType(RustSdpAddrType addr);

Let's use a consistent naming convention for these.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpGlue.cpp:1
(Diff revision 2)
> +#include <string>

Boilerplate here too.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:255
(Diff revision 2)
> +
> +  size_t sdp_get_rid_count(RustAttributeList* aList);
> +  void sdp_get_rids(RustAttributeList* aList, size_t listSize, StringView* ret);
> +
> +  typedef struct RustSdpAttributeExtmap {
> +    uint32_t id;

Let's make this a 16 bit field.
Comment on attachment 8945873 [details]
Bug 1379265 - Add C++ bindings for rsdparsa;

https://reviewboard.mozilla.org/r/215960/#review225776

Yet more partial review.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpGlue.cpp:8
(Diff revision 2)
> +#include "signaling/src/sdp/RsdparsaSdpInc.h"
> +#include "signaling/src/sdp/RsdparsaSdpGlue.h"
> +namespace mozilla
> +{
> +
> +std::string stringViewToString(StringView str) {

Bracket gets its own line.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpGlue.cpp:29
(Diff revision 2)
> +  return ret;
> +}
> +
> +sdp::AddrType convertAddressType(RustSdpAddrType addrType) {
> +  switch(addrType) {
> +  case kRustAddrNone:

Fixup indent.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:1
(Diff revision 2)
> +#ifndef _RUSTSDPINC_H_

Boilerplate.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:41
(Diff revision 2)
> +    size_t len;
> +  } StringView;
> +
> +  typedef struct RustSdpConnection {
> +    RustIpAddr addr;
> +    int64_t ttl;

This should be a uint8_t:

https://tools.ietf.org/html/rfc4566#section-5.7

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:42
(Diff revision 2)
> +  } StringView;
> +
> +  typedef struct RustSdpConnection {
> +    RustIpAddr addr;
> +    int64_t ttl;
> +    int64_t amount;

This should be unsigned.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:72
(Diff revision 2)
> +  enum RustSdpFormatType {
> +    kRustIntegers,
> +    kRustStrings
> +  };
> +
> +  size_t string_vec_len(StringVec* vec);

Lots of args in the functions in this file could be "const", right? How hard would it be to tighten this up? It would make it a little easier to distinguish outparams.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:90
(Diff revision 2)
> +  StringView sdp_get_error_message(RustSdpError* err);
> +  void sdp_free_error(RustSdpError* err);
> +
> +  RustSdpOrigin sdp_get_origin(RustSdpSession* aSess);
> +
> +  typedef struct BandwidthVec BandwidthVec;

Any particular reason this isn't with the other typedefs up top?

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:93
(Diff revision 2)
> +  BandwidthVec* sdp_get_media_bandwidth_vec(const RustMediaSection* aMediaSection);
> +  char* sdp_serialize_bandwidth(BandwidthVec* bandwidths);
> +  bool sdp_session_has_connection(RustSdpSession* aSess);
> +
> +  nsresult sdp_get_session_connection(RustSdpSession* aSess, RustSdpConnection* ret);
> +  RustAttributeList* get_sdp_session_attributes(RustSdpSession* aSess);

Line break in a funny place.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:103
(Diff revision 2)
> +  RustAttributeList* get_sdp_session_attributes(RustSdpSession* aSess);
> +
> +  size_t sdp_media_section_count(RustSdpSession* aSess);
> +  nsresult sdp_get_media_section(RustSdpSession* aSess, size_t aLevel,
> +				 RustMediaSection** aMediaSection);
> +  // Sipcc already has sdp_get_media_type

This needs to be removed, right?

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:112
(Diff revision 2)
> +  StringVec* sdp_get_format_string_vec(const RustMediaSection* aMediaSection);
> +  U32Vec* sdp_get_format_u32_vec(const RustMediaSection* aMediaSection);
> +  uint32_t sdp_get_media_port(const RustMediaSection* aMediaSection);
> +  uint32_t sdp_get_media_port_count(const RustMediaSection* aMediaSection);
> +  uint32_t sdp_get_media_bandwidth(const RustMediaSection* aMediaSection,
> +				  const char* aBandwidthType);

Fixup indent.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:160
(Diff revision 2)
> +
> +  size_t sdp_get_rtpmap_count(RustAttributeList* aList);
> +  void sdp_get_rtpmaps(RustAttributeList* aList, size_t listSize, RustSdpAttributeRtpmap* ret);
> +
> +  typedef struct RustSdpAttributeFmtp {
> +    uint32_t payloadType;

Probably makes sense to make this a uint8_t.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:208
(Diff revision 2)
> +    kRustDecodingDependency,
> +    kRustBundle,
> +  };
> +
> +  typedef struct RustSdpAttributeGroup {
> +    RustSdpAttributeGroupSemantic semantics;

Let's call this |semantic|.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:216
(Diff revision 2)
> +
> +  size_t sdp_get_group_count(RustAttributeList* aList);
> +  nsresult sdp_get_groups(RustAttributeList* aList, size_t listSize, RustSdpAttributeGroup* ret);
> +
> +  typedef struct RustSdpAttributeRtcp {
> +    uint32_t port;

uint16_t

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:226
(Diff revision 2)
> +
> +  size_t sdp_get_imageattr_count(RustAttributeList* aList);
> +  void sdp_get_imageattrs(RustAttributeList* aList, size_t listSize, StringView* ret);
> +
> +  typedef struct RustSdpAttributeSctpmap {
> +    uint32_t port;

uint16_t

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:242
(Diff revision 2)
> +  typedef struct RustSdpAttributeRemoteCandidate {
> +    uint32_t component;
> +    RustIpAddr address;
> +    uint32_t port;
> +  } RustSdpAttributeRemoteCandidate;
> +
> +  size_t sdp_get_remote_candidate_count(RustAttributeList* aList);
> +  void sdp_get_remote_candidates(RustAttributeList* aList, size_t listSize, RustSdpAttributeRemoteCandidate* ret);

Looks like there's some confusion about a=candidate and a=remote-candidates.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:254
(Diff revision 2)
> +  typedef struct RustSdpAttributeExtmap {
> +    uint32_t id;
> +    RustDirection direction;
> +    StringView url;
> +    StringView extensionAttributes;
> +  } RustSdpAttributeExtmap;

This will need a bool field showing whether the direction was specified.
Comment on attachment 8945873 [details]
Bug 1379265 - Add C++ bindings for rsdparsa;

https://reviewboard.mozilla.org/r/215960/#review226060

Ok, first pass is done.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpMediaSection.h:32
(Diff revision 2)
> +  virtual MediaType
> +  GetMediaType() const override

Just use "override" on this stuff.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpMediaSection.cpp:7
(Diff revision 2)
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Bug 1433534 tracks cleaning up the TODOs in the file

These bugs need to have their description updated to match the rename of these files.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpMediaSection.cpp:48
(Diff revision 2)
> +  }
> +
> +  RsdparsaSessionHandle attributeSession(sdp_new_reference(mSession.get()));
> +  RustAttributeList* attributeList = sdp_get_media_attribute_list(section);
> +  mAttributeList.reset(new RsdparsaSdpAttributeList(Move(attributeSession),
> +                                                attributeList,

Fixup indent.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpMediaSection.cpp:139
(Diff revision 2)
> +void
> +RsdparsaSdpMediaSection::AddCodec(const std::string& pt, const std::string& name,
> +                               uint32_t clockrate, uint16_t channels)
> +{
> +
> +}
> +
> +void
> +RsdparsaSdpMediaSection::ClearCodecs()
> +{
> +
> +}
> +
> +void
> +RsdparsaSdpMediaSection::AddDataChannel(const std::string& name, uint16_t port,
> +                                     uint16_t streams, uint32_t message_size)
> +{
> +}

We need some TODOs here. Also fixup indent.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpMediaSection.cpp:177
(Diff revision 2)
> +
> +  if (mConnection) {
> +    os << *mConnection;
> +  }
> +
> +  // mBandwidths.Serialize(os);

Remove?

::: media/webrtc/signaling/src/sdp/RsdparsaSdpMediaSection.cpp:208
(Diff revision 2)
> +    StringVec* vec = sdp_get_format_string_vec(mSection);
> +    mFormats = convertStringVec(vec);
> +  }
> +}
> +
> +UniquePtr<SdpConnection> convertRustConnection(RustSdpConnection conn) {

Bracket on newline.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpMediaSection.cpp:209
(Diff revision 2)
> +    mFormats = convertStringVec(vec);
> +  }
> +}
> +
> +UniquePtr<SdpConnection> convertRustConnection(RustSdpConnection conn) {
> +  std::string addr(reinterpret_cast<char *>(&conn.addr.unicastAddr));

Is this cast necessary? Isn't unicastAddr already a char array?

::: media/webrtc/signaling/src/sdp/RsdparsaSdpMediaSection.cpp:210
(Diff revision 2)
> +  sdp::AddrType type = sdp::AddrType::kAddrTypeNone;
> +  if (conn.addr.addrType == kRustAddrIp4) {
> +    type = sdp::AddrType::kIPv4;
> +  } else if (conn.addr.addrType == kRustAddrIp6) {
> +    type = sdp::AddrType::kIPv6;
> +  }

Use convertAddressType here.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpMediaSection.cpp:216
(Diff revision 2)
> +  // TODO: Should this be on the rust side?
> +  uint8_t ttl = 0;
> +  if (conn.ttl >= 0) {
> +    ttl = conn.ttl;
> +  }
> +  uint32_t count = 0;
> +  if (conn.amount >= 0) {
> +    count = conn.amount;
> +  }

Yeah, if possible.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpMediaSection.cpp:239
(Diff revision 2)
> +    nr = sdp_get_media_connection(mSection, &conn);
> +    if (NS_SUCCEEDED(nr)) {
> +      mConnection = convertRustConnection(conn);
> +    }
> +  } else if (sdp_session_has_connection(mSession.get())){
> +    // TODO: Does rsdparsa need to ensure there is a connection at the session level if it is missing at a media level?

It probably makes sense to do that there, yes, since that's a base-spec error.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpMediaSection.cpp:245
(Diff revision 2)
> +    nr = sdp_get_session_connection(mSession.get(), &conn);
> +    if (NS_SUCCEEDED(nr)) {
> +      mConnection = convertRustConnection(conn);
> +    }
> +  }
> +  MOZ_ASSERT(mConnection);

Let's not assert on invalid SDP. We probably need a way to indicate errors encountered in the various Load functions, and that means we probably need a separate Load function like SipccSdpMediaSection.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpParser.cpp:28
(Diff revision 2)
> +{
> +  ClearParseErrors();
> +  const char* rawString = sdpText.c_str();
> +  RustSdpSession* result;
> +  RustSdpError* err;
> +  nsresult rv = parse_sdp(rawString, sdpText.length() + 1, false,

Why +1? Does rust care about the null terminator?

::: media/webrtc/signaling/src/sdp/RsdparsaSdpParser.cpp:31
(Diff revision 2)
> +  RustSdpSession* result;
> +  RustSdpError* err;
> +  nsresult rv = parse_sdp(rawString, sdpText.length() + 1, false,
> +                          &result, &err);
> +  if (rv != NS_OK) {
> +    if (err != nullptr) { // TODO: err should eventually never be null if rv != NS_OK

Need a bug for this TODO

::: media/webrtc/signaling/src/sdp/RsdparsaSdpParser.cpp:37
(Diff revision 2)
> +      size_t line = sdp_get_error_line_num(err);
> +      std::string errMsg = stringViewToString(sdp_get_error_message(err));
> +      sdp_free_error(err);
> +      AddParseError(line, errMsg);
> +    }
> +    return nullptr;

We probably want some sort of placeholder error if there isn't one from rust.

::: media/webrtc/signaling/src/sdp/moz.build:41
(Diff revision 2)
> +    # Building these as part of the unified build leads to multiply defined
> +    # symbols on windows.
> +    'RsdparsaSdp.cpp',
> +    'RsdparsaSdpAttributeList.cpp',
> +    'RsdparsaSdpGlue.cpp',
> +    'RsdparsaSdpMediaSection.cpp',
> +    'RsdparsaSdpParser.cpp',

From where?
Attachment #8945873 - Flags: review?(docfaraday) → review-
Comment on attachment 8945873 [details]
Bug 1379265 - Add C++ bindings for rsdparsa;

https://reviewboard.mozilla.org/r/215960/#review224530

> There's a whole bunch of error checking in the sipcc version here, just want to make sure the rust parser is dealing with that now.

Nope, I filed Bug 1437169 for this under the assumption we'd want to do it in the rust parser. If not, maybe we can move the error checking to a base class shared by both parsers.

> ptime is the duration, measured in ms, of each media packet (only really makes sense in audio). So, this should be unsigned, and probably not 64-bit.

The rust parser is returning -1 here to indicate a missing attribute, so using a 64 bit int allows for 32 bits of valid values and an error code. We could change this, but we'd have to change the rust parser too. I think the intention is to continue to develop the rust parser on GitHub, so I'd prefer to defer any kind of change like this to a follow up, rather than fixing it there, and redoing the import as part of this bug.
Comment on attachment 8945873 [details]
Bug 1379265 - Add C++ bindings for rsdparsa;

https://reviewboard.mozilla.org/r/215960/#review226060

> From where?

These were the multiply defined symbols:

21:23:03     INFO -  Unified_cpp_signaling_src_sdp0.obj : error LNK2005: "public: unsigned int __cdecl std::_Atomic_uint::fetch_sub(unsigned int,enum std::memory_order)" (?fetch_sub@_Atomic_uint@std@@QEAAIIW4memory_order@2@@Z) already defined in UnifiedProtocols1.obj
21:23:03     INFO -  Unified_cpp_gfx_skia8.obj : error LNK2005: "public: bool __cdecl std::_Atomic_int::compare_exchange_weak(int &,int,enum std::memory_order)" (?compare_exchange_weak@_Atomic_int@std@@QEAA_NAEAHHW4memory_order@2@@Z) already defined in Unified_cpp_signaling_src_sdp0.obj
21:23:03     INFO -  js_static.lib(Interpreter.obj) : error LNK2005: "public: unsigned int __cdecl std::_Atomic_uint::load(enum std::memory_order)const " (?load@_Atomic_uint@std@@QEBAIW4memory_order@2@@Z) already defined in Unified_cpp_signaling_src_sdp0.obj
21:23:03     INFO -     Creating library xul.lib and object xul.exp
21:23:03     INFO -  xul.dll : fatal error LNK1169: one or more multiply defined symbols found

At the time, I didn't have a Windows build set up and debugging this on Try looked painful.
Comment on attachment 8945873 [details]
Bug 1379265 - Add C++ bindings for rsdparsa;

https://reviewboard.mozilla.org/r/215960/#review226060

> Why +1? Does rust care about the null terminator?

Yes, inside parse_sdp the string is converted using CStr::from_bytes_with_nul which checks for a null byte at the end.
Comment on attachment 8945873 [details]
Bug 1379265 - Add C++ bindings for rsdparsa;

https://reviewboard.mozilla.org/r/215960/#review225776

> Lots of args in the functions in this file could be "const", right? How hard would it be to tighten this up? It would make it a little easier to distinguish outparams.

Done. It appears that these functions already took const pointers on the Rust side, so I've tidied up this file to match.

> This will need a bool field showing whether the direction was specified.

Filed Bug 1438536.
Comment on attachment 8945873 [details]
Bug 1379265 - Add C++ bindings for rsdparsa;

https://reviewboard.mozilla.org/r/215960/#review224530

> I don't see an "other" in here; does the rust parser ignore group attributes with an unknown semantic, or does it fail to parse? Ignoring is fine, failing is not.

std::string sdp =
    "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
    "t=0 0" CRLF
    "a=group:UNKNOWN first second" CRLF
    "m=video 56436 RTP/SAVPF 120" CRLF
    "a=rtpmap:120 VP8/90000";

The above test string causes a parse error, I just wanted to make sure it was an ok test before filing a bug for this.
Comment on attachment 8945873 [details]
Bug 1379265 - Add C++ bindings for rsdparsa;

https://reviewboard.mozilla.org/r/215960/#review226060

> It probably makes sense to do that there, yes, since that's a base-spec error.

Filed Bug 1438539
Comment on attachment 8945873 [details]
Bug 1379265 - Add C++ bindings for rsdparsa;

https://reviewboard.mozilla.org/r/215960/#review224962

> Hmm. Not really. "inactive" can appear in extmap. The rust parser is going to need to include a bool flag here. We also probably want to have a direction conversion function that can be used here and in other places.

Filed Bug 1438544.
Comment on attachment 8945873 [details]
Bug 1379265 - Add C++ bindings for rsdparsa;

https://reviewboard.mozilla.org/r/215960/#review225776

> Looks like there's some confusion about a=candidate and a=remote-candidates.

Filed Bug 1438545.
Setting needinfo for https://bugzilla.mozilla.org/show_bug.cgi?id=1379265#c43 so it doesn't get lost in the shuffle.
Flags: needinfo?(docfaraday)
Comment on attachment 8945873 [details]
Bug 1379265 - Add C++ bindings for rsdparsa;

https://reviewboard.mozilla.org/r/215960/#review224530

> std::string sdp =
>     "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
>     "t=0 0" CRLF
>     "a=group:UNKNOWN first second" CRLF
>     "m=video 56436 RTP/SAVPF 120" CRLF
>     "a=rtpmap:120 VP8/90000";
> 
> The above test string causes a parse error, I just wanted to make sure it was an ok test before filing a bug for this.

So "first" and "second" aren't valid mids in there. I would expect the parse to fail because of that. If you take a known valid SDP, and put an a=group:UNKNOWN <some valid mid> in there, it should still parse.
Comment on attachment 8945873 [details]
Bug 1379265 - Add C++ bindings for rsdparsa;

https://reviewboard.mozilla.org/r/215960/#review224530

> So "first" and "second" aren't valid mids in there. I would expect the parse to fail because of that. If you take a known valid SDP, and put an a=group:UNKNOWN <some valid mid> in there, it should still parse.

Thanks! Filed Bug 1438574.
Comment on attachment 8945873 [details]
Bug 1379265 - Add C++ bindings for rsdparsa;

https://reviewboard.mozilla.org/r/215960/#review226900

r+ with fixes. Need to make one final pass over the whole thing instead of the interdiff, hold tight.

::: media/webrtc/signaling/src/sdp/RsdparsaSdp.cpp:36
(Diff revisions 2 - 3)
>  
>    size_t section_count = sdp_media_section_count(mSession.get());
>    for (size_t level = 0; level < section_count; level++) {
> -    RustMediaSection* mediaSection;
> -    nsresult nr = sdp_get_media_section(mSession.get(), level, &mediaSection);
> -    // Failure here indicates we asked for invalid section, which seems unlikely
> +    RustMediaSection* mediaSection = sdp_get_media_section(mSession.get(), level);
> +    if (!mediaSection) {
> +      MOZ_ASSERT(false, "sdp_get_media_section failed because level was out of bounds, but we did a bounds check!");

Nit: Wrap to 80.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:46
(Diff revisions 2 - 3)
>      size_t len;
>    } StringView;
>  
>    typedef struct RustSdpConnection {
>      RustIpAddr addr;
> -    int64_t ttl;
> +    int8_t ttl;

Needs to be uint_8

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:115
(Diff revisions 2 - 3)
>    StringVec* sdp_get_format_string_vec(const RustMediaSection* aMediaSection);
>    U32Vec* sdp_get_format_u32_vec(const RustMediaSection* aMediaSection);
>    uint32_t sdp_get_media_port(const RustMediaSection* aMediaSection);
>    uint32_t sdp_get_media_port_count(const RustMediaSection* aMediaSection);
>    uint32_t sdp_get_media_bandwidth(const RustMediaSection* aMediaSection,
> -				  const char* aBandwidthType);
> +								  const char* aBandwidthType);

Eek! Tabs!

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:233
(Diff revisions 2 - 3)
>  
> -  nsresult sdp_get_rtcp(RustAttributeList* aList, RustSdpAttributeRtcp* ret);
> +  nsresult sdp_get_rtcp(const RustAttributeList* aList,
> +                        RustSdpAttributeRtcp* ret);
>  
> -  size_t sdp_get_imageattr_count(RustAttributeList* aList);
> +  size_t sdp_get_imageattr_count(const RustAttributeList* aList);
>    void sdp_get_imageattrs(RustAttributeList* aList, size_t listSize, StringView* ret);

const here, right?

::: media/webrtc/signaling/src/sdp/RsdparsaSdpParser.cpp:49
(Diff revisions 2 - 3)
>    RustSdpOrigin rustOrigin = sdp_get_origin(uniqueResult.get());
>    sdp::AddrType addrType = convertAddressType(rustOrigin.addr.addrType);
> -  SdpOrigin origin(stringViewToString(rustOrigin.username),
> +  SdpOrigin origin(convertStringView(rustOrigin.username),
>                     rustOrigin.sessionId, rustOrigin.sessionVersion,
>                     addrType,
>                     std::string(reinterpret_cast<char *> (&rustOrigin.addr.unicastAddr)));

We can remove this cast too, right?
Attachment #8945873 - Flags: review?(docfaraday)
Comment on attachment 8945873 [details]
Bug 1379265 - Add C++ bindings for rsdparsa;

https://reviewboard.mozilla.org/r/215960/#review226908

r+ with bunches of formatting nits.

::: media/webrtc/signaling/src/sdp/RsdparsaSdp.h:48
(Diff revision 3)
> +  {
> +    return sdp_media_section_count(mSession.get());
> +  }
> +
> +  const SdpAttributeList&
> +  GetAttributeList() const override {

Nit: Fix bracket.

::: media/webrtc/signaling/src/sdp/RsdparsaSdp.h:53
(Diff revision 3)
> +  GetAttributeList() const override {
> +    return *mAttributeList;
> +  }
> +
> +  SdpAttributeList&
> +  GetAttributeList() override {

Nit: Fix bracket.

::: media/webrtc/signaling/src/sdp/RsdparsaSdp.cpp:34
(Diff revision 3)
> +  RsdparsaSessionHandle attributeSession(sdp_new_reference(mSession.get()));
> +  mAttributeList.reset(new RsdparsaSdpAttributeList(Move(attributeSession)));
> +
> +  size_t section_count = sdp_media_section_count(mSession.get());
> +  for (size_t level = 0; level < section_count; level++) {
> +    RustMediaSection* mediaSection = sdp_get_media_section(mSession.get(), level);

Nit: Wrap.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:544
(Diff revision 3)
> +{
> +  StringVec* options;
> +  nsresult nr = sdp_get_iceoptions(attributeList, &options);
> +  if (NS_SUCCEEDED(nr)) {
> +    std::vector<std::string> optionsVec;
> +    auto optionsAttr = MakeUnique<SdpOptionsAttribute>(SdpAttribute::kIceOptionsAttribute);

Nit: Wrap.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:561
(Diff revision 3)
> +{
> +  size_t numFingerprints = sdp_get_fingerprint_count(attributeList);
> +  if (numFingerprints == 0) {
> +    return;
> +  }
> +  auto rustFingerprints = MakeUnique<RustSdpAttributeFingerprint[]>(numFingerprints);

Nit: Wrap.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:571
(Diff revision 3)
> +    std::string algorithm = convertStringView(fingerprint.hashAlgorithm);
> +    std::string fingerprintToken = convertStringView(fingerprint.fingerprint);
> +    std::vector<uint8_t> fingerprintBytes =
> +      SdpFingerprintAttributeList::ParseFingerprint(fingerprintToken);
> +    if (fingerprintBytes.size() == 0) {
> +      // TODO: Should we load fingerprint earlier to detect if it is malformed and throw a proper error?

Wrap to 80.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:771
(Diff revision 3)
> +  auto rustMsidSemantics = MakeUnique<RustSdpAttributeMsidSemantic[]>(numMsidSemantic);
> +  sdp_get_msid_semantics(attributeList, numMsidSemantic, rustMsidSemantics.get());

Nit: Wrap.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:914
(Diff revision 3)
> +{
> +  size_t numCandidates = sdp_get_remote_candidate_count(attributeList);
> +  if (numCandidates == 0) {
> +    return;
> +  }
> +  auto rustCandidates = MakeUnique<RustSdpAttributeRemoteCandidate[]>(numCandidates);

Wrap.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:923
(Diff revision 3)
> +  for(size_t i = 0; i < numCandidates; i++) {
> +    RustSdpAttributeRemoteCandidate& rustCandidate = rustCandidates[i];
> +    SdpRemoteCandidatesAttribute::Candidate candidate;
> +    candidate.port = rustCandidate.port;
> +    candidate.id = std::to_string(rustCandidate.component);
> +    candidate.address = std::string(reinterpret_cast<char *>(&rustCandidate.address.unicastAddr));

Can we do without this cast? If not, wrap to 80.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:926
(Diff revision 3)
> +  SdpRemoteCandidatesAttribute* candidatesList = new SdpRemoteCandidatesAttribute(candidates);
> +  SetAttribute(candidatesList);

Nit: Wrap to 80, or maybe combine.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:984
(Diff revision 3)
> +        // TODO: Fix this, see Bug 1438544
> +        direction = SdpDirectionAttribute::kSendrecv;
> +        directionSpecified = false;
> +        break;
> +    }
> +    std::string extensionAttributes = convertStringView(rustExtmap.extensionAttributes);

Nit: wrap

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:78
(Diff revision 3)
> +    kRustIntegers,
> +    kRustStrings
> +  };
> +
> +  size_t string_vec_len(const StringVec* vec);
> +  nsresult string_vec_get_view(const StringVec* vec, size_t index, StringView* str);

Nit: wrap

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:98
(Diff revision 3)
> +  RustSdpOrigin sdp_get_origin(const RustSdpSession* aSess);
> +
> +  uint32_t get_sdp_bandwidth(const RustSdpSession* aSess,
> +                             const char* aBandwidthType);
> +  BandwidthVec* sdp_get_session_bandwidth_vec(const RustSdpSession* aSess);
> +  BandwidthVec* sdp_get_media_bandwidth_vec(const RustMediaSection* aMediaSection);

Nit: wrap

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:106
(Diff revision 3)
> +  RustMediaSection* sdp_get_media_section(const RustSdpSession* aSess, size_t aLevel);
> +  RustSdpMediaValue sdp_rust_get_media_type(const RustMediaSection* aMediaSection);
> +  RustSdpProtocolValue sdp_get_media_protocol(const RustMediaSection* aMediaSection);

Nit: wrap

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:117
(Diff revision 3)
> +  nsresult sdp_get_media_connection(const RustMediaSection* aMediaSection, RustSdpConnection* ret);
> +
> +  RustAttributeList* sdp_get_media_attribute_list(const RustMediaSection* aMediaSection);

Nit: wrap

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:132
(Diff revision 3)
> +    StringView hashAlgorithm;
> +    StringView fingerprint;
> +  } RustSdpAttributeFingerprint;
> +
> +  size_t sdp_get_fingerprint_count(const RustAttributeList* aList);
> +  void sdp_get_fingerprints(const RustAttributeList* aList, size_t listSize, RustSdpAttributeFingerprint* ret);

Nit: wrap

::: media/webrtc/signaling/src/sdp/RsdparsaSdpInc.h:233
(Diff revision 3)
> +
> +  nsresult sdp_get_rtcp(const RustAttributeList* aList,
> +                        RustSdpAttributeRtcp* ret);
> +
> +  size_t sdp_get_imageattr_count(const RustAttributeList* aList);
> +  void sdp_get_imageattrs(RustAttributeList* aList, size_t listSize, StringView* ret);

Nit: wrap

::: media/webrtc/signaling/src/sdp/RsdparsaSdpMediaSection.cpp:26
(Diff revision 3)
> +RsdparsaSdpMediaSection::RsdparsaSdpMediaSection(size_t level,
> +                                         RsdparsaSessionHandle session,
> +                                         const RustMediaSection* const section,
> +                                         const RsdparsaSdpAttributeList* sessionLevel)

Nit: Fix indent and wrap.

::: media/webrtc/signaling/src/sdp/RsdparsaSdpMediaSection.cpp:76
(Diff revision 3)
> +
> +SdpMediaSection::Protocol
> +RsdparsaSdpMediaSection::GetProtocol() const
> +{
> +  switch(sdp_get_media_protocol(mSection)) {
> +  case kRustRtpSavpf:

Nit: fix indent

::: media/webrtc/signaling/src/sdp/RsdparsaSdpMediaSection.cpp:138
(Diff revision 3)
> +{
> +  return SdpDirectionAttribute(mAttributeList->GetDirection());
> +}
> +
> +void
> +RsdparsaSdpMediaSection::AddCodec(const std::string& pt, const std::string& name,

Nit: wrap

::: media/webrtc/signaling/src/sdp/RsdparsaSdpParser.cpp:31
(Diff revision 3)
> +  RustSdpSession* result;
> +  RustSdpError* err;
> +  nsresult rv = parse_sdp(rawString, sdpText.length() + 1, false,
> +                          &result, &err);
> +  if (rv != NS_OK) {
> +     // TODO: err should eventually never be null if rv != NS_OK, see Bug 1433529

Nit: wrap

::: media/webrtc/signaling/src/sdp/RsdparsaSdpParser.cpp:49
(Diff revision 3)
> +  RustSdpOrigin rustOrigin = sdp_get_origin(uniqueResult.get());
> +  sdp::AddrType addrType = convertAddressType(rustOrigin.addr.addrType);
> +  SdpOrigin origin(convertStringView(rustOrigin.username),
> +                   rustOrigin.sessionId, rustOrigin.sessionVersion,
> +                   addrType,
> +                   std::string(reinterpret_cast<char *> (&rustOrigin.addr.unicastAddr)));

Do we need this cast? If so, wrap.
Attachment #8945873 - Flags: review+
Flags: needinfo?(docfaraday)
Comment on attachment 8945874 [details]
Bug 1379265 - Add RsdparsaSdpParser to JsepSessionImp;

https://reviewboard.mozilla.org/r/215962/#review227528

r+ with minor fixes

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1269
(Diff revision 4)
> +#if 0
> +    // TODO: Register these telemetry probes, see Bug 1432955
> +    // See https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe
> +    if (!parsed && !rustParsed) {
> +      Telemetry::Accumulate(Telemetry::HistogramDUMMY1, // Telemetry::WEBRTC_RUST_SDP_PARSE_STATUS,
> +			    3);
> +    } else if (parsed && !rustParsed) {
> +      Telemetry::Accumulate(Telemetry::HistogramDUMMY1, // Telemetry::WEBRTC_RUST_SDP_PARSE_STATUS,
> +			    2);
> +    } else if (!parsed && rustParsed) {
> +      Telemetry::Accumulate(Telemetry::HistogramDUMMY1, // Telemetry::WEBRTC_RUST_SDP_PARSE_STATUS,
> +			    1);
> +    } else {
> +      Telemetry::Accumulate(Telemetry::HistogramDUMMY1, // Telemetry::WEBRTC_RUST_SDP_PARSE_STATUS,
> +			    0);
> +      if (mSdpHelper.SdpMatch(*parsed, *rustParsed)) {
> +        Telemetry::Accumulate(Telemetry::HistogramDUMMY1, // Telemetry::WEBRTC_RUST_SDP_PARSE_MATCH,
> +			      1);
> +      } else {
> +        Telemetry::Accumulate(Telemetry::HistogramDUMMY1, // Telemetry::WEBRTC_RUST_SDP_PARSE_MATCH,
> +			      0);
> +      }
> +    }
> +#endif

Let's not put code here, put it in bug 1432955 instead.

::: media/webrtc/signaling/src/sdp/SdpHelper.cpp:822
(Diff revision 4)
> +bool
> +AttributeListMatch(const SdpAttributeList& list1, const SdpAttributeList& list2)
> +{

Let's make this function static.

::: media/webrtc/signaling/src/sdp/SdpHelper.cpp:822
(Diff revision 4)
> +bool
> +AttributeListMatch(const SdpAttributeList& list1, const SdpAttributeList& list2)
> +{
> +  // TODO: Consider adding telemetry in this function to record which
> +  // attributes don't match. See Bug 1432955.
> +  for (int i = SdpAttribute::kFirstAttribute; i <= SdpAttribute::kLastAttribute; i++) {
> +    auto attributeType = static_cast<SdpAttribute::AttributeType>(i);
> +    if (list1.HasAttribute(attributeType, false) != list2.HasAttribute(attributeType, false)) {
> +      return false;
> +    }
> +  }
> +  return true;
> +}

We need a TODO (with bug number) somewhere for more thorough checking. Perhaps based on serializing the pair of attributes and string comparison?

::: media/webrtc/signaling/src/sdp/SdpHelper.cpp:829
(Diff revision 4)
> +{
> +  // TODO: Consider adding telemetry in this function to record which
> +  // attributes don't match. See Bug 1432955.
> +  for (int i = SdpAttribute::kFirstAttribute; i <= SdpAttribute::kLastAttribute; i++) {
> +    auto attributeType = static_cast<SdpAttribute::AttributeType>(i);
> +    if (list1.HasAttribute(attributeType, false) != list2.HasAttribute(attributeType, false)) {

Nit: Wrap to 80.

::: media/webrtc/signaling/src/sdp/SdpHelper.cpp:836
(Diff revision 4)
> +bool
> +MediaSectionMatch(const SdpMediaSection& mediaSection1,
> +                  const SdpMediaSection& mediaSection2)

Let's make this function static too.

::: media/webrtc/signaling/src/sdp/SdpHelper.cpp:836
(Diff revision 4)
> +bool
> +MediaSectionMatch(const SdpMediaSection& mediaSection1,
> +                  const SdpMediaSection& mediaSection2)
> +{
> +  if (!AttributeListMatch(mediaSection1.GetAttributeList(),
> +			  mediaSection2.GetAttributeList())) {
> +    return false;
> +  }
> +  if (mediaSection1.GetPort() != mediaSection2.GetPort()) {
> +    return false;
> +  }
> +  const std::vector<std::string>& formats1 = mediaSection1.GetFormats();
> +  const std::vector<std::string>& formats2 = mediaSection2.GetFormats();
> +  auto formats1Set = std::set<std::string>(formats1.begin(), formats1.end());
> +  auto formats2Set = std::set<std::string>(formats2.begin(), formats2.end());
> +  if (formats1Set != formats2Set) {
> +    return false;
> +  }
> +  return true;
> +}

We need a TODO for more thorough checking.
Attachment #8945874 - Flags: review?(docfaraday) → review+
Comment on attachment 8945875 [details]
Bug 1379265 - Add RsdparsaSdpParser to sdp_unittests;

https://reviewboard.mozilla.org/r/215964/#review227534

LGTM.
Attachment #8945875 - Flags: review?(docfaraday) → review+
Comment on attachment 8945873 [details]
Bug 1379265 - Add C++ bindings for rsdparsa;

https://reviewboard.mozilla.org/r/215960/#review225776

> uint16_t

This ended up causing me some trouble. After the reviews, I made a final try push and I ended up getting consistent failures parsing rtcp, but only on try. After I spent some time I realized that maybe there was newer version of the rust toolchain.  After a rustup upgrade, I could reproduce the failure locally. With some debugging it was apparent that everything was ok on the Rust side, but the data was mangled on the C++ side. Then I wondered about a struct padding mismatch between C++ and Rust. Sure enough, changing this back to a 32 bit value fixed things, so it looks like one side or the other was inserting some padding to make the next field align to a 32 bit boundary.

I'm going to change this back to uint32_t for now. I filed Bug 1440689 to have a deeper look at this.
 Backed out for GTest crashes on RsdparsaSdpAttributeList::GetGroup

backout: https://hg.mozilla.org/mozilla-central/rev/1f44de15ffdb1c0ac9c2719efa2bbe12ea75d06b

push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=addf903ba0158f04e78b57ac98856d3ef291b02e

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=164104166&repo=mozilla-inbound&lineNumber=310543

task 2018-02-24T09:06:10.127Z] 09:06:10     INFO -  Hit MOZ_CRASH() at /builds/worker/workspace/build/src/media/webrtc/signaling/src/sdp/RsdparsaSdpAttributeList.cpp:168
[task 2018-02-24T09:06:10.127Z] 09:06:10     INFO -  #01: test::NewSdpTest_CheckGroups_Test::TestBody [media/webrtc/signaling/gtest/sdp_unittests.cpp:2964]
[task 2018-02-24T09:06:10.127Z] 09:06:10     INFO -  #02: testing::Test::Run [testing/gtest/gtest/src/gtest.cc:2483]
[task 2018-02-24T09:06:10.128Z] 09:06:10     INFO -  #03: testing::TestInfo::Run [testing/gtest/gtest/src/gtest.cc:2658]
[task 2018-02-24T09:06:10.128Z] 09:06:10     INFO -  #04: testing::TestCase::Run [testing/gtest/gtest/src/gtest.cc:2776]
[task 2018-02-24T09:06:10.128Z] 09:06:10     INFO -  #05: testing::internal::UnitTestImpl::RunAllTests [testing/gtest/gtest/src/gtest.cc:4649]
[task 2018-02-24T09:06:10.128Z] 09:06:10     INFO -  #06: testing::UnitTest::Run [testing/gtest/gtest/src/gtest.cc:3877]
[task 2018-02-24T09:06:10.128Z] 09:06:10     INFO -  #07: mozilla::RunGTestFunc [xpcom/base/nsCOMPtr.h:424]
[task 2018-02-24T09:06:10.129Z] 09:06:10     INFO -  #08: XREMain::XRE_mainStartup [toolkit/xre/nsAppRunner.cpp:3884]
[task 2018-02-24T09:06:10.129Z] 09:06:10     INFO -  #09: XREMain::XRE_main [toolkit/xre/nsAppRunner.cpp:4800]
[task 2018-02-24T09:06:10.129Z] 09:06:10     INFO -  #10: XRE_main [toolkit/xre/nsAppRunner.cpp:4906]
[task 2018-02-24T09:06:10.129Z] 09:06:10     INFO -  #11: do_main [browser/app/nsBrowserApp.cpp:231]
[task 2018-02-24T09:06:10.130Z] 09:06:10     INFO -  #12: main [browser/app/nsBrowserApp.cpp:306]
[task 2018-02-24T09:06:10.130Z] 09:06:10     INFO -  #13: libc.so.6 + 0x20830
[task 2018-02-24T09:06:10.130Z] 09:06:10     INFO -  #14: _start
[task 2018-02-24T09:06:10.130Z] 09:06:10     INFO -  ExceptionHandler::GenerateDump cloned child 14640
[task 2018-02-24T09:06:10.130Z] 09:06:10     INFO -  ExceptionHandler::SendContinueSignalToChild sent continue signal to child
[task 2018-02-24T09:06:10.131Z] 09:06:10     INFO -  ExceptionHandler::WaitForContinueSignal waiting for continue signal...
[task 2018-02-24T09:06:10.131Z] 09:06:10     INFO -  mozcrash INFO | Copy/paste: /usr/local/bin/linux64-minidump_stackwalk /builds/worker/workspace/build/tests/gtest/57912c40-76c3-b724-c7b6-814d75001652.dmp /builds/worker/workspace/build/symbols
[task 2018-02-24T09:06:10.131Z] 09:06:10     INFO -  mozcrash INFO | Saved minidump as /builds/worker/workspace/build/blobber_upload_dir/57912c40-76c3-b724-c7b6-814d75001652.dmp
[task 2018-02-24T09:06:10.131Z] 09:06:10     INFO -  mozcrash INFO | Saved app info as /builds/worker/workspace/build/blobber_upload_dir/57912c40-76c3-b724-c7b6-814d75001652.extra
[task 2018-02-24T09:06:10.132Z] 09:06:10  WARNING -  PROCESS-CRASH | gtest | application crashed [@ mozilla::RsdparsaSdpAttributeList::GetGroup]
[task 2018-02-24T09:06:10.132Z] 09:06:10     INFO -  Crash dump filename: /builds/worker/workspace/build/tests/gtest/57912c40-76c3-b724-c7b6-814d75001652.dmp
[task 2018-02-24T09:06:10.132Z] 09:06:10     INFO -  Operating system: Linux
[task 2018-02-24T09:06:10.132Z] 09:06:10     INFO -                    0.0.0 Linux 4.4.0-98-generic #121~14.04.1-Ubuntu SMP Wed Oct 11 11:54:55 UTC 2017 x86_64
[task 2018-02-24T09:06:10.133Z] 09:06:10     INFO -  CPU: amd64
[task 2018-02-24T09:06:10.133Z] 09:06:10     INFO -       family 6 model 62 stepping 4
[task 2018-02-24T09:06:10.133Z] 09:06:10     INFO -       4 CPUs
[task 2018-02-24T09:06:10.133Z] 09:06:10     INFO -  GPU: UNKNOWN
[task 2018-02-24T09:06:10.134Z] 09:06:10     INFO -  Crash reason:  SIGSEGV
[task 2018-02-24T09:06:10.134Z] 09:06:10     INFO -  Crash address: 0x0
[task 2018-02-24T09:06:10.134Z] 09:06:10     INFO -  Process uptime: not available
Status: RESOLVED → REOPENED
Flags: needinfo?(dminor)
Resolution: FIXED → ---
Target Milestone: mozilla60 → ---
Comment on attachment 8945872 [details]
Bug 1379265 - Add C API for rsdparsa;

https://reviewboard.mozilla.org/r/215958/#review228856

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/attribute.rs:548
(Diff revision 4)
> +    }).collect();
> +    let groups = slice::from_raw_parts_mut(ret_groups, ret_size);
> +    groups.copy_from_slice(attrs.as_slice());
> +}
> +
> +pub struct RustSdpAttributeRtcp {

This is missing `repr(C)`. Please consider autogenerating the bindings instead of writing by hand, there are tools for both directions used in mozilla-central already. bindgen for going C++ -> Rust, and cbindgen for going Rust -> C++.

In this case cbindgen wouldn't have generated the C++ equivalent of this struct.

::: media/webrtc/signaling/src/sdp/rsdparsa_capi/src/lib.rs:154
(Diff revision 4)
> +        RustSdpTiming {start: timing.start, stop: timing.stop}
> +    }
> +}
> +
> +#[no_mangle]
> +pub unsafe extern "C" fn has_timing(session: *const SdpSession) -> bool {

These symbols are really generic, please consider using more namespaced symbols (`sdp_session_has_timing`?)
Depends on: 1441826
I'll look at this again once the soft code freeze is over.
Flags: needinfo?(dminor)
Comment on attachment 8945872 [details]
Bug 1379265 - Add C API for rsdparsa;

https://reviewboard.mozilla.org/r/215958/#review228856

> This is missing `repr(C)`. Please consider autogenerating the bindings instead of writing by hand, there are tools for both directions used in mozilla-central already. bindgen for going C++ -> Rust, and cbindgen for going Rust -> C++.
> 
> In this case cbindgen wouldn't have generated the C++ equivalent of this struct.

It might be worth noting that this code was written several months ago. I think at this point it's more helpful to land the hand written code. And then file a follow up bug to switch to use generated code.
(In reply to Nils Ohlmeier [:drno] from comment #68)
> Comment on attachment 8945872 [details]
> Bug 1379265 - Add C API for rsdparsa;
> 
> https://reviewboard.mozilla.org/r/215958/#review228856
> 
> > This is missing `repr(C)`. Please consider autogenerating the bindings instead of writing by hand, there are tools for both directions used in mozilla-central already. bindgen for going C++ -> Rust, and cbindgen for going Rust -> C++.
> > 
> > In this case cbindgen wouldn't have generated the C++ equivalent of this struct.
> 
> It might be worth noting that this code was written several months ago. I
> think at this point it's more helpful to land the hand written code. And
> then file a follow up bug to switch to use generated code.

I would agree with that. I compared the generated output to the hand written output for the structures and found no further differences. I'll also look at the function declarations, but it seems less likely that they would differ.
I've disabled the problematic test and things look green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f87c7334a3d6e77ae7399c136d635203d382701

I've filed Bug 1444354 to track the disabled test.

I'm going to try landing this again early next week.
Blocks: 1456374
You need to log in before you can comment on or make changes to this bug.