Closed Bug 1307042 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 --- affected
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: jesup, Assigned: jesup)

References

Details

Attachments

(3 files)

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).
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)
Attachment #8797043 - Flags: review?(pehrson)
Rank: 18
Priority: -- → P1
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.
Attachment #8797043 - Flags: review?(pehrson) → review+
Attachment #8797176 - Flags: review?(pehrson)
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.
Attachment #8797176 - Flags: review?(pehrson) → review+
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc4680ea08a9
generate fake audio for getUserMedia from MSG callbacks r=pehrsons
See Also: → 1212460
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, ...
Attachment #8802175 - Flags: review?(drno) → review+
Pushed by rjesup@wgate.com:
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
Flags: needinfo?(rjesup)
https://hg.mozilla.org/mozilla-central/rev/1d480bd0f758
https://hg.mozilla.org/mozilla-central/rev/c701835f10d7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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?
Flags: needinfo?(rjesup)
(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?
Flags: needinfo?(rjesup) → needinfo?(pehrson)
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 :-(
Flags: needinfo?(pehrson)
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
Attachment #8797043 - Flags: approval-mozilla-aurora?
Attachment #8802175 - Flags: approval-mozilla-aurora?
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.
Attachment #8797043 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8802175 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/c701835f10d7 needs rebasing for aurora
Flags: needinfo?(rjesup)
Rebased; running try for good measure due to all the test-disables here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3594bea3548070ccac1bae3b417b3284a1dc407
Flags: needinfo?(rjesup)
Flags: needinfo?(rjesup)
You need to log in before you can comment on or make changes to this bug.