Add native log and uma stat when all input shared memory buffers are full

Add native log and uma stat when all input shared memory buffers are full.

It is a serious problem when all the buffer are full,  theoretically this might be able to happen if the renderer is slowed on consuming the buffer and eventually seriously underrun. This patch adds log for us to figure out if it can happen on real life.



BUG=399835
TEST=bots

[no longer working on chromium, 2014-08-29 08:51:00]

xians@chromium.org changed reviewers:
+ asvitkine@chromium.org, tommi@chromium.org

[no longer working on chromium, 2014-08-29 08:51:20]

Tommi and Alex, could you please review this CL? 

Thanks,
SX

[tommi (sloooow) - chröme, 2014-08-29 09:16:43]

It seems that we're changing the behavior a little bit in addition to adding
stats - or am I misunderstanding?

https://codereview.chromium.org/510913005/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/510913005/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:21:
MEDIA_INPUT_SINGLE_BUFFER = 0,
nit: can we include 'AUDIO' in the enum names?  E.g. 
MEDIA_AUDIO_INPUT_SINGLE_BUFFER etc.
I'd also be fine with dropping "MEDIA" from the names.

https://codereview.chromium.org/510913005/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:27: void
LogAllBuffersAreFull(int shared_memory_segment_size) {
|shared_memory_segment_size| sounds to me to represent the size in bytes of a
single memory segment.  However, this is being logged as "number of buffers",
which sounds like it would be a completely different number?  which is it?

https://codereview.chromium.org/510913005/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:30: type = 
MEDIA_INPUT_CIRCULAR_BUFFER;
spacing

https://codereview.chromium.org/510913005/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:32: "AIS,
circular buffers is full. Number of buffers is " +
grammar nit: circular buffers are full

https://codereview.chromium.org/510913005/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:33:
base::IntToString(shared_memory_segment_size);
nit: use StringPrintf() (fewer string objects required)

https://codereview.chromium.org/510913005/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:42:
AudioInputSyncWriter::AudioInputSyncWriter(base::SharedMemory* shared_memory,
empty line above this one

https://codereview.chromium.org/510913005/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:76: uint32 bytes)
{
is there a way to verify that |bytes| is a valid (and correct) value?

https://codereview.chromium.org/510913005/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:126:
socket_->Send(&bytes, sizeof(bytes));
can the number of bytes not be derived from the AudioBus?  If the AudioBus
buffer does not represent the same data as |bytes| does, then that needs further
clarification (and perhaps an improved variable name)

https://codereview.chromium.org/510913005/diff/20001/media/audio/audio_input_...
File media/audio/audio_input_controller.h (right):

https://codereview.chromium.org/510913005/diff/20001/media/audio/audio_input_...
media/audio/audio_input_controller.h:136: // writer on the number of bytes in
the sound card which has been recorded.
grammar nit: // writer of the number of bytes that have been received from the
sound card.

Is this number cumulative though (as the comment seems to suggest) or does it
represent the size of a single buffer?

https://codereview.chromium.org/510913005/diff/20001/media/audio/audio_input_...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/510913005/diff/20001/media/audio/audio_input_...
media/audio/audio_input_device.cc:314: buffer->params.size = 0;
do we have unit tests?

[no longer working on chromium, 2014-08-29 11:10:11]

Patchset #3 (id:40001) has been deleted

[no longer working on chromium, 2014-08-29 11:14:16]

Hi Tommi, additional to the logging, I addressed Henrik's TODO to combine
Write() and Send() methods:
// TODO(henrika): Combine into one method (including Write).

Hope it is fine since it is just some trivial clean-up, otherwise I will be fine
to do it in another CL.

But there is no other behaviour change.

I have already addressed your comments, PTAL.

Thanks,
SX

https://codereview.chromium.org/510913005/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/510913005/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:21:
MEDIA_INPUT_SINGLE_BUFFER = 0,
On 2014/08/29 09:16:43, tommi wrote:
> nit: can we include 'AUDIO' in the enum names?  E.g. 
> MEDIA_AUDIO_INPUT_SINGLE_BUFFER etc.
> I'd also be fine with dropping "MEDIA" from the names.

Ah, good catch. I changed the name from MediaInputBufferType to
AudioInputBufferType, but forgot the contents.

Done with changing MEDIA to AUDIO

https://codereview.chromium.org/510913005/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:27: void
LogAllBuffersAreFull(int shared_memory_segment_size) {
On 2014/08/29 09:16:42, tommi wrote:
> |shared_memory_segment_size| sounds to me to represent the size in bytes of a
> single memory segment.  However, this is being logged as "number of buffers",
> which sounds like it would be a completely different number?  which is it?

shared_memory_segment_size_ is used by AudioInputSyncWriter as a member variable
for the number of shared memory buffers.
But I agree it is better to change number_of_buffers for this helper method.

Done. Hope it is fine now.

https://codereview.chromium.org/510913005/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:30: type = 
MEDIA_INPUT_CIRCULAR_BUFFER;
On 2014/08/29 09:16:42, tommi wrote:
> spacing

Done.

https://codereview.chromium.org/510913005/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:32: "AIS,
circular buffers is full. Number of buffers is " +
On 2014/08/29 09:16:42, tommi wrote:
> grammar nit: circular buffers are full

Done.

https://codereview.chromium.org/510913005/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:33:
base::IntToString(shared_memory_segment_size);
On 2014/08/29 09:16:42, tommi wrote:
> nit: use StringPrintf() (fewer string objects required)

Done.

https://codereview.chromium.org/510913005/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:42:
AudioInputSyncWriter::AudioInputSyncWriter(base::SharedMemory* shared_memory,
On 2014/08/29 09:16:42, tommi wrote:
> empty line above this one

Done.

https://codereview.chromium.org/510913005/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:76: uint32 bytes)
{
On 2014/08/29 09:16:42, tommi wrote:
> is there a way to verify that |bytes| is a valid (and correct) value?

I was confused by the comment of UpdateRecordedBytes(uint32 bytes) API, which I
remove in this CL.

We actually pass the delay value by this |bytes|, I updated the variable name.

https://codereview.chromium.org/510913005/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:126:
socket_->Send(&bytes, sizeof(bytes));
On 2014/08/29 09:16:42, tommi wrote:
> can the number of bytes not be derived from the AudioBus?  If the AudioBus
> buffer does not represent the same data as |bytes| does, then that needs
further
> clarification (and perhaps an improved variable name)

variable name has been changed.

https://codereview.chromium.org/510913005/diff/20001/media/audio/audio_input_...
File media/audio/audio_input_controller.h (right):

https://codereview.chromium.org/510913005/diff/20001/media/audio/audio_input_...
media/audio/audio_input_controller.h:136: // writer on the number of bytes in
the sound card which has been recorded.
On 2014/08/29 09:16:43, tommi wrote:
> grammar nit: // writer of the number of bytes that have been received from the
> sound card.
> 
> Is this number cumulative though (as the comment seems to suggest) or does it
> represent the size of a single buffer?

The previous comments are just wrong, I tried correcting it, please take another
look.

[no longer working on chromium, 2014-08-29 11:15:20]

https://codereview.chromium.org/510913005/diff/20001/media/audio/audio_input_...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/510913005/diff/20001/media/audio/audio_input_...
media/audio/audio_input_device.cc:314: buffer->params.size = 0;
On 2014/08/29 09:16:43, tommi wrote:
> do we have unit tests?

Unfortunately we don't have unittest for this shard memory, but I have already
verified that it works as expected.

[Alexei Svitkine (slow), 2014-08-29 22:22:19]

https://codereview.chromium.org/510913005/diff/60001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/510913005/diff/60001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:38: }
Nit: Add "// namespace" and a blank line before this.

https://codereview.chromium.org/510913005/diff/60001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/510913005/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10996: +    Counters for
AudioInputSyncWriter::Write() where all the buffers are full.
Can the description be expanded to explain the enum values? (i.e. if all buffers
are full, how does the buffer type fit into it?)

[no longer working on chromium, 2014-09-02 10:00:20]

xians@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[no longer working on chromium, 2014-09-02 10:00:21]

Tommi and Alexei, please take another look.

Dale, may I have your owner stamp for media.gyp?

Thanks,
SX

https://codereview.chromium.org/510913005/diff/60001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/510913005/diff/60001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:38: }
On 2014/08/29 22:22:18, Alexei Svitkine wrote:
> Nit: Add "// namespace" and a blank line before this.

Done.

https://codereview.chromium.org/510913005/diff/60001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/510913005/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:10996: +    Counters for
AudioInputSyncWriter::Write() where all the buffers are full.
On 2014/08/29 22:22:18, Alexei Svitkine wrote:
> Can the description be expanded to explain the enum values? (i.e. if all
buffers
> are full, how does the buffer type fit into it?)

Done, PTAL.

[tommi (sloooow) - chröme, 2014-09-02 12:03:34]

https://codereview.chromium.org/510913005/diff/80001/media/audio/audio_input_...
File media/audio/audio_input_device_unittest.cc (right):

https://codereview.chromium.org/510913005/diff/80001/media/audio/audio_input_...
media/audio/audio_input_device_unittest.cc:96: 441),
Did you get a chance to run tests with very small (or large) buffer sizes to see
if we could ever run into the buffer overwriting/overlapping situation we've
theorized?

[Alexei Svitkine (slow), 2014-09-02 14:45:03]

lgtm

[DaleCurtis, 2014-09-03 21:55:48]

What about the PPAPI / NaCl input code?

[no longer working on chromium, 2014-09-11 11:41:15]

Hi guys,

I found out that in fast shutdown mode, the renderer goes away without notifying
the browser, and the browser can continue to send until the sync socket is full,
thus corrupts the uma stat.

Dale, do you know if there is any good way to know check the renderer has gone
away? If not, I might have to move those uma stat logging to AudioInputDevice to
work around the problem.
And I will fix the PPAPI code after figuring out the solution to this fast
shutdown case.

[DaleCurtis, 2014-09-11 17:52:49]

Hmm, I thought AudioRendererHost::OnChannelClosing/OnDestruct() handled that.

[Henrik Grunell, 2014-10-06 05:55:14]

On 2014/09/11 17:52:49, DaleCurtis wrote:
> Hmm, I thought AudioRendererHost::OnChannelClosing/OnDestruct() handled that.

Any update on the progress here?