Refactor AudioInputController and split stat by stream type.

Refactor AudioInputController and split stat by stream type.

Clean up confusing usage of "low latency". Remove a factory function since there were two similar ones.

Before this CL, "low latency" could either refer to requesting a low latency stream type from the AudioManager or that the stream was requested over IPC, which was confusing. Now, it always refers to the kind of stream requested. This also means that the streams for which Media.AudioInputControllerSessionSilenceReport is reported changes.

The Media.AudioInputControllerCaptureStartupSuccess stat is split into three separate stats: Media.{{High,Low}Latency,Virtual}AudioCaptureStartupSuccess, where Virtual refers to stream types obtained by redirecting output streams rather than going through the platform audio system (such as tab capture).

BUG=600536
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2665163002
Cr-Commit-Position: refs/heads/master@{#447973}
Committed: https://chromium.googlesource.com/chromium/src/+/3500cacdae6e8bcdb6371576ca7d08d7465eb9e7

[Max Morin, 2017-01-31 15:05:22]

Description was changed from

==========
Refactor AudioInputController.

Clean up usage of "low latency". Remove a factory function.

BUG=
==========

to

==========
Refactor AudioInputController.

Clean up usage of "low latency". Remove a factory function.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Max Morin, 2017-01-31 16:59:29]

Description was changed from

==========
Refactor AudioInputController.

Clean up usage of "low latency". Remove a factory function.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Refactor AudioInputController and split stat by stream type.

Clean up confusing usage of "low latency". Remove a factory function since there
were two similar ones.

Before this CL, "low latency" could either refer to requesting a low latency
stream type from the AudioManager or that the stream was requested over IPC,
which was confusing. Now, it always refers to the kind of stream requested. This
also means that the streams for which
Media.AudioInputControllerSessionSilenceReport is reported changes.

The Media.AudioInputControllerCaptureStartupSuccess stat is split into three
separate stats: Media.{{High,Low}Latency,Virtual}AudioCaptureStartupSuccess,
where Virtual refers to stream types obtained by redirecting output streams
rather than going through the platform audio system (such as tab capture).

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Max Morin, 2017-01-31 17:12:37]

Description was changed from

==========
Refactor AudioInputController and split stat by stream type.

Clean up confusing usage of "low latency". Remove a factory function since there
were two similar ones.

Before this CL, "low latency" could either refer to requesting a low latency
stream type from the AudioManager or that the stream was requested over IPC,
which was confusing. Now, it always refers to the kind of stream requested. This
also means that the streams for which
Media.AudioInputControllerSessionSilenceReport is reported changes.

The Media.AudioInputControllerCaptureStartupSuccess stat is split into three
separate stats: Media.{{High,Low}Latency,Virtual}AudioCaptureStartupSuccess,
where Virtual refers to stream types obtained by redirecting output streams
rather than going through the platform audio system (such as tab capture).

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Refactor AudioInputController and split stat by stream type.

Clean up confusing usage of "low latency". Remove a factory function since there
were two similar ones.

Before this CL, "low latency" could either refer to requesting a low latency
stream type from the AudioManager or that the stream was requested over IPC,
which was confusing. Now, it always refers to the kind of stream requested. This
also means that the streams for which
Media.AudioInputControllerSessionSilenceReport is reported changes.

The Media.AudioInputControllerCaptureStartupSuccess stat is split into three
separate stats: Media.{{High,Low}Latency,Virtual}AudioCaptureStartupSuccess,
where Virtual refers to stream types obtained by redirecting output streams
rather than going through the platform audio system (such as tab capture).

BUG=600536
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Max Morin, 2017-01-31 17:12:37]

maxmorin@chromium.org changed reviewers:
+ tommi@chromium.org

[Max Morin, 2017-02-01 09:32:58]

Tommi: PTAL

[tommi (sloooow) - chröme, 2017-02-01 21:07:26]

https://codereview.chromium.org/2665163002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (left):

