Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(14)

Issue 19001: Revised ADM for Gonk

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 7 months ago by ekr-webrtc
Modified:
7 years, 6 months ago
Reviewers:
jib, tterribe
Visibility:
Public.

Description

Revised ADM for Gonk

Patch Set 1 #

Total comments: 1

Patch Set 2 : JIB's revision #

Patch Set 3 : #

Total comments: 14

Patch Set 4 : JIB's patch #

Total comments: 8

Patch Set 5 : Patchset 4 #

Total comments: 11

Patch Set 6 : patch version 5 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
A content/media/webrtc/AudioDeviceGonk.cpp View 1 2 3 4 5 1 chunk +484 lines, -0 lines 0 comments Download
A content/media/webrtc/AudioDeviceGonk.h View 1 2 3 4 5 1 chunk +257 lines, -0 lines 0 comments Download
A content/media/webrtc/AudioDeviceGonkStubs.cpp View 1 2 3 4 5 1 chunk +847 lines, -0 lines 0 comments Download
M content/media/webrtc/Makefile.in View 1 chunk +5 lines, -0 lines 0 comments Download
M content/media/webrtc/MediaEngineWebRTC.cpp View 1 2 3 2 chunks +18 lines, -12 lines 0 comments Download
M content/media/webrtc/MediaEngineWebRTC.h View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M content/media/webrtc/MediaEngineWebRTCAudio.cpp View 1 2 3 2 chunks +9 lines, -1 line 0 comments Download
M content/media/webrtc/moz.build View 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 4
jib
http://firefox-codereview.appspot.com/19001/diff/1/content/media/webrtc/AudioDeviceGonk.cpp File content/media/webrtc/AudioDeviceGonk.cpp (right): http://firefox-codereview.appspot.com/19001/diff/1/content/media/webrtc/AudioDeviceGonk.cpp#newcode124 content/media/webrtc/AudioDeviceGonk.cpp:124: , mAudioFlinger(nullptr) You need , mInitialized(false) or things crash ...
7 years, 7 months ago #1
ekr-webrtc
I see a number of issues as noted below. It seems the stubs still need ...
7 years, 6 months ago #2
ekr-webrtc
Tim, here is my recent review. It might be useful for you to go over ...
7 years, 6 months ago #3
ekr-webrtc
7 years, 6 months ago #4
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 905+:3555e821a5cd+