1271585 - Back out and rewrite the resampling bypass code and WebRTCEngine to MSG code

Status: RESOLVED FIXED

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

Description

[Paul Adenot (:padenot)]

It's broken, not sure why yet, but it seems to add quite a lot of latency. 

I'm going to back it out and rewrite all that properly.

In particular, we should not use a dedicated thread for the processing, now that it's all on the same thread. We can now do the echo cancelling (if needed) and synchronously insert in the graph, which gets us lower latency and probably less CPU usage anyways.

Also the code using the packetizer continuously allocates, and packetizing is not needed anyway when not using processing, plus various other issues.

Comment 1

[Paul Adenot (:padenot)]

Attachment: Back out and rewrite the resampling bypass code

It's adding latency for an unknown yet reason.

Comment 2

[Paul Adenot (:padenot)]

This patch is just a backout of bug 1268428.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/88b42ae1269f

Comment 4

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

Attachment: Remove AudioGUM thread from MediaEngine getUserMedia input

part 1, removing AudioGUM thread

Comment 5

[Paul Adenot (:padenot)]

Attachment: Part 1 - Keep a buffer around instead of allocating for each packetizer packets for input data

This what was meant to happen, but it didn't work because `mInputBufferLen` was
never set. An nsTArray prevents this to happen.

MozReview-Commit-ID: EZ4RG4XofX1

Comment 6

[Paul Adenot (:padenot)]

Comment on attachment 8750758 [details] [diff] [review]
Remove AudioGUM thread from MediaEngine getUserMedia input

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

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp
@@ +740,5 @@
>                (i+1 < len) ? 0 : 1, insertTime);
>  
>        // This is safe from any thread, and is safe if the track is Finished
>        // or Destroyed.
> +      mSources[i]->AppendToTrack(mTrackID, segment, (AudioSegment *) nullptr);

The third argument default to `nullptr` already.

Comment 7

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

Attachment: Proxy audio data to a separate thread for encoding

Moved all the audio insertion in the pipeline to a shared thread, to replace the AudioGUM thread in GUM and avoid doing Opus/etc encodes on the audio input callback thread

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/88b42ae1269f

Comment 9

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8750953 [details] [diff] [review]
Proxy audio data to a separate thread for encoding

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

I'll just note that if you had used MozReview it'd have figured out what you had changed and what had just moved. Now I just assume the big chunks of added/removed code is old code that was moved.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +479,5 @@
> +// An async inserter for audio data, to avoid running audio codec encoders
> +// on the MSG/input audio thread.  Basically just bounces all the audio
> +// data to a single audio processing/input queue.  We could if we wanted to
> +// use multiple threads and a TaskQueue.
> +//

nit: delete this line

Comment 10

[Paul Adenot (:padenot)]

Attachment: Part 2 - Synchronously insert audio frames from the microphone in the MSG if possible

This is not optimal in the sense that it's keeping webrtc.org objects around,
but it's seems hard to deallocate them properly, it's a bit entangled.

Comment 11

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

Comment on attachment 8751750 [details] [diff] [review]
Part 2 - Synchronously insert audio frames from the microphone in the MSG if possible

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

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp
@@ +774,5 @@
>  
>    uint32_t len = mSources.Length();
>    for (uint32_t i = 0; i < len; i++) {
> +    MOZ_ASSERT(!isStereo);
> +    InsertInGraph<int16_t>(audio10ms, length, 1);

So this patch (here and InsertInGraph) appears to lose the ability to process stereo input, right?  was stereo every actually working through gUM?  (Note that we'd want it to work for things like audio-screencapture, and in some cases stereo mics, especially if you disable all procesing.

At minimum we should verify if we can ever get a stereo signal here and trigger the assert, and file a followup and include hte bug # here, and have that block audio screencapture.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01262a32c45f
https://hg.mozilla.org/integration/mozilla-inbound/rev/3967d009efc0

Comment 13

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f52e4417aa2f for media test failures:
https://treeherder.mozilla.org/logviewer.html#?job_id=27794517&repo=mozilla-inbound

Comment 14

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

Attachment: add missing RefPtr to make WrapRunnable hold refs to the object

This should fix the crash on packetizer_ - it's a timing issue on close/free of the pipelines; likely why it showed up on Android

Comment 15

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=206d069765d1

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/454ff63e5995
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba3e7b53306b

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for crash in mediatest test_dataChannel_basicAudio.html on Android

https://hg.mozilla.org/integration/mozilla-inbound/rev/10ea1cabf6d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/349f5ef87e5b

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ba3e7b53306bbd0e1e758ce14073895ebd86ff5e
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=27939080&repo=mozilla-inbound

06:48:34     INFO -  3 INFO TEST-START | dom/media/tests/mochitest/test_dataChannel_basicAudio.html
06:51:58     INFO -  INFO | automation.py | Application ran for: 0:04:17.513232
06:51:58     INFO -  INFO | zombiecheck | Reading PID log: /tmp/tmpD5hJcqpidlog
06:51:59     INFO -  /data/tombstones does not exist; tombstone check skipped
06:51:59     INFO -  mozcrash Downloading symbols from: https://queue.taskcluster.net/v1/task/CRT3wxqHRA2fGGSM_jv8rQ/artifacts/public/build/fennec-49.0a1.en-US.android-arm.crashreporter-symbols.zip
06:52:03     INFO -  mozcrash Copy/paste: /builds/slave/test/build/linux64-minidump_stackwalk /tmp/tmpvN_8wQ/68db250b-9a26-9c16-180fb2ae-0d1b1baf.dmp /tmp/tmpWL1UhU
06:52:13     INFO -  mozcrash Saved minidump as /builds/slave/test/build/blobber_upload_dir/68db250b-9a26-9c16-180fb2ae-0d1b1baf.dmp
06:52:13     INFO -  mozcrash Saved app info as /builds/slave/test/build/blobber_upload_dir/68db250b-9a26-9c16-180fb2ae-0d1b1baf.extra
06:52:13  WARNING -  PROCESS-CRASH | dom/media/tests/mochitest/test_dataChannel_basicAudio.html | application crashed [@ mozilla::AudioProxyThread::InternalProcessAudioChunk]
06:52:13     INFO -  Crash dump filename: /tmp/tmpvN_8wQ/68db250b-9a26-9c16-180fb2ae-0d1b1baf.dmp
06:52:13     INFO -  Operating system: Android
06:52:13     INFO -                    0.0.0 Linux 2.6.29-gea477bb #1 Wed Sep 26 11:04:45 PDT 2012 armv7l
06:52:13     INFO -  CPU: arm
06:52:13     INFO -       ARMv7 ARM Cortex-A8 features: swp,half,thumb,fastmult,vfpv2,edsp,neon,vfpv3
06:52:13     INFO -       1 CPU
06:52:13     INFO -  Crash reason:  SIGSEGV
06:52:13     INFO -  Crash address: 0xe5e5e63d
06:52:13     INFO -  Process uptime: not available
06:52:13     INFO -  Thread 55 (crashed)
06:52:13     INFO -   0  libxul.so!mozilla::AudioProxyThread::InternalProcessAudioChunk [MediaPipeline.cpp:ba3e7b53306b : 559 + 0xa]
06:52:13     INFO -       r0 = 0x60ff9500    r1 = 0x65abee38    r2 = 0x000001b9    r3 = 0xe5e5e5e5
06:52:13     INFO -       r4 = 0x000001b9    r5 = 0x64070040    r6 = 0x65abee34    r7 = 0x000001b9
06:52:13     INFO -       r8 = 0x611a0820    r9 = 0x65abee38   r10 = 0x60ff9500   r12 = 0x00000527
06:52:13     INFO -       fp = 0x0000ac44    sp = 0x65abee20    lr = 0x5bbe7587    pc = 0x5bbe7cee
06:52:13     INFO -      Found by: given as instruction pointer in context
06:52:13     INFO -   1  libxul.so!mozilla::runnable_args_memfn<RefPtr<mozilla::AudioProxyThread>, void (mozilla::AudioProxyThread::*)(mozilla::AudioSessionConduit*, int, mozilla::AudioChunk&, bool), mozilla::AudioSessionConduit*, int, mozilla::AudioChunk, bool>::Run [runnable_utils.h:ba3e7b53306b : 102 + 0x7]
06:52:13     INFO -       r4 = 0x60dc4ce0    r5 = 0x5bbe7afd    r6 = 0x00000000    r7 = 0x6109eb30
06:52:13     INFO -       r8 = 0x65abfda8    r9 = 0x65abfdb4   r10 = 0x65abfdac    fp = 0x65abfdb0
06:52:13     INFO -       sp = 0x65abfd60    pc = 0x5bbe68b5
06:52:13     INFO -      Found by: call frame info
06:52:13     INFO -   2  libxul.so!nsThreadPool::Run [nsThreadPool.cpp:ba3e7b53306b : 227 + 0x3]
06:52:13     INFO -       r4 = 0x6109eb20    r5 = 0x00000000    r6 = 0x00000000    r7 = 0x6109eb30
06:52:13     INFO -       r8 = 0x65abfda8    r9 = 0x65abfdb4   r10 = 0x65abfdac    fp = 0x65abfdb0
06:52:13     INFO -       sp = 0x65abfd80    pc = 0x5b8b77fb
06:52:13     INFO -      Found by: call frame info
06:52:13     INFO -   3  libxul.so!nsThread::ProcessNextEvent [nsThread.cpp:ba3e7b53306b : 1073 + 0x3]
06:52:13     INFO -       r4 = 0x678d66d0    r5 = 0x00000000    r6 = 0x65abfe00    r7 = 0x65abfe0c
06:52:13     INFO -       r8 = 0x00000000    r9 = 0x65abfe47   r10 = 0x00000000    fp = 0x678d6704
06:52:13     INFO -       sp = 0x65abfde0    pc = 0x5b8b65e9
06:52:13     INFO -      Found by: call frame info
06:52:13     INFO -   4  libxul.so!NS_ProcessNextEvent [nsThreadUtils.cpp:ba3e7b53306b : 290 + 0xb]
06:52:13     INFO -       r4 = 0x00000000    r5 = 0x00000000    r6 = 0x6108c060    r7 = 0x678d66d0
06:52:13     INFO -       r8 = 0x61d18440    r9 = 0x678d6704   r10 = 0x65abff00    fp = 0x2a366028
06:52:13     INFO -       sp = 0x65abfe40    pc = 0x5b8c86f5
06:52:13     INFO -      Found by: call frame info
06:52:13     INFO -   5  libxul.so!mozilla::ipc::MessagePumpForNonMainThreads::Run [MessagePump.cpp:ba3e7b53306b : 352 + 0x7]
06:52:13     INFO -       r4 = 0x61d18430    r5 = 0x00000000    r6 = 0x6108c060    r7 = 0x678d66d0
06:52:13     INFO -       r8 = 0x61d18440    r9 = 0x678d6704   r10 = 0x65abff00    fp = 0x2a366028
06:52:13     INFO -       sp = 0x65abfe50    pc = 0x5ba2679d
06:52:13     INFO -      Found by: call frame info
06:52:13     INFO -   6  libxul.so!MessageLoop::Run [message_loop.cc:ba3e7b53306b : 226 + 0x5]
06:52:13     INFO -       r4 = 0x6108c060    r5 = 0x65abfea0    r6 = 0x65abfe9c    r7 = 0x6108c060
06:52:13     INFO -       r8 = 0x678d66dc    r9 = 0x678d6704   r10 = 0x65abff00    fp = 0x2a366028
06:52:13     INFO -       sp = 0x65abfe80    pc = 0x5ba17513
06:52:13     INFO -      Found by: call frame info
06:52:13     INFO -   7  libxul.so!nsThread::ThreadFunc [nsThread.cpp:ba3e7b53306b : 465 + 0x5]
06:52:13     INFO -       r4 = 0x678d66d0    r5 = 0x65abfea0    r6 = 0x65abfe9c    r7 = 0x6108c060
06:52:13     INFO -       r8 = 0x678d66dc    r9 = 0x678d6704   r10 = 0x65abff00    fp = 0x2a366028
06:52:13     INFO -       sp = 0x65abfe98    pc = 0x5b8b815b
06:52:13     INFO -      Found by: call frame info
06:52:13     INFO -   8  libnss3.so!_pt_root [ptthread.c:ba3e7b53306b : 216 + 0x5]
06:52:13     INFO -       r4 = 0x659f4500    r5 = 0x00000000    r6 = 0x52b096d0    r7 = 0x2a366028
06:52:13     INFO -       r8 = 0x52b096d0    r9 = 0x65a80000   r10 = 0x65abff00    fp = 0x2a366028
06:52:13     INFO -       sp = 0x65abfec8    pc = 0x52ac0ae9
06:52:13     INFO -      Found by: call frame info
06:52:13     INFO -   9  libc.so!__thread_entry [pthread_create.cpp : 92 + 0x6]
06:52:13     INFO -       r4 = 0x65abff00    r5 = 0x2a366028    r6 = 0x52ac0a2d    r7 = 0x659f4500
06:52:13     INFO -       r8 = 0x66f7f92c    r9 = 0x65a80000   r10 = 0x65abff00    fp = 0x2a366028
06:52:13     INFO -       sp = 0x65abfee8    pc = 0x40033a5c
06:52:13     INFO -      Found by: call frame info
06:52:13     INFO -  10  libc.so!pthread_create [pthread_create.cpp : 201 + 0x16]
06:52:13     INFO -       r3 = 0x659f4500    r4 = 0x00000000    r5 = 0x00040000    r6 = 0x659f4500
06:52:13     INFO -       r7 = 0x00000078    r8 = 0x66f7f92c    r9 = 0x65a80000   r10 = 0x65abff00
06:52:13     INFO -       fp = 0x2a366028    sp = 0x65abff00    pc = 0x40033bd8
06:52:13     INFO -      Found by: call frame info

Comment 18

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

Attachment: Conduit needs to be held as a reference as well

We need to ensure the conduit sticks around as well, and we can't just make it a RefPtr<>& because it has to be released on MainThread.  Simpler to encapsulate it in the AudioProxyThread object to start with. Try: (will check this time!) https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5D5Dc5477bd9e33

Comment 19

[Andreas Pehrson [:pehrsons]]

The try didn't work?

Comment 20

[Paul Adenot (:padenot)]

(In reply to Randell Jesup [:jesup] from comment #11)
> Comment on attachment 8751750 [details] [diff] [review]
> Part 2 - Synchronously insert audio frames from the microphone in the MSG if
> possible
> 
> Review of attachment 8751750 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp
> @@ +774,5 @@
> >  
> >    uint32_t len = mSources.Length();
> >    for (uint32_t i = 0; i < len; i++) {
> > +    MOZ_ASSERT(!isStereo);
> > +    InsertInGraph<int16_t>(audio10ms, length, 1);
> 
> So this patch (here and InsertInGraph) appears to lose the ability to
> process stereo input, right?  was stereo every actually working through gUM?
> (Note that we'd want it to work for things like audio-screencapture, and in
> some cases stereo mics, especially if you disable all procesing.

No, it works with audio capture and web audio and HTMLMediaElement -> PeerConnection, but not with microphones, I think. It's easy to enable stereo mic in this processing code, but currently mono input is hard coded in GraphDriver.cpp.

Comment 21

[Paul Adenot (:padenot)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=841b5ee3c208

Comment 22

[Paul Adenot (:padenot)]

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

Comment 23

[Paul Adenot (:padenot)]

Attachment: MozReview Request: Bug 1271585 - Part 2 - Synchronously insert audio frames from the microphone in the MSG if possible. r?pehrsons

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

Comment 24

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8757822 [details]
MozReview Request: Bug 1271585 - Part 2 - Synchronously insert audio frames from the microphone in the MSG if possible.  r?pehrsons

https://reviewboard.mozilla.org/r/56226/#review52850

Looks good. I only have minor issues and nits.

::: dom/media/webrtc/MediaEngine.h:64
(Diff revision 1)
>    static const int DEFAULT_SAMPLE_RATE = 16000;
>  #endif
> +  // This allows using whatever rate the graph is using for the
> +  // MediaStreamTrack. This is useful for microphone data, we know it's already
> +  // at the correct rate for insertion in the MSG.
> +  static const int USE_GRAPH_RATE = 0;

In my book 0 should be avoided since it's the default value for int. Can we do with -1?

::: dom/media/webrtc/MediaEngineWebRTC.h:521
(Diff revision 1)
> +  bool PassThrough() {
> +    return mSkipProcessing;
> +  }
> +  template<typename T>
> +  void InsertInGraph(const T* aBuffer,
> +                     size_t aFrames,
> +                     uint32_t aChannels);
> +  void PacketizeAndProcess(MediaStreamGraph* aGraph,
> +                           const AudioDataValue* aBuffer,
> +                           size_t aFrames,
> +                           TrackRate aRate,
> +                           uint32_t aChannels);

Empty line between methods for readability.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:497
(Diff revision 1)
>      }
>      int16_t* packet = mInputBuffer.Elements();
>      mPacketizer->Output(packet);
>  
> -    mVoERender->ExternalRecordingInsertData(packet, samplesPerPacket, aRate, 0);
> +    mVoERender->ExternalRecordingInsertData(packet, samplesPerPacket,
> +        aRate, 0);

Indentation.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:498
(Diff revision 1)
>      int16_t* packet = mInputBuffer.Elements();
>      mPacketizer->Output(packet);
>  
> -    mVoERender->ExternalRecordingInsertData(packet, samplesPerPacket, aRate, 0);
> +    mVoERender->ExternalRecordingInsertData(packet, samplesPerPacket,
> +        aRate, 0);
> +    }

Indentation.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:511
(Diff revision 1)
> +  uint32_t len = mSources.Length();
> +  for (uint32_t i = 0; i < len; i++) {

Doesn't Length() return size_t?

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:524
(Diff revision 1)
> +    LogTime(AsyncLatencyLogger::AudioTrackInsertion, LATENCY_STREAM_ID(mSources[i].get(), mTrackID),
> +        (i+1 < len) ? 0 : 1, insertTime);

Line length, indentation.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:529
(Diff revision 1)
> +    LogTime(AsyncLatencyLogger::AudioTrackInsertion, LATENCY_STREAM_ID(mSources[i].get(), mTrackID),
> +        (i+1 < len) ? 0 : 1, insertTime);
> +
> +    nsAutoPtr<AudioSegment> segment(new AudioSegment());
> +    AutoTArray<const T*, 1> channels;
> +    // XXX handle stereo

Add a bug number.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:530
(Diff revision 1)
> +        (i+1 < len) ? 0 : 1, insertTime);
> +
> +    nsAutoPtr<AudioSegment> segment(new AudioSegment());
> +    AutoTArray<const T*, 1> channels;
> +    // XXX handle stereo
> +    MOZ_ASSERT(aChannels == 1);

I think a message would be useful here!

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:538
(Diff revision 1)
> +                         mPrincipalHandles[i]);
> +    segment->GetStartTime(insertTime);
> +
> +    RUN_ON_THREAD(mThread,
> +                  WrapRunnable(mSources[i], &SourceMediaStream::AppendToTrack,
> +                               mTrackID, segment, (AudioSegment*)nullptr),

Use c++-style static_cast instead.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:554
(Diff revision 1)
> +  if (!PassThrough()) {
> +    PacketizeAndProcess(aGraph, aBuffer, aFrames, aRate, aChannels);
> +  } else {
> +    InsertInGraph<AudioDataValue>(aBuffer, aFrames, aChannels);
>    }

Remove the negation in the conditional for readability.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/466534e08cbc
https://hg.mozilla.org/integration/mozilla-inbound/rev/999676db297b

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a2d02375445

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/466534e08cbc
https://hg.mozilla.org/mozilla-central/rev/999676db297b
https://hg.mozilla.org/mozilla-central/rev/7a2d02375445

Comment 28

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

rebased the audio threading change:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3c87509bf9b

Comment 29

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

Attachment: Proxy audio data to a separate thread for encoding

Rebased patch, merges the conduit reference fix

Comment 30

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

Comment on attachment 8777322 [details] [diff] [review]
Proxy audio data to a separate thread for encoding

carry forward r=pehrsons

Comment 31

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

Attachment: Remove AudioGUM thread from MediaEngine getUserMedia input

rebased removal of Audio GUM thread

Comment 32

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

Comment on attachment 8777328 [details] [diff] [review]
Remove AudioGUM thread from MediaEngine getUserMedia input

carry forward r=padenot

Comment 33

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

Updated try with both patches:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f351d714abf

Comment 34

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

Attachment: Use longer retries for stats checks after the first one

To reduce load from running the checks

Comment 35

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

Attachment: rework trackDisabling test to avoid running two analyzers at once

To reduce load, especially on VMs and doubly on android emulators

Comment 36

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

Attachment: reduce load running clone trackDisabling tests by reducing the number of analyzers

May lengthen the time to complete slightly

Comment 37

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

Attachment: Slow down webaudio+webrtc analysis result checks to reduce emulator test load

Comment 38

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

Attachment: Disable a number of webrtc tests on android emulator

Comment 39

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8778691 [details] [diff] [review]
rework trackDisabling test to avoid running two analyzers at once

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

Perhaps splitting audio and video into separate tests is a better long-term solution.

::: dom/media/tests/mochitest/test_peerConnection_trackDisabling.html
@@ +79,5 @@
>          .then(() => checkAudioEnabled(localAnalyser))
> +        .then(() => test.pcLocal._pc.getLocalStreams()[0].getAudioTracks()[0].enabled = false)
> +        .then(() => info("Checking local audio disabled"))
> +        .then(() => checkAudioDisabled(localAnalyser))
> +        .then(() => localAnalyser = null)

Does this rely on GC?

Comment 40

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8778692 [details] [diff] [review]
reduce load running clone trackDisabling tests by reducing the number of analyzers

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

Same thoughts here with splitting audio and video into separate tests.

Comment 41

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8778693 [details] [diff] [review]
Slow down webaudio+webrtc analysis result checks to reduce emulator test load

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

Would changing the analyser's fftSize change how much data to process?

Comment 42

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

Attachment: Disable trackCloning tests on android and addtrack_removetrackevents

Comment 43

[Nils Ohlmeier [:drno]]

Comment on attachment 8778689 [details] [diff] [review]
Use longer retries for stats checks after the first one

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

LGTM

::: dom/media/tests/mochitest/pc.js
@@ +1464,3 @@
>        .then(stats => hasFlow(stats)? ok(true, "RTP flowing for track " + track.id) :
> +            wait(delay).then(retry(1000)));
> +    return retry(200);

Makes me wonder if this should get implemented as a kind of exponential back off...

Comment 44

[Nils Ohlmeier [:drno]]

Comment on attachment 8778693 [details] [diff] [review]
Slow down webaudio+webrtc analysis result checks to reduce emulator test load

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

Just a clarification question, but otherwise LGTM

::: dom/media/tests/mochitest/head.js
@@ +127,5 @@
>            return;
>          }
>          // else, we need more time
> +	// would love to make it somewhat adaptive (backoff)  
> +        setTimeout(analysisLoop, 500);

Second place where an exponential back off appears to make sense. Maybe we should add some helper function to head.js for that.

Did you on purpose drop the requestAnimationFrame() call?
I guess it is pointless as you sleep 500ms now. Just wanted to make sure that you did not drop this by accident.

Comment 45

[Nils Ohlmeier [:drno]]

Comment on attachment 8778694 [details] [diff] [review]
Disable a number of webrtc tests on android emulator

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

Would prefer if you would make the suggested change, but technically not needed before landing.

::: dom/media/tests/mochitest/mochitest.ini
@@ +24,5 @@
>    !/dom/media/test/gizmo.mp4
>  
>  [test_a_noOp.html]
>  [test_dataChannel_basicAudio.html]
> +skip-if = toolkit == 'gonk' || buildapp == 'mulet' || (android_version == '18' && debug) # Bug 962984 for debug, bug 963244 for opt

I think you should replace the "android_version == '18'" checks in this patch with "toolkit == 'android'", as I don't see how these test failures are specific to a version of Android.

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eee98c38241e
Remove AudioGUM thread from MediaEngine getUserMedia input r=padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/e49e6e9abae3
Proxy audio data to a separate thread for encoding r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/5315624028d5
Use longer retries for stats checks after the first one r=drno
https://hg.mozilla.org/integration/mozilla-inbound/rev/a82c9e7d2030
Disable a number of webrtc tests on android emulator r=drno
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b36a3160529
Disable trackCloning tests on android and addtrack_removetrackevents r=pehrsons

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/354e31887dbd
Disable peerconnection_addtrack_removetrack_events, not getusermedia rs=jesup

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eee98c38241e
https://hg.mozilla.org/mozilla-central/rev/e49e6e9abae3
https://hg.mozilla.org/mozilla-central/rev/5315624028d5
https://hg.mozilla.org/mozilla-central/rev/a82c9e7d2030
https://hg.mozilla.org/mozilla-central/rev/6b36a3160529
https://hg.mozilla.org/mozilla-central/rev/354e31887dbd