Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2341783004:
[chromecast] Slew stream volume changes in StreamMixerAlsa. (Closed)
Description[chromecast] Slew stream volume changes in StreamMixerAlsa.
BUG=internal 29459582
BUG=internal 31162371
TEST=cast_alsa_cma_backend_unittests --gtest_filter=StreamMixerAlsaTest.*
exercise stream volume API; listen for smooth volume transitions
Committed: https://crrev.com/146e6b47128a16c54772ad0bbda59cefbda1bb2a
Cr-Commit-Position: refs/heads/master@{#419540}
Patch Set 1 #
Total comments: 21
Patch Set 2 : address comments #
Total comments: 10
Patch Set 3 : Slew stream volume changes in StreamMixerAlsa. #
Total comments: 6
Patch Set 4 : Slew stream volume changes in StreamMixerAlsa. #Patch Set 5 : fix unittests #
Total comments: 3
Patch Set 6 : rebase #Patch Set 7 : Initial commit #Patch Set 8 : [Chromecast] Make SetPostProcessorConfig() a method. #Messages
Total messages: 37 (10 generated)
jyw@chromium.org changed reviewers: + bshaya@google.com, kmackay@chromium.org, tianyuwang@google.com, wzhong@chromium.org
Tested by mirroring and by listening during ducking.
https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/BUILD.gn (right): https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/BUILD.gn:104: "//base", also //media for FMAC/FMUL https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/slew_volume.cc (right): https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/slew_volume.cc:42: bool SlewVolume::ProcessFMAC(const float* src, int frames, float* dest) { Do we need to return bool? It always returns true https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/slew_volume.cc:47: } if (current_volume_ == volume_scale_) { ::media::vector_math::FMAC(src, volume_scale_, frames, dest); return; } (could also check for volume_scale_ == 0.0/1.0) https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/slew_volume.cc:73: int unaligned_frames = prefer to just do the simple naive method here rather than trying to align and use FMAC. The common case can use FMAC above. https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/slew_volume.cc:102: } if (current_volume_ == volume_scale_) { ::media::vector_math::FMUL(src, volume_scale_, frames * 2, dest); return; } (could also check for volume_scale_ == 0.0/1.0) https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/slew_volume.cc:106: (*data) *= current_volume_; It might be nice to have a helper function/macro for these 5 lines, since they are repeated 3 times. https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/slew_volume.h (right): https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/slew_volume.h:7: #ifndef CHROMECAST_INTERNAL_CHIRP_WAVES_SLEW_VOLUME_H_ Change header guard to reflect new location https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/slew_volume.h:12: #include "base/macros.h" not actually used? Need DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/slew_volume.h:34: double volume_scale_ = .1; why 0.1 here? seems like both should be set to 1.0 https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (left): https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:833: float volume_scalar = input->volume_multiplier(); volume_multipler() method not needed anymore https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.cc (right): https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.cc:92: slew_volume_.SetSampleRate(input_samples_per_second_); this doesn't seem right. It is initialized correctly in Initialize()
https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/BUILD.gn (right): https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/BUILD.gn:104: "//base", On 2016/09/14 20:48:14, kmackay wrote: > also //media for FMAC/FMUL Done. https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/slew_volume.cc (right): https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/slew_volume.cc:42: bool SlewVolume::ProcessFMAC(const float* src, int frames, float* dest) { On 2016/09/14 20:48:15, kmackay wrote: > Do we need to return bool? It always returns true Done. https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/slew_volume.cc:47: } On 2016/09/14 20:48:15, kmackay wrote: > if (current_volume_ == volume_scale_) { > ::media::vector_math::FMAC(src, volume_scale_, frames, dest); > return; > } > > (could also check for volume_scale_ == 0.0/1.0) Done. https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/slew_volume.cc:73: int unaligned_frames = On 2016/09/14 20:48:14, kmackay wrote: > prefer to just do the simple naive method here rather than trying to align and > use FMAC. The common case can use FMAC above. Done. https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/slew_volume.cc:102: } On 2016/09/14 20:48:14, kmackay wrote: > if (current_volume_ == volume_scale_) { > ::media::vector_math::FMUL(src, volume_scale_, frames * 2, dest); > return; > } > > (could also check for volume_scale_ == 0.0/1.0) can't use FMUL, which requires floats. data is an int32_t array. https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/slew_volume.h (right): https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/slew_volume.h:7: #ifndef CHROMECAST_INTERNAL_CHIRP_WAVES_SLEW_VOLUME_H_ On 2016/09/14 20:48:15, kmackay wrote: > Change header guard to reflect new location Done. https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/slew_volume.h:12: #include "base/macros.h" On 2016/09/14 20:48:15, kmackay wrote: > not actually used? Need DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/slew_volume.h:34: double volume_scale_ = .1; On 2016/09/14 20:48:15, kmackay wrote: > why 0.1 here? seems like both should be set to 1.0 Done. https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (left): https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:833: float volume_scalar = input->volume_multiplier(); On 2016/09/14 20:48:15, kmackay wrote: > volume_multipler() method not needed anymore Done. https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.cc (right): https://codereview.chromium.org/2341783004/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.cc:92: slew_volume_.SetSampleRate(input_samples_per_second_); On 2016/09/14 20:48:15, kmackay wrote: > this doesn't seem right. It is initialized correctly in Initialize() Done.
https://codereview.chromium.org/2341783004/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/slew_volume.cc (right): https://codereview.chromium.org/2341783004/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/slew_volume.cc:18: const int kRequiredAlignment = 16; not used? https://codereview.chromium.org/2341783004/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/slew_volume.cc:53: } Include alignment requirements in the header comments for this function. https://codereview.chromium.org/2341783004/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/slew_volume.cc:86: bool SlewVolume::ProcessInterleaved(int32_t* data, int frames) { this one also only ever returns true. https://codereview.chromium.org/2341783004/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/slew_volume.cc:98: data[i] *= volume_scale_; *= current_volume_, for consistency https://codereview.chromium.org/2341783004/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (right): https://codereview.chromium.org/2341783004/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:832: input->VolumeScaleAccumulate(temp_->channel(c), chunk_size, does this work properly? seems like it will slew one channel, and then continue slewing the next channel, and so on (so channel 0 will slew from eg. 0.0 -> 0.05, and channel 1 will slew from 0.05 -> 0.1).
https://codereview.chromium.org/2341783004/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/slew_volume.cc (right): https://codereview.chromium.org/2341783004/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/slew_volume.cc:18: const int kRequiredAlignment = 16; On 2016/09/15 01:51:36, kmackay wrote: > not used? Done. https://codereview.chromium.org/2341783004/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/slew_volume.cc:53: } On 2016/09/15 01:51:36, kmackay wrote: > Include alignment requirements in the header comments for this function. Done. https://codereview.chromium.org/2341783004/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/slew_volume.cc:86: bool SlewVolume::ProcessInterleaved(int32_t* data, int frames) { On 2016/09/15 01:51:36, kmackay wrote: > this one also only ever returns true. There's an internal callsite for ProcessInterleaved() where it makes sense to return a bool. https://codereview.chromium.org/2341783004/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/slew_volume.cc:98: data[i] *= volume_scale_; On 2016/09/15 01:51:36, kmackay wrote: > *= current_volume_, for consistency Done. https://codereview.chromium.org/2341783004/diff/20001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc (right): https://codereview.chromium.org/2341783004/diff/20001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/stream_mixer_alsa.cc:832: input->VolumeScaleAccumulate(temp_->channel(c), chunk_size, On 2016/09/15 01:51:36, kmackay wrote: > does this work properly? seems like it will slew one channel, and then continue > slewing the next channel, and so on (so channel 0 will slew from eg. 0.0 -> > 0.05, and channel 1 will slew from 0.05 -> 0.1). Done.
https://codereview.chromium.org/2341783004/diff/40001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/slew_volume.cc (right): https://codereview.chromium.org/2341783004/diff/40001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/slew_volume.cc:32: max_slew_up_ = (1000.0 / (max_slew_time_up_ms_ * sample_rate)); Nit: () is not needed. https://codereview.chromium.org/2341783004/diff/40001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/slew_volume.cc:44: DCHECK(src); DCHECK alignment. https://codereview.chromium.org/2341783004/diff/40001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/slew_volume.cc:58: if (current_volume_ == 0.0) { NO need to clear dest?
https://codereview.chromium.org/2341783004/diff/40001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/slew_volume.cc (right): https://codereview.chromium.org/2341783004/diff/40001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/slew_volume.cc:32: max_slew_up_ = (1000.0 / (max_slew_time_up_ms_ * sample_rate)); On 2016/09/16 01:03:46, wzhong wrote: > Nit: () is not needed. Done. https://codereview.chromium.org/2341783004/diff/40001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/slew_volume.cc:44: DCHECK(src); On 2016/09/16 01:03:47, wzhong wrote: > DCHECK alignment. Done. https://codereview.chromium.org/2341783004/diff/40001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/slew_volume.cc:58: if (current_volume_ == 0.0) { On 2016/09/16 01:03:47, wzhong wrote: > NO need to clear dest? FMAC does dest[i] += src[i] * current_volume_; so yes, no need to clear dest.
lgtm
The unit tests are failing for me on x86 with this CL.
I think kmackay@ is working on a change to send more gradual multipliers for ducking to cast_shell. If we have a slew stream volume change here do we still need the other change? It seems weird to have two different places that has similar slew function for volume change.
Slew is needed for volume changes triggered by all types of input source. Also it should apply to all stream types.
I think ducking affects a subset of the audio types, which should be covered by Slew? On Fri, Sep 16, 2016 at 5:41 PM <wzhong@chromium.org> wrote: > Slew is needed for volume changes triggered by all types of input source. > Also > it should apply to all stream types. > > https://codereview.chromium.org/2341783004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm Yes. I believe slew here on is more generic and should be enough to handle all cases including ducking.
> Yes. I believe slew here on is more generic and should be enough to handle all > cases including ducking. Yes, this covers all stream volume changes including ducking. However, it isn't sufficient to achieve the desired fading behaviour for ducking/unducking; the fading should be slower, and needs to control the master volume and the stream volumes in concert.
Description was changed from
==========
[chromecast] Slew stream volume changes in StreamMixerAlsa.
BUG=internal 29459582
BUG=internal 31162371
TEST=exercise stream volume API; listen for smooth volume transitions
==========
to
==========
[chromecast] Slew stream volume changes in StreamMixerAlsa.
BUG=internal 29459582
BUG=internal 31162371
TEST=cast_alsa_cma_backend_unittests --gtest_filter=StreamMixerAlsaTest.*
exercise stream volume API; listen for smooth volume transitions
==========
On 2016/09/16 23:30:40, kmackay wrote: > The unit tests are failing for me on x86 with this CL. Done
lgtm
lgtm https://codereview.chromium.org/2341783004/diff/80001/chromecast/media/cma/ba... File chromecast/media/cma/backend/alsa/slew_volume.cc (right): https://codereview.chromium.org/2341783004/diff/80001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/slew_volume.cc:30: // Slew rate should be 1 / (slew_time * sample_rate) Please add period. https://codereview.chromium.org/2341783004/diff/80001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/slew_volume.cc:68: } else if (current_volume_ < volume_scale_) { Nit: else is not needed. https://codereview.chromium.org/2341783004/diff/80001/chromecast/media/cma/ba... chromecast/media/cma/backend/alsa/slew_volume.cc:113: } else if (current_volume_ < volume_scale_) { Ditto.
The CQ bit was checked by jyw@chromium.org
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2016/09/19 17:05:35, commit-bot: I haz the power wrote: > No L-G-T-M from a valid reviewer yet. > CQ run can only be started by full committers or once the patch has > received an L-G-T-M from a full committer. > Even if an L-G-T-M may have been provided, it was from a non-committer, > _not_ a full super star committer. > See http://www.chromium.org/getting-involved/become-a-committer > Note that this has nothing to do with OWNERS files. rs lgtm
The CQ bit was checked by lcwu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jyw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wzhong@chromium.org, lcwu@chromium.org, kmackay@chromium.org, tianyuwang@google.com Link to the patchset: https://codereview.chromium.org/2341783004/#ps100001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
[chromecast] Slew stream volume changes in StreamMixerAlsa.
BUG=internal 29459582
BUG=internal 31162371
TEST=cast_alsa_cma_backend_unittests --gtest_filter=StreamMixerAlsaTest.*
exercise stream volume API; listen for smooth volume transitions
==========
to
==========
[chromecast] Slew stream volume changes in StreamMixerAlsa.
BUG=internal 29459582
BUG=internal 31162371
TEST=cast_alsa_cma_backend_unittests --gtest_filter=StreamMixerAlsaTest.*
exercise stream volume API; listen for smooth volume transitions
==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from
==========
[chromecast] Slew stream volume changes in StreamMixerAlsa.
BUG=internal 29459582
BUG=internal 31162371
TEST=cast_alsa_cma_backend_unittests --gtest_filter=StreamMixerAlsaTest.*
exercise stream volume API; listen for smooth volume transitions
==========
to
==========
[chromecast] Slew stream volume changes in StreamMixerAlsa.
BUG=internal 29459582
BUG=internal 31162371
TEST=cast_alsa_cma_backend_unittests --gtest_filter=StreamMixerAlsaTest.*
exercise stream volume API; listen for smooth volume transitions
Committed: https://crrev.com/146e6b47128a16c54772ad0bbda59cefbda1bb2a
Cr-Commit-Position: refs/heads/master@{#419540}
==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/146e6b47128a16c54772ad0bbda59cefbda1bb2a Cr-Commit-Position: refs/heads/master@{#419540} |
