Issue 674103  is also about sdpMLineIndex but I filed a separate bug because I don't know if it makes sense to fix together or not.

It's likely that this works because mid overrides mLineIndex and is always present in Chrome-generated SDP, so the sdpMLineIndex never got looked at.

0 is a valid value for sdpmLineIndex - it indicates the first m-line.

http://w3c.github.io/webrtc-pc/#dom-rtcicecandidate-sdpmlineindex

Suggestion: add a layout test where you set up a connection, but remove the mid element from all the candidates as they are passed.
That should exercise the code involved.

Reassigning to hbos@ for reassessing.

The interface does not support optional:
https://cs.chromium.org/chromium/src/third_party/webrtc/api/jsep.h?type=cs&sq=package:chromium&l=62

And at first glance it looks like the assumption is that it is a valid value, e.g:
https://cs.chromium.org/chromium/src/third_party/webrtc/pc/peerconnection.cc?type=cs&sq=package:chromium&l=5245

deadbeef@ do we need to update third_party/webrtc code?

Not sure what the significance of it is, at addIceCandidate():
https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-addicecandidate

"If candidate.sdpMLineIndex is equal to or larger than the number of media descriptions in remoteDescription, reject p with a newly created OperationError and abort these steps."

It sounds like it doesn't matter what its value is as long as it is not out of bounds? Does default 0 actually lead to expected behavior?

I assume "In parallel, add the ICE candidate candidate as described in [JSEP] (section 4.1.17.)." will fail when the index is wrong but I have not read it. I'm guessing we need to update the interface to support optional.

Defaulting to 0 is bad not because it's illegal but because it's legal.
It means that all candidates that don't have mid or mLineIndex specified end up pointing at the first m-line instead of being rejected as they should.

@hbos: We could update the interface to use rtc::Optional, though that would require changing a bunch of existing code. We could also just say "-1 represents an absent value", which was our convention before rtc::Optional came around.

Whenever we need to change things up a bit I'd like to push for optional but not worth it for some little thing, -1 is fine for this.

I guess we don't need to change things up a bit and make it a reference counted class because all its members are readonly attributes and we might as well copy it?

Hmm when are these values updated?

What makes more sense, a ref counted object referenced, and each time you want to use a readonly attribute you ask the webrtc layer object for its current value...

Or its not ref counted and copied by value. Any time the ice candidate is modified an event is sent to Chrome to update its JavaScript version of it?

Sorry, I'm not following your last two comments. What needs to be reference-counted, and how does it relate to this bug?

Sorry I was talking about webrtc::IceCandidateInterface if that should be reference counted or not to align with the JavaScript layer version of the ice candidate. If there is any risk of the application holding on to an ice candidate and then its state is updated in the lower layer.

It does not relate to this bug other than if it turns out we have to make breaking changes to IceCandidateInterface we could add Optional<> while we're at it.

IceCandidateInterface doesn't need to be reference-counted; it's meant to be immutable, there's no danger of its state being updated.

Okay awesome :)