Use multiple shared memory buffers cyclically for audio capture.

Use multiple shared memory buffers cyclically for audio capture.

Currently, only one buffer of shared memory (10ms) is used to transfer captured audio data from browser process to renderer process. This results in data overwriting on both desktop and mobile, since it gives renderer process only 10ms to pick up the data and renderer process will likely miss reading data in time from time to time due to various reasons, e.g., thread scheduling.

This patch adds a scheme using multiple buffers of shared memory cyclically so that renderer process can have much more time to pick up the data. This can mitigate most data owerwriting cases which happen typically in burst mode. This approach does not introduce any new delay in browser process.

The number of buffers is currently set to 10. It can be set to any positive number and be tuned if needed.

BUG=178040
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187393

[henrika (OOO until Aug 14), 2013-03-04 13:12:51]

Thanks Wei,

I have tested the path on my Windows machine for different WebRTC clients and it
works well. Also adding some minor initial comments.

I am adding Dale as well since he is an owner and knows this are very well. We
do use different methods to ensure that the browser process does not read
garbage from the renderer. I just want to ensure that we do our best to keep
both sides as similar as possible since we have a long-term goal to merge. The
current patch hard codes a value of one buffer only for output, but both sides
uses the AudioDeviceThread. Is this an OK solution.

https://codereview.chromium.org/12379071/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/12379071/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_input_sync_writer.cc:24: uint32 index
= static_cast<uint32>(current_buffer_id_);
Please add a few lines of comments here which explains why two Send calls are
needed and that a ring buffer is utilized.

https://codereview.chromium.org/12379071/diff/1/media/audio/audio_device_thre...
File media/audio/audio_device_thread.h (right):

https://codereview.chromium.org/12379071/diff/1/media/audio/audio_device_thre...
media/audio/audio_device_thread.h:54: // Called whenever we receive
notifications about pending data.
No comment about |index|.

https://codereview.chromium.org/12379071/diff/1/media/audio/audio_device_thre...
media/audio/audio_device_thread.h:105: bool read_index_;
It feels odd to use index as name in a flag. When I read the code I did the
error to think that is was an actual index. Also, please add a comment about
this guy always being false on the output side.

https://codereview.chromium.org/12379071/diff/1/media/audio/audio_input_devic...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/12379071/diff/1/media/audio/audio_input_devic...
media/audio/audio_input_device.cc:15: int kRequestdSharedMemoryCount = 10;
Why not "requested"? Also, rather central constant which needs some explanation.
And, should it really be platform independent?

https://codereview.chromium.org/12379071/diff/1/media/audio/audio_input_devic...
media/audio/audio_input_device.cc:35: virtual void Process(int pending_data, int
index) OVERRIDE;
Comment |index|.

https://codereview.chromium.org/12379071/diff/1/media/audio/audio_input_ipc.h
File media/audio/audio_input_ipc.h (right):

