906105 - RTCIceCandidate increments sdpMLineIndex

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[tom]

Attachment: ff-ice-candidate-bug.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20130816030205

Steps to reproduce:

Create a mozRTCIceCandidate with sdpMLineIndex of 1. 


Actual results:

Read out the sdpMLineIndex and see that it is 2.


Expected results:

The sdpMLineIndex should still be 1. I have attached a simple test case. The culprit seems to be this line in dom/media/PeerConnection.js:

this.sdpMLineIndex = ("sdpMLineIndex" in dict)? dict.sdpMLineIndex+1 : null;

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Yeah it's clearly wrong that you get 2 back when you set 1.

Some internal details:

This +1 business predates me touching the code. It almost looks like 1 is added to the input value to distinguish it from "not provided" which is the default. I need to figure out whether the low-level code is expecting it to be plus one and subtracting one when using the value.

If this is the case I can merely subtract one on get().
If this is not the case, I need to figure out which value to use when no value is given in the call.

I was only able to track it to here: http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/sipcc/core/ccapp/cc_call_feature.c#118

Adam, do you know how this should work?

Comment 2

[Adam Roach [:abr]]

What's happening here is the WebRTC spec uses a zero-indexed value for m-lines, while SIPCC uses a one-indexed value. Zero isn't special; it's just not used by SIPCC.

My personal preference would be to add 1 to the index when we actually push it down towards SIPCC in the PeerConnectionImpl, rather than adding 1 when we store it and subtracting 1 when we check it. In terms of distinguishing "0" from "not provided," I would think that "undefined" would serve the purpose of indicating a non-provided value just fine (just remember to use === to compare against undefined, not ==).

____
FWIW, after going through some uninteresting inter-thread queues and data massaging, the data you were chasing comes out here:

http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c#4045
http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c#1618

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Store external 0-based sdpMLineIndex rather than internal 1-based one

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

> My personal preference would be to add 1 to the index when we actually push it
> down towards SIPCC in the PeerConnectionImpl, rather than adding 1 when we
> store it and subtracting 1 when we check it.

Thanks, that's what I went with (assuming you meant where JS pushes down to c++).

> I would think that "undefined" would serve the purpose of indicating a 
> non-provided value just fine (just remember to use === to compare against
> undefined, not ==).

That turned out not to be a problem, as null=not provided in this case. I just misread the webidl.

Comment 5

[Ethan Hugg [:ehugg]]

Comment on attachment 794090 [details] [diff] [review]
Store external 0-based sdpMLineIndex rather than internal 1-based one

Review of attachment 794090 [details] [diff] [review]:
-----------------------------------------------------------------

Built it to double-check and worked fine.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9409e2f9c885

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9409e2f9c885