1213773 - properly handle a=inactive in the remote SDP during renegotiation

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Anil Kumar]

Attachment: WebRTC.log

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

Steps to reproduce:

firefox v41.0.1
chrome v46.0.2490.64 beta-m (64-bit)

1. Make webRTC - webRTC audio call(from chrome to firefox)
2. Call connected and two way media path is successful
3. set the call to hold(firefox) by creating the offer again. Before applying it to setLocalDescription , i modified the SDP to have "a =inactive" and applied successfully


Actual results:

Step 3 was successful and the SDP was applied to setLocalDescription. But the problem is ,even though i changed the media attribute(direction) to "inactive" ,i still see packets flowing from this end .


Expected results:

Step 3 should be successful and after changing the media Attribute to "inactive" , there should not be any packet flow from this end.

Comment 1

[Anil Kumar]

first i tried to modify the initial SDP generated during call. But i saw this bug https://bugzilla.mozilla.org/show_bug.cgi?id=1196701, which said you have to create new offer/answer in order to modify any media attributes(direction)before applying to setLocalDescription.

Comment 2

[rajkumaradass]

I too have observed same problem that the packets are being sent after doing hold(recreate offer, modify SDP(to have inactive) and setlocaldescription).

Comment 3

[Anil Kumar]

Any updates on this issue ???

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

We generally don't allow modification of the SDP for a LocalDescription (see the other bug).  You can ask that an SDP be generated such that the value is what you want.  

> I too have observed same problem that the packets are being sent after doing hold(recreate offer, modify SDP(to have inactive) and setlocaldescription).

You have to do more than just SetLocalDescription, you must send it to the far side and get a response and install it with SetRemoteDescription for it to go into effect.  I.e. you must complete re-negotiation - and the far side could reject it.


However, for Hold, you should not need to do a re-invite (and a re-invite for Hold is problematic anyways, as it's asynchronous and can be slow, and in the meantime you wouldn't be "on hold".  The best way to do it is to simply send track.enabled = false for all tracks, causing them to generate silence/black immediately.  

For a much fancier version, you could use RTPSender.replaceTrack() and captureStream() on a canvas or a media element to provide some sort of Hold music or a static slate.  Note that I think these are only available in Firefox right now, though they're being standardized in the WGs.  (canvas.captureStream() isn't available in Release yet, in fact).

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

It's unclear what exactly you're doing without more details. See comment 4

Comment 6

[Anil Kumar]

Attachment: WebRTC.log

Comment 7

[Anil Kumar]

we made changes as mentioned in comment 4(createOffer --> modify SDP to have "inactive" direction attribute --> setLocalDescription --> sent Modified SDP to FarEnd --> recieved SDP from FarEnd --> applied the SDP to setRemoteDescription) , but still we see packets flowing from Firefox end to other side. I have attached the debug logs and about:webrtc dump.

I can not mute the Tracks as it will affect multiple call(ie., Same localStream is used in multiple peerconnections) scenarios.

please let me know if any more information is required.

Comment 8

[Anil Kumar]

Attachment: aboutWebrtc.html

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Anil Kumar from comment #7)
> we made changes as mentioned in comment 4(createOffer --> modify SDP to have
> "inactive" direction attribute --> setLocalDescription --> sent Modified SDP
> to FarEnd --> recieved SDP from FarEnd --> applied the SDP to
> setRemoteDescription) , but still we see packets flowing from Firefox end to
> other side. I have attached the debug logs and about:webrtc dump.

Nothing you can do will affect the localdescription used (we don't let you modify it currently).  If the far end responds with a=inactive (or a=sendonly), we shouldn't send data to them.  Otherwise we will.   You can try modifying the far-end's response as well (switching it to a=inactive or a=sendonly) before SetRemoteDescription().

From the SDP you put up, both sides show as a=inactive, so we shouldn't be sending media.  Note that you'll never see 0 packets - RTCP is still sent in both directions for a=inactive.  I also still see a global a=sendrecv (which should be overridden, but still it might confuse something).

I modified https://mozilla.github.io/webrtc-landing/pc_test.html to replace a=sendrecv with a=inactive, and it appears to work.  about:webrtc shows no packets going through.  This is on Nightly/44; please try either Developer Edition (aka Aurora), or Nightly, or both.

> I can not mute the Tracks as it will affect multiple call(ie., Same
> localStream is used in multiple peerconnections) scenarios.

Ok.

Comment 10

[Anil Kumar]

Thanks Randell for your responses.

I tried the below approach as you mentioned in your previous comment.

For Hold,

step1: modified the FarEnds SDP to "a=inactive" and applied it to setRemoteDescription
step2: after applying Remote SDP ,we created an answer(SDP generated with audio port "0") and applied the same to setLocalDescription.

i do not see any SRTP packets flowing. So hold is successful.

For Unhold.

step1: modified the FarEnds SDP to "a=sendrecv" and applied it to setRemoteDescription
step2: after applying Remote SDP ,we created an answer(SDP generated with ealier negotiated audio port "60806") and applied the same to setLocalDescription.

i do not see any speech path or any packets flowing. I believe this is due to ice checking happening for different ports(60807) after unhold instead of (60806). 

I have attached wireshark traces and debug logs.

Comment 11

[Anil Kumar]

Attachment: sniffer.pcapng

sniffer

Comment 12

[Anil Kumar]

Attachment: nspr.log .child-1

debug logs

Comment 13

[Anil Kumar]

(In reply to Anil Kumar from comment #10)
> Thanks Randell for your responses.
> 
> I tried the below approach as you mentioned in your previous comment.
> 
> For Hold,
> 
> step1: modified the FarEnds SDP to "a=inactive" and applied it to
> setRemoteDescription
> step2: after applying Remote SDP ,we created an answer(SDP generated with
> audio port "0") and applied the same to setLocalDescription.
> 
> i do not see any SRTP packets flowing. So hold is successful.
> 
> For Unhold.
> 
> step1: modified the FarEnds SDP to "a=sendrecv" and applied it to
> setRemoteDescription
> step2: after applying Remote SDP ,we created an answer(SDP generated with
> ealier negotiated audio port "60806") and applied the same to
> setLocalDescription.
> 
> i do not see any speech path or any packets flowing. I believe this is due
> to ice checking happening for different ports(60807) after unhold instead of
> (60806). 
> 
> I have attached wireshark traces and debug logs.

tested in nightly v44.0a1 (2015-10-22)

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

> i do not see any speech path or any packets flowing. I believe this is due to ice checking happening for different ports(60807) after unhold instead of (60806). 
> 
> I have attached wireshark traces and debug logs.

Nils, Byron, could you have a look?

Comment 15

[Byron Campen [:bwc]]

(In reply to Anil Kumar from comment #10)
> Thanks Randell for your responses.
> 
> I tried the below approach as you mentioned in your previous comment.
> 
> For Hold,
> 
> step1: modified the FarEnds SDP to "a=inactive" and applied it to
> setRemoteDescription
> step2: after applying Remote SDP ,we created an answer(SDP generated with
> audio port "0") and applied the same to setLocalDescription.
> 
> i do not see any SRTP packets flowing. So hold is successful.
> 
> For Unhold.
> 
> step1: modified the FarEnds SDP to "a=sendrecv" and applied it to
> setRemoteDescription
> step2: after applying Remote SDP ,we created an answer(SDP generated with
> ealier negotiated audio port "60806") and applied the same to
> setLocalDescription.
> 
> i do not see any speech path or any packets flowing. I believe this is due
> to ice checking happening for different ports(60807) after unhold instead of
> (60806). 
> 
> I have attached wireshark traces and debug logs.

   When you disable an m-section, the ICE context for that m-section is destroyed, and the ports are closed (unless that m-section does not "own" the ICE context because of bundle). You cannot reuse it.

Comment 16

[rajkumaradass]

There's no explicit bundle policy used here and all the approaches of SDP munging works good in chrome browser - with which addition of features(especially when multiple calls are On) are feasible. This blocks all renegotiation cases as well media upgrade(audio only call to audio+video call) and downgrade(audio+video call to a audio only call) cases as well.

Comment 17

[Byron Campen [:bwc]]

(In reply to rajkumaradass from comment #16)
> There's no explicit bundle policy used here and all the approaches of SDP
> munging works good in chrome browser - with which addition of
> features(especially when multiple calls are On) are feasible. This blocks
> all renegotiation cases as well media upgrade(audio only call to audio+video
> call) and downgrade(audio+video call to a audio only call) cases as well.

   Renegotiation works fine. Adding/removing tracks is something we test in CI hundreds of times a day. The issue here is what kinds of SDP rewriting we support. Sadly, there is literally no specification on what should be supported. Every implementer is making it up as they go along.

Comment 18

[Byron Campen [:bwc]]

(In reply to Anil Kumar from comment #10)
> Thanks Randell for your responses.
> 
> I tried the below approach as you mentioned in your previous comment.
> 
> For Hold,
> 
> step1: modified the FarEnds SDP to "a=inactive" and applied it to
> setRemoteDescription
> step2: after applying Remote SDP ,we created an answer(SDP generated with
> audio port "0") and applied the same to setLocalDescription.
> 
> i do not see any SRTP packets flowing. So hold is successful.
> 
> For Unhold.
> 
> step1: modified the FarEnds SDP to "a=sendrecv" and applied it to
> setRemoteDescription
> step2: after applying Remote SDP ,we created an answer(SDP generated with
> ealier negotiated audio port "60806") and applied the same to
> setLocalDescription.
> 
> i do not see any speech path or any packets flowing. I believe this is due
> to ice checking happening for different ports(60807) after unhold instead of
> (60806). 
> 
> I have attached wireshark traces and debug logs.


Please try:

1. At the Firefox end, call CreateOffer and SetLocal (you can rewrite to a=inactive, but that won't be necessary to prevent Firefox from transmitting RTP).
2. Rewrite the remote answer to have a=inactive, and SetRemote.

   This should result in no RTP in either direction, although RTCP will still be flowing (less than one a second). If this is not the result, please attach the output of about:webrtc, a pcap, and an nspr logfile from the same run.

Comment 19

[Maire Reavy [:mreavy]]

Please see comment 18.

Comment 20

[rajkumaradass]

I have tried as mentioned in comment 18, but still see outbound RTP going. Upon closer look after at the about:webrtc dump after hold(on both local-offer and remote-answer), see there is a session level direction attribute(a=sendrecv) being present although this was not present in the SDP while i modifying the recreated offer(for hold) - this may be a reason for not stopping the outbound RTP.

I have attached debug logs here with file names suffixed as 28Oct2015.

thanks

Comment 21

[rajkumaradass]

Attachment: aboutWebrtc_28Oct2015.html

Comment 22

[rajkumaradass]

Attachment: nspr_28Oct2015.log.child-1

Comment 23

[rajkumaradass]

Attachment: wireshark_28Oct2015

AEC logging

Comment 24

[rajkumaradass]

Attachment: ff_hold.pcapng

wireshark on desktop

Comment 25

[rajkumaradass]

Just clarifing my comment 20, I see there is a session level direction attribute being present 'a=sendrecv' in about:webrtc although this was not present in both the actual local(resulting after createOffer)/remote SDPs.

Comment 26

[Byron Campen [:bwc]]

(In reply to rajkumaradass from comment #20)
> I have tried as mentioned in comment 18, but still see outbound RTP going.
> Upon closer look after at the about:webrtc dump after hold(on both
> local-offer and remote-answer), see there is a session level direction
> attribute(a=sendrecv) being present although this was not present in the SDP
> while i modifying the recreated offer(for hold) - this may be a reason for
> not stopping the outbound RTP.

   Don't worry about the session-level a=sendrecv; that is always there, implicitly or explicitly (if there is no direction attribute, the implicit default according to the spec is sendrecv), and will be overridden if there is a direction attribute at the media level (which there is).

   I does appear that we aren't handling a=inactive in the remote SDP during renegotiation. I'll have to look into a fix.

Comment 27

[rajkumaradass]

Thanks for confirming it. Could you please also let us know when this fix could be possibly be available

Comment 28

[rajkumaradass]

Could you please let us know the targeted release version for this fix.

thanks.

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

Once we do a patch for it and land it, you'll see the Target Milestone set to the current Nightly rev at that time; that's the revision when it would be expected to hit release unless we feel it's important to uplift it to Developer Edition or Beta, which I doubt this would be.  With priority P2, we might fix it in the next few months, but that's a very rough estimate.  Patches are always welcome!

Comment 30

[Maire Reavy [:mreavy]]

To the reporter: this doesn't have a target release currently.  Only P1 bugs or high-level features have true targets.  P2 bugs are typically fixed within 2 to 3 releases of being reported, but no guarantees.  As Randell says, patches are always welcome.

If you feel a bug is needed sooner or needs a target release and you can't work on it yourself, please explain why it's needed in the bug and "needinfo" me.  

I'm going to re-title this "properly handle a=inactive in the remote SDP during renegotiation" since I believe that's the bug we're talking about.

Comment 31

[Anil Kumar]

This bug has blocked our enterprise based mid call features(Hold, Unhold, MultiplesCalls, Transfer, Mid-Call renegotiations especially where direction attribute is changed at the far end) support on Firefox for quite a while now. Because of this our product(WebRTC based Enterprise solution) has not officially supported on Firefox browser(Only on chrome/opera browsers).

This should be a common issue seen with other WebRTC supported enterprise products as well(as VOIP endpoints do mid-call negotiations based on the direction attribute only). We would request to move the priority of this bug to high so as to have this available soon.

Comment 32

[Anil Kumar]

This bug has blocked our enterprise based mid call features(Hold, Unhold, MultiplesCalls, Transfer, Mid-Call renegotiations especially where direction attribute is changed at the far end) support on Firefox for quite a while now. Because of this our product(WebRTC based Enterprise solution) has not officially supported on Firefox browser(Only on chrome/opera browsers).

This should be a common issue seen with other WebRTC supported enterprise products as well(as VOIP endpoints do mid-call negotiations based on the direction attribute only). We would request to move the priority of this bug to high so as to have this available soon.

Comment 33

[Anil Kumar]

This bug has blocked our enterprise based mid call features(Hold, Unhold, MultiplesCalls, Transfer, Mid-Call renegotiations especially where direction attribute is changed at the far end) support on Firefox for quite a while now. Because of this our product(WebRTC based Enterprise solution) has not officially supported on Firefox browser(Only on chrome/opera browsers).

This should be a common issue seen with other WebRTC supported enterprise products as well(as VOIP endpoints do mid-call negotiations based on the direction attribute only). We would request to move the priority of this bug to high so as to have this available soon.

Comment 34

[Maire Reavy [:mreavy]]

Byron -- can you give me a ballpark estimate of how involved a fix for this would be?  (Specifically, I'm asking what it would take to properly handle a=inactive in the remote SDP during renegotiation.)  Thanks!

Comment 35

[Byron Campen [:bwc]]

I'm not totally sure. I suspect it won't be very hard though.

Comment 36

[Maire Reavy [:mreavy]]

Given that this is causing pain, I'm inclined to do this sooner than later.  Let's see if we can get it into Fx47 -- or Fx48 if we get unlucky.

Byron -- Would you like to take this?  Or would you like me to find someone else?

Comment 37

[Byron Campen [:bwc]]

I have some room on my plate now, I can take it.

Comment 38

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1213773: Better handling of answer with direction of inactive in renegotiation. r=mt

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

Comment 39

[Byron Campen [:bwc]]

Comment on attachment 8717083 [details]
MozReview Request: Bug 1213773: Better handling of answer with direction of inactive in renegotiation. r=mt

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

Comment 40

[Byron Campen [:bwc]]

Comment on attachment 8717083 [details]
MozReview Request: Bug 1213773: Better handling of answer with direction of inactive in renegotiation. r=mt

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

Comment 41

[Byron Campen [:bwc]]

Could you try one of the binaries here and see if they behave as expected?

http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-ec563be7bc5461e6fd532746f4a5b9f1f4c60222/

Comment 42

[Anil Kumar]

i am using win 64-bit. How do we test this ?? Do we have to install
http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-ec563be7bc5461e6fd532746f4a5b9f1f4c60222/try-win64-debug/firefox-47.0a1.en-US.win64.installer.exe and test it ?? or can we test this in latest nightly build available ?

Comment 43

[Nils Ohlmeier [:drno]]

No you can't test it with the latest Nightly as Byron's code has not landed in our central code case. So you have use instead a Firefox from the link he provided. I would recommend to use the ZIP file instead of the installer exe, because the ZIP should contain the Firefox exe which you run without having to install anything (and thus no over writing of your existing Firefox installation).

Comment 44

[Anil Kumar]

http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-ec563be7bc5461e6fd532746f4a5b9f1f4c60222/try-win64-debug/firefox-47.0a1.en-US.win64.zip. 
We ran firefox.exe from the above zip file and tried to make calls. But when ever we try to create peerConnection, firefox is crashing.

Comment 45

[Byron Campen [:bwc]]

(In reply to Anil Kumar from comment #44)
> http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-
> ec563be7bc5461e6fd532746f4a5b9f1f4c60222/try-win64-debug/firefox-47.0a1.en-
> US.win64.zip. 
> We ran firefox.exe from the above zip file and tried to make calls. But when
> ever we try to create peerConnection, firefox is crashing.

   This is very strange. Everything in continuous integration looks fine. Can you go to about:crashes and send us the crash-stats link for this crash?

Comment 46

[Byron Campen [:bwc]]

Alternately, if you could give us some steps to reproduce this crash (either a minimal jsfiddle, or access to whatever service you're working on), we could try to debug.

Comment 47

[Anil Kumar]

we tried in the binary build you gave.But it is crashing while creating initial offer itself and more over i don't find any crash report from about:crashes. The same code works well in GA and nightly build v46.0a1 (2015-12-17). Is there a way where we can get WebRTCmoz debug logs ?? is it same like nightly ?

Comment 48

[Byron Campen [:bwc]]

None of the code-paths involved in createOffer have been modified in this patch, so I don't see how it could be crashing there. Can you post some steps to reproduce?

Alternately, if you have a linux machine handy, you could reproduce with the ASAN build, which will tell you what is going on (http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-ec563be7bc5461e6fd532746f4a5b9f1f4c60222/try-linux64-asan-debug/)

Comment 49

[Byron Campen [:bwc]]

Comment on attachment 8717083 [details]
MozReview Request: Bug 1213773: Better handling of answer with direction of inactive in renegotiation. r=mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34037/diff/3-4/

Comment 50

[Martin Thomson [:mt:]]

Comment on attachment 8717083 [details]
MozReview Request: Bug 1213773: Better handling of answer with direction of inactive in renegotiation. r=mt

https://reviewboard.mozilla.org/r/34037/#review36021

This looks fine; though I have to wonder how this manifests on the MediaStreamTrack instances we create.  You currently have a test that looks at the content of the tracks; shouldn't we also be marking the tracks in some way?  I realize that the transition from receiving to inactive during renegotiation is somewhat ambiguous, we don't get an RTCP BYE, do we?  Maybe it's something that requires some extra work and could be done in another bug, more so since the spec is unclear on this point.

An audio test would be good; the code is somewhat divergent and it's possible that a regression on the audio path wouldn't be caught.

::: dom/media/tests/mochitest/sdpUtils.js:55
(Diff revision 4)
> +            .replace("a=recvonly", "a=inactive");

You need to set the first argument to a regex with a /g option or this will only replace the first instance of a=... with a=inactive.

::: dom/media/tests/mochitest/test_peerConnection_renegotiationInactiveAnswer.html:13
(Diff revision 4)
> -    title: "Renegotiation: add second audio stream"
> +    title: "Renegotiation: answerer uses a=inactive"

Are you sure that this test is performant enough not to get bogged down on try?  I've had lots of trouble with performance there and this does two renegotiations and canvas capture.  That's some serious heavy lifting.

::: dom/media/tests/mochitest/test_peerConnection_simulcastOffer.html:32
(Diff revision 4)
>        var helper = new CaptureStreamTestHelper2D(50,50);
>        var canvas = helper.createAndAppendElement('canvas', 'source_canvas');
>        helper.drawColor(canvas, helper.green); // Make sure this is initted

You probably want to factor this out too.  If only to capture the options.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp
(Diff revision 4)
> -    if (!answer.GetMediaSection(i).IsSending() &&
> -        !answer.GetMediaSection(i).IsReceiving()) {
> -      MOZ_MTLOG(ML_DEBUG, "Inactive m-section, skipping creation of negotiated "
> -                          "track pair.");
> -      continue;
> -    }

So just to confirm, the rationale here is that a=inactive will generate remote tracks, we just won't send into them and we won't expect to receive from them.  I think that we need more visible signals about media liveness in this case, see top-comment.

::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp:828
(Diff revision 4)
> +    if (aTrack.GetActive()) {
> +      error = conduit->StartTransmitting();
> +      if (error) {
> +        MOZ_MTLOG(ML_ERROR, "StartTransmitting failed: " << error);
> +        return NS_ERROR_FAILURE;
> +      }
> +    }

This seems to be common.

CreateOrUpdateMediaPipeline creates both the audio and video conduits and could run this code.

Comment 51

[Anil Kumar]

we could'nt test w.r.t to Linux Machine. But however i can tell the steps to reproduce this. 
Create a peerconn, create offer for this peerconn and apply this to LocalDescription and wait for candidate gathering before sending complete offer SDP to signaling Server.There is no SDP manipulation during the same.
The only thing that i am confused is , the same code works fine without any crash in GA build.

Comment 52

[Byron Campen [:bwc]]

(In reply to Anil Kumar from comment #51)
> we could'nt test w.r.t to Linux Machine. But however i can tell the steps to
> reproduce this. 
> Create a peerconn, create offer for this peerconn and apply this to
> LocalDescription and wait for candidate gathering before sending complete
> offer SDP to signaling Server.There is no SDP manipulation during the same.
> The only thing that i am confused is , the same code works fine without any
> crash in GA build.

   That's really weird, because we have a few tests in continuous integration for this and they're fine. Those tests only use audio, so maybe there's a problem when both audio and video are involved. I'll check that possibility out. Here's a few options for what you can do to help:

1) Get a stack out of a debugger on windows.

2) Give me access to whatever service you're working on so I can repro.

3) Create a jsfiddle that reproduces the problem, based on the code your service uses.

4) Try the newer builds here and see what happens: http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-493795edb2ed364bcfec2c3278c01b656b1dabb0/

Comment 53

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/34037/#review36021

I believe we will get an RTCP BYE due to bug 1232234, but once that's fixed we shouldn't. I can write an audio test, although I'm not sure if we have a way to check whether stuff is rendering like we do for video.

> Are you sure that this test is performant enough not to get bogged down on try?  I've had lots of trouble with performance there and this does two renegotiations and canvas capture.  That's some serious heavy lifting.

We'll have to determine that empirically, sadly. This test is disabled on the same platforms that can't handle renegotiation tests, but we may need to disable on more. (It's pretty sad that we can't manage a 50x50 canvas capture efficiently...)

> So just to confirm, the rationale here is that a=inactive will generate remote tracks, we just won't send into them and we won't expect to receive from them.  I think that we need more visible signals about media liveness in this case, see top-comment.

That's the notion. I would have to do some reading on how to mark the track as inactive, and I'd be willing to bet that it warrants a separate bug.

Comment 54

[Byron Campen [:bwc]]

Comment on attachment 8717083 [details]
MozReview Request: Bug 1213773: Better handling of answer with direction of inactive in renegotiation. r=mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34037/diff/4-5/

Comment 55

[Byron Campen [:bwc]]

Comment on attachment 8717083 [details]
MozReview Request: Bug 1213773: Better handling of answer with direction of inactive in renegotiation. r=mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34037/diff/5-6/

Comment 56

[Byron Campen [:bwc]]

Comment on attachment 8717083 [details]
MozReview Request: Bug 1213773: Better handling of answer with direction of inactive in renegotiation. r=mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34037/diff/6-7/

Comment 57

[Byron Campen [:bwc]]

Comment on attachment 8717083 [details]
MozReview Request: Bug 1213773: Better handling of answer with direction of inactive in renegotiation. r=mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34037/diff/7-8/

Comment 58

[Byron Campen [:bwc]]

Comment on attachment 8717083 [details]
MozReview Request: Bug 1213773: Better handling of answer with direction of inactive in renegotiation. r=mt

Re-asking review for audio test-case.

Comment 59

[Martin Thomson [:mt:]]

Comment on attachment 8717083 [details]
MozReview Request: Bug 1213773: Better handling of answer with direction of inactive in renegotiation. r=mt

https://reviewboard.mozilla.org/r/34037/#review41517

::: dom/media/tests/mochitest/head.js:438
(Diff revision 8)
> +        helper._stream.requestFrame();
> +      } catch (e) {
> +        // ignore; stream might have shut down, and we don't bother clearing
> +        // the setInterval.
> +      }
> +    }, 500);

Does this need to be 500, or can it be made smaller?

::: dom/media/tests/mochitest/head.js:458
(Diff revision 8)
> +      this.waitForFrames(canvas).then(() => {
> +        if (!doneWaiting) {
> +          ok(false, "Color should not change")
> +        }
> +      }),
> +      wait(2000).then(() => {

This, on the other hand, might be a little short.

::: dom/media/tests/mochitest/sdpUtils.js:53
(Diff revision 8)
> +  return sdp.replace(/a=sendrecv/g, "a=inactive")
> +            .replace(/a=sendonly/g, "a=inactive")
> +            .replace(/a=recvonly/g, "a=inactive");

Maybe /\r\na=... just to be sure

::: dom/media/tests/mochitest/test_peerConnection_simulcastOffer.html:41
(Diff revision 8)
> -        function PC_LOCAL_CANVAS_ALTERNATE_COLOR(test) {
> -          alternateRedAndGreen(helper, canvas, stream);
> +        function PC_LOCAL_CANVAS_START_CAPTURE(test) {
> +          helper.startCapturingFrames();
>          }

I wonder if, for performance and simplicity reasons, this might be merged with `waitForFrames`.

::: dom/media/tests/mochitest/test_peerConnection_simulcastOffer.html:80
(Diff revision 8)
>            var vremote = document.getElementById('pcRemote_remote1_video');
>            ok(vremote, "Should have remote video element for pcRemote");
> -          return waitForColorChange(helper, vremote);
> +          return helper.waitForFrames(vremote);

I think perhaps that the helper could be initialized with vremote rather than have the caller pass it in here.

Comment 60

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e869db54869

Comment 61

[Byron Campen [:bwc]]

I've been unable to reach Anil to figure out why this change is not behaving. Does this work for you?

Comment 62

[Anil Kumar]

Hey Byron,

In the build link you gave http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-493795edb2ed364bcfec2c3278c01b656b1dabb0/ which file should i use to reproduce. I see lot of files under 
try-win64-debug/ Directory. Can you please point me to the build i have to use.

Mean while i will try to reproduce it on jsfiddle.

Comment 63

[Nils Ohlmeier [:drno]]

Hi Anil,

I think you should try this one here http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-493795edb2ed364bcfec2c3278c01b656b1dabb0/try-win64-debug/firefox-48.0a1.en-US.win64.zip
Simply extract the firefox folder from that ZIP file and run the firefox.exe in that folder. By using the ZIP file instead of the installer you can keep using you regular installed Firefox.

Comment 64

[Anil Kumar]

Thanks Nils, 

I tested with the above build you pointed. Good that we couldn't reproduce the crash issue this time but had some other problem.

Normal video call between two peers is failing because of Ice Connectivity failure. It says "ICE failed, see about:webrtc for more details" . so we wont have media flow at all.

i have attached the debug logs and about:webrtc page details.

however the same code seems to be working in GA v43.0.2.

Attached files:
newBuild_webRTC.log is the debug log file.
newBuild_aboutWebrtc is the about webrtc page.

Comment 65

[Anil Kumar]

Attachment: newBuild_webRTC.log

Comment 66

[Anil Kumar]

Attachment: newBuild_aboutWebrtc.html

Comment 67

[Byron Campen [:bwc]]

This is probably bug 1259842 rearing its head again. Let me build a new binary with that fix.

Comment 68

[Byron Campen [:bwc]]

Comment on attachment 8717083 [details]
MozReview Request: Bug 1213773: Better handling of answer with direction of inactive in renegotiation. r=mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34037/diff/8-9/

Comment 69

[Byron Campen [:bwc]]

Give this a try: http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-ed0aee025ed418ecec8d7831ae63906dfe3fb6c9/try-win64/firefox-49.0a1.en-US.win64.zip

The debug version did not build due to some infra issue, I kicked off another build that should show up here before long: http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-ed0aee025ed418ecec8d7831ae63906dfe3fb6c9/try-win64-debug/

Comment 70

[Byron Campen [:bwc]]

Comment on attachment 8717083 [details]
MozReview Request: Bug 1213773: Better handling of answer with direction of inactive in renegotiation. r=mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34037/diff/9-10/

Comment 71

[Byron Campen [:bwc]]

Comment on attachment 8717083 [details]
MozReview Request: Bug 1213773: Better handling of answer with direction of inactive in renegotiation. r=mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34037/diff/10-11/

Comment 72

[Maire Reavy [:mreavy]]

Thanks for your help reporting the bug and helping us test.  However, we need to know if our solution works for you.

We are holding off landing the patch on this bug until we get positive verification that it solves your problem.  Please help us verify that our fix works.  Once we get verification, we'll land.

Comment 73

[Anil Kumar]

Hi Maire,

I tested this issue with the latest build link given in previous comments. Seems like its working fine(Signalling). Even though i just tested normal hold/unhold scenarios, where creating new offers for hold/unhold and applying it locally and remote worked fine. There is no errors like before while trying to apply modified  local and remote SDP's. But with respect to normal call, i dont hear any voice path. Instead of voice i hear only cranky noise. But i dont see this voice issue in other Firefox builds. It looks like this noise issue is only in this particular build. 

I will also check some other scenarios around this issue.So will update this issue in couple of days time.

Thanks for taking this issue and fixing it.

Comment 74

[Maire Reavy [:mreavy]]

Thanks, Anil.  The audio issue you're reporting should have nothing to do with this patch.  What I'd like to do then is land this patch and ask you to test with Nightly right after it lands.  I'd appreciate it if you can verify (1) that Nightly fixes your problem and (2) that you don't hear any audio problems.  If Nightly doesn't work as expected, please file a new bug ASAP and needinfo me on the new bug.  

Byron -- Please land this as soon as you get a chance.  Thanks.

Comment 75

[Anil Kumar]

Thanks maire. please let us know when this will be available in nightly build so that we can test these scenarios.

Comment 76

[Byron Campen [:bwc]]

Comment on attachment 8717083 [details]
MozReview Request: Bug 1213773: Better handling of answer with direction of inactive in renegotiation. r=mt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34037/diff/11-12/

Comment 77

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4ef8b4ff0bc

Comment 78

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b4ef8b4ff0bc

Comment 79

[Maire Reavy [:mreavy]]

Hi Anil -- The patch just landed in central this morning.  So tomorrow's Nightly (date of May 25th) should have this fix in it.  Please try it tomorrow (make sure you're using the Nightly build from the 25th) and let us know how it works for you.  Cheers!

Comment 80

[Anil Kumar]

Hi Maire, 

I tested in v49.0a1 (2016-05-25) nightly. But i am still having the voice path issue. Instead of voice i hear only cranky noise. Apart from voice path issue i do not see any other problem doing hold/unhold ,applying SDP's etc. 

I tried the same thing in v43.0.2 and there is not voice path issue.

Comment 81

[Maire Reavy [:mreavy]]

Thanks for retesting and flagging this new issue.  Can you file a new bug using this template: https://bugzilla.mozilla.org/enter_bug.cgi?product=Core&component=WebRTC%3A%20Audio%2FVideo ?

When you file this new bug, can you include the following in the new bug report?:
1) What microphone or audio input device are you using?
2) What OS (or OSs) are you testing on?  (Mac, Windows, Linux?)
3) Do you get the same noise when you try our gUM demo here:  https://mozilla.github.io/webrtc-landing/gum_test.html?  (Just click on Audio or Audio & Video)
4) Can you try versions 46, 47 and 48 of Firefox?  46 is our current release version, 47 is in Beta, 48 will be going to Beta in less than 2 weeks.
5) Using Nightly (from 05-25 or from today), can you go to about:config, search for "full_duplex", and flip the pref with "full_duplex" in the title from true to false and retest?

My current theory is that your microphone or audio device may be either having problems with our latest feature (full duplex), which is enabled in 49 for all Desktop OSs and in 48 for Mac and Linux, or problems with a feature we landed in version 44 of Firefox to capture gUM at 32kHz (see  Bug 953265 for details).

Thanks very much for reporting and for testing!

Comment 82

[Anil Kumar]

sure i will file a new bug for this voice issue. mean while i tested in v46.0.1 in the link you gave https://mozilla.github.io/webrtc-landing/gum_test.html. i do not see any voice path issue. But the thing is , did anything change w.r.t getUserMedia recently ?? . I started getting "SourceUnavailableError: Failed to allocate audiosource" error in v46.01. Then after connecting an headset it was fine. Is connecting an headset during getUserMedia is mandatory step ?? 

I will provide the remaining information you asked in the new bug.

Thanks.

Comment 83

[Maire Reavy [:mreavy]]

(In reply to Anil Kumar from comment #82)
> sure i will file a new bug for this voice issue. mean while i tested in
> v46.0.1 in the link you gave
> https://mozilla.github.io/webrtc-landing/gum_test.html. i do not see any
> voice path issue. But the thing is , did anything change w.r.t getUserMedia
> recently ?? . I started getting "SourceUnavailableError: Failed to allocate
> audiosource" error in v46.01. Then after connecting an headset it was fine.
> Is connecting an headset during getUserMedia is mandatory step ?? 
Connecting a headset is not mandatory.

There have been changes in gUM recently, but the most likely cause of the error you're seeing would be if a different instance of a browser had the internal mic allocated (i.e. grabbed exclusive access to the mic).

> I will provide the remaining information you asked in the new bug.

Thanks.  We can do a much deeper dive when you file it, and I can circle in the media devs on my team.  This bug is all about the signaling side and I'm very keen on keeping all the info for a particular problem in one place -- which would be the new bug ticket once it's filed.

Comment 84

[ushunmugan]

Is this issue fixed in version 49?  I've tested a hold scenario with version 50.0.2 (latest stable), and I see the following issue.  When the remote offer SDP contains a=inactive for all m-lines, the answer generated by Firefox contains disabled m-lines (with port 0), rather than having valid ports and SSRC for keeping RTCP active. Here is an example of an offer, and the answer generated by Firefox:

grtc: 27.361: This is the modified remote offer SDP:
v=0
o=Genesys 309 3 IN IP4 0.0.0.0
s=WEBRTCGW-8.5.207.84
c=IN IP4 0.0.0.0
t=0 0
a=group:BUNDLE sdparta_0 sdparta_1
a=msid-semantic: WMS A4E3312A-8101-4B5A-B151-1C92CAA51B70
m=audio 36000 UDP/TLS/RTP/SAVPF 9
a=inactive
a=rtcp-mux
a=mid:sdparta_0
a=ssrc:488428232 cname:DrTvXvebRUq70V2/QpaElPVg
a=ssrc:488428232 label:a1-312A-8101-4B5
a=ssrc:488428232 msid:A4E3312A-8101-4B5A-B151-1C92CAA51B70 a1-312A-8101-4B5
a=ssrc:488428232 mslabel:A4E3312A-8101-4B5A-B151-1C92CAA51B70
a=ice-ufrag:aoVO
a=ice-pwd:RrqpVOrnx4FId/LPbw1Dtq
a=rtpmap:9 g722/8000
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
m=video 36000 UDP/TLS/RTP/SAVPF 120
b=AS:4000
a=inactive
a=rtcp-mux
a=mid:sdparta_1
a=rtcp-fb:* ccm fir
a=rtcp-fb:* nack
a=rtcp-fb:* nack pli
a=rtcp-fb:* goog-remb
a=ssrc:800992233 cname:DrTvXvebRUq70V2/QpaElPVg
a=ssrc:800992233 label:v1--8101-4B5A-B1
a=ssrc:800992233 msid:A4E3312A-8101-4B5A-B151-1C92CAA51B70 v1--8101-4B5A-B1
a=ssrc:800992233 mslabel:A4E3312A-8101-4B5A-B151-1C92CAA51B70
a=ice-ufrag:aoVO
a=ice-pwd:RrqpVOrnx4FId/LPbw1Dtq
a=rtpmap:120 vp8/90000
a=fmtp:120 max-fs=12288;max-fr=60
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

grtc: 27.372: setRemoteDescription() success  grtc.js:1005:13
grtc: 27.420: Create answer with constraints: {"offerToReceiveAudio":true,"offerToReceiveVideo":true}  grtc.js:1005:13
grtc: 27.422: createAnswer success  grtc.js:1005:13
grtc: 27.423: This is the modified local answer SDP:
v=0
o=mozilla...THIS_IS_SDPARTA-50.0.2 7627979012376577260 1 IN IP4 0.0.0.0
s=-
t=0 0
a=fingerprint:sha-256 61:41:15:85:6F:65:79:61:FF:08:A1:E8:64:41:EB:59:5F:C0:18:B3:25:0B:85:07:86:D7:AD:27:2F:A3:61:3D
a=ice-options:trickle
a=msid-semantic:WMS *
m=audio 0 UDP/TLS/RTP/SAVPF 0
c=IN IP4 0.0.0.0
a=inactive
a=rtpmap:0 PCMU/8000
m=video 0 UDP/TLS/RTP/SAVPF 120
b=AS:4000
c=IN IP4 0.0.0.0
a=inactive
a=rtpmap:120 VP8/90000

grtc: 27.425: setLocalDescription() success - iceGatheringState: complete

Comment 85

[Randell Jesup [:jesup] (needinfo me)]

Please file a new bug on the all-media-elements-disabled issue.  Thanks!
byron: FYI

Comment 86

[ushunmugan]

OK, bug 1322358 has been created.