RTCPeerConnection.removeTrack causes freezing video when removing an audio track

VERIFIED FIXED in Firefox 49

Status

()

Core
WebRTC: Audio/Video
P1
normal
Rank:
19
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: leahcimic, Assigned: dminor)

Tracking

44 Branch
mozilla51
Points:
---

Firefox Tracking Flags

(firefox48 wontfix, firefox49 verified, firefox50 verified, firefox51 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
str
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.84 Safari/537.36

Steps to reproduce:

1. Create 2 peer connections
2. Use getUserMedia to get a stream with audio and video
3. Add the stream to the first peer connection
4. Connect the two peer connections
5. Add the stream from the 2nd peer connection into a video tag
6. Remove the audio track from the 1st peer connection with removeTrack() using the RTCRtpSender that has an audio track in it
7. Renegotiate the two peer connections

I have created a jsbin that reproduces the issue: http://output.jsbin.com/lotuqicanu


Actual results:

The video freezes.


Expected results:

The video to continue playing.

Comment 1

2 years ago
testcase
Created attachment 8761883 [details]
jsbin.lotuqicanu.1.html

Updated

2 years ago
Component: Untriaged → WebRTC: Audio/Video
Product: Firefox → Core

Comment 2

2 years ago
Just a heads up that this is a TokBox related issue with a feature for Firefox Hello we're trying to implement.
My unwarranted suspicion is that removing the audio track that a video track is synced to breaks avsync in the receiver.  We need to ensure the SyncTo in the VideoConduit gets severed on a renegotiation where the audio track of an audio/video pair is removed.  (Again - wild guess)
Status: UNCONFIRMED → NEW
Rank: 15
Ever confirmed: true
Priority: -- → P1

Comment 4

2 years ago
I got a regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=86d1b3a8aecb978a7e656e8c8018e227333d9ae5&tochange=7b5b900fc4948552db6a3ea92730ac4665509ba5


Before the bug 1101885, the testcase doesn't show any video (Firefox doesn't display any notification to share webcam/microphone), after the bug, there is a video but it freezes after 5 sec.
Flags: needinfo?(jyavenard)

Updated

2 years ago
Version: 47 Branch → 44 Branch
none of the code path found by this regression range is involved in the crash at hand. This is a WebRTC issue, not playback
Flags: needinfo?(jyavenard)
Dan -- Can you take a look at this while Randell is on PTO?  He's back August 1st.
Assignee: nobody → dminor
Rank: 15 → 19
Flags: needinfo?(dminor)
(Assignee)

Comment 7

2 years ago
Will do.
Flags: needinfo?(dminor)
(Assignee)

Comment 8

2 years ago
I believe I have a fix for this. Try push here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2907101f5644
Status: NEW → ASSIGNED
(Assignee)

Comment 9

2 years ago
Created attachment 8776904 [details]
Bug 1279135 - Reattach Pipeline to PipelineTransport in AttachTransport_s;

When we call MediaPipeline::UpdateTransport_s we in turn call DetachTransport_s
which detaches the pipeline from PipelineTransport. The subsequent call to
AttachTransport_s does not currently reattach the pipeline, causing
subsequent sends to fail due to a detached pipeline. Since
PipelineTransport::SendRtpRtcpPacket_s returns NS_OK if a send fails due to a
detached pipeline, this failure is not straightforward to detect.

This patch adds an Attach() method to PipelineTransport and calls it from
AttachTransport_s.

Review commit: https://reviewboard.mozilla.org/r/67976/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67976/
Attachment #8776904 - Flags: review?(rjesup)

Updated

2 years ago
Attachment #8776904 - Flags: review?(rjesup) → review+
Comment on attachment 8776904 [details]
Bug 1279135 - Reattach Pipeline to PipelineTransport in AttachTransport_s;

https://reviewboard.mozilla.org/r/67976/#review65702
Worth considering uplifting to 50 given simplicity, once it's landed and verified

Comment 12

2 years ago
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/be822ebbacc0
Reattach Pipeline to PipelineTransport in AttachTransport_s; r=jesup

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/be822ebbacc0
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Comment 14

2 years ago
[Tracking Requested - why for this release]:
status-firefox50: --- → affected
(Assignee)

Updated

2 years ago
status-firefox49: --- → affected
(Assignee)

Comment 15

2 years ago
Comment on attachment 8776904 [details]
Bug 1279135 - Reattach Pipeline to PipelineTransport in AttachTransport_s;

Approval Request Comment
[Feature/regressing bug #]: 1279135
[User impact if declined]: Developers would have to work around this bug in Firefox by adding a new call to getUserMedia for just the video, which requires the user to click through another permissions box for approval.
[Describe test coverage new/current, TreeHerder]: Test case for manual testing is attached to this bug. No automated tests.
[Risks and why]: This is low risk, the patch is tiny and unlikely to affect other functionality.
[String/UUID change made/needed]: None.
Attachment #8776904 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

2 years ago
status-firefox48: --- → affected
(Assignee)

Comment 16

2 years ago
Comment on attachment 8776904 [details]
Bug 1279135 - Reattach Pipeline to PipelineTransport in AttachTransport_s;

Approval Request Comment
[Feature/regressing bug #]: 1279135
[User impact if declined]: Developers would have to work around this bug in Firefox by adding a new call to getUserMedia for just the video, which requires the user to click through another permissions box for approval.
[Describe test coverage new/current, TreeHerder]: Test case for manual testing is attached to this bug. No automated tests.
[Risks and why]: This is low risk, the patch is tiny and unlikely to affect other functionality.
[String/UUID change made/needed]: None.
Attachment #8776904 - Flags: approval-mozilla-beta?
Comment on attachment 8776904 [details]
Bug 1279135 - Reattach Pipeline to PipelineTransport in AttachTransport_s;

Sounds good to avoid extra user clicking in this scenario. Let's uplift to aurora and beta. If we see any regressions please back it out of beta, though.
Attachment #8776904 - Flags: approval-mozilla-beta?
Attachment #8776904 - Flags: approval-mozilla-beta+
Attachment #8776904 - Flags: approval-mozilla-aurora?
Attachment #8776904 - Flags: approval-mozilla-aurora+

Comment 18

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/deb2509ac6d9
status-firefox50: affected → fixed

Comment 19

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/62a0728183c4
status-firefox49: affected → fixed
Flags: qe-verify+

Updated

2 years ago
status-firefox48: affected → wontfix
Reproduced the video freeze on Firefox 48.0.2, verified that Firefox 49 beta 8, latest Aurora 50.0a2 and latest Nightly 51.0a1 does not manifest the same behavior on Mac OS X 10.10.5, Ubuntu 16.04 x64 and Windows 10 x64.
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified
status-firefox50: fixed → verified
status-firefox51: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.