[Chromecast] Complete PostProcessingPipeline changes

[Chromecast] Complete PostProcessingPipeline changes

- Add mix & linearize AudioPostProcessor hooks.
- Use delay from AudioPostProcessor::ProcessFrames to update timing estimates.
- Allow configuring mixing multiple device_id's in a single AudioPostProcessor.
- Pass Cast Volume to PostProcessors (rather than raw multiplier).
- Add unittest for assignment of PostProcessors + delay accounting.

BUG=internal/36299959
TEST=cast_alsa_cma_backend_unittests

Change-Id: I5503f7de39d0ac502b8e861322162fee9aade8dd
Review-Url: https://codereview.chromium.org/2847673002
Cr-Commit-Position: refs/heads/master@{#468399}
Committed: https://chromium.googlesource.com/chromium/src/+/4d46c8d9c3c74747b2f0bcee490262e45dd9180f

[bshaya, 2017-04-27 05:10:58]

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

[bshaya, 2017-04-27 22:46:24]

bshaya@google.com changed reviewers:
+ gfhuang@chromium.org

[kmackay, 2017-04-28 00:31:42]

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

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/filter_group.cc:79: if (active_inputs_.empty()
&& volume == 0.0f &&
The volume == 0 check is a bit subtle, please add a comment

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/filter_group.cc:83: // our data is read
laster.
Fix comment formatting and 'laster'

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/filter_group.cc:92: // Mix InputQueue's
nit: no apostrophe

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/filter_group.cc:104: // Mix FilterGroup's
nit: no apostrophe

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/filter_group.cc:114: if (volume != 0.0f) {
Why the if check? seems like last_volume_ should be set even if it's 0

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

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/filter_group.h:40: // entry in |input_types|
to be processed by this group).
Could you add comments about the other parameters?

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/filter_group.h:41: FilterGroup(int channels,
num_channels

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/filter_group.h:55: const
std::unordered_set<std::string>& input_types);
The two extra constructors seem unneccessary; callers can easily pass in {}.

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/filter_group.h:68: // Mixes all active inputs
and passes them through the audio filter.
comment about return value

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

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processing_pipeline.h:24: bool
is_silence) = 0;
Do we need is_silence? Could just check volume==0 internally.

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

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processing_pipeline_parser.h:59: static
std::string file_path();
GetFilePath()

[bshaya, 2017-04-28 01:37:37]

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

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/filter_group.cc:79: if (active_inputs_.empty()
&& volume == 0.0f &&
On 2017/04/28 00:31:41, kmackay wrote:
> The volume == 0 check is a bit subtle, please add a comment

Done.

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/filter_group.cc:83: // our data is read
laster.
On 2017/04/28 00:31:42, kmackay wrote:
> Fix comment formatting and 'laster'

Done.

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/filter_group.cc:92: // Mix InputQueue's
On 2017/04/28 00:31:41, kmackay wrote:
> nit: no apostrophe

Done.

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/filter_group.cc:104: // Mix FilterGroup's
On 2017/04/28 00:31:41, kmackay wrote:
> nit: no apostrophe

Done.

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/filter_group.cc:114: if (volume != 0.0f) {
On 2017/04/28 00:31:42, kmackay wrote:
> Why the if check? seems like last_volume_ should be set even if it's 0

I need to hold on to last_volume_ until ringing is done. This also ensures that
we only return 0 in the early return case (no data to mix).

For instance, if the stream is at volume=0.5 and gets paused, we should allow
the post processor to ring out with volume=0.5 rather than changing volume to 0.

If the stream is chanced to volume=0, it doesn't matter what volume is, because
the data in the stream will be 0's.

Edited to make more clear.

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

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/filter_group.h:40: // entry in |input_types|
to be processed by this group).
On 2017/04/28 00:31:42, kmackay wrote:
> Could you add comments about the other parameters?

Done.

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/filter_group.h:41: FilterGroup(int channels,
On 2017/04/28 00:31:42, kmackay wrote:
> num_channels

Done.

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/filter_group.h:55: const
std::unordered_set<std::string>& input_types);
On 2017/04/28 00:31:42, kmackay wrote:
> The two extra constructors seem unneccessary; callers can easily pass in {}.

Done.

Although you do have to pass in std::vector<FilterGroup>() or
std::unordered_set<std::string>(), which is cumbersome.

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/filter_group.h:68: // Mixes all active inputs
and passes them through the audio filter.
On 2017/04/28 00:31:42, kmackay wrote:
> comment about return value

Done.

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

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processing_pipeline.h:24: bool
is_silence) = 0;
On 2017/04/28 00:31:42, kmackay wrote:
> Do we need is_silence? Could just check volume==0 internally.

I fixed this in filter_group so that we ring out at the last volume, so
is_silence can be true while current_volume is non-zero.

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

https://codereview.chromium.org/2847673002/diff/40001/chromecast/media/cma/ba...
chromecast/media/cma/backend/alsa/post_processing_pipeline_parser.h:59: static
std::string file_path();
On 2017/04/28 00:31:42, kmackay wrote:
> GetFilePath()

Done.

[kmackay, 2017-04-28 21:20:08]

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

https://codereview.chromium.org/2847673002/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/filter_group.cc:61: // 0 if:
nit: move '0 if:' to previous line

https://codereview.chromium.org/2847673002/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/filter_group.cc:79: // Mix InputQueue
nit: InputQueues.

https://codereview.chromium.org/2847673002/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/filter_group.cc:91: // Mix FilterGroup
nit: FilterGroups.

https://codereview.chromium.org/2847673002/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/filter_group.cc:101: bool is_silence = volume
== 0.0f;
nit: add () around (volume == 0.0f)

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

https://codereview.chromium.org/2847673002/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:260:
std::vector<FilterGroup*>() /* mixed_inputs */));
can't you just pass in {} for mixed_inputs?

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

