Add a simple, safe gain PostProcessor.

Add a simple gain PostProcessor.

BUG=internal/62571379
BUG=internal/63080799
TEST=On Device

Change-Id: Ied56d594db00bee5eb9c407c80d9befd8fbd7210
Review-Url: https://codereview.chromium.org/2958393002
Cr-Commit-Position: refs/heads/master@{#485027}
Committed: https://chromium.googlesource.com/chromium/src/+/1f461df4f62cdd1bf5bf8db3524b795dc9ef5bd5

[bshaya, 2017-06-28 23:48:23]

bshaya@google.com changed reviewers:
+ alokp@chromium.org, kmackay@chromium.org, tianyuwang@google.com

[bshaya, 2017-06-28 23:48:31]

The CQ bit was checked by bshaya@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-28 23:49:02]

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-06-29 01:29:23]

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

[commit-bot: I haz the power, 2017-06-29 01:29:24]

Dry run: This issue passed the CQ dry run.

[bshaya, 2017-06-30 01:19:26]

Description was changed from

==========
Add a simple, safe gain PostProcessor.

BUG=internal/62571379
BUG=internal/63080799
TEST=On Device

Change-Id: Ic233e92401c5345d1fd50d6eede5fd48084d3071
==========

to

==========
Add a simple gain PostProcessor.

BUG=internal/62571379
BUG=internal/63080799
TEST=On Device

Change-Id: Ic233e92401c5345d1fd50d6eede5fd48084d3071
==========

[alokp, 2017-06-30 17:15:35]

