Revised ADM for Gonk

Revised ADM for Gonk

[jib]

http://firefox-codereview.appspot.com/19001/diff/1/content/media/webrtc/Audio...
File content/media/webrtc/AudioDeviceGonk.cpp (right):

http://firefox-codereview.appspot.com/19001/diff/1/content/media/webrtc/Audio...
content/media/webrtc/AudioDeviceGonk.cpp:124: , mAudioFlinger(nullptr)
You need

 , mInitialized(false)

or things crash (mThreadRec == null).

[ekr-webrtc]

I see a number of issues as noted below.

It seems the stubs still need more review. Please go through and
examine them for:

- Side effects on the arguments when they are unimplemented. (Why?)
- Differences from the Chromium code.

Each such instance needs an explanation and probably a comment.

The threading model seems confusing and I believe contains errors.
In addition to addressing the threading issues noted below, please.

1. Add a block comment documenting the model.
2. Document each member with the threads it is used in.

http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A...
File content/media/webrtc/AudioDeviceGonk.cpp (right):

http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A...
content/media/webrtc/AudioDeviceGonk.cpp:48: PRLogModuleInfo* GetAudioLog()
Please do not name extern functions this generically. Why isn't this a class
static?

http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A...
content/media/webrtc/AudioDeviceGonk.cpp:50: static PRLogModuleInfo *sLog =
PR_NewLogModule("audio");
If this is a static, why are you getting it repeatedly?

Doesn't this need a thread lock?

http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A...
content/media/webrtc/AudioDeviceGonk.cpp:53: #define LOG(msg)
PR_LOG(GetAudioLog(), PR_LOG_DEBUG, msg)
PR_LOG already has this wrapper. That's why it's been done as a macro. No need
for an additional conditional compilation stage.

http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A...
content/media/webrtc/AudioDeviceGonk.cpp:68: AudioDeviceModule*
AudioDeviceGonk::Create(const int32_t id,
Please fix indents, here and below

http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A...
content/media/webrtc/AudioDeviceGonk.cpp:317: //SetSpeakerVolume(0);
If you're not going to use this, remotve it, not comment out.

http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A...
content/media/webrtc/AudioDeviceGonk.cpp:379: mAudioRecord->stop();
What are the API guarantees here? Does mAudioRecord promise synchronous
termination? Note that if (for instance) it was implemented with a message
queue, you might not get the expected behavior.

http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A...
content/media/webrtc/AudioDeviceGonk.cpp:488: struct AudioRecord::Buffer *b =
static_cast<AudioRecord::Buffer *>(info);
If you are going to have a CritSect here you need to actually check for
recording being terminated. Otherwise you can have:

T1:                                         T2:
Acquire lock
                                              RecordingCbf()
                                              Try to acquire (wait)
Stop recording
Release Lock
                                              Lock acquired
                                              try to process frames

Note that you are also trying to modify mNumPartialFrames concurrently in two
threads, since StopRecording does it outside of the stop critsect.

http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A...
content/media/webrtc/AudioDeviceGonk.cpp:507:
mAudioDeviceBuffer.SetRecordedBufferCombine(mPartialBuffer,
Please rewrite this to buffer internally rather than modifying GIPS.

http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A...
File content/media/webrtc/AudioDeviceGonk.h (right):

http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A...
content/media/webrtc/AudioDeviceGonk.h:229: mutable mozilla::ReentrantMonitor
mCritSect;
Please annotate each of these variables with which threads they are used in.

http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A...
File content/media/webrtc/AudioDeviceGonkStubs.cpp (right):

http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A...
content/media/webrtc/AudioDeviceGonkStubs.cpp:54: #define WARN(msg)
Again, no need for this.

http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A...
content/media/webrtc/AudioDeviceGonkStubs.cpp:103: MOZ_ADM_SATISFY_GIPS(-1);
This returns 0 in Chromium. Why not here.

http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A...
content/media/webrtc/AudioDeviceGonkStubs.cpp:112: MOZ_ADM_SATISFY_GIPS(-1);
Returns 0 in chromiumm

http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A...
content/media/webrtc/AudioDeviceGonkStubs.cpp:122: *available = true;
Returns 0 in chromium.

http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A...
content/media/webrtc/AudioDeviceGonkStubs.cpp:174: *volume = 0;
Why are you setting values when these are supposed to be asserted?

[ekr-webrtc]

Tim, here is my recent review. It might be useful for you to go over this as
well.

[ekr-webrtc]

There are a bunch of nits here, but I think you have some actual bugs in the
cbf.

https://firefox-codereview.appspot.com/19001/diff/18001/content/media/webrtc/...
File content/media/webrtc/AudioDeviceGonk.cpp (right):

https://firefox-codereview.appspot.com/19001/diff/18001/content/media/webrtc/...
content/media/webrtc/AudioDeviceGonk.cpp:50: #define WARN(msg)
PR_LOG(GetUserMediaLog(), PR_LOG_WARNING, msg)
Do you need #undef WARN

https://firefox-codereview.appspot.com/19001/diff/18001/content/media/webrtc/...
content/media/webrtc/AudioDeviceGonk.cpp:236: // members are guaranteed to exist
for the life of callback. No ASAN risk.
I'm not sure you want to use ASAN/TSAN here.
Those are just tools.

https://firefox-codereview.appspot.com/19001/diff/18001/content/media/webrtc/...
content/media/webrtc/AudioDeviceGonk.cpp:253: false,                 //
threadCanCallJava
Are these tabs?

https://firefox-codereview.appspot.com/19001/diff/18001/content/media/webrtc/...
content/media/webrtc/AudioDeviceGonk.cpp:316: // (calls may not stop
immediately, but access is neutered).
can you rewrite this comment. I'm not sure what access is neutered means.

https://firefox-codereview.appspot.com/19001/diff/18001/content/media/webrtc/...
File content/media/webrtc/AudioDeviceGonk.h (right):

https://firefox-codereview.appspot.com/19001/diff/18001/content/media/webrtc/...
content/media/webrtc/AudioDeviceGonk.h:233: void SetLatency(uint16_t latency);
Whitespace.

https://firefox-codereview.appspot.com/19001/diff/18001/content/media/webrtc/...
content/media/webrtc/AudioDeviceGonk.h:233: void SetLatency(uint16_t latency);
Maybe rename latency to latencyMS or something?

https://firefox-codereview.appspot.com/19001/diff/18001/content/media/webrtc/...
content/media/webrtc/AudioDeviceGonk.h:235: // All member access is locked
s/locked/protected by mLock/

https://firefox-codereview.appspot.com/19001/diff/18001/content/media/webrtc/...
content/media/webrtc/AudioDeviceGonk.h:246: mutable mozilla::ReentrantMonitor
mLock;
What is mutable doing here?

https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/...
File content/media/webrtc/AudioDeviceGonk.cpp (right):

https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/...
content/media/webrtc/AudioDeviceGonk.cpp:50: #define WARN(msg)
PR_LOG(GetUserMediaLog(), PR_LOG_WARNING, msg)
#undef WARN?

https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/...
content/media/webrtc/AudioDeviceGonk.cpp:238: // TSAN: All members accessed by
RecordingBuffer::Cbf are inside that subclass
I wouldn't use ASAN and TSAN as terms this way. They're tools for detecting
specific kinds of errors. You should refer to the actual errors.

https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/...
content/media/webrtc/AudioDeviceGonk.cpp:241: // around stop as well, and test
on RecordingBuffer::mRecording in callback
Wouldn't it be a lot simpler to just instantiate a new mAudioRecord with each
call to StartRecording(). Then you wouldn't need any recording interlock here.

https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/...
content/media/webrtc/AudioDeviceGonk.cpp:262: // TODO(jib@mozilla.com): Find
optimal delay for AEC (may be hardware specific)
Is this TODO still active? Where would you get it other than from here?

https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/...
content/media/webrtc/AudioDeviceGonk.cpp:316: // (calls may not stop
immediately, but access is neutered).
This comment is pretty unclear. What does "access is neutered" mean?

https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/...
content/media/webrtc/AudioDeviceGonk.cpp:458: if (mNumPartialFrames) {
What happens if Android removes the 1024 restriction. As a thought experiment,
say b->frameCount is always 1. This is not going to be good.

https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/...
content/media/webrtc/AudioDeviceGonk.cpp:471: SetVQEData(0, mLatency, 0);
Don't you need to recompute latency here, since you are not delivering packets
all at once.

https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/...
File content/media/webrtc/AudioDeviceGonk.h (right):

https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/...
content/media/webrtc/AudioDeviceGonk.h:58: const uint32_t N_REC_SAMPLES_PER_SEC
= 16000; // Same as for android
I thought Android didn't guarantee that rates != 44.1 kHz would work.

https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/...
content/media/webrtc/AudioDeviceGonk.h:224: // by the AudioRecord API's
mClientRecordingThread, which we're using.
I don't understand this reference to mClientRecordThread. That's an internal
detail. The question is what the API guarantees are.

https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/...
content/media/webrtc/AudioDeviceGonk.h:236: mutable mozilla::ReentrantMonitor
mLock;
Why is this "mutable". Are there const methods that require the lock?

https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/...
content/media/webrtc/AudioDeviceGonk.h:246: mutable mozilla::ReentrantMonitor
mLock;
Same question as above.