Closed Bug 1011149 Opened 7 years ago Closed 7 years ago

Video freezes when audio track is disabled on remote stream

Categories

(Core :: WebRTC: Audio/Video, defect, P2)

32 Branch
x86
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox31 --- verified
firefox32 --- verified
firefox33 --- verified
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: msander, Assigned: rlin)

References

()

Details

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.131 Safari/537.36

Steps to reproduce:

1. Open https://apprtc.appspot.com/ in two tabs
2. In on tab call pc.getRemoteStreams()[0].getAudioTracks()[0].enabled=false in the console


Actual results:

Video freezes for the stream with disabled audio.


Expected results:

Video should not freeze when audio is disabled.
Version: 25 Branch → 32 Branch
Confirmed 32.0a1 (2014-05-15), win 7 x64
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Target Milestone: --- → mozilla33
Found this patch affect this issue.
# HG changeset patch
# User Paul Adenot <paul@paul.cx>
# Date 1398431721 -7200
#      Fri Apr 25 15:15:21 2014 +0200
# Node ID 8a69388a1a2116e1b9c3b36e11bd78ba9b98d378
# Parent  27cd1a1fd1d7f8ec0384888f0936fd6c8e1b5f0b
Bug 998711.

Hi Paul, 
Any ideas?
Flags: needinfo?(paul)
Assignee: nobody → rlin
Attached patch patch v1 (obsolete) — Splinter Review
Fix the mathematical calculation error.

c.mDuration *= aOutRate / aInRate;

mDuration type is int64_t.
In this case, it would divide first and round into integer, then multiplicative mDuration, so we got deviation.
Attachment #8436707 - Flags: feedback?(cku)
Flags: needinfo?(paul)
Comment on attachment 8436707 [details] [diff] [review]
patch v1

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

Looks right.  Once this lands, please ask for uplift at least to Aurora, and maybe beta as well.
Attachment #8436707 - Flags: feedback+
Comment on attachment 8436707 [details] [diff] [review]
patch v1

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

::: content/media/AudioSegment.h
@@ +184,5 @@
>        nsAutoTArray<const T*, GUESS_AUDIO_CHANNELS> bufferPtrs;
>        AudioChunk& c = *ci;
>        // If this chunk is null, don't bother resampling, just alter its duration
>        if (c.IsNull()) {
> +        c.mDuration = c.mDuration * aOutRate / aInRate;

Make it more explicit 
c.mDuration = (c.mDuration * aOutRate) / aInRate;
Attachment #8436707 - Flags: feedback?(cku) → feedback+
Attached patch patch v1.1 (obsolete) — Splinter Review
Address cj's comment. I will also request uplift at least to Aurora.
Attachment #8436707 - Attachment is obsolete: true
Attachment #8437402 - Flags: review?(rjesup)
Comment on attachment 8437402 [details] [diff] [review]
patch v1.1

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

Let's ask for Aurora and Beta, as beta just opened
Attachment #8437402 - Flags: review?(rjesup) → review+
Request to uplift to other branches.
Attachment #8437402 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/c953fc808c1f

Friendly reminder that your commit message should be summarizing what the patch is actually doing, not just restating the problem it's fixing. Thanks!
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment
Keywords: checkin-needed
Comment on attachment 8438114 [details] [diff] [review]
check-in patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 998711
User impact if declined: When user do the webRTC video call, if they disable the remote stream's audio track and that would make video freeze and can't recover.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky):NA 
String or IDL/UUID changes made by this patch:NA
Attachment #8438114 - Flags: approval-mozilla-beta?
Attachment #8438114 - Flags: approval-mozilla-aurora?
Randy, FYI,  tracking-firefox31 is the way to get our attention. Not status-firefox31.
Attachment #8438114 - Flags: approval-mozilla-beta?
Attachment #8438114 - Flags: approval-mozilla-beta+
Attachment #8438114 - Flags: approval-mozilla-aurora?
Attachment #8438114 - Flags: approval-mozilla-aurora+
Reproduced on Nightly 32.0a1 by performing calls between Win 7 64-bit and Mac OSX 10.9.3.

Verified as fixed on Firefox 31 beta 6, latest Aurora 32.0a2 20140702004004 and latest Nightly 33.0a1 20140630030202.
You need to log in before you can comment on or make changes to this bug.