https://codereview.chromium.org/12379071/diff/1/media/audio/audio_input_ipc.h...
media/audio/audio_input_ipc.h:32: virtual void
OnStreamCreated(base::SyncSocket::Handle socket_handle,
Nothing is mentioned about |total_handles| here.

https://codereview.chromium.org/12379071/diff/1/media/audio/audio_input_ipc.h...
media/audio/audio_input_ipc.h:38: // objects.  The implementation of
OnStreamCreated takes ownership.
Add comments about |length| and |index|.

[DaleCurtis, 2013-03-04 17:33:50]

Very cool! Haven't had a chance to review yet, but cc: crogers since he was one
of the original shared memory architects.

[wjia(left Chromium), 2013-03-05 02:40:13]

PTAL. This patch is focused on webrtc audio capture. So it doesn't change # of
shared memory buffers for audio output and Pepper.

https://codereview.chromium.org/12379071/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/12379071/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_input_sync_writer.cc:24: uint32 index
= static_cast<uint32>(current_buffer_id_);
On 2013/03/04 13:12:51, henrika wrote:
> Please add a few lines of comments here which explains why two Send calls are
> needed and that a ring buffer is utilized.

Done.

https://codereview.chromium.org/12379071/diff/1/media/audio/audio_device_thre...
File media/audio/audio_device_thread.h (right):

https://codereview.chromium.org/12379071/diff/1/media/audio/audio_device_thre...
media/audio/audio_device_thread.h:54: // Called whenever we receive
notifications about pending data.
On 2013/03/04 13:12:51, henrika wrote:
> No comment about |index|.

Done.

https://codereview.chromium.org/12379071/diff/1/media/audio/audio_device_thre...
media/audio/audio_device_thread.h:105: bool read_index_;
On 2013/03/04 13:12:51, henrika wrote:
> It feels odd to use index as name in a flag. When I read the code I did the
> error to think that is was an actual index. Also, please add a comment about
> this guy always being false on the output side.

Changed name to need_read_index_ to be self explained.

https://codereview.chromium.org/12379071/diff/1/media/audio/audio_input_devic...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/12379071/diff/1/media/audio/audio_input_devic...
media/audio/audio_input_device.cc:15: int kRequestdSharedMemoryCount = 10;
On 2013/03/04 13:12:51, henrika wrote:
> Why not "requested"? Also, rather central constant which needs some
explanation.
> And, should it really be platform independent?

That's a typo. It should be Requested. Thanks for catching that!

Added some comments. This number should be dependent on how fast renderer
process can pick up data from shared memory.

https://codereview.chromium.org/12379071/diff/1/media/audio/audio_input_devic...
media/audio/audio_input_device.cc:35: virtual void Process(int pending_data, int
index) OVERRIDE;
On 2013/03/04 13:12:51, henrika wrote:
> Comment |index|.

Done.

https://codereview.chromium.org/12379071/diff/1/media/audio/audio_input_ipc.h
File media/audio/audio_input_ipc.h (right):

https://codereview.chromium.org/12379071/diff/1/media/audio/audio_input_ipc.h...
media/audio/audio_input_ipc.h:32: virtual void
OnStreamCreated(base::SyncSocket::Handle socket_handle,
On 2013/03/04 13:12:51, henrika wrote:
> Nothing is mentioned about |total_handles| here.

Done.

https://codereview.chromium.org/12379071/diff/1/media/audio/audio_input_ipc.h...
media/audio/audio_input_ipc.h:38: // objects.  The implementation of
OnStreamCreated takes ownership.
On 2013/03/04 13:12:51, henrika wrote:
> Add comments about |length| and |index|.

Done.

[Chris Rogers, 2013-03-05 04:08:15]

+miu

As Henrik mentions, we need to be careful to not diverge too much between the
input and output code since we want to converge them (maybe soon).  So let's try
to be careful.

One other high-level point is that a part of the SyncSocket design was to send
one single byte from browser to renderer to minimize any possibilities of kernel
thread-scheduling jank introduced by sending multiple bytes.  Thus if we need to
transmit additional info such as the buffer-index, then it's more robust  to put
this as "side-channel" info in the shared memory itself.

Also, it's important to make sure that for the output side to maintain the
single-buffer, because otherwise we're just introducing more latency.  I know
this doesn't affect the input-only case because the renderer can consume the
input stream as fast as possible, and only if it's late will it get a buffer (or
more) behind.  I also can see that this code adds to the possibility to have
multiple buffers but can still support one buffer (which we want to maintain for
output).

[miu, 2013-03-05 04:58:55]

It doesn't feel right that we are creating N separate shared memory mappings,
and then sending N+1 messages to initialize a single audio input stream.  This,
and the introduction of "SharedMemoryVector" seems to be adding a lot of
complexity throughout the code.

How about this approach: Just use one allocation of SharedMemory, except make it
buffer_size * N in size.  Then, renderer-side code only has to worry about
reading at the proper offset in the single SharedMemory allocation.  The sending
of any AudioInputMsg_NotifySharedMemoryCreated messages becomes unnecessary.

Note that, as a further bonus, there would be almost zero code change to pepper
impl code!  :-)

https://codereview.chromium.org/12379071/diff/13002/content/renderer/pepper/p...
File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right):

https://codereview.chromium.org/12379071/diff/13002/content/renderer/pepper/p...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:100: if
(base::MessageLoopProxy::current() != main_message_loop_proxy_) {
Please don't duplicate code.  It makes everything harder to maintain.

[henrika (OOO until Aug 14), 2013-03-05 10:53:14]

LGTM. Assuming you will reply to feedback from Yuri as well.

Nice work Wei!

[Chris Rogers, 2013-03-05 18:23:54]

On 2013/03/05 10:53:14, henrika wrote:
> LGTM. Assuming you will reply to feedback from Yuri as well.
> 
> Nice work Wei!

Please also address my comments as well :) Especially, we need to take extra
care not to send more than one byte on the SyncSocket.

