Closed
Bug 1271585
Opened 9 years ago
Closed 8 years ago
Back out and rewrite the resampling bypass code and WebRTCEngine to MSG code
Categories
(Core :: WebRTC: Audio/Video, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: padenot, Assigned: jesup)
References
Details
Attachments
(12 files, 4 obsolete files)
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 |
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•9 years ago
|
||
It's adding latency for an unknown yet reason.
Attachment #8750721 -
Flags: review?(achronop)
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → padenot
Status: NEW → ASSIGNED
Reporter | ||
Comment 2•9 years ago
|
||
This patch is just a backout of bug 1268428.
Updated•9 years ago
|
Attachment #8750721 -
Flags: review+
Reporter | ||
Updated•9 years ago
|
Keywords: leave-open
Assignee | ||
Comment 4•9 years ago
|
||
part 1, removing AudioGUM thread
Attachment #8750758 -
Flags: review?(padenot)
Assignee | ||
Updated•9 years ago
|
Assignee: padenot → rjesup
Reporter | ||
Comment 5•9 years ago
|
||
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•9 years ago
|
Assignee: rjesup → padenot
Reporter | ||
Comment 6•9 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•9 years ago
|
Attachment #8750721 -
Flags: review?(achronop)
Assignee | ||
Updated•9 years ago
|
Attachment #8750761 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 7•9 years ago
|
||
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 8•9 years ago
|
||
bugherder |
Comment 9•9 years ago
|
||
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•9 years ago
|
Rank: 18
Priority: -- → P1
Reporter | ||
Comment 10•9 years ago
|
||
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•9 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+
Comment 12•9 years ago
|
||
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
Flags: needinfo?(padenot)
Assignee | ||
Comment 14•9 years ago
|
||
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•9 years ago
|
Assignee: padenot → rjesup
Assignee | ||
Comment 15•9 years ago
|
||
Assignee: rjesup → padenot
Flags: needinfo?(padenot)
Updated•9 years ago
|
Attachment #8752658 -
Flags: review?(pehrsons) → review+
Comment 16•9 years ago
|
||
Comment 17•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
Assignee: padenot → rjesup
Updated•9 years ago
|
Attachment #8752941 -
Flags: review?(pehrsons) → review+
Comment 19•9 years ago
|
||
The try didn't work?
Reporter | ||
Comment 20•9 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 21•9 years ago
|
||
Reporter | ||
Comment 22•9 years ago
|
||
Reporter | ||
Comment 23•9 years ago
|
||
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 24•9 years ago
|
||
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+
Comment 25•9 years ago
|
||
Comment 26•9 years ago
|
||
Comment 27•9 years ago
|
||
bugherder |
Assignee | ||
Comment 28•8 years ago
|
||
rebased the audio threading change:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3c87509bf9b
Flags: needinfo?(rjesup)
Assignee | ||
Comment 29•8 years ago
|
||
Rebased patch, merges the conduit reference fix
Assignee | ||
Comment 30•8 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•8 years ago
|
Attachment #8750953 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8752941 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8752658 -
Attachment is obsolete: true
Assignee | ||
Comment 31•8 years ago
|
||
rebased removal of Audio GUM thread
Assignee | ||
Updated•8 years ago
|
Attachment #8750758 -
Attachment is obsolete: true
Assignee | ||
Comment 32•8 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•8 years ago
|
||
Updated try with both patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f351d714abf
Keywords: leave-open
Assignee | ||
Comment 34•8 years ago
|
||
To reduce load from running the checks
Attachment #8778689 -
Flags: review?(drno)
Assignee | ||
Comment 35•8 years ago
|
||
To reduce load, especially on VMs and doubly on android emulators
Assignee | ||
Comment 36•8 years ago
|
||
May lengthen the time to complete slightly
Assignee | ||
Comment 37•8 years ago
|
||
Attachment #8778693 -
Flags: review?(drno)
Assignee | ||
Comment 38•8 years ago
|
||
Attachment #8778694 -
Flags: review?(drno)
Assignee | ||
Updated•8 years ago
|
Attachment #8778691 -
Flags: review?(pehrson)
Assignee | ||
Updated•8 years ago
|
Attachment #8778692 -
Flags: review?(pehrson)
Comment 39•8 years ago
|
||
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 40•8 years ago
|
||
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 41•8 years ago
|
||
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•8 years ago
|
||
Attachment #8778884 -
Flags: review?(pehrson)
Updated•8 years ago
|
Attachment #8778884 -
Flags: review?(pehrson) → review+
Comment 43•8 years ago
|
||
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 44•8 years ago
|
||
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 45•8 years ago
|
||
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•8 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•8 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
Comment 48•8 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•