https://codereview.chromium.org/2665163002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host.cc:374: //
TODO(grunell): Clean up the low latency terminology so that it's less
good to clean this up. I wasn't aware of this TODO (although I do know about the
confusion around the terminology)

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
media/audio/audio_input_controller.cc:328: log_silence_state_ = true;
I think we also only want to do this if the APM is in the picture (i.e. webrtc
w/echo cancellation), right?

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
media/audio/audio_input_controller.cc:362: // Set AGC state using mode in
|enable_agc_| which can only be enabled in
nit: |enable_agc|

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
media/audio/audio_input_controller.cc:363: // CreateLowLatency().
CreateLowLatency is now gone

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
media/audio/audio_input_controller.cc:412: // Since the usage pattern of
getUserMedia commonly is that a stream
nit: since you've removed the if() that checks for the timestamp, the below code
is no longer specific to getUserMedia.  Hmm... not sure what's the best way to
update the comment... maybe just point it out as an example usage pattern
instead?

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
media/audio/audio_input_controller.cc:428: if
(audio_callback_->received_callback()) {
I wonder if we should add another InputStreamDuration stat.  One that's for
streams that never received any callbacks. It could be a good indicator to see
how good the somewhat arbitrary chosen 500ms breakpoint is.  If these turn out
to be generally short lived too, it might be an indicator that slow/overloaded
machines take longer to initialize but no callbacks might not necessarily mean
failure.

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
File media/audio/audio_input_controller.h (right):

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
media/audio/audio_input_controller.h:47: // Crate() ==>        DoCreate()
nit: Create

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
media/audio/audio_input_controller.h:130: enum Type {
can we call this StreamType or InputStreamType?  Type is a bit too ambiguous
since there are lots of things being tied together (is it AudioInputController
type, sync writer type, stream type, etc).

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
media/audio/audio_input_controller.h:228: CAPTURE_STARTUP_OK = 0,
can we avoid the numeric change?  Even though the data might be incompatible
moving backwards, this matches a completely different stat now.

besides a numeric change means that the histogram xml file needs to be updated
and then it's incorrect for all versions even if you're looking at only a
specific version (say M55), then data reported for 0, would show up as "OK" but
in actuality be "no data callback".

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
media/audio/audio_input_controller.h:355: bool power_measurement_is_enabled_ =
false;
initializing variables here does mean that some are initialized here and some
are initialized in the cc file. I haven't checked the guidelines so can you make
sure that's OK?  It does clean up the initializer list fwiw.

[Max Morin, 2017-02-02 09:54:57]

PTAL. Also CC Ossu and R Dale.
Dale: Do these new stats look good to you?

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
media/audio/audio_input_controller.cc:328: log_silence_state_ = true;
On 2017/02/01 21:07:25, tommi (krómíum) wrote:
> I think we also only want to do this if the APM is in the picture (i.e. webrtc
> w/echo cancellation), right?

Probably yes. I'll use enable_agc as an indicator of webrtc being used (since
it's the only easily available heuristic). I'll also merge this bool with
|power_measurement_is_enabled_|.

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
media/audio/audio_input_controller.cc:428: if
(audio_callback_->received_callback()) {
On 2017/02/01 21:07:25, tommi (krómíum) wrote:
> I wonder if we should add another InputStreamDuration stat.  One that's for
> streams that never received any callbacks. It could be a good indicator to see
> how good the somewhat arbitrary chosen 500ms breakpoint is.  If these turn out
> to be generally short lived too, it might be an indicator that slow/overloaded
> machines take longer to initialize but no callbacks might not necessarily mean
> failure.

Added Media.InputStreamWithoutCallbackDuration.

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
File media/audio/audio_input_controller.h (right):

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
media/audio/audio_input_controller.h:228: CAPTURE_STARTUP_OK = 0,
On 2017/02/01 21:07:26, tommi (krómíum) wrote:
> can we avoid the numeric change?  Even though the data might be incompatible
> moving backwards, this matches a completely different stat now.
> 
> besides a numeric change means that the histogram xml file needs to be updated
> and then it's incorrect for all versions even if you're looking at only a
> specific version (say M55), then data reported for 0, would show up as "OK"
but
> in actuality be "no data callback".

The old stats are left intact (since the histogram and enum in histograms.xml is
intact). The new stats use new histograms and a new enum.

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
media/audio/audio_input_controller.h:355: bool power_measurement_is_enabled_ =
false;
On 2017/02/01 21:07:26, tommi (krómíum) wrote:
> initializing variables here does mean that some are initialized here and some
> are initialized in the cc file. I haven't checked the guidelines so can you
make
> sure that's OK?  It does clean up the initializer list fwiw.

Should be fine, I don't see anything about it in any style guide.

[Max Morin, 2017-02-02 09:56:16]

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

[Max Morin, 2017-02-02 09:56:17]

Really +R Dale.

[tommi (sloooow) - chröme, 2017-02-02 14:14:21]

lgtm. I have some suggestions below that I'll leave up to you.

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
media/audio/audio_input_controller.cc:328: log_silence_state_ = true;
On 2017/02/02 09:54:56, Max Morin wrote:
> On 2017/02/01 21:07:25, tommi (krómíum) wrote:
> > I think we also only want to do this if the APM is in the picture (i.e.
webrtc
> > w/echo cancellation), right?
> 
> Probably yes. I'll use enable_agc as an indicator of webrtc being used (since
> it's the only easily available heuristic). I'll also merge this bool with
> |power_measurement_is_enabled_|.

Acknowledged.

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
File media/audio/audio_input_controller.h (right):

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
media/audio/audio_input_controller.h:228: CAPTURE_STARTUP_OK = 0,
On 2017/02/02 09:54:56, Max Morin wrote:
> On 2017/02/01 21:07:26, tommi (krómíum) wrote:
> > can we avoid the numeric change?  Even though the data might be incompatible
> > moving backwards, this matches a completely different stat now.
> > 
> > besides a numeric change means that the histogram xml file needs to be
updated
> > and then it's incorrect for all versions even if you're looking at only a
> > specific version (say M55), then data reported for 0, would show up as "OK"
> but
> > in actuality be "no data callback".
> 
> The old stats are left intact (since the histogram and enum in histograms.xml
is
> intact). The new stats use new histograms and a new enum.

ah, sorry, I thought I had deleted this comment. I realized that later when I
saw the cc file changes.

https://codereview.chromium.org/2665163002/diff/60001/media/audio/audio_input...
media/audio/audio_input_controller.h:355: bool power_measurement_is_enabled_ =
false;
On 2017/02/02 09:54:56, Max Morin wrote:
> On 2017/02/01 21:07:26, tommi (krómíum) wrote:
> > initializing variables here does mean that some are initialized here and
some
> > are initialized in the cc file. I haven't checked the guidelines so can you
> make
> > sure that's OK?  It does clean up the initializer list fwiw.
> 
> Should be fine, I don't see anything about it in any style guide.

Acknowledged. Looks neater this way.

https://codereview.chromium.org/2665163002/diff/80001/media/audio/audio_input...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/2665163002/diff/80001/media/audio/audio_input...
media/audio/audio_input_controller.cc:428:
UMA_HISTOGRAM_LONG_TIMES("Media.InputStreamWithoutCallbackDuration",
thanks - what about "InputStreamDurationWithoutCallback" so that both match
"InputStreamDuration*"?

https://codereview.chromium.org/2665163002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2665163002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:24234: -<histogram
name="Media.Audio.Capture.CallbackError" enum="BooleanError">
I think you'll need to mark this as deprecated (see examples elsewhere)

https://codereview.chromium.org/2665163002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2665163002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:24262: + 
<owner>tommi@chromium.org</owner>
want to put yourself as owner? :)  same below

https://codereview.chromium.org/2665163002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:80580: +<enum
name="CaptureStartupResult2" type="int">
I find that "Audio" is missing from the enum name. I know this is very similar
to the other one, but I think we should add clarity since we're adding a new
one.  E.g. AudioCaptureStartupResult.

[Max Morin, 2017-02-02 15:37:52]

maxmorin@chromium.org changed reviewers:
+ holte@chromium.org

[Max Morin, 2017-02-02 15:37:53]

Holte: PTAL at histograms.xml

https://codereview.chromium.org/2665163002/diff/80001/media/audio/audio_input...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/2665163002/diff/80001/media/audio/audio_input...
media/audio/audio_input_controller.cc:428:
UMA_HISTOGRAM_LONG_TIMES("Media.InputStreamWithoutCallbackDuration",
On 2017/02/02 14:14:21, tommi (krómíum) wrote:
> thanks - what about "InputStreamDurationWithoutCallback" so that both match
> "InputStreamDuration*"?

Done.

https://codereview.chromium.org/2665163002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2665163002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:24234: -<histogram
name="Media.Audio.Capture.CallbackError" enum="BooleanError">
On 2017/02/02 14:14:21, tommi (krómíum) wrote:
> I think you'll need to mark this as deprecated (see examples elsewhere)

Done.

https://codereview.chromium.org/2665163002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2665163002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:24262: + 
<owner>tommi@chromium.org</owner>
On 2017/02/02 14:14:21, tommi (krómíum) wrote:
> want to put yourself as owner? :)  same below

Done.

https://codereview.chromium.org/2665163002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:80580: +<enum
name="CaptureStartupResult2" type="int">
On 2017/02/02 14:14:21, tommi (krómíum) wrote:
> I find that "Audio" is missing from the enum name. I know this is very similar
> to the other one, but I think we should add clarity since we're adding a new
> one.  E.g. AudioCaptureStartupResult.

Done.

[Max Morin, 2017-02-02 15:49:41]

The CQ bit was checked by maxmorin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 15:49:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 16:33:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-02 16:33:10]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[DaleCurtis, 2017-02-02 20:43:54]

dalecurtis@chromium.org changed reviewers:
- dalecurtis@chromium.org

[DaleCurtis, 2017-02-02 20:43:55]

Defer to your team for this. What makes them fine or not is whatever plans you
have for using them. So long as you've thought about what type of questions you
want to answer the rest is up to you and input folk :)

[DaleCurtis, 2017-02-02 20:44:45]

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

[DaleCurtis, 2017-02-02 20:44:46]

rs-lgtm in case you need my stamp in addition to tommi's.

[Steven Holte, 2017-02-03 02:56:33]

histograms lgtm

[Max Morin, 2017-02-03 08:38:49]

The CQ bit was checked by maxmorin@chromium.org

[Max Morin, 2017-02-03 08:38:50]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
dalecurtis@chromium.org, holte@chromium.org
Link to the patchset: https://codereview.chromium.org/2665163002/#ps120001
(title: "Compile")

[commit-bot: I haz the power, 2017-02-03 08:39:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 10:14:31]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1486111129797690,
"parent_rev": "49a6b5d0a4738663b0740324b4666a70eec295f2", "commit_rev":
"3500cacdae6e8bcdb6371576ca7d08d7465eb9e7"}