I think we're getting to the point with the shared-memory layout that it would
be helpful to wrap the memory in a C++ class which "overlays" the memory.  This
class could be used on both the browser and renderer side, hiding the internal
details of the memory layout.

Even right now we have duplicated code (I know 'cause I wrote it) for handling
the memory layout of synchronized I/O buffers.  If as Yuri recommends (and I
agree) to allocate multiple buffers in one large shared-memory region then this
wrapper C++ class would be even more useful.

[DaleCurtis, 2013-03-06 01:12:51]

I don't think there's any need to send any more information across the shared
memory.  Both the renderer side and browser side can maintain independent
modular counts which increment with each socket_.Receive() and socket_.Send()
respectively.  (I'm assuming we don't have a problem with lost SyncSocket
messages).

I also think having a single "jumbo" shared memory is probably simpler and
faster in the long run, even if it is a bit more complex up front.  Especially
since on OSX (and perhaps other platforms) creating a new shared memory mapping
can be very slow (it's actually a file on the file system).  

We'll probably also want to enable this for all input clients, not just WebRTC,
so that means some pepper side work.  Otherwise we can't remove the Sleep()
calls this patch is intended to prevent.

I suggest if you need an immediate fix you add the common workaround of
Sleep(5ms) to the android input driver, this CL can then be for the proper fix.

https://codereview.chromium.org/12379071/diff/28002/media/audio/audio_input_d...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/12379071/diff/28002/media/audio/audio_input_d...
media/audio/audio_input_device.cc:19: int kRequestedSharedMemoryCount = 10;
Seems like a lot.  How did you choose this number?

[wjia(left Chromium), 2013-03-06 05:32:22]

I will try to address all comments in this message:
C1: Do not diverge audio input and output.
R: This can be done by using same scheme for audio output.

C2: Sending multiple bytes may trigger kernel thread-scheduling jank.
R: The existing code is sending 4 bytes each time. If it's really sensitive on
how many bytes would be sent via sync socket, it should be optimized to send 3
bytes since it's very unlikely the data size would exceed 16MB (which is 24
bits). The new info (segment id) can be expressed with one byte. Another way is
to always write segment id in sync socket and move recorded_bytes into shared
memory. It can't be done the other way around since the receiver needs to know
which buffer segment is used.

C3: audio output has to maintain single buffer.
R: This patch doesn't affect output and Pepper. I haven't looked into output
code. But I don't think single-buffer should be a limitation for output. Using
multiple buffers should help output side as well. It will not introduce any new
delay. It can only help improve smoothness of the data passing.

C4: aggregate buffers.
R: This is done with new patch. They are basically same, just different
bookkeeping.

C5: create a helper class to wrap shared memory layout.
R: I'd leave this to future refactoring.

C6: no need to send segment index to receiver.
R: I prefer to do that since it will put too much constraint on both side
without segment index.

C7: Sleep(5ms) can be a workaround.
R: I have seen some cases where renderer process missed contiguous 5 to 6
buffers.

C8: enable this for all input clients.
R: I'd leave this to the module owners. They can choose single buffer or
multiple buffers.

https://codereview.chromium.org/12379071/diff/28002/media/audio/audio_input_d...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/12379071/diff/28002/media/audio/audio_input_d...
media/audio/audio_input_device.cc:19: int kRequestedSharedMemoryCount = 10;
On 2013/03/06 01:12:51, DaleCurtis wrote:
> Seems like a lot.  How did you choose this number?

I have seen renderer process missed 5 to 6 buffers contiguously. Considering the
buffer size for 10ms, this number is not that much.

[miu, 2013-03-06 18:06:41]

https://codereview.chromium.org/12379071/diff/38001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/12379071/diff/38001/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_renderer_host.cc:33: uint32
shared_memory_segment_size;
I think you can get rid of shared_memory_segment_size, since it's never used
anywhere except in AIRH::OnCreateStream().

https://codereview.chromium.org/12379071/diff/38001/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_renderer_host.cc:34: uint32
shared_memory_segment_count;
C++ Style: Should be int instead of uint32.

https://codereview.chromium.org/12379071/diff/38001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/12379071/diff/38001/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_sync_writer.cc:20:
DCHECK(shared_memory_segment_count);
Should be DCHECK_LT(0, shared_memory_segment_count_);

https://codereview.chromium.org/12379071/diff/38001/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_sync_writer.cc:34:
socket_->Send(&current_segment_id_, sizeof(current_segment_id_));
Agree with Dale: That you don't need to send this if the socket is a reliable
data channel, as this is data that both sides already have.

https://codereview.chromium.org/12379071/diff/38001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_input_sync_writer.h (right):

https://codereview.chromium.org/12379071/diff/38001/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_sync_writer.h:25: uint32
shared_memory_segment_count);
s/uint32/int/