I just reviewed non-cma files. Defer the cma reviews to kmackay@.

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/BUILD.gn (right):

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/BUILD.gn:177:
test("libcast_saturated_gain_unittests") {
why do we have so many tests under a module. It would be preferable to have a
single cast_media_unittests so that it is easy to discover and run.

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/BUILD.gn:217: source_set("saturated_gain") {
why do you need this source_set? Can't the test directly depend on
libcast_saturated_gain_1.0?

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/servic...
File chromecast/media/service/cast_renderer.cc (right):

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/servic...
chromecast/media/service/cast_renderer.cc:111: } else if (audio_device_id_ ==
kTtsAudioDeviceId ||
Can we delete kTtsAudioDeviceId now?

[kmackay, 2017-06-30 17:52:19]

Please switch to the new code review at some point (not for this CL) -
https://chromium-review.googlesource.com

[kmackay, 2017-06-30 17:59:38]

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/cast_audio_json.cc (right):

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/cast_audio_json.cc:10: std::string
GetCastAudioJsonFilePath() {
Why does this need to be a function instead of a constant?

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/post_processors/saturated_gain.cc
(right):

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/saturated_gain.cc:9: #include
<memory>
<memory> not used

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/saturated_gain.cc:74: return
static_cast<chromecast::media::AudioPostProcessor*>(
Why do you need static_cast?

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/post_processors/saturated_gain.h (right):

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/saturated_gain.h:9: #include
<vector>
<memory> and <vector> not used.

#include <string>

[kmackay, 2017-06-30 20:57:58]

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/volume_map.h (right):

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/volume_map.h:15: struct LevelToDb {
Does this need to be exposed (ie, could it be private to VolumeMap?)

[bshaya, 2017-07-06 21:04:45]

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/BUILD.gn (right):

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/BUILD.gn:177:
test("libcast_saturated_gain_unittests") {
On 2017/06/30 17:15:35, alokp wrote:
> why do we have so many tests under a module. It would be preferable to have a
> single cast_media_unittests so that it is easy to discover and run.

Done.

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/BUILD.gn:217: source_set("saturated_gain") {
On 2017/06/30 17:15:35, alokp wrote:
> why do you need this source_set? Can't the test directly depend on
> libcast_saturated_gain_1.0?

Done.

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/cast_audio_json.cc (right):

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/cast_audio_json.cc:10: std::string
GetCastAudioJsonFilePath() {
On 2017/06/30 17:59:38, kmackay wrote:
> Why does this need to be a function instead of a constant?

Done.

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/post_processors/saturated_gain.cc
(right):

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/saturated_gain.cc:9: #include
<memory>
On 2017/06/30 17:59:38, kmackay wrote:
> <memory> not used

Done.

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/saturated_gain.cc:74: return
static_cast<chromecast::media::AudioPostProcessor*>(
On 2017/06/30 17:59:38, kmackay wrote:
> Why do you need static_cast?

Thought bcf told me I needed it in a similar CL. Guess not.

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/post_processors/saturated_gain.h (right):

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/saturated_gain.h:9: #include
<vector>
On 2017/06/30 17:59:38, kmackay wrote:
> <memory> and <vector> not used.
> 
> #include <string>

Done.

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/volume_map.h (right):

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/volume_map.h:15: struct LevelToDb {
On 2017/06/30 20:57:58, kmackay wrote:
> Does this need to be exposed (ie, could it be private to VolumeMap?)

Done.

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/servic...
File chromecast/media/service/cast_renderer.cc (right):

https://codereview.chromium.org/2958393002/diff/40001/chromecast/media/servic...
chromecast/media/service/cast_renderer.cc:111: } else if (audio_device_id_ ==
kTtsAudioDeviceId ||
On 2017/06/30 17:15:35, alokp wrote:
> Can we delete kTtsAudioDeviceId now?

We still need different DSP treatment for TTS and phone calls, unfortunately
(calls come in at a very different volume from TTS/media).

[kmackay, 2017-07-06 22:16:28]

https://codereview.chromium.org/2958393002/diff/60001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/cast_audio_json.h (right):

https://codereview.chromium.org/2958393002/diff/60001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/cast_audio_json.h:8: #include <string>
<string> unused

https://codereview.chromium.org/2958393002/diff/60001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/post_processors/governor_unittest.cc
(right):

https://codereview.chromium.org/2958393002/diff/60001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/governor_unittest.cc:115:
base::WrapUnique<AudioPostProcessor>(new Governor(config, kNumChannels));
Why not make this work the same way as saturated_gain_unittest.cc (ie, use the
factory to create the post processor)?

[bshaya, 2017-07-06 22:38:11]

https://codereview.chromium.org/2958393002/diff/60001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/cast_audio_json.h (right):

https://codereview.chromium.org/2958393002/diff/60001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/cast_audio_json.h:8: #include <string>
On 2017/07/06 22:16:28, kmackay wrote:
> <string> unused

Done.

https://codereview.chromium.org/2958393002/diff/60001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/post_processors/governor_unittest.cc
(right):

https://codereview.chromium.org/2958393002/diff/60001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/governor_unittest.cc:115:
base::WrapUnique<AudioPostProcessor>(new Governor(config, kNumChannels));
On 2017/07/06 22:16:28, kmackay wrote:
> Why not make this work the same way as saturated_gain_unittest.cc (ie, use the
> factory to create the post processor)?

Done.

[kmackay, 2017-07-07 00:05:33]

lgtm

[bshaya, 2017-07-07 00:33:53]

bshaya@google.com changed reviewers:
+ montvelishsky@google.com

[mont, 2017-07-07 00:37:44]

https://codereview.chromium.org/2958393002/diff/100001/chromecast/media/cma/b...
File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (right):

https://codereview.chromium.org/2958393002/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:1090: const std::string&
message) {
message should be renamed to config

https://codereview.chromium.org/2958393002/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:1091:
RUN_ON_MIXER_THREAD(&StreamMixerAlsa::SetPostProcessorConfig, name, message);
config

[alokp, 2017-07-07 16:52:34]

lgtm

[bshaya, 2017-07-07 17:50:16]

The CQ bit was checked by bshaya@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-07-07 17:50:33]

Dry run: CQ is trying da patch.

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

[bshaya, 2017-07-07 17:51:34]

Description was changed from

==========
Add a simple gain PostProcessor.

BUG=internal/62571379
BUG=internal/63080799
TEST=On Device

Change-Id: Ic233e92401c5345d1fd50d6eede5fd48084d3071
==========

to

==========
Add a simple gain PostProcessor.

BUG=internal/62571379
BUG=internal/63080799
TEST=On Device

Change-Id: Ied56d594db00bee5eb9c407c80d9befd8fbd7210
==========

[commit-bot: I haz the power, 2017-07-07 20:06:58]

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

[commit-bot: I haz the power, 2017-07-07 20:06:59]

Dry run: This issue passed the CQ dry run.

[bshaya, 2017-07-07 20:21:25]

The CQ bit was checked by bshaya@google.com

[bshaya, 2017-07-07 20:21:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
kmackay@chromium.org, alokp@chromium.org
Link to the patchset: https://codereview.chromium.org/2958393002/#ps120001
(title: "Unrelated cleanup")

[commit-bot: I haz the power, 2017-07-07 20:21:58]

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-07-07 20:26:21]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1499458885975500,
"parent_rev": "698e72cd72a1ba9e03d1484ebdd98ec934c4afa6", "commit_rev":
"1f461df4f62cdd1bf5bf8db3524b795dc9ef5bd5"}

[commit-bot: I haz the power, 2017-07-07 20:27:11]

Description was changed from

==========
Add a simple gain PostProcessor.

BUG=internal/62571379
BUG=internal/63080799
TEST=On Device

Change-Id: Ied56d594db00bee5eb9c407c80d9befd8fbd7210
==========

to

==========
Add a simple gain PostProcessor.

BUG=internal/62571379
BUG=internal/63080799
TEST=On Device

Change-Id: Ied56d594db00bee5eb9c407c80d9befd8fbd7210
Review-Url: https://codereview.chromium.org/2958393002
Cr-Commit-Position: refs/heads/master@{#485027}
Committed:
https://chromium.googlesource.com/chromium/src/+/1f461df4f62cdd1bf5bf8db3524b...
==========

[commit-bot: I haz the power, 2017-07-07 20:27:12]

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