[Chromecast] Correct libcast_governor behavior.

[Chromecast] Correct libcast_governor behavior.

libcast_governor was using FMAC instead of FMUL
(out = in + in * multiplier instead of out = in * multiplier),
resulting in ~doubling the output volume.

Also added unit tests for Governor and SlewVolume.

BUG=internal/37788000
TEST=libcast_governor_unittests

Change-Id: Ifd5c914bf035bfd8101b2c5827d68164683adc78
Review-Url: https://codereview.chromium.org/2860673003
Cr-Commit-Position: refs/heads/master@{#469886}
Committed: https://chromium.googlesource.com/chromium/src/+/6ccec431840b7d8ed164cf13567a052bda3940b8

[bshaya, 2017-05-03 05:04:37]

bshaya@google.com changed reviewers:
+ bcf@chromium.org, halliwell@chromium.org, jyw@chromium.org,
kmackay@chromium.org, wzhong@chromium.org

[bcf, 2017-05-03 06:14:07]

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

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/BUILD.gn:167: public_configs = [
"//chromecast/public:public_config" ]
Why is this needed?

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

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/governor_unittest.cc:25: const
float kDefaultClamp = .6f;
nit: 0.6f

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/governor_unittest.cc:26: const
double kPi = 3.1415;
Use M_PI from <cmath>

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/governor_unittest.cc:29: const
int kBytesPerSample = sizeof(int);
sizeof(int32_t);

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/governor_unittest.cc:39:
std::vector<int> sine(frames * 2);
int32_t here and for other samples.

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/governor_unittest.cc:72:
ASSERT_FLOAT_EQ(expected[ch][f], actual[ch][f])
EXPECT_FLOAT_EQ

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/slew_volume.cc (right):

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/slew_volume.cc:113: while (frames &&
((original_frames - frames) % 4) != 0) {
I think this would be more explict as:

while (frames && (static_cast<uintptr_t>(src) %
media::vector_math::kRequiredAlignment) != 0) {

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/slew_volume.cc:149: memset(dest, 0, frames *
sizeof(*dest));
return after this?

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/slew_volume.cc:177: while (frames &&
((original_frames - frames) % 4) != 0) {
ditto. Also the statement becomes self documenting then.

[bshaya, 2017-05-03 16:51:56]

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

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/BUILD.gn:167: public_configs = [
"//chromecast/public:public_config" ]
On 2017/05/03 06:14:06, bcf wrote:
> Why is this needed?

This config adds //chromecast/public as an include directory, which is needed if
including anything from //chromecast/public as they all include
"chromecast_export.h" (no complete path).

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

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/governor_unittest.cc:25: const
float kDefaultClamp = .6f;
On 2017/05/03 06:14:07, bcf wrote:
> nit: 0.6f

Done.

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/governor_unittest.cc:26: const
double kPi = 3.1415;
On 2017/05/03 06:14:07, bcf wrote:
> Use M_PI from <cmath>

Done.

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/governor_unittest.cc:29: const
int kBytesPerSample = sizeof(int);
On 2017/05/03 06:14:06, bcf wrote:
> sizeof(int32_t);

Done.

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/governor_unittest.cc:39:
std::vector<int> sine(frames * 2);
On 2017/05/03 06:14:07, bcf wrote:
> int32_t here and for other samples.

Done.

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/governor_unittest.cc:72:
ASSERT_FLOAT_EQ(expected[ch][f], actual[ch][f])
On 2017/05/03 06:14:06, bcf wrote:
> EXPECT_FLOAT_EQ

Done.

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/slew_volume.cc (right):

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/slew_volume.cc:113: while (frames &&
((original_frames - frames) % 4) != 0) {
On 2017/05/03 06:14:07, bcf wrote:
> I think this would be more explict as:
> 
> while (frames && (static_cast<uintptr_t>(src) %
> media::vector_math::kRequiredAlignment) != 0) {

Done.

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/slew_volume.cc:149: memset(dest, 0, frames *
sizeof(*dest));
On 2017/05/03 06:14:07, bcf wrote:
> return after this?

Done.

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/slew_volume.cc:177: while (frames &&
((original_frames - frames) % 4) != 0) {
On 2017/05/03 06:14:07, bcf wrote:
> ditto. Also the statement becomes self documenting then.

Done.

[bcf, 2017-05-03 18:07:09]

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

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/governor_unittest.cc:26: const
double kPi = 3.1415;
On 2017/05/03 16:51:55, bshaya wrote:
> On 2017/05/03 06:14:07, bcf wrote:
> > Use M_PI from <cmath>
> 
> Done.

Need to include <cmath>

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/slew_volume.cc (right):

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/slew_volume.cc:113: while (frames &&
((original_frames - frames) % 4) != 0) {
On 2017/05/03 16:51:56, bshaya wrote:
> On 2017/05/03 06:14:07, bcf wrote:
> > I think this would be more explict as:
> > 
> > while (frames && (static_cast<uintptr_t>(src) %
> > media::vector_math::kRequiredAlignment) != 0) {
> 
> Done.

This LGTM, but FYI the compiler will realize kRequiredAlignment is a power of
two and perform an optimization like this automatically.

[bshaya, 2017-05-04 02:57:07]

Description was changed from

==========
[Chromecast] Correct libcast_governor behavior.

libcast_governor was using FMAC instead of FMUL
(out = in + in * multiplier instead of out = in * multiplier),
resulting in ~doubling the output volume.

Also added a unittest for libcast_governor.

BUG=internal/37788000
TEST=libcast_governor_unittests

Change-Id: Ifd5c914bf035bfd8101b2c5827d68164683adc78
==========

to

==========
[Chromecast] Correct libcast_governor behavior.

libcast_governor was using FMAC instead of FMUL
(out = in + in * multiplier instead of out = in * multiplier),
resulting in ~doubling the output volume.

Also added unit tests for Governor and SlewVolume.

BUG=internal/37788000
TEST=libcast_governor_unittests

Change-Id: Ifd5c914bf035bfd8101b2c5827d68164683adc78
==========

[bshaya, 2017-05-04 03:01:18]

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

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/governor_unittest.cc:26: const
double kPi = 3.1415;
On 2017/05/03 18:07:09, bcf wrote:
> On 2017/05/03 16:51:55, bshaya wrote:
> > On 2017/05/03 06:14:07, bcf wrote:
> > > Use M_PI from <cmath>
> > 
> > Done.
> 
> Need to include <cmath>

Done.

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/slew_volume.cc (right):

https://codereview.chromium.org/2860673003/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/slew_volume.cc:113: while (frames &&
((original_frames - frames) % 4) != 0) {
On 2017/05/03 18:07:09, bcf wrote:
> On 2017/05/03 16:51:56, bshaya wrote:
> > On 2017/05/03 06:14:07, bcf wrote:
> > > I think this would be more explict as:
> > > 
> > > while (frames && (static_cast<uintptr_t>(src) %
> > > media::vector_math::kRequiredAlignment) != 0) {
> > 
> > Done.
> 
> This LGTM, but FYI the compiler will realize kRequiredAlignment is a power of
> two and perform an optimization like this automatically.

That's fine. This is the same code that vector_math.cc uses to DCHECK, so it
makes sense to be consistent.

[bcf, 2017-05-04 03:45:06]

lgtm

https://codereview.chromium.org/2860673003/diff/80001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/post_processors/governor_shlib.h (right):

https://codereview.chromium.org/2860673003/diff/80001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/governor_shlib.h:30: 
// AudioPostProcessor implementation:

Add overrides

https://codereview.chromium.org/2860673003/diff/80001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/governor_shlib.h:47:
std::unique_ptr<SlewVolume> slew_volume_;
ptr necessary? Can we just have SlewVolume inside?

https://codereview.chromium.org/2860673003/diff/80001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/slew_volume_unittests.cc (right):

https://codereview.chromium.org/2860673003/diff/80001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/slew_volume_unittests.cc:107: ASSERT_LE(end,
num_frames_);
ASSERT_GE(end, 0)

[bshaya, 2017-05-04 18:15:41]

https://codereview.chromium.org/2860673003/diff/80001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/post_processors/governor_shlib.h (right):

https://codereview.chromium.org/2860673003/diff/80001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/governor_shlib.h:30: 
On 2017/05/04 03:45:06, bcf wrote:
> // AudioPostProcessor implementation:
> 
> Add overrides

Done.

https://codereview.chromium.org/2860673003/diff/80001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processors/governor_shlib.h:47:
std::unique_ptr<SlewVolume> slew_volume_;
On 2017/05/04 03:45:06, bcf wrote:
> ptr necessary? Can we just have SlewVolume inside?

Done.

https://codereview.chromium.org/2860673003/diff/80001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/alsa/slew_volume_unittests.cc (right):

https://codereview.chromium.org/2860673003/diff/80001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/slew_volume_unittests.cc:107: ASSERT_LE(end,
num_frames_);
On 2017/05/04 03:45:06, bcf wrote:
> ASSERT_GE(end, 0)

Done.

[kmackay, 2017-05-04 18:29:34]

nits

https://codereview.chromium.org/2860673003/diff/100001/chromecast/media/cma/b...
File chromecast/media/cma/backend/alsa/post_processors/governor_shlib.h (right):

https://codereview.chromium.org/2860673003/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/post_processors/governor_shlib.h:21: // while
providing dyanmic range at low output level.
Fix formatting and typo.

https://codereview.chromium.org/2860673003/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/post_processors/governor_shlib.h:30: //
AudioPostProcessor implementations:
implementation:

https://codereview.chromium.org/2860673003/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/post_processors/governor_shlib.h:47:
SlewVolume slew_volume_;
add blank line before DISALLOW...

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

https://codereview.chromium.org/2860673003/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/slew_volume.cc:147: }
Can this common code (between FMAC and FMUL) be moved to a helper function?

[bshaya, 2017-05-04 23:53:39]

https://codereview.chromium.org/2860673003/diff/100001/chromecast/media/cma/b...
File chromecast/media/cma/backend/alsa/post_processors/governor_shlib.h (right):

https://codereview.chromium.org/2860673003/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/post_processors/governor_shlib.h:21: // while
providing dyanmic range at low output level.
On 2017/05/04 18:29:34, kmackay wrote:
> Fix formatting and typo.

Done.

https://codereview.chromium.org/2860673003/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/post_processors/governor_shlib.h:30: //
AudioPostProcessor implementations:
On 2017/05/04 18:29:34, kmackay wrote:
> implementation:

Done.

https://codereview.chromium.org/2860673003/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/post_processors/governor_shlib.h:47:
SlewVolume slew_volume_;
On 2017/05/04 18:29:34, kmackay wrote:
> add blank line before DISALLOW...

Done.

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

https://codereview.chromium.org/2860673003/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/slew_volume.cc:147: }
On 2017/05/04 18:29:34, kmackay wrote:
> Can this common code (between FMAC and FMUL) be moved to a helper function?

Done.

[kmackay, 2017-05-05 00:13:30]

https://codereview.chromium.org/2860673003/diff/120001/chromecast/media/cma/b...
File chromecast/media/cma/backend/alsa/slew_volume.cc (right):

https://codereview.chromium.org/2860673003/diff/120001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/slew_volume.cc:25: #define
PROCESS_DATA(BULK_OPERATOR, SINGLE_OPERATOR, ZERO_OPERATOR,      \
Not exactly what I had in mind...

[bshaya, 2017-05-05 00:30:35]

https://codereview.chromium.org/2860673003/diff/120001/chromecast/media/cma/b...
File chromecast/media/cma/backend/alsa/slew_volume.cc (right):

https://codereview.chromium.org/2860673003/diff/120001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/slew_volume.cc:25: #define
PROCESS_DATA(BULK_OPERATOR, SINGLE_OPERATOR, ZERO_OPERATOR,      \
On 2017/05/05 00:13:30, kmackay wrote:
> Not exactly what I had in mind...

Done.

[kmackay, 2017-05-05 00:37:18]

lgtm % nit

https://codereview.chromium.org/2860673003/diff/140001/chromecast/media/cma/b...
File chromecast/media/cma/backend/alsa/slew_volume.cc (right):

https://codereview.chromium.org/2860673003/diff/140001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/slew_volume.cc:24: // |UNITY_OPERATOR| is run
when |current_volume_| is 1
Update this comment block

[halliwell, 2017-05-05 17:24:50]

rs-lgtm

https://codereview.chromium.org/2860673003/diff/160001/chromecast/media/cma/b...
File chromecast/media/cma/backend/alsa/post_processors/governor_shlib.h (right):

https://codereview.chromium.org/2860673003/diff/160001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/post_processors/governor_shlib.h:25: class
Governor : public AudioPostProcessor {
why are class name, file name different?

https://codereview.chromium.org/2860673003/diff/160001/chromecast/media/cma/b...
File chromecast/media/cma/backend/alsa/slew_volume.cc (right):

https://codereview.chromium.org/2860673003/diff/160001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/slew_volume.cc:52: memset(dest, 0, frames *
sizeof(*dest));
inconsistent with std::memcpy below?

https://codereview.chromium.org/2860673003/diff/160001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/slew_volume.cc:127: /* Ensure |src| and |dest|
are 16-byte aligned. */
why switching comment style in this file?

[bshaya, 2017-05-05 17:54:04]

https://codereview.chromium.org/2860673003/diff/160001/chromecast/media/cma/b...
File chromecast/media/cma/backend/alsa/post_processors/governor_shlib.h (right):

https://codereview.chromium.org/2860673003/diff/160001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/post_processors/governor_shlib.h:25: class
Governor : public AudioPostProcessor {
On 2017/05/05 17:24:50, halliwell wrote:
> why are class name, file name different?

Done.

https://codereview.chromium.org/2860673003/diff/160001/chromecast/media/cma/b...
File chromecast/media/cma/backend/alsa/slew_volume.cc (right):

https://codereview.chromium.org/2860673003/diff/160001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/slew_volume.cc:52: memset(dest, 0, frames *
sizeof(*dest));
On 2017/05/05 17:24:50, halliwell wrote:
> inconsistent with std::memcpy below?

Done.

https://codereview.chromium.org/2860673003/diff/160001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/slew_volume.cc:127: /* Ensure |src| and |dest|
are 16-byte aligned. */
On 2017/05/05 17:24:50, halliwell wrote:
> why switching comment style in this file?

Done.

[bshaya, 2017-05-05 17:54:10]

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

[commit-bot: I haz the power, 2017-05-05 17:54:49]

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-05-05 20:04:37]

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

[commit-bot: I haz the power, 2017-05-05 20:04:38]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[bshaya, 2017-05-05 20:08:27]

The CQ bit was checked by bshaya@google.com

[bshaya, 2017-05-05 20:08:27]

The patchset sent to the CQ was uploaded after l-g-t-m from bcf@chromium.org,
kmackay@chromium.org, halliwell@chromium.org
Link to the patchset: https://codereview.chromium.org/2860673003/#ps180001
(title: "address halliwell@'s comments")

[commit-bot: I haz the power, 2017-05-05 20:09:19]

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-05-05 23:31:41]

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

[commit-bot: I haz the power, 2017-05-05 23:31:42]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[bshaya, 2017-05-05 23:46:00]

The CQ bit was checked by bshaya@google.com

[commit-bot: I haz the power, 2017-05-05 23:46:46]

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-05-06 19:24:18]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1494027960944870,
"parent_rev": "6dc6477fbfd23d601d72dc1ad941699c65a47353", "commit_rev":
"6ccec431840b7d8ed164cf13567a052bda3940b8"}

[commit-bot: I haz the power, 2017-05-06 19:24:30]

Description was changed from

==========
[Chromecast] Correct libcast_governor behavior.

libcast_governor was using FMAC instead of FMUL
(out = in + in * multiplier instead of out = in * multiplier),
resulting in ~doubling the output volume.

Also added unit tests for Governor and SlewVolume.

BUG=internal/37788000
TEST=libcast_governor_unittests

Change-Id: Ifd5c914bf035bfd8101b2c5827d68164683adc78
==========

to

==========
[Chromecast] Correct libcast_governor behavior.

libcast_governor was using FMAC instead of FMUL
(out = in + in * multiplier instead of out = in * multiplier),
resulting in ~doubling the output volume.

Also added unit tests for Governor and SlewVolume.

BUG=internal/37788000
TEST=libcast_governor_unittests

Change-Id: Ifd5c914bf035bfd8101b2c5827d68164683adc78
Review-Url: https://codereview.chromium.org/2860673003
Cr-Commit-Position: refs/heads/master@{#469886}
Committed:
https://chromium.googlesource.com/chromium/src/+/6ccec431840b7d8ed164cf13567a...
==========

[commit-bot: I haz the power, 2017-05-06 19:24:32]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/6ccec431840b7d8ed164cf13567a...