Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2701613006:
[Chromecast] Process streams with different post-processing. (Closed)
|
Created:
3 years, 10 months ago by bshaya Modified:
3 years, 9 months ago CC:
chromium-reviews, alokp+watch_chromium.org, lcwu+watch_chromium.org, gfhuang+watch_chromium.org, halliwell+watch_chromium.org, feature-media-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Chromecast] Process streams with different post-processing.
Adds |name| to stream_mixer_alsa_input_impl.
stream_mixer_alsa can then mix streams of the same time separately,
and apply different post-processing to each.
Also reduce buffering of system streams from 300ms to 60ms.
BUG=internal b/34817384
TEST=manual
Change-Id: Ieb91e419d7d34c374d83941c02f2546696b4f489
Review-Url: https://codereview.chromium.org/2701613006
Cr-Commit-Position: refs/heads/master@{#453522}
Committed: https://chromium.googlesource.com/chromium/src/+/e975664dab5f430b66817238898839b333822ac3
Patch Set 1 #
Total comments: 22
Patch Set 2 : Fix nits #
Total comments: 32
Patch Set 3 : Move filter state into filter_group.cc #Patch Set 4 : Move audio_device_ids to chromecast/media/base #
Total comments: 4
Patch Set 5 : Split streams into TTS, Media, Alarm, Communication #Patch Set 6 : Rebase on head #
Total comments: 2
Patch Set 7 : Fix nit #
Total comments: 20
Patch Set 8 : Assign a FilterGroup to each InputQueue on creation. #Patch Set 9 : constexpr #
Total comments: 12
Patch Set 10 : Fix more comments. #
Total comments: 2
Patch Set 11 : Fix #endif comment #Patch Set 12 : Fix BUILD.gn #Patch Set 13 : Fix clang/test compile errors #Patch Set 14 : Fix unittests #Messages
Total messages: 77 (36 generated)
bshaya@google.com changed reviewers: + kmackay@chromium.org, tianyuwang@google.com
https://codereview.chromium.org/2701613006/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (right): https://codereview.chromium.org/2701613006/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:888: bool filter_frames = true; The design for mixing seems a bit clunky. I'd rather have something like: * Have a FIlterGroup class that holds state for each filter group. StreamMixerAlsa will have a vector of these. * When iterating through the active inputs, loop through the FilterGroups calling AddActiveInput() until one of them returns true (ie, the FilterGroup determines which inputs it cares about, based on the name). * Once all inputs have been iterated through, call Execute() or something on every FilterGroup. This will mix all of the input that that group accepted to a buffer that is internal to the FilterGroup, and then run the filter on it (if any). Execute() returns false if there is no output from that filter group. * If no filter groups have output, push silence. If only 1 filter group has output, push that group's buffer. Otherwise, mix the buffers of all filter groups that have output together, and push the result. Then it's easy to also do volume control at the same time. https://codereview.chromium.org/2701613006/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:936: size_t interleaved_size = static_cast<size_t>(frames * kNumOutputChannels) * use int instead of size_t https://codereview.chromium.org/2701613006/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:944: if (post_loopback_filter_ && filter_frames) { Do we ever use post_loopback_filter? If not, let's get rid of it. Then filter_frames isn't necessary either. https://codereview.chromium.org/2701613006/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.h (right): https://codereview.chromium.org/2701613006/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/stream_mixer_alsa.h:206: // Mixes |acitve_inputs| and processes the resulting buffer active_inputs https://codereview.chromium.org/2701613006/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/stream_mixer_alsa.h:250: // writing to |interleaved_|. Could you clarify the comment a bit? Something about how we mix data for each stream type, apply appropriate filters, and then mix into |interleaved_| https://codereview.chromium.org/2701613006/diff/1/chromecast/public/media/aud... File chromecast/public/media/audio_device_ids.cc (right): https://codereview.chromium.org/2701613006/diff/1/chromecast/public/media/aud... chromecast/public/media/audio_device_ids.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2701613006/diff/1/chromecast/public/media/aud... chromecast/public/media/audio_device_ids.cc:13: const char kSystemAudioDeviceId[] = "assistant-system"; I'm not convinced that this is the correct name based on what you are going to use it for. https://codereview.chromium.org/2701613006/diff/1/chromecast/public/media/aud... File chromecast/public/media/audio_device_ids.h (right): https://codereview.chromium.org/2701613006/diff/1/chromecast/public/media/aud... chromecast/public/media/audio_device_ids.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017
https://codereview.chromium.org/2701613006/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (right): https://codereview.chromium.org/2701613006/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:888: bool filter_frames = true; On 2017/02/17 06:11:25, kmackay wrote: > The design for mixing seems a bit clunky. I'd rather have something like: > * Have a FIlterGroup class that holds state for each filter group. > StreamMixerAlsa will have a vector of these. > * When iterating through the active inputs, loop through the FilterGroups > calling AddActiveInput() until one of them returns true (ie, the FilterGroup > determines which inputs it cares about, based on the name). > * Once all inputs have been iterated through, call Execute() or something on > every FilterGroup. This will mix all of the input that that group accepted to a > buffer that is internal to the FilterGroup, and then run the filter on it (if > any). Execute() returns false if there is no output from that filter group. > * If no filter groups have output, push silence. If only 1 filter group has > output, push that group's buffer. Otherwise, mix the buffers of all filter > groups that have output together, and push the result. > Hmm, how about that, and: * Have an InputGroup class that contains a list of inputs, a filter, filter state, and necessary buffers. * When an InputQueue is added, it is added to the appropriate group directly (similar to the Alsa implementation). * Call InputGroup->Update to check for ChunkSize and blocked Primary inputs. * Execute and mix as you suggested. > Then it's easy to also do volume control at the same time. At the same time as what? If you mean stream volume control, I'd rather not, since that either a) adds an additional level of mixing (streams->groups->filter groups->everything) or requires duplicate filters (since we have more groups than filter types). I guess the ideal would be for the architecture to be completely modular (InputGroup implements InputQueue so that they can be fed into each other, and you just call GetInterleavedData from the final InputGroup when you want data. That might be too much re-designing considering we want to remove all of this... https://codereview.chromium.org/2701613006/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:936: size_t interleaved_size = static_cast<size_t>(frames * kNumOutputChannels) * On 2017/02/17 06:11:25, kmackay wrote: > use int instead of size_t See the first point in "Types" in https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md size_t is correct for vector size/indices, int is considered unsafe. https://codereview.chromium.org/2701613006/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:944: if (post_loopback_filter_ && filter_frames) { On 2017/02/17 06:11:25, kmackay wrote: > Do we ever use post_loopback_filter? If not, let's get rid of it. Then > filter_frames isn't necessary either. Done. https://codereview.chromium.org/2701613006/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.h (right): https://codereview.chromium.org/2701613006/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/stream_mixer_alsa.h:206: // Mixes |acitve_inputs| and processes the resulting buffer On 2017/02/17 06:11:25, kmackay wrote: > active_inputs Done. https://codereview.chromium.org/2701613006/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/stream_mixer_alsa.h:250: // writing to |interleaved_|. On 2017/02/17 06:11:25, kmackay wrote: > Could you clarify the comment a bit? Something about how we mix data for each > stream type, apply appropriate filters, and then mix into |interleaved_| Done. https://codereview.chromium.org/2701613006/diff/1/chromecast/public/media/aud... File chromecast/public/media/audio_device_ids.cc (right): https://codereview.chromium.org/2701613006/diff/1/chromecast/public/media/aud... chromecast/public/media/audio_device_ids.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/17 06:11:25, kmackay wrote: > 2017 Done. https://codereview.chromium.org/2701613006/diff/1/chromecast/public/media/aud... File chromecast/public/media/audio_device_ids.h (right): https://codereview.chromium.org/2701613006/diff/1/chromecast/public/media/aud... chromecast/public/media/audio_device_ids.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/17 06:11:25, kmackay wrote: > 2017 Done.
gfhuang@chromium.org changed reviewers: + gfhuang@chromium.org
https://codereview.chromium.org/2701613006/diff/1/chromecast/public/media/aud... File chromecast/public/media/audio_device_ids.cc (right): https://codereview.chromium.org/2701613006/diff/1/chromecast/public/media/aud... chromecast/public/media/audio_device_ids.cc:13: const char kSystemAudioDeviceId[] = "assistant-system"; why change from enum to string? we never put any .cc file under public/ interface. https://codereview.chromium.org/2701613006/diff/1/chromecast/public/media/med... File chromecast/public/media/media_pipeline_device_params.h (right): https://codereview.chromium.org/2701613006/diff/1/chromecast/public/media/med... chromecast/public/media/media_pipeline_device_params.h:69: const std::string name; This public change will break OEM cast updates and requires a full system update. Please either find a different design, or if necessary, call for a full system update agreement with leads.
bshaya@google.com changed reviewers: + wzhong@chromium.org
https://codereview.chromium.org/2701613006/diff/1/chromecast/public/media/aud... File chromecast/public/media/audio_device_ids.cc (right): https://codereview.chromium.org/2701613006/diff/1/chromecast/public/media/aud... chromecast/public/media/audio_device_ids.cc:13: const char kSystemAudioDeviceId[] = "assistant-system"; On 2017/02/17 18:27:21, gfhuang wrote: > why change from enum to string? > we never put any .cc file under public/ interface. +kmackay https://codereview.chromium.org/2701613006/diff/1/chromecast/public/media/med... File chromecast/public/media/media_pipeline_device_params.h (right): https://codereview.chromium.org/2701613006/diff/1/chromecast/public/media/med... chromecast/public/media/media_pipeline_device_params.h:69: const std::string name; On 2017/02/17 18:27:21, gfhuang wrote: > This public change will break OEM cast updates and requires a full system > update. > > Please either find a different design, > or if necessary, call for a full system update agreement with leads. Yes. wzhong supports adding this to 1.23. Vendors should not need to change any code as a result.
The intention is to bring all necessary API changes to support new audio/assistant pipeline refactoring in 1.13 as 1.13 needs a system update already.
https://codereview.chromium.org/2701613006/diff/1/chromecast/public/media/med... File chromecast/public/media/media_pipeline_device_params.h (right): https://codereview.chromium.org/2701613006/diff/1/chromecast/public/media/med... chromecast/public/media/media_pipeline_device_params.h:69: const std::string name; On 2017/02/17 18:27:21, gfhuang wrote: > This public change will break OEM cast updates and requires a full system > update. > > Please either find a different design, > or if necessary, call for a full system update agreement with leads. Why does it require a system update? It should be ABI compatible.
https://codereview.chromium.org/2701613006/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (right): https://codereview.chromium.org/2701613006/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:888: bool filter_frames = true; On 2017/02/17 18:26:53, bshaya wrote: > On 2017/02/17 06:11:25, kmackay wrote: > > The design for mixing seems a bit clunky. I'd rather have something like: > > * Have a FIlterGroup class that holds state for each filter group. > > StreamMixerAlsa will have a vector of these. > > * When iterating through the active inputs, loop through the FilterGroups > > calling AddActiveInput() until one of them returns true (ie, the FilterGroup > > determines which inputs it cares about, based on the name). > > * Once all inputs have been iterated through, call Execute() or something on > > every FilterGroup. This will mix all of the input that that group accepted to > a > > buffer that is internal to the FilterGroup, and then run the filter on it (if > > any). Execute() returns false if there is no output from that filter group. > > * If no filter groups have output, push silence. If only 1 filter group has > > output, push that group's buffer. Otherwise, mix the buffers of all filter > > groups that have output together, and push the result. > > > > Hmm, how about that, and: > * Have an InputGroup class that contains a list of inputs, a filter, filter > state, and necessary buffers. > * When an InputQueue is added, it is added to the appropriate group directly > (similar to the Alsa implementation). > * Call InputGroup->Update to check for ChunkSize and blocked Primary inputs. > * Execute and mix as you suggested. > > > Then it's easy to also do volume control at the same time. > At the same time as what? If you mean stream volume control, I'd rather not, > since that either a) adds an additional level of mixing (streams->groups->filter > groups->everything) or requires duplicate filters (since we have more groups > than filter types). > > I guess the ideal would be for the architecture to be completely modular > (InputGroup implements InputQueue so that they can be fed into each other, and > you just call GetInterleavedData from the final InputGroup when you want data. > That might be too much re-designing considering we want to remove all of this... Your suggestions sound good. I meant for the per-stream-type volume control that I am planning to implement. I want to be able to implement that cleanly here in case we aren't able to get the ALSA plugins working quickly enough.
https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:855: bool is_silence = true; Do you need thi is_silence? Just use active_inputs.empty() below. https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:881: bool filter_frames = true; filter_frames doesn't seem to change. https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:882: if (is_silence) { move this block up after if (!active_inputs.empty()) {}? https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.h (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa.h:288: std::unique_ptr<AudioFilterInterface> pre_loopback_filter_[kNumFilterGroups]; Since there's no post_loopback_filter_ anymore, just call this output_filter or playout_filter? https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/pi... File chromecast/media/cma/pipeline/load_type.h (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/pi... chromecast/media/cma/pipeline/load_type.h:15: kLoadTypeLiveSource, Not sure what "Live" means. What about RtcSource? Add comments here to explain the difference load types. https://codereview.chromium.org/2701613006/diff/20001/chromecast/public/media... File chromecast/public/media/audio_device_ids.h (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/public/media... chromecast/public/media/audio_device_ids.h:16: const int kDefaultAudioStreamType = 0; do we still need the int types? https://codereview.chromium.org/2701613006/diff/20001/chromecast/public/media... File chromecast/public/media/media_pipeline_device_params.h (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/public/media... chromecast/public/media/media_pipeline_device_params.h:69: const std::string name; why not just call this device_id to be consistent with chromium device_id.
https://codereview.chromium.org/2701613006/diff/20001/chromecast/public/media... File chromecast/public/media/media_pipeline_device_params.h (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/public/media... chromecast/public/media/media_pipeline_device_params.h:69: const std::string name; This should be last, after task_runner, to preserve ABI compatibility.
https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:183: dest[i] += source[i]; No clipping or saturation? https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa_input.h (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa_input.h:55: std::string name); const std::string& https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.cc (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.cc:68: std::string name, ditto. https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.h (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.h:94: std::string name, ditto. https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/pi... File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/pi... chromecast/media/cma/pipeline/media_pipeline_impl.cc:50: base::TimeDelta::FromMilliseconds(60)); Nah... Those are not POD and should be banned. https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/servic... File chromecast/media/service/cast_renderer.cc (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/servic... chromecast/media/service/cast_renderer.cc:88: load_type = kLoadTypeLiveSource; Why are those 2 streams live? I thought live is only for streams like calling, BT audio and v2mirroring, etc.
https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/pi... File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/pi... chromecast/media/cma/pipeline/media_pipeline_impl.cc:50: base::TimeDelta::FromMilliseconds(60)); On 2017/02/21 16:03:46, wzhong wrote: > Nah... Those are not POD and should be banned. These are OK now as long as they are constexpr
https://codereview.chromium.org/2701613006/diff/20001/chromecast/public/media... File chromecast/public/media/media_pipeline_device_params.h (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/public/media... chromecast/public/media/media_pipeline_device_params.h:69: const std::string name; On 2017/02/19 23:09:09, kmackay wrote: > This should be last, after task_runner, to preserve ABI compatibility. I don't think it will preserve ABI compatibility since the object size is changed. And, why do we change from enum to string? as mentioned in the other comments, public/ used to contain pure .h without .cc
https://codereview.chromium.org/2701613006/diff/20001/chromecast/public/media... File chromecast/public/media/media_pipeline_device_params.h (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/public/media... chromecast/public/media/media_pipeline_device_params.h:69: const std::string name; On 2017/02/21 20:06:31, gfhuang wrote: > On 2017/02/19 23:09:09, kmackay wrote: > > This should be last, after task_runner, to preserve ABI compatibility. > > I don't think it will preserve ABI compatibility since the object size is > changed. > > And, why do we change from enum to string? as mentioned in the other comments, > public/ used to contain pure .h without .cc Object size doesn't matter, since we don't pass it by value.
https://codereview.chromium.org/2701613006/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (right): https://codereview.chromium.org/2701613006/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:888: bool filter_frames = true; On 2017/02/17 19:42:42, kmackay wrote: > On 2017/02/17 18:26:53, bshaya wrote: > > On 2017/02/17 06:11:25, kmackay wrote: > > > The design for mixing seems a bit clunky. I'd rather have something like: > > > * Have a FIlterGroup class that holds state for each filter group. > > > StreamMixerAlsa will have a vector of these. > > > * When iterating through the active inputs, loop through the FilterGroups > > > calling AddActiveInput() until one of them returns true (ie, the FilterGroup > > > determines which inputs it cares about, based on the name). > > > * Once all inputs have been iterated through, call Execute() or something > on > > > every FilterGroup. This will mix all of the input that that group accepted > to > > a > > > buffer that is internal to the FilterGroup, and then run the filter on it > (if > > > any). Execute() returns false if there is no output from that filter group. > > > * If no filter groups have output, push silence. If only 1 filter group has > > > output, push that group's buffer. Otherwise, mix the buffers of all filter > > > groups that have output together, and push the result. > > > > > > > Hmm, how about that, and: > > * Have an InputGroup class that contains a list of inputs, a filter, filter > > state, and necessary buffers. > > * When an InputQueue is added, it is added to the appropriate group directly > > (similar to the Alsa implementation). > > * Call InputGroup->Update to check for ChunkSize and blocked Primary inputs. > > * Execute and mix as you suggested. > > > > > Then it's easy to also do volume control at the same time. > > At the same time as what? If you mean stream volume control, I'd rather not, > > since that either a) adds an additional level of mixing > (streams->groups->filter > > groups->everything) or requires duplicate filters (since we have more groups > > than filter types). > > > > I guess the ideal would be for the architecture to be completely modular > > (InputGroup implements InputQueue so that they can be fed into each other, and > > you just call GetInterleavedData from the final InputGroup when you want data. > > That might be too much re-designing considering we want to remove all of > this... > > Your suggestions sound good. I meant for the per-stream-type volume control that > I am planning to implement. I want to be able to implement that cleanly here in > case we aren't able to get the ALSA plugins working quickly enough. Done. https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:183: dest[i] += source[i]; On 2017/02/21 16:03:46, wzhong wrote: > No clipping or saturation? Done. https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:855: bool is_silence = true; On 2017/02/18 00:17:08, tianyuwang1 wrote: > Do you need thi is_silence? Just use active_inputs.empty() below. Outdated https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:881: bool filter_frames = true; On 2017/02/18 00:17:08, tianyuwang1 wrote: > filter_frames doesn't seem to change. Done. https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:882: if (is_silence) { On 2017/02/18 00:17:08, tianyuwang1 wrote: > move this block up after if (!active_inputs.empty()) {}? Done. https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.h (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa.h:288: std::unique_ptr<AudioFilterInterface> pre_loopback_filter_[kNumFilterGroups]; On 2017/02/18 00:17:08, tianyuwang1 wrote: > Since there's no post_loopback_filter_ anymore, just call this output_filter or > playout_filter? Done. https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa_input.h (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa_input.h:55: std::string name); On 2017/02/21 16:03:46, wzhong wrote: > const std::string& Done. https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.cc (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.cc:68: std::string name, On 2017/02/21 16:03:46, wzhong wrote: > ditto. Done. https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.h (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.h:94: std::string name, On 2017/02/21 16:03:46, wzhong wrote: > ditto. Done. https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/pi... File chromecast/media/cma/pipeline/load_type.h (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/pi... chromecast/media/cma/pipeline/load_type.h:15: kLoadTypeLiveSource, On 2017/02/18 00:17:08, tianyuwang1 wrote: > Not sure what "Live" means. What about RtcSource? Add comments here to explain > the difference load types. Done. https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/pi... File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/cma/pi... chromecast/media/cma/pipeline/media_pipeline_impl.cc:50: base::TimeDelta::FromMilliseconds(60)); On 2017/02/21 16:08:47, kmackay wrote: > On 2017/02/21 16:03:46, wzhong wrote: > > Nah... Those are not POD and should be banned. > > These are OK now as long as they are constexpr Acknowledged. https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/servic... File chromecast/media/service/cast_renderer.cc (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/media/servic... chromecast/media/service/cast_renderer.cc:88: load_type = kLoadTypeLiveSource; On 2017/02/21 16:03:46, wzhong wrote: > Why are those 2 streams live? > > I thought live is only for streams like calling, BT audio and v2mirroring, etc. Live is for any stream that benefits from minimum latency and handles its own buffering (if necessary). https://codereview.chromium.org/2701613006/diff/20001/chromecast/public/media... File chromecast/public/media/audio_device_ids.h (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/public/media... chromecast/public/media/audio_device_ids.h:16: const int kDefaultAudioStreamType = 0; On 2017/02/18 00:17:09, tianyuwang1 wrote: > do we still need the int types? +kmackay The int types are currently used for volume control. The DeviceId's don't have a 1:1 correspondence with StreamType
bshaya@google.com changed reviewers: + jyw@chromium.org
https://codereview.chromium.org/2701613006/diff/60001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/filter_group.h (right): https://codereview.chromium.org/2701613006/diff/60001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/filter_group.h:65: ssize_t bytes_per_output_format_sample_; Initialize POD to default value here or in constructor. https://codereview.chromium.org/2701613006/diff/60001/chromecast/public/media... File chromecast/public/media/media_pipeline_device_params.h (right): https://codereview.chromium.org/2701613006/diff/60001/chromecast/public/media... chromecast/public/media/media_pipeline_device_params.h:60: std::string name_in, const &
bshaya@google.com changed reviewers: + alokp@chromium.org
https://codereview.chromium.org/2701613006/diff/60001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/filter_group.h (right): https://codereview.chromium.org/2701613006/diff/60001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/filter_group.h:65: ssize_t bytes_per_output_format_sample_; On 2017/02/22 16:13:01, wzhong wrote: > Initialize POD to default value here or in constructor. Done. https://codereview.chromium.org/2701613006/diff/60001/chromecast/public/media... File chromecast/public/media/media_pipeline_device_params.h (right): https://codereview.chromium.org/2701613006/diff/60001/chromecast/public/media... chromecast/public/media/media_pipeline_device_params.h:60: std::string name_in, On 2017/02/22 16:13:01, wzhong wrote: > const & Done.
The CQ bit was checked by bshaya@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by bshaya@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
https://codereview.chromium.org/2701613006/diff/100001/chromecast/media/cma/b... File chromecast/media/cma/backend/alsa/filter_group.cc (right): https://codereview.chromium.org/2701613006/diff/100001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/filter_group.cc:52: if (silence_frames_filtered_ < silence_frames_to_filter) { Initialize silence_frames_filtered_ to 0 in ctor. Otherwise a negative number could result in lots of filtering for zero-input at the beginning.
https://codereview.chromium.org/2701613006/diff/100001/chromecast/media/cma/b... File chromecast/media/cma/backend/alsa/filter_group.cc (right): https://codereview.chromium.org/2701613006/diff/100001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/filter_group.cc:52: if (silence_frames_filtered_ < silence_frames_to_filter) { On 2017/02/23 05:38:58, wzhong wrote: > Initialize silence_frames_filtered_ to 0 in ctor. Otherwise a negative number > could result in lots of filtering for zero-input at the beginning. Done.
https://codereview.chromium.org/2701613006/diff/20001/chromecast/public/media... File chromecast/public/media/audio_device_ids.h (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/public/media... chromecast/public/media/audio_device_ids.h:16: const int kDefaultAudioStreamType = 0; On 2017/02/21 23:30:14, bshaya wrote: > On 2017/02/18 00:17:09, tianyuwang1 wrote: > > do we still need the int types? > > +kmackay > The int types are currently used for volume control. The DeviceId's don't have a > 1:1 correspondence with StreamType We can get rid of the int type. Just use string is enough. Don't need to do the conversion.
https://codereview.chromium.org/2701613006/diff/20001/chromecast/public/media... File chromecast/public/media/audio_device_ids.h (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/public/media... chromecast/public/media/audio_device_ids.h:16: const int kDefaultAudioStreamType = 0; On 2017/02/23 20:25:37, tianyuwang1 wrote: > On 2017/02/21 23:30:14, bshaya wrote: > > On 2017/02/18 00:17:09, tianyuwang1 wrote: > > > do we still need the int types? > > > > +kmackay > > The int types are currently used for volume control. The DeviceId's don't have > a > > 1:1 correspondence with StreamType > > We can get rid of the int type. Just use string is enough. Don't need to do > the conversion. Agree, see https://eureka-internal-review.git.corp.google.com/#/c/65141/ where I did the same thing
https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/base/... File chromecast/media/base/audio_device_ids.h (right): https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/base/... chromecast/media/base/audio_device_ids.h:14: extern const char kSystemAudioDeviceId[]; Still not sure that 'system' is the correct name https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... File chromecast/media/cma/backend/alsa/filter_group.h (right): https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/filter_group.h:7: <memory> <vector> <stdint.h> https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/filter_group.h:29: // |filter_type| is passed to AudioFilterFactory to create an AudioFilter. I guess technically we could determine which filter group an input belongs to when the input is created (since the device id isn't going to change). So we could store a pointer to the filter group in the StreamMixerAlsaInputImpl itself, and then when adding active inputs, just add it to the stored filter group rather than iterating through all the groups. https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/filter_group.h:76: const FilterGroup& operator=(const FilterGroup&) = delete; DISALLOW_COPY_AND_ASSIGN is still preferred (?) Or did they change the style guide? https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (right): https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:173: int32_t SaturateSum(int32_t a, int32_t b) { base/numerics/saturated_arithmetic.h https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/p... File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/p... chromecast/media/cma/pipeline/media_pipeline_impl.cc:47: const base::TimeDelta kLowBufferThresholdCommunication( constexpr https://codereview.chromium.org/2701613006/diff/120001/chromecast/public/medi... File chromecast/public/media/media_pipeline_device_params.h (right): https://codereview.chromium.org/2701613006/diff/120001/chromecast/public/medi... chromecast/public/media/media_pipeline_device_params.h:71: const std::string name; let's rename to 'device_id' as others suggested
https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (right): https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:282: AudioFilterFactory::MEDIA_AUDIO_FILTER)); I don't think this is the right thing to do. These code means name (or device_id) is no longer an opaque id through the public API boundary. Can you use AudioContentType in Ken's CL? https://eureka-internal-review.git.corp.google.com/#/c/65141/
https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (right): https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:282: AudioFilterFactory::MEDIA_AUDIO_FILTER)); On 2017/02/23 21:24:56, gfhuang wrote: > I don't think this is the right thing to do. > > These code means name (or device_id) is no longer an opaque id through the > public API boundary. > > Can you use AudioContentType in Ken's CL? > https://eureka-internal-review.git.corp.google.com/#/c/65141/ I think this is OK. It's only temporary (as I understand it); and, this is our code anyway (so I figure it's OK to use the name field if we want to). In the short term we want to have streams with the same volume control behaviour (eg content_type), but different filters. This won't be necessary in the long term, I think (at least for this use case).
On 2017/02/23 21:30:36, kmackay wrote: > https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... > File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (right): > > https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... > chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:282: > AudioFilterFactory::MEDIA_AUDIO_FILTER)); > On 2017/02/23 21:24:56, gfhuang wrote: > > I don't think this is the right thing to do. > > > > These code means name (or device_id) is no longer an opaque id through the > > public API boundary. > > > > Can you use AudioContentType in Ken's CL? > > https://eureka-internal-review.git.corp.google.com/#/c/65141/ > > I think this is OK. It's only temporary (as I understand it); and, this is our > code anyway (so I figure it's OK to use the name field if we want to). > > In the short term we want to have streams with the same volume control behaviour > (eg content_type), but different filters. This won't be necessary in the long > term, I think (at least for this use case). Ok, I don't know this is temporary. If that's the case, please add a comment note saying this is a temp hack, and open a bug to remove this later.
https://codereview.chromium.org/2701613006/diff/20001/chromecast/public/media... File chromecast/public/media/audio_device_ids.h (right): https://codereview.chromium.org/2701613006/diff/20001/chromecast/public/media... chromecast/public/media/audio_device_ids.h:16: const int kDefaultAudioStreamType = 0; On 2017/02/23 20:32:00, kmackay wrote: > On 2017/02/23 20:25:37, tianyuwang1 wrote: > > On 2017/02/21 23:30:14, bshaya wrote: > > > On 2017/02/18 00:17:09, tianyuwang1 wrote: > > > > do we still need the int types? > > > > > > +kmackay > > > The int types are currently used for volume control. The DeviceId's don't > have > > a > > > 1:1 correspondence with StreamType > > > > We can get rid of the int type. Just use string is enough. Don't need to do > > the conversion. > > Agree, see https://eureka-internal-review.git.corp.google.com/#/c/65141/ where I > did the same thing I'm leaving this for now, since Ken is already removing the int types in his CL. https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/base/... File chromecast/media/base/audio_device_ids.h (right): https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/base/... chromecast/media/base/audio_device_ids.h:14: extern const char kSystemAudioDeviceId[]; On 2017/02/23 20:52:56, kmackay wrote: > Still not sure that 'system' is the correct name Done. https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... File chromecast/media/cma/backend/alsa/filter_group.h (right): https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/filter_group.h:7: On 2017/02/23 20:52:56, kmackay wrote: > <memory> > <vector> > <stdint.h> Done. https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/filter_group.h:29: // |filter_type| is passed to AudioFilterFactory to create an AudioFilter. On 2017/02/23 20:52:56, kmackay wrote: > I guess technically we could determine which filter group an input belongs to > when the input is created (since the device id isn't going to change). So we > could store a pointer to the filter group in the StreamMixerAlsaInputImpl > itself, and then when adding active inputs, just add it to the stored filter > group rather than iterating through all the groups. Done. https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/filter_group.h:76: const FilterGroup& operator=(const FilterGroup&) = delete; On 2017/02/23 20:52:56, kmackay wrote: > DISALLOW_COPY_AND_ASSIGN is still preferred (?) Or did they change the style > guide? The Google Style Guide only mentions = delete. The Chromium Style Guide is silent on the matter. https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (right): https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:173: int32_t SaturateSum(int32_t a, int32_t b) { On 2017/02/23 20:52:56, kmackay wrote: > base/numerics/saturated_arithmetic.h Done. https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:282: AudioFilterFactory::MEDIA_AUDIO_FILTER)); On 2017/02/23 21:30:36, kmackay wrote: > On 2017/02/23 21:24:56, gfhuang wrote: > > I don't think this is the right thing to do. > > > > These code means name (or device_id) is no longer an opaque id through the > > public API boundary. > > > > Can you use AudioContentType in Ken's CL? > > https://eureka-internal-review.git.corp.google.com/#/c/65141/ > > I think this is OK. It's only temporary (as I understand it); and, this is our > code anyway (so I figure it's OK to use the name field if we want to). > > In the short term we want to have streams with the same volume control behaviour > (eg content_type), but different filters. This won't be necessary in the long > term, I think (at least for this use case). The long term probably still has this. For instance, a podcast app should use the TTS filter and the media volume. https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/p... File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/p... chromecast/media/cma/pipeline/media_pipeline_impl.cc:47: const base::TimeDelta kLowBufferThresholdCommunication( On 2017/02/23 20:52:56, kmackay wrote: > constexpr Done. https://codereview.chromium.org/2701613006/diff/120001/chromecast/public/medi... File chromecast/public/media/media_pipeline_device_params.h (right): https://codereview.chromium.org/2701613006/diff/120001/chromecast/public/medi... chromecast/public/media/media_pipeline_device_params.h:71: const std::string name; On 2017/02/23 20:52:56, kmackay wrote: > let's rename to 'device_id' as others suggested Done.
halliwell@chromium.org changed reviewers: + halliwell@chromium.org
https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... File chromecast/media/cma/backend/alsa/filter_group.h (right): https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/filter_group.h:76: const FilterGroup& operator=(const FilterGroup&) = delete; On 2017/02/24 00:17:07, bshaya wrote: > On 2017/02/23 20:52:56, kmackay wrote: > > DISALLOW_COPY_AND_ASSIGN is still preferred (?) Or did they change the style > > guide? > > The Google Style Guide only mentions = delete. > The Chromium Style Guide is silent on the matter. Search the chromium-dev mailing list, there was a discussion there.
https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (right): https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:282: AudioFilterFactory::MEDIA_AUDIO_FILTER)); On 2017/02/24 00:17:08, bshaya wrote: > On 2017/02/23 21:30:36, kmackay wrote: > > On 2017/02/23 21:24:56, gfhuang wrote: > > > I don't think this is the right thing to do. > > > > > > These code means name (or device_id) is no longer an opaque id through the > > > public API boundary. > > > > > > Can you use AudioContentType in Ken's CL? > > > https://eureka-internal-review.git.corp.google.com/#/c/65141/ > > > > I think this is OK. It's only temporary (as I understand it); and, this is our > > code anyway (so I figure it's OK to use the name field if we want to). > > > > In the short term we want to have streams with the same volume control > behaviour > > (eg content_type), but different filters. This won't be necessary in the long > > term, I think (at least for this use case). > > The long term probably still has this. For instance, a podcast app should use > the TTS filter and the media volume. We haven't planned any way to distinguish content from apps. I don't think it is really feasible to require different filter behaviour for streams with the same volume control behaviour; the complexity of implementation goes way up. For example if we were using ALSA plugins we would need <num volume control types>*<num filter types> different pipelines to handle all possible cases. So I'd prefer not to make this a long-term thing.
https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/base/... File chromecast/media/base/audio_device_ids.cc (right): https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/base/... chromecast/media/base/audio_device_ids.cc:13: const char kSystemAudioDeviceId[] = "assistant-system"; missing in .h file? https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/cma/b... File chromecast/media/cma/backend/alsa/filter_group.cc (right): https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/filter_group.cc:40: active_inputs_.push_back(input); Don't add to active_inputs_ here https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/cma/b... File chromecast/media/cma/backend/alsa/filter_group.h (right): https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/filter_group.h:44: // an entry in |input_types_|. Comment is out of date https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/cma/b... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (right): https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:654: LOG(INFO) << __FUNCTION__; Necessary logging? https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:665: bool found_filter_group = false; Move down into the case statement? (you'll need braces) https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/cma/p... File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/cma/p... chromecast/media/cma/pipeline/media_pipeline_impl.cc:43: const base::TimeDelta kHighBufferThresholdMediaSource( constexpr here too
https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (right): https://codereview.chromium.org/2701613006/diff/120001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:282: AudioFilterFactory::MEDIA_AUDIO_FILTER)); On 2017/02/24 17:24:57, kmackay wrote: > On 2017/02/24 00:17:08, bshaya wrote: > > On 2017/02/23 21:30:36, kmackay wrote: > > > On 2017/02/23 21:24:56, gfhuang wrote: > > > > I don't think this is the right thing to do. > > > > > > > > These code means name (or device_id) is no longer an opaque id through the > > > > public API boundary. > > > > > > > > Can you use AudioContentType in Ken's CL? > > > > https://eureka-internal-review.git.corp.google.com/#/c/65141/ > > > > > > I think this is OK. It's only temporary (as I understand it); and, this is > our > > > code anyway (so I figure it's OK to use the name field if we want to). > > > > > > In the short term we want to have streams with the same volume control > > behaviour > > > (eg content_type), but different filters. This won't be necessary in the > long > > > term, I think (at least for this use case). > > > > The long term probably still has this. For instance, a podcast app should use > > the TTS filter and the media volume. > > We haven't planned any way to distinguish content from apps. I don't think it is > really feasible to require different filter behaviour for streams with the same > volume control behaviour; the complexity of implementation goes way up. For > example if we were using ALSA plugins we would need <num volume control > types>*<num filter types> different pipelines to handle all possible cases. So > I'd prefer not to make this a long-term thing. Done. https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/base/... File chromecast/media/base/audio_device_ids.cc (right): https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/base/... chromecast/media/base/audio_device_ids.cc:13: const char kSystemAudioDeviceId[] = "assistant-system"; On 2017/02/24 18:03:55, kmackay wrote: > missing in .h file? Done. https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/cma/b... File chromecast/media/cma/backend/alsa/filter_group.cc (right): https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/filter_group.cc:40: active_inputs_.push_back(input); On 2017/02/24 18:03:55, kmackay wrote: > Don't add to active_inputs_ here Done. https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/cma/b... File chromecast/media/cma/backend/alsa/filter_group.h (right): https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/filter_group.h:44: // an entry in |input_types_|. On 2017/02/24 18:03:55, kmackay wrote: > Comment is out of date Done. https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/cma/b... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (right): https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:654: LOG(INFO) << __FUNCTION__; On 2017/02/24 18:03:55, kmackay wrote: > Necessary logging? Done. https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/cma/b... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:665: bool found_filter_group = false; On 2017/02/24 18:03:55, kmackay wrote: > Move down into the case statement? (you'll need braces) Done. https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/cma/p... File chromecast/media/cma/pipeline/media_pipeline_impl.cc (right): https://codereview.chromium.org/2701613006/diff/160001/chromecast/media/cma/p... chromecast/media/cma/pipeline/media_pipeline_impl.cc:43: const base::TimeDelta kHighBufferThresholdMediaSource( On 2017/02/24 18:03:55, kmackay wrote: > constexpr here too Done.
lgtm
lgtm
The CQ bit was checked by bshaya@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2701613006/diff/180001/chromecast/media/base/... File chromecast/media/base/audio_device_ids.h (right): https://codereview.chromium.org/2701613006/diff/180001/chromecast/media/base/... chromecast/media/base/audio_device_ids.h:22: #endif // CHROMECAST_PUBLIC_MEDIA_AUDIO_DEVICE_IDS_H_ nit, doesn't match top
https://codereview.chromium.org/2701613006/diff/180001/chromecast/media/base/... File chromecast/media/base/audio_device_ids.h (right): https://codereview.chromium.org/2701613006/diff/180001/chromecast/media/base/... chromecast/media/base/audio_device_ids.h:22: #endif // CHROMECAST_PUBLIC_MEDIA_AUDIO_DEVICE_IDS_H_ On 2017/02/27 21:18:35, halliwell wrote: > nit, doesn't match top Done.
The CQ bit was checked by bshaya@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by bshaya@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by bshaya@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by bshaya@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bshaya@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from wzhong@chromium.org, halliwell@chromium.org, kmackay@chromium.org Link to the patchset: https://codereview.chromium.org/2701613006/#ps260001 (title: "Fix unittests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1488262761833220,
"parent_rev": "6b6094a419b2d7e45a7ec9dccda1cd5398090c91", "commit_rev":
"e975664dab5f430b66817238898839b333822ac3"}
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Process streams with different post-processing. Adds |name| to stream_mixer_alsa_input_impl. stream_mixer_alsa can then mix streams of the same time separately, and apply different post-processing to each. Also reduce buffering of system streams from 300ms to 60ms. BUG=internal b/34817384 TEST=manual Change-Id: Ieb91e419d7d34c374d83941c02f2546696b4f489 ========== to ========== [Chromecast] Process streams with different post-processing. Adds |name| to stream_mixer_alsa_input_impl. stream_mixer_alsa can then mix streams of the same time separately, and apply different post-processing to each. Also reduce buffering of system streams from 300ms to 60ms. BUG=internal b/34817384 TEST=manual Change-Id: Ieb91e419d7d34c374d83941c02f2546696b4f489 Review-Url: https://codereview.chromium.org/2701613006 Cr-Commit-Position: refs/heads/master@{#453522} Committed: https://chromium.googlesource.com/chromium/src/+/e975664dab5f430b668172388988... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/e975664dab5f430b668172388988...
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/2722673004/ by bshaya@google.com. The reason for reverting is: Breaks some chromecast builds due to missing include for <string> in media_pipeline_device_params.h. |