https://codereview.chromium.org/2847673002/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc:164: )json";
There should be a test with multiple processors for a single stream type.

https://codereview.chromium.org/2847673002/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc:448: char
test_pipeline_json[sizeof(kTestPipelineJsonTemplate) * 2];
The size calculation here seems a bit ad-hoc. Why not use base::StringPrintf()?

https://codereview.chromium.org/2847673002/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc:1082: char
test_pipeline_json[sizeof(kTestPipelineJson) * 2];
base::StringPrintf()?

[bshaya, 2017-04-28 22:59:31]

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

https://codereview.chromium.org/2847673002/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/filter_group.cc:61: // 0 if:
On 2017/04/28 21:20:08, kmackay wrote:
> nit: move '0 if:' to previous line

Done.

https://codereview.chromium.org/2847673002/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/filter_group.cc:79: // Mix InputQueue
On 2017/04/28 21:20:08, kmackay wrote:
> nit: InputQueues.

Done.

https://codereview.chromium.org/2847673002/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/filter_group.cc:91: // Mix FilterGroup
On 2017/04/28 21:20:08, kmackay wrote:
> nit: FilterGroups.

Done.

https://codereview.chromium.org/2847673002/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/filter_group.cc:101: bool is_silence = volume
== 0.0f;
On 2017/04/28 21:20:08, kmackay wrote:
> nit: add () around (volume == 0.0f)

Done.

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

https://codereview.chromium.org/2847673002/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:260:
std::vector<FilterGroup*>() /* mixed_inputs */));
On 2017/04/28 21:20:08, kmackay wrote:
> can't you just pass in {} for mixed_inputs?

No, MakeUnique isn't smart enough to cast.

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

https://codereview.chromium.org/2847673002/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc:164: )json";
On 2017/04/28 21:20:08, kmackay wrote:
> There should be a test with multiple processors for a single stream type.

Done.

https://codereview.chromium.org/2847673002/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc:448: char
test_pipeline_json[sizeof(kTestPipelineJsonTemplate) * 2];
On 2017/04/28 21:20:08, kmackay wrote:
> The size calculation here seems a bit ad-hoc. Why not use
base::StringPrintf()?

Done.

https://codereview.chromium.org/2847673002/diff/100001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/stream_mixer_alsa_unittest.cc:1082: char
test_pipeline_json[sizeof(kTestPipelineJson) * 2];
On 2017/04/28 21:20:08, kmackay wrote:
> base::StringPrintf()?

Done.

[kmackay, 2017-04-29 00:45:22]

lgtm

[wzhong, 2017-04-29 01:22:29]

BUG=internal b/36299959

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

https://codereview.chromium.org/2847673002/diff/120001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/filter_group.cc:66: if (active_inputs_.empty()
&& volume == 0.0f &&
Why is this &&? Contrary to comment.

[bshaya, 2017-04-29 16:53:26]

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