[commit-bot: I haz the power, 2017-02-03 10:15:05]

Description was changed from

==========
Refactor AudioInputController and split stat by stream type.

Clean up confusing usage of "low latency". Remove a factory function since there
were two similar ones.

Before this CL, "low latency" could either refer to requesting a low latency
stream type from the AudioManager or that the stream was requested over IPC,
which was confusing. Now, it always refers to the kind of stream requested. This
also means that the streams for which
Media.AudioInputControllerSessionSilenceReport is reported changes.

The Media.AudioInputControllerCaptureStartupSuccess stat is split into three
separate stats: Media.{{High,Low}Latency,Virtual}AudioCaptureStartupSuccess,
where Virtual refers to stream types obtained by redirecting output streams
rather than going through the platform audio system (such as tab capture).

BUG=600536
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Refactor AudioInputController and split stat by stream type.

Clean up confusing usage of "low latency". Remove a factory function since there
were two similar ones.

Before this CL, "low latency" could either refer to requesting a low latency
stream type from the AudioManager or that the stream was requested over IPC,
which was confusing. Now, it always refers to the kind of stream requested. This
also means that the streams for which
Media.AudioInputControllerSessionSilenceReport is reported changes.

The Media.AudioInputControllerCaptureStartupSuccess stat is split into three
separate stats: Media.{{High,Low}Latency,Virtual}AudioCaptureStartupSuccess,
where Virtual refers to stream types obtained by redirecting output streams
rather than going through the platform audio system (such as tab capture).

BUG=600536
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2665163002
Cr-Commit-Position: refs/heads/master@{#447973}
Committed:
https://chromium.googlesource.com/chromium/src/+/3500cacdae6e8bcdb6371576ca7d...
==========

[commit-bot: I haz the power, 2017-02-03 10:15:07]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/3500cacdae6e8bcdb6371576ca7d...