1307042 - Make fake audio implementation operate off NotifyPull, avoiding timers and underruns

Status: RESOLVED FIXED

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

Description

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

Underruns due to timers/threading/etc (i.e. push-based audio not synced to pulls) may be causing some of our intermittents ontest VMs.  Much cleaner to generate the audio on NotifyPull, and more similar to full-duplex (no threading issues causing underruns).

Comment 1

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

Attachment: generate fake audio for getUserMedia from MSG callbacks

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d62fb55edcc5  Seems to work well on pc_test, including under rr (where the audio does glitch some, but I expected that)

Comment 2

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

Comment on attachment 8797043 [details] [diff] [review]
generate fake audio for getUserMedia from MSG callbacks

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

Thanks!

::: dom/media/webrtc/MediaEngineDefault.cpp
@@ +368,5 @@
>  /**
>   * Default audio source.
>   */
> +
> +NS_IMPL_ISUPPORTS0(MediaEngineDefaultAudioSource)

No need for nsISupports any longer.

@@ +489,5 @@
>    return NS_OK;
>  }
>  
>  void
>  MediaEngineDefaultAudioSource::AppendToSegment(AudioSegment& aSegment,

Only one caller to this now right? Good time to inline.

@@ +512,2 @@
>  {
> +  MOZ_ASSERT(aID == mTrackID);

Do we need a guard off mState here, like in the other MediaEngines?

::: dom/media/webrtc/MediaEngineDefault.h
@@ +179,2 @@
>  
> +  TrackTicks mLastNotify;

It'd be great if you could add some comments on thread access to the members.

Comment 3

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

Attachment: nit resolution interdiffs

Comment 4

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

Comment on attachment 8797176 [details] [diff] [review]
nit resolution interdiffs

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

With inline I meant to move the AppendToSegment code into NotifyPull if possible. But it's a nit. Feel free to handle at your own discretion.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc4680ea08a9
generate fake audio for getUserMedia from MSG callbacks r=pehrsons

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for being the most likely cause of android mda test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=36937459&repo=mozilla-inbound

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

Comment 7

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

Attachment: Disables of tests to get android to pass with fake audio updates

Comment 8

[Nils Ohlmeier [:drno]]

Comment on attachment 8802175 [details] [diff] [review]
Disables of tests to get android to pass with fake audio updates

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

Sad, sad, ...

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d480bd0f758
generate fake audio for getUserMedia from MSG callbacks r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/c701835f10d7
Disables of tests to get android to pass with fake audio updates r=drno

Comment 10

[Alex Chronopoulos [:achronop]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69066d0e0c3f

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d480bd0f758
https://hg.mozilla.org/mozilla-central/rev/c701835f10d7

Comment 12

[Ryan VanderMeulen [:RyanVM]]

This fixed a number of failures that also affect Aurora and Beta. Is it safe to uplift to one or both of those as well?

Comment 13

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> This fixed a number of failures that also affect Aurora and Beta. Is it safe
> to uplift to one or both of those as well?

Well, this blocked a bunch that now can land.

This patch itself *only* affects test code (fake:true streams), so I see no reason to request beta approval.

It should be very safe for aurora, with the side-effect that it will mean someone has to monitor autophone tests on aurora, because this disables ~30 tests on android emulator.  We're going to start doing that in webrtc triage, but we want to move autophone to tier 1 soon.  I don't know how safe/easy the blocked bugs are for aurora uplift.  I think this can be uplifted if we have some plan for monitoring android on aurora (and beta when it gets there)

andreas - which bugs would you like to uplift to aurora/51?

Comment 14

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

This one fixed some intermittents on mainly Windows7VM, so that would be a good candidate. Apart from this I have one patch that I'm trying to get uplifted to aurora and beta in bug 1294605, but that now got backed out again :-(

Comment 15

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

Comment on attachment 8797043 [details] [diff] [review]
generate fake audio for getUserMedia from MSG callbacks

Approval Request Comment
(This applies to the set of patches that landed)

[Feature/regressing bug #]: N/A

[User impact if declined]: oranges in the trees

[Describe test coverage new/current, TreeHerder]: This is all about improving 'fake' audio used in testing.

[Risks and why]: Low risk.  Biggest risk is that there are tests disabling on 52 that aren't (yet) disabled on 51 for Android.  If so, we'd just disable there as well.

[String/UUID change made/needed]: none

Comment 16

[Gerry Chang [:gchang]]

Comment on attachment 8797043 [details] [diff] [review]
generate fake audio for getUserMedia from MSG callbacks

Fixed a number of failures. Take it in 51 aurora.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c701835f10d7 needs rebasing for aurora

Comment 18

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

Rebased; running try for good measure due to all the test-disables here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3594bea3548070ccac1bae3b417b3284a1dc407

Comment 19

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

mis-merge, trying again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8dc779c874e6cb628a4ef36a8055f06ac6b5c80

Comment 20

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/ffae4aede5a1ed6c8374af2b443b701714576119
https://hg.mozilla.org/releases/mozilla-aurora/rev/95425214df817dc7359a9e96095bdb57f46ea261