https://codereview.chromium.org/2847673002/diff/120001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/filter_group.cc:66: if (active_inputs_.empty()
&& volume == 0.0f &&
On 2017/04/29 01:22:29, wzhong wrote:
> Why is this &&? Contrary to comment.

We have two sets of inputs: active_inputs_ and mixed_inputs_. active_inputs_ are
InputQueues, i.e. raw input streams. mixed_inputs_ are other FilterGroups, i.e.
mixed inputs.

So this if statement reads: "If (no data from active_inputs_) and (no data from
mixed_inputs_)"

[wzhong, 2017-05-01 13:30:09]

lgtm

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

https://codereview.chromium.org/2847673002/diff/120001/chromecast/media/cma/b...
chromecast/media/cma/backend/alsa/filter_group.cc:66: if (active_inputs_.empty()
&& volume == 0.0f &&
On 2017/04/29 16:53:26, bshaya wrote:
> On 2017/04/29 01:22:29, wzhong wrote:
> > Why is this &&? Contrary to comment.
> 
> We have two sets of inputs: active_inputs_ and mixed_inputs_. active_inputs_
are
> InputQueues, i.e. raw input streams. mixed_inputs_ are other FilterGroups,
i.e.
> mixed inputs.
> 
> So this if statement reads: "If (no data from active_inputs_) and (no data
from
> mixed_inputs_)"

Acknowledged.

[bshaya, 2017-05-01 17:15:01]

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

[commit-bot: I haz the power, 2017-05-01 17:15:43]

Dry run: CQ is trying da patch.

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

[slan, 2017-05-01 17:18:30]

rs lgtm; deferring to kmackay@ and wzhong@ for review.

[commit-bot: I haz the power, 2017-05-01 17:26:27]

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

[commit-bot: I haz the power, 2017-05-01 17:26:28]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[bshaya, 2017-05-01 17:42:38]

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

[commit-bot: I haz the power, 2017-05-01 17:43:10]

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-01 19:51:36]

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

[commit-bot: I haz the power, 2017-05-01 19:51:37]

Dry run: This issue passed the CQ dry run.

[bshaya, 2017-05-01 20:22:44]

The CQ bit was checked by bshaya@google.com

[bshaya, 2017-05-01 20:22:45]

The patchset sent to the CQ was uploaded after l-g-t-m from wzhong@chromium.org,
slan@chromium.org, kmackay@chromium.org
Link to the patchset: https://codereview.chromium.org/2847673002/#ps140001
(title: "Fix deps")

[commit-bot: I haz the power, 2017-05-01 20:23:28]

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-01 20:29:46]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493670164982320,
"parent_rev": "27ed7650a26ddb0f447963a0c6b5ff1e9ed0e994", "commit_rev":
"4d46c8d9c3c74747b2f0bcee490262e45dd9180f"}

[commit-bot: I haz the power, 2017-05-01 20:30:08]

Description was changed from

==========
[Chromecast] Complete PostProcessingPipeline changes

- Add mix & linearize AudioPostProcessor hooks.
- Use delay from AudioPostProcessor::ProcessFrames to update timing estimates.
- Allow configuring mixing multiple device_id's in a single AudioPostProcessor.
- Pass Cast Volume to PostProcessors (rather than raw multiplier).
- Add unittest for assignment of PostProcessors + delay accounting.

BUG=internal/36299959
TEST=cast_alsa_cma_backend_unittests

Change-Id: I5503f7de39d0ac502b8e861322162fee9aade8dd
==========

to

==========
[Chromecast] Complete PostProcessingPipeline changes

- Add mix & linearize AudioPostProcessor hooks.
- Use delay from AudioPostProcessor::ProcessFrames to update timing estimates.
- Allow configuring mixing multiple device_id's in a single AudioPostProcessor.
- Pass Cast Volume to PostProcessors (rather than raw multiplier).
- Add unittest for assignment of PostProcessors + delay accounting.

BUG=internal/36299959
TEST=cast_alsa_cma_backend_unittests

Change-Id: I5503f7de39d0ac502b8e861322162fee9aade8dd
Review-Url: https://codereview.chromium.org/2847673002
Cr-Commit-Position: refs/heads/master@{#468399}
Committed:
https://chromium.googlesource.com/chromium/src/+/4d46c8d9c3c74747b2f0bcee4902...
==========

[commit-bot: I haz the power, 2017-05-01 20:30:10]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/4d46c8d9c3c74747b2f0bcee4902...