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

RESOLVED FIXED in Firefox 51

Status

()

P1
normal
Rank:
18
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: padenot, Assigned: jesup)

Tracking

(Depends on: 1 bug)

45 Branch
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(12 attachments, 4 obsolete attachments)

3.57 KB, patch
achronop
: review+
Details | Diff | Splinter Review
3.68 KB, patch
jesup
: review+
Details | Diff | Splinter Review
11.46 KB, patch
jesup
: review+
Details | Diff | Splinter Review
58 bytes, text/x-review-board-request
pehrsons
: review+
Details
16.67 KB, patch
jesup
: review+
Details | Diff | Splinter Review
5.71 KB, patch
jesup
: review+
Details | Diff | Splinter Review
1.41 KB, patch
drno
: review+
Details | Diff | Splinter Review
2.77 KB, patch
Details | Diff | Splinter Review
4.89 KB, patch
Details | Diff | Splinter Review
1.04 KB, patch
drno
: review+
Details | Diff | Splinter Review
10.17 KB, patch
drno
: review+
Details | Diff | Splinter Review
3.41 KB, patch
pehrsons
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
Created attachment 8750721 [details] [diff] [review]
Back out and rewrite the resampling bypass code

It's adding latency for an unknown yet reason.
Attachment #8750721 - Flags: review?(achronop)
(Reporter)

Updated

3 years ago
Assignee: nobody → padenot
Status: NEW → ASSIGNED
(Reporter)

Updated

3 years ago
Depends on: 1268428
(Reporter)

Comment 2

3 years ago
This patch is just a backout of bug 1268428.
(Reporter)

Updated

3 years ago
Keywords: leave-open
(Assignee)

Comment 4

3 years ago
Created attachment 8750758 [details] [diff] [review]
Remove AudioGUM thread from MediaEngine getUserMedia input

part 1, removing AudioGUM thread
Attachment #8750758 - Flags: review?(padenot)
(Assignee)

Updated

3 years ago
Assignee: padenot → rjesup
(Reporter)

Comment 5

3 years ago
Created attachment 8750761 [details] [diff] [review]
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
Attachment #8750761 - Flags: review?(rjesup)
(Reporter)

Updated

3 years ago
Assignee: rjesup → padenot
(Reporter)

Comment 6

3 years ago
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.
Attachment #8750758 - Flags: review?(padenot) → review+
(Reporter)

Updated

3 years ago
Attachment #8750721 - Flags: review?(achronop)
(Assignee)

Updated

3 years ago
Attachment #8750761 - Flags: review?(rjesup) → review+
(Assignee)

Comment 7

3 years ago
Created attachment 8750953 [details] [diff] [review]
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
Attachment #8750953 - Flags: review?(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
Attachment #8750953 - Flags: review?(pehrsons) → review+
(Assignee)

Updated

3 years ago
Rank: 18
Priority: -- → P1
(Reporter)

Comment 10

3 years ago
Created attachment 8751750 [details] [diff] [review]
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.
Attachment #8751750 - Flags: review?(rjesup)
(Assignee)

Comment 11

3 years ago
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.
Attachment #8751750 - Flags: review?(rjesup) → review+
(Assignee)

Comment 14

3 years ago
Created attachment 8752658 [details] [diff] [review]
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
Attachment #8752658 - Flags: review?(pehrsons)
(Assignee)

Updated

3 years ago
Assignee: padenot → rjesup
(Assignee)

Comment 15

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=206d069765d1
Assignee: rjesup → padenot
Flags: needinfo?(padenot)
Attachment #8752658 - Flags: review?(pehrsons) → review+
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
Flags: needinfo?(rjesup)
(Assignee)

Comment 18

3 years ago
Created attachment 8752941 [details] [diff] [review]
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
Attachment #8752941 - Flags: review?(pehrsons)
(Assignee)

Updated

3 years ago
Assignee: padenot → rjesup
Attachment #8752941 - Flags: review?(pehrsons) → review+
The try didn't work?
(Reporter)

Comment 20

3 years ago
(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.
(Reporter)

Comment 23

3 years ago
Created attachment 8757822 [details]
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/
Attachment #8757822 - Flags: review?(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.
Attachment #8757822 - Flags: review?(pehrsons) → review+
(Assignee)

Comment 28

2 years ago
rebased the audio threading change:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3c87509bf9b
Flags: needinfo?(rjesup)
(Assignee)

Comment 29

2 years ago
Created attachment 8777322 [details] [diff] [review]
Proxy audio data to a separate thread for encoding

Rebased patch, merges the conduit reference fix
(Assignee)

Comment 30

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

carry forward r=pehrsons
Attachment #8777322 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8750953 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8752941 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8752658 - Attachment is obsolete: true
(Assignee)

Comment 31

2 years ago
Created attachment 8777328 [details] [diff] [review]
Remove AudioGUM thread from MediaEngine getUserMedia input

rebased removal of Audio GUM thread
(Assignee)

Updated

2 years ago
Attachment #8750758 - Attachment is obsolete: true
(Assignee)

Comment 32

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

carry forward r=padenot
Attachment #8777328 - Flags: review+
(Assignee)

Comment 33

2 years ago
Updated try with both patches:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f351d714abf
Keywords: leave-open
(Assignee)

Comment 34

2 years ago
Created attachment 8778689 [details] [diff] [review]
Use longer retries for stats checks after the first one

To reduce load from running the checks
Attachment #8778689 - Flags: review?(drno)
(Assignee)

Comment 35

2 years ago
Created attachment 8778691 [details] [diff] [review]
rework trackDisabling test to avoid running two analyzers at once

To reduce load, especially on VMs and doubly on android emulators
(Assignee)

Comment 36

2 years ago
Created attachment 8778692 [details] [diff] [review]
reduce load running clone trackDisabling tests by reducing the number of analyzers

May lengthen the time to complete slightly
(Assignee)

Comment 37

2 years ago
Created attachment 8778693 [details] [diff] [review]
Slow down webaudio+webrtc analysis result checks to reduce emulator test load
Attachment #8778693 - Flags: review?(drno)
(Assignee)

Comment 38

2 years ago
Created attachment 8778694 [details] [diff] [review]
Disable a number of webrtc tests on android emulator
Attachment #8778694 - Flags: review?(drno)
(Assignee)

Updated

2 years ago
Attachment #8778691 - Flags: review?(pehrson)
(Assignee)

Updated

2 years ago
Attachment #8778692 - Flags: review?(pehrson)
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?
Attachment #8778691 - Flags: review?(pehrson)
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.
Attachment #8778692 - Flags: review?(pehrson)
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?
(Assignee)

Comment 42

2 years ago
Created attachment 8778884 [details] [diff] [review]
Disable trackCloning tests on android and addtrack_removetrackevents
Attachment #8778884 - Flags: review?(pehrson)
Attachment #8778884 - Flags: review?(pehrson) → review+
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...
Attachment #8778689 - Flags: review?(drno) → review+
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.
Attachment #8778693 - Flags: review?(drno) → review+
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.
Attachment #8778694 - Flags: review?(drno) → review+

Comment 46

2 years ago
Pushed by rjesup@wgate.com:
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

2 years ago
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/354e31887dbd
Disable peerconnection_addtrack_removetrack_events, not getusermedia rs=jesup
Depends on: 1293531
(Assignee)

Updated

2 years ago
Depends on: 1297083

Updated

2 years ago
See Also: → bug 1369967
You need to log in before you can comment on or make changes to this bug.