MediaRecorder video and audio are very out of sync on win10 if setting media.navigator.audio.full_duplex=false

RESOLVED FIXED in Firefox 51

Status

()

Core
Audio/Video: MediaStreamGraph
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Alice0775 White, Assigned: ctai)

Tracking

({regression})

51 Branch
mozilla51
x86
Windows 10
regression
Points:
---

Firefox Tracking Flags

(firefox48 unaffected, firefox49 unaffected, firefox50 unaffected, firefox51+ fixed)

Details

MozReview Requests

()

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

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
[Tracking Requested - why for this release]:

+++ This bug was initially created as a clone of Bug #1300650 +++

Build Identifier: 
https://hg.mozilla.org/releases/mozilla-aurora/rev/2231ab09a51c3b21dae6fa1627c1a64cfd43b3b0
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0 ID:20160905004005

Steps to reproduce:
1. Open https://www.webrtc-experiment.com/RecordRTC/
2. "Start Recording" and shear video/audio.
3. After for a while, Click "Stop Recording"

Actual results:
The audio lags behind the video by about 1 second all evidence of how far off it is. 


Expected results:
The audio and video should be in sync.



Regression window on Nightly51.01 with setting media.navigator.audio.full_duplex=false :
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e0a401691ee7b6c0979d228bc55f6c8db291663d&tochange=492d8382a49c86b16f758edb67b46d061adac5fc

Regressed by: 492d8382a49c	Terrence Cole — Bug 1291928 - Expose JSSavedStack's stack before returning; r=bz
Flags: needinfo?(terrence)
Flags: needinfo?(bzbarsky)
(Reporter)

Comment 2

2 years ago
I forgot a step.

0. setting media.navigator.audio.full_duplex=false
(Reporter)

Comment 3

2 years ago
Recorded video : https://youtu.be/7yy8kNVoU34

Reference video(for calibration) : https://www.youtube.com/watch?v=rzF6bNSXZac
Moreover, we should only be using JSSavedStack at all when profiling, which I don't think is active here.
More to the point: if the increased latency from running a single extra barrier permanently delays A/V sync by over a second, our A/V component has major, major problems. The sort of problems that a GC expert is not going to be able to address in any way.
Flags: needinfo?(terrence)
> Moreover, we should only be using JSSavedStack at all when profiling

That's certainly false.  It's used all over the place in Promise code, in exceptions, etc.  But yes, comment 5 is basically what I was thinking...

Alice0775, how confident are you about this being the patch that triggers the problem?
(Reporter)

Comment 7

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #6)
>
> Alice0775, how confident are you about this being the patch that triggers
> the problem?

Okay. 
I reinstalled Windows10... :)
And I re-checked the range without running foreground application except WMP.

I got
m-c:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=975ba208687a97ecb9fd439c1ee52bfa3350e25b&tochange=d320ef56876f52db9bc0eb79554c7332d4793769

m-i
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=79415ce5af65c6b207ae185825770ac039af13a8&tochange=a02925fe4ded04e2f5523fbd7e5ddfa46b190d39

Regressed by: Bug 1201363

(If possible, cross check)
Blocks: 1201363
No longer blocks: 1291928
Component: DOM: Core & HTML → Audio/Video: MediaStreamGraph
Flags: needinfo?(rjesup)
Flags: needinfo?(ctai)
status-firefox48: --- → unaffected
status-firefox49: --- → unaffected
Tracking 51+ for this regression in Audio/Video.
tracking-firefox51: ? → +
Thanks Alice; that makes far more sense than JS stuff :-).  ctai is going to look at it in the morning; we've already discussed it on IRC
Flags: needinfo?(rjesup)
Removing bz and terrence since this shouldn't be anything they have to care about
Comment hidden (mozreview-request)
Assignee: nobody → ctai
Flags: needinfo?(ctai)
Duplicate of this bug: 1300169
Duplicate of this bug: 1300650
Comment hidden (mozreview-request)
Duplicate of this bug: 1298305

Comment 17

2 years ago
mozreview-review
Comment on attachment 8789221 [details]
Bug 1300871 - Rollback to use original duration in video case.

https://reviewboard.mozilla.org/r/77518/#review75928

r+ -- is it reasonable to look to re-land the new version of this after bugfixing, or should we just go back to the old way permanently?  And have you tested it?
Attachment #8789221 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #17)
> Comment on attachment 8789221 [details]
> Bug 1300871 - Rollback to use original duration in video case.
> 
> https://reviewboard.mozilla.org/r/77518/#review75928
> 
> r+ -- is it reasonable to look to re-land the new version of this after
> bugfixing, or should we just go back to the old way permanently?  And have
> you tested it?

Yea, I have tested it. I don't think we need to re-land the new one. But it is reasonable to change to use timestamp only(bug 1290777). Because each encoder will change the duration into timestamp in their implementation. That will be redundant if we already have timestamp. The root cause of this bug is the skipping the duration of null image in the beginning.
Keywords: checkin-needed

Comment 19

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4e77ec88d85a
Rollback to use original duration in video case. r=jesup
Keywords: checkin-needed

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4e77ec88d85a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.