Closed Bug 1322358 Opened 3 years ago Closed 3 years ago

All media lines get disabled in answer SDP when all m-lines in offer are inactive

Categories

(Core :: WebRTC: Signaling, defect, P2, major)

50 Branch
x86_64
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed
Blocking Flags:

People

(Reporter: ushunmugan, Assigned: bwc)

References

Details

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.75 Safari/537.36

Steps to reproduce:

During a WebRTC video call, a new offer was received by Firefox, with all m-lines having a=inactive.  After doing setRemoteDescription, an answer was created.


Actual results:

The answer SDP had all m-lines disabled with port numbers set to 0, and had no SSRC attributes.  Note that this behavior is always reproducible.  The testing was done with latest stable version of Firefox (50.0.2).


Expected results:

All m-lines should have valid ports, with a=inactive and SSRC attributes.  This would ensure ICE connection remains, and RTCP is sent and received.
Severity: normal → major
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64
Product: Firefox → Core
Component: Untriaged → WebRTC: Signaling
Byron what are your thoughts on this?
Status: UNCONFIRMED → NEW
backlog: --- → webrtc/webaudio+
Rank: 25
Ever confirmed: true
Flags: needinfo?(docfaraday)
Priority: -- → P2
Pretty sure this is not the behavior we want. I'll look into it.
Assignee: nobody → docfaraday
Flags: needinfo?(docfaraday)
Any update on this?  I hope that this issue gets looked at soon and hopefully resolved.  Due to this issue, hold/resume of a call wouldn't work on Firefox with a SIP endpoint, connected via a WebRTC gateway. FYI, the same scenario works fine on Chrome. As a workaround, we had to add a hack on the client side, which modifies a=inactive to a=sendonly in the offer SDP before doing the setRemoteDescription. However, this is not good, as the change is hacky, and the stream isn't really made inactive.
Flags: needinfo?(docfaraday)
Comment on attachment 8830540 [details]
Bug 1322358: Don't disable m-sections in answer just because they're inactive.

https://reviewboard.mozilla.org/r/107294/#review108460

Assuming the answer to my question below is yes.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 1)
> -  if (!msection.IsReceiving() && !msection.IsSending()) {
> -    mSdpHelper.DisableMsection(sdp, &msection);
> -    return NS_OK;
> -  }

Do we have code which still handles the case of missing direction attributes (which appears to me the functionality we loose by deleting this)?
Attachment #8830540 - Flags: review?(drno) → review+
Comment on attachment 8830540 [details]
Bug 1322358: Don't disable m-sections in answer just because they're inactive.

https://reviewboard.mozilla.org/r/107294/#review108460

> Do we have code which still handles the case of missing direction attributes (which appears to me the functionality we loose by deleting this)?

We interpret missing direction attributes as a=sendrecv, as in the spec, so we're fine there.
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c913bf6e11c4
Don't disable m-sections in answer just because they're inactive. r=drno
https://hg.mozilla.org/mozilla-central/rev/c913bf6e11c4
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Flags: needinfo?(docfaraday)
Is this fixed in Firefox nightly already?  I was testing today with the latest nightly version, 54.0a1 (2017-02-07), and I still see the same issue.
BTW, there seems to be some new video issue in nightly as well as in developer (53.0a2) versions, where video is not played after a renegotiation.  This problem doesn't exist on version 50, or beta 52.0b4.
(In reply to ushunmugan from comment #10)
> BTW, there seems to be some new video issue in nightly as well as in
> developer (53.0a2) versions, where video is not played after a
> renegotiation.  This problem doesn't exist on version 50, or beta 52.0b4.

Please file a separate bug; we know renegotiation work in 53, but perhaps there's a bug in a specific case, or perhaps there's a bug in the test you're running.  Major rework of the media and renegotiation code landed in 53.

(In reply to ushunmugan from comment #9)
> Is this fixed in Firefox nightly already?  I was testing today with the
> latest nightly version, 54.0a1 (2017-02-07), and I still see the same issue.

the original bug is supposed to be fixed in Nightly/54 - nils?
Flags: needinfo?(ushunmugan)
Flags: needinfo?(drno)
(In reply to Randell Jesup [:jesup] from comment #11)
> (In reply to ushunmugan from comment #9)
> > Is this fixed in Firefox nightly already?  I was testing today with the
> > latest nightly version, 54.0a1 (2017-02-07), and I still see the same issue.
> 
> the original bug is supposed to be fixed in Nightly/54 - nils?

I'm not the one who fixed it. But it looks like Byron's fix landed in Nightly. Byron?
So if it's not working I guess we should re-open this bug.

Is there a test page available where we re-produce this issue?
Flags: needinfo?(drno) → needinfo?(docfaraday)
There are currently only three cases where we will disable an m-section, looking at the code:

1. We are creating an offer, have no track to send, and are not configured to offer to receive.
2. We are creating an answer, and the m-section was disabled (port==0 and not bundle-only) in the offer.
3. We are creating an answer, and negotiation has failed for that m-section.

Can I get a test-case that I can run that reproduces this for you?
Flags: needinfo?(docfaraday)
I have a feeling what we're seeing here is case 1 in the above comment. I _think_ we should just do a=inactive in that case.
Ok, I think the behavior in case 1 is correct; when creating a new offer, if the transceiver is stopped, the m-section gets disabled. We don't have transceivers yet, so it is a little muddy, but removing the local track _and_ not offering to receive seems about the same.
Hi Byron,

Thanks!  I think that it's case #1 too.  I couldn't give you a test script right now, but it should be very easy to reproduce.  After an audio-only call is established, do a setRemoteDescription with an offer SDP that has a valid m-line (with non-zero port) with a=inactive set, and create the answer - the answer will have the m-line disabled with the port set to 0.  Here is an example:

This is the new offer SDP from peer:
v=0
o=xxx 833 3 IN IP4 0.0.0.0
s=-
c=IN IP4 0.0.0.0
t=0 0
a=group:BUNDLE sdparta_0
a=msid-semantic: WMS F9C4D094-2D57-4072-877C-94B6FDB63273
m=audio 36000 RTP/SAVPF 9 101
a=inactive
a=rtcp-mux
a=mid:sdparta_0
a=ssrc:316236972 cname:HOkMnEjWnUih9fX/fyrWM+UR
a=ssrc:316236972 label:a1-D094-2D57-407
a=ssrc:316236972 msid:F9C4D094-2D57-4072-877C-94B6FDB63273 a1-D094-2D57-407
a=ssrc:316236972 mslabel:F9C4D094-2D57-4072-877C-94B6FDB63273
a=ice-ufrag:sCah
a=ice-pwd:WePX71BPcCZghb2sSOIz38
a=rtpmap:9 g722/8000
a=rtpmap:101 telephone-event/8000
a=fmtp:101 0-15
a=fingerprint:sha-256 EB:95:EC:03:8B:21:39:58:81:10:8D:9D:0A:B1:3C:6D:DC:08:9D:59:59:3A:51:8A:BC:88:80:C8:4F:FE:6D:4F

This is the answer SDP created by Firefox nightly 54.0a1:
v=0
o=mozilla...THIS_IS_SDPARTA-54.0a1 3442797379914774747 1 IN IP4 0.0.0.0
s=-
t=0 0
a=fingerprint:sha-256 30:70:F0:B6:ED:2A:36:F9:2B:7D:BC:AE:DD:30:D7:E2:CD:A7:91:C5:56:F3:50:FF:59:0A:E3:32:59:2F:2C:0B
a=ice-options:trickle
a=msid-semantic:WMS *
m=audio 0 RTP/SAVPF 0
c=IN IP4 0.0.0.0
a=inactive
a=rtpmap:0 PCMU/8000
I agree with comment #14 (just set a=inactive but don't disable the m-line), and I am not sure what is exactly meant by comment #15.  Note, with an inactive m-line, RTP should stop, but RTCP should continue to flow.
FYI, re. the media issue mentioned in comment #10, I've created issue 1338005.
Flags: needinfo?(ushunmugan)
See Also: → 1338005
You need to log in before you can comment on or make changes to this bug.