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

Status: RESOLVED FIXED

(Core :: Audio/Video: MediaStreamGraph, defect, P1)

Description

[Alice0775 White]

[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

Comment 1

[Boris Zbarsky [:bzbarsky]]

See bug 1300650 comment 9.

Comment 2

[Alice0775 White]

I forgot a step.

0. setting media.navigator.audio.full_duplex=false

Comment 3

[Alice0775 White]

Recorded video : https://youtu.be/7yy8kNVoU34

Reference video(for calibration) : https://www.youtube.com/watch?v=rzF6bNSXZac

Comment 4

[Terrence Cole [:terrence]]

Moreover, we should only be using JSSavedStack at all when profiling, which I don't think is active here.

Comment 5

[Terrence Cole [:terrence]]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 7

[Alice0775 White]

(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)

Comment 8

[Marcia Knous [:marcia]]

Tracking 51+ for this regression in Audio/Video.

Comment 9

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

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

Comment 10

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

Removing bz and terrence since this shouldn't be anything they have to care about

Comment 11

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Bug 1300871 - Rollback to use original duration in video case.

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

Comment 14

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8789221 [details]
Bug 1300871 - Rollback to use original duration in video case.

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

Comment 16

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9441a2c6c1d&selectedJob=27139605

Comment 17

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

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?

Comment 18

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

(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.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4e77ec88d85a
Rollback to use original duration in video case. r=jesup

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e77ec88d85a

Comment 21

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: gUM.png