[miu, 2013-03-06 18:07:17]

BTW--Testing?

[henrika (OOO until Aug 14), 2013-03-06 20:52:01]

Would it be possible if each reviewer states what they really feel must be
solved in the CL for an OK? 

Please note that Wei is solving a real problem for WebRTC on Android here and it
is always possible to reduce the number of buffers and more or less disable the
scheme if needed.

Dale, what I like with this scheme (compared to using Sleep()) is that it can
adapt more dynamically,and IMHO, feels more future proof.

[Chris Rogers, 2013-03-06 21:13:02]

On 2013/03/06 20:52:01, henrika wrote:
> Would it be possible if each reviewer states what they really feel must be
> solved in the CL for an OK? 
> 
> Please note that Wei is solving a real problem for WebRTC on Android here and
it
> is always possible to reduce the number of buffers and more or less disable
the
> scheme if needed.
> 
> Dale, what I like with this scheme (compared to using Sleep()) is that it can
> adapt more dynamically,and IMHO, feels more future proof.

Hi Henrik, I understand the value of this scheme so don't need to be convinced. 
My one really big concern is that we don't end up calling socket_->Send() two
times (and calling socket_.Receive() two times in the renderer).  I believe this
can lead to subtle thread scheduling issues.  So I strongly would recommend that
we find another way to transmit the "buffer index".  It could either be written
somewhere in the shared-memory itself, or even better yet as Yuri and Dale
recommend to track this value independently in the browser and renderer, since
they should never fall out of sync. I'm sure we can find a solution for that.

I still think making an overlay C++ class would help make the management of the
shared memory more sane, but that could be deferred to later.

Otherwise, I think we should just review this code in the normal way to make
sure the code is made as clean as possible, with good variable naming with the
view that this code may some day soon be integrated into more shared
input/output code.

[wjia(left Chromium), 2013-03-06 22:12:41]

PTAL. I did the change to remove writing segment index into socket even though I
disagree. Just want to move forward and free bandwidth for other high priority
things.

Regarding test, I didn't see test for current single buffer scheme. I'd defer it
a bit.

https://codereview.chromium.org/12379071/diff/38001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/12379071/diff/38001/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_renderer_host.cc:33: uint32
shared_memory_segment_size;
On 2013/03/06 18:06:41, Yuri wrote:
> I think you can get rid of shared_memory_segment_size, since it's never used
> anywhere except in AIRH::OnCreateStream().

Done.

https://codereview.chromium.org/12379071/diff/38001/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_renderer_host.cc:34: uint32
shared_memory_segment_count;
On 2013/03/06 18:06:41, Yuri wrote:
> C++ Style: Should be int instead of uint32.

Done.

https://codereview.chromium.org/12379071/diff/38001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/12379071/diff/38001/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_sync_writer.cc:20:
DCHECK(shared_memory_segment_count);
On 2013/03/06 18:06:41, Yuri wrote:
> Should be DCHECK_LT(0, shared_memory_segment_count_);

Done.

https://codereview.chromium.org/12379071/diff/38001/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_sync_writer.cc:34:
socket_->Send(&current_segment_id_, sizeof(current_segment_id_));
On 2013/03/06 18:06:41, Yuri wrote:
> Agree with Dale: That you don't need to send this if the socket is a reliable
> data channel, as this is data that both sides already have.

Done. I still think such constraint shouldn't be put in the code.

https://codereview.chromium.org/12379071/diff/38001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_input_sync_writer.h (right):

https://codereview.chromium.org/12379071/diff/38001/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_sync_writer.h:25: uint32
shared_memory_segment_count);
On 2013/03/06 18:06:41, Yuri wrote:
> s/uint32/int/

Done.

[DaleCurtis, 2013-03-07 02:04:37]

nothing but nits.

https://codereview.chromium.org/12379071/diff/41004/content/browser/renderer_...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/12379071/diff/41004/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_sync_writer.cc:20: DCHECK_LT(0,
shared_memory_segment_count);
No yoda checks please; it's against style.
 
DCHECK_GE(shared_memory_segment_count, 0)

https://codereview.chromium.org/12379071/diff/41004/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_sync_writer.cc:35: uint8* ptr =
reinterpret_cast<uint8*>(shared_memory_->memory());
Should be able static_cast a void* ?

https://codereview.chromium.org/12379071/diff/41004/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_sync_writer.cc:43: if
(++current_segment_id_ >= shared_memory_segment_count_)
current_segment_id_ = (current_segment_id_ + 1) % shared_memory_segement_count_;

to avoid the conditional.

https://codereview.chromium.org/12379071/diff/41004/content/renderer/media/au...
File content/renderer/media/audio_input_message_filter.h (right):

https://codereview.chromium.org/12379071/diff/41004/content/renderer/media/au...
content/renderer/media/audio_input_message_filter.h:48: uint32
shared_memory_count) OVERRIDE;
Please use consistent names. You use |shared_memory_count| here and
|total_segments| below.

https://codereview.chromium.org/12379071/diff/41004/content/renderer/pepper/p...
File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right):

https://codereview.chromium.org/12379071/diff/41004/content/renderer/pepper/p...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:75: DCHECK_NE(-1,
handle.fd);
None of these should be yoda checks.

https://codereview.chromium.org/12379071/diff/41004/content/renderer/pepper/p...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:79: DCHECK_EQ(1,
total_segments);
Please add a TODO() and file a bug that PepperInput should take advantage of
this.

https://codereview.chromium.org/12379071/diff/41004/media/audio/audio_device_...
File media/audio/audio_device_thread.cc (right):

https://codereview.chromium.org/12379071/diff/41004/media/audio/audio_device_...
media/audio/audio_device_thread.cc:166: DCHECK_EQ(0U, bytes_read);
/shakes fist! :) nooooo!

https://codereview.chromium.org/12379071/diff/41004/media/audio/audio_device_...
media/audio/audio_device_thread.cc:191: CHECK_NE(0, bytes_per_ms_);  // Catch
division by zero early.
:head explodes:

https://codereview.chromium.org/12379071/diff/41004/media/audio/audio_input_d...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/12379071/diff/41004/media/audio/audio_input_d...
media/audio/audio_input_device.cc:19: int kRequestedSharedMemoryCount = 10;
static const int or make it an enum.  media/ code doesn't traditionally use the
unnamed namespace approach.

https://codereview.chromium.org/12379071/diff/41004/media/audio/audio_input_d...
media/audio/audio_input_device.cc:337: uint8* ptr =
reinterpret_cast<uint8*>(shared_memory_.memory());
static_cast ?

https://codereview.chromium.org/12379071/diff/41004/media/audio/audio_input_d...
media/audio/audio_input_device.cc:348: if (++current_segment_id_ >=
total_segments_)
Same modulus comment.

https://codereview.chromium.org/12379071/diff/41004/media/audio/audio_input_i...
File media/audio/audio_input_ipc.h (right):

https://codereview.chromium.org/12379071/diff/41004/media/audio/audio_input_i...
media/audio/audio_input_ipc.h:85: uint32 shared_memory_count) = 0;
Consistent naming.

https://codereview.chromium.org/12379071/diff/41004/media/audio/audio_output_...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/12379071/diff/41004/media/audio/audio_output_...
media/audio/audio_output_device.cc:269: CHECK_EQ(1, total_segments_);
TMYK: http://what-if.xkcd.com/3/

[wjia(left Chromium), 2013-03-10 18:19:49]

PTAL. Regarding parameter order in CHECK, I'd like to have clarification since
I'v got conflicted signal.

https://codereview.chromium.org/12379071/diff/41004/content/browser/renderer_...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/12379071/diff/41004/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_sync_writer.cc:20: DCHECK_LT(0,
shared_memory_segment_count);
On 2013/03/07 02:04:37, DaleCurtis wrote:
> No yoda checks please; it's against style.
>  
> DCHECK_GE(shared_memory_segment_count, 0)

During my readability code review, I was told to put expected value as first
argument of CHECK. Yuri also mentioned this in his comment
(https://codereview.chromium.org/12379071/diff/38001/content/browser/renderer_...).

I'd like to have clarification.

https://codereview.chromium.org/12379071/diff/41004/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_sync_writer.cc:35: uint8* ptr =
reinterpret_cast<uint8*>(shared_memory_->memory());
On 2013/03/07 02:04:37, DaleCurtis wrote:
> Should be able static_cast a void* ?

Done.

https://codereview.chromium.org/12379071/diff/41004/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_sync_writer.cc:43: if
(++current_segment_id_ >= shared_memory_segment_count_)
On 2013/03/07 02:04:37, DaleCurtis wrote:
> current_segment_id_ = (current_segment_id_ + 1) %
shared_memory_segement_count_;
> 
> to avoid the conditional.

I think it's more clearer and less expensive to use if condition.

https://codereview.chromium.org/12379071/diff/41004/content/renderer/media/au...
File content/renderer/media/audio_input_message_filter.h (right):

https://codereview.chromium.org/12379071/diff/41004/content/renderer/media/au...
content/renderer/media/audio_input_message_filter.h:48: uint32
shared_memory_count) OVERRIDE;
On 2013/03/07 02:04:37, DaleCurtis wrote:
> Please use consistent names. You use |shared_memory_count| here and
> |total_segments| below.

Done.

https://codereview.chromium.org/12379071/diff/41004/content/renderer/pepper/p...
File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right):

https://codereview.chromium.org/12379071/diff/41004/content/renderer/pepper/p...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:75: DCHECK_NE(-1,
handle.fd);
On 2013/03/07 02:04:37, DaleCurtis wrote:
> None of these should be yoda checks.

See reply in audio_input_sync_writter.cc.

https://codereview.chromium.org/12379071/diff/41004/content/renderer/pepper/p...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:79: DCHECK_EQ(1,
total_segments);
On 2013/03/07 02:04:37, DaleCurtis wrote:
> Please add a TODO() and file a bug that PepperInput should take advantage of
> this.

Done.

https://codereview.chromium.org/12379071/diff/41004/media/audio/audio_input_d...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/12379071/diff/41004/media/audio/audio_input_d...
media/audio/audio_input_device.cc:19: int kRequestedSharedMemoryCount = 10;
On 2013/03/07 02:04:37, DaleCurtis wrote:
> static const int or make it an enum.  media/ code doesn't traditionally use
the
> unnamed namespace approach.

anonymous namespace is better than static since it doesn't pollute namespace
media. I do see some const is defined in anonymous namespace in media/.

https://codereview.chromium.org/12379071/diff/41004/media/audio/audio_input_d...
media/audio/audio_input_device.cc:337: uint8* ptr =
reinterpret_cast<uint8*>(shared_memory_.memory());
On 2013/03/07 02:04:37, DaleCurtis wrote:
> static_cast ?

Done.

https://codereview.chromium.org/12379071/diff/41004/media/audio/audio_input_d...
media/audio/audio_input_device.cc:348: if (++current_segment_id_ >=
total_segments_)
On 2013/03/07 02:04:37, DaleCurtis wrote:
> Same modulus comment.

Same reply. :-)

https://codereview.chromium.org/12379071/diff/41004/media/audio/audio_input_i...
File media/audio/audio_input_ipc.h (right):

https://codereview.chromium.org/12379071/diff/41004/media/audio/audio_input_i...
media/audio/audio_input_ipc.h:85: uint32 shared_memory_count) = 0;
On 2013/03/07 02:04:37, DaleCurtis wrote:
> Consistent naming.

Done.

[DaleCurtis, 2013-03-11 20:00:47]

https://codereview.chromium.org/12379071/diff/41004/content/browser/renderer_...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/12379071/diff/41004/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_sync_writer.cc:20: DCHECK_LT(0,
shared_memory_segment_count);
On 2013/03/10 18:19:49, wjia wrote:
> On 2013/03/07 02:04:37, DaleCurtis wrote:
> > No yoda checks please; it's against style.
> >  
> > DCHECK_GE(shared_memory_segment_count, 0)
> 
> During my readability code review, I was told to put expected value as first
> argument of CHECK. Yuri also mentioned this in his comment
>
(https://codereview.chromium.org/12379071/diff/38001/content/browser/renderer_...).
> 
> I'd like to have clarification.

http://dev.chromium.org/developers/coding-style specifically: "Prefer (foo == 0)
to (0 == foo)."  Also I had Yuri change all his in a recent CL too :)

https://codereview.chromium.org/12379071/diff/41004/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_sync_writer.cc:43: if
(++current_segment_id_ >= shared_memory_segment_count_)
On 2013/03/10 18:19:49, wjia wrote:
> On 2013/03/07 02:04:37, DaleCurtis wrote:
> > current_segment_id_ = (current_segment_id_ + 1) %
> shared_memory_segement_count_;
> > 
> > to avoid the conditional.
> 
> I think it's more clearer and less expensive to use if condition. 

Just a nit, so fine with me either way.  I assume a branch is more expensive
than a modulus operation, but feel free to paint your bike shed whatever color
you want :)

https://codereview.chromium.org/12379071/diff/41004/media/audio/audio_input_d...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/12379071/diff/41004/media/audio/audio_input_d...
media/audio/audio_input_device.cc:19: int kRequestedSharedMemoryCount = 10;
On 2013/03/10 18:19:49, wjia wrote:
> On 2013/03/07 02:04:37, DaleCurtis wrote:
> > static const int or make it an enum.  media/ code doesn't traditionally use
> the
> > unnamed namespace approach.
> 
> anonymous namespace is better than static since it doesn't pollute namespace
> media. I do see some const is defined in anonymous namespace in media/.

While the Chromium style guide explicitly says unnamed namespaces are preferred,
consistency with the surrounding code should be favored. It's not directly
comparable, but there is an order of magnitude difference between the two:

$ git grep "namespace {" | wc -l
32
$ git grep "static const" | wc -l
597

I just discussed with the team and the answers were:
1. It doesn't matter for pure const.
2. Favor consistency.
3. If you don't mind the code churn, converting all static const to unnamed
namespace would be valid.
4. http://go/static-or-anonymous-namespace (but more for functions).

Please favor consistency in this instance.

[wjia(left Chromium), 2013-03-11 20:34:27]

PTAL. I leave pepper file untouched in order to favor consistency there.

https://codereview.chromium.org/12379071/diff/41004/content/browser/renderer_...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/12379071/diff/41004/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_sync_writer.cc:20: DCHECK_LT(0,
shared_memory_segment_count);
On 2013/03/11 20:00:47, DaleCurtis wrote:
> On 2013/03/10 18:19:49, wjia wrote:
> > On 2013/03/07 02:04:37, DaleCurtis wrote:
> > > No yoda checks please; it's against style.
> > >  
> > > DCHECK_GE(shared_memory_segment_count, 0)
> > 
> > During my readability code review, I was told to put expected value as first
> > argument of CHECK. Yuri also mentioned this in his comment
> >
>
(https://codereview.chromium.org/12379071/diff/38001/content/browser/renderer_...).
> > 
> > I'd like to have clarification.
> 
> http://dev.chromium.org/developers/coding-style specifically: "Prefer (foo ==
0)
> to (0 == foo)."  Also I had Yuri change all his in a recent CL too :)

Done.

https://codereview.chromium.org/12379071/diff/41004/media/audio/audio_input_d...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/12379071/diff/41004/media/audio/audio_input_d...
media/audio/audio_input_device.cc:19: int kRequestedSharedMemoryCount = 10;
On 2013/03/11 20:00:47, DaleCurtis wrote:
> On 2013/03/10 18:19:49, wjia wrote:
> > On 2013/03/07 02:04:37, DaleCurtis wrote:
> > > static const int or make it an enum.  media/ code doesn't traditionally
use
> > the
> > > unnamed namespace approach.
> > 
> > anonymous namespace is better than static since it doesn't pollute namespace
> > media. I do see some const is defined in anonymous namespace in media/.
> 
> While the Chromium style guide explicitly says unnamed namespaces are
preferred,
> consistency with the surrounding code should be favored. It's not directly
> comparable, but there is an order of magnitude difference between the two:
> 
> $ git grep "namespace {" | wc -l
> 32
> $ git grep "static const" | wc -l
> 597
> 
> I just discussed with the team and the answers were:
> 1. It doesn't matter for pure const.
> 2. Favor consistency.
> 3. If you don't mind the code churn, converting all static const to unnamed
> namespace would be valid.
> 4. http://go/static-or-anonymous-namespace (but more for functions).
> 
> Please favor consistency in this instance.

Done.

[DaleCurtis, 2013-03-11 21:03:33]

lgtm

[yzshen1, 2013-03-11 22:14:44]

On 2013/03/11 21:03:33, DaleCurtis wrote:
> lgtm

file:.*pepper.* LGTM

[wjia(left Chromium), 2013-03-11 22:41:21]

Committed patchset #9 manually as r187393.