Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2073223003:
[Chromecast] Use perceptually linear scaling for stream volumes (Closed)
|
Created:
4 years, 6 months ago by jyw Modified:
4 years, 6 months ago CC:
chromium-reviews, alokp+watch_chromium.org, lcwu+watch_chromium.org, halliwell+watch_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Chromecast] Use perceptually linear scaling for stream volumes
SetVolumeMultiplier(0.5) should give audio that sounds half as loud as
SetVolumeMultiplier(1.0).
BUG=internal b/29253848
TEST=manual
Committed: https://crrev.com/42e51772feb06a971921e4cfcd89ba7661cdf22a
Cr-Commit-Position: refs/heads/master@{#401149}
Patch Set 1 #
Total comments: 2
Patch Set 2 : remove last layer of clamping #Messages
Total messages: 18 (6 generated)
Description was changed from ========== [Chromecast] Use perceptually linear scaling for stream volumes SetVolumeMultiplier(0.5) should give audio that sounds half as loud as SetVolumeMultiplier(1.0). BUG=internal b/29253848 TEST=manual ========== to ========== [Chromecast] Use perceptually linear scaling for stream volumes SetVolumeMultiplier(0.5) should give audio that sounds half as loud as SetVolumeMultiplier(1.0). BUG=internal b/29253848 TEST=manual ==========
jyw@chromium.org changed reviewers: + alokp@chromium.org, igorc@chromium.org, kmackay@chromium.org
https://codereview.chromium.org/2073223003/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.cc (right): https://codereview.chromium.org/2073223003/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.cc:484: return std::max(0.0f, std::min(powf(volume_level, 0.5f * log2(10.0f)), 1.0f)); Is it necessary to clamp the result? If 0 <= a <= 1, then 0 <= a^n <= 1 anyway.
On 2016/06/17 21:59:54, kmackay wrote: > https://codereview.chromium.org/2073223003/diff/1/chromecast/media/cma/backen... > File chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.cc (right): > > https://codereview.chromium.org/2073223003/diff/1/chromecast/media/cma/backen... > chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.cc:484: return > std::max(0.0f, std::min(powf(volume_level, 0.5f * log2(10.0f)), 1.0f)); > Is it necessary to clamp the result? If 0 <= a <= 1, then 0 <= a^n <= 1 anyway. Indeed, but I wasn't 100% sure that powf(1.0, 0.5f * log2(10.0f)) wouldn't be 1.0f + epsilon (for epsilon > 0).
On 2016/06/17 22:03:44, jyw wrote: > On 2016/06/17 21:59:54, kmackay wrote: > > > https://codereview.chromium.org/2073223003/diff/1/chromecast/media/cma/backen... > > File chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.cc > (right): > > > > > https://codereview.chromium.org/2073223003/diff/1/chromecast/media/cma/backen... > > chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.cc:484: return > > std::max(0.0f, std::min(powf(volume_level, 0.5f * log2(10.0f)), 1.0f)); > > Is it necessary to clamp the result? If 0 <= a <= 1, then 0 <= a^n <= 1 > anyway. > > Indeed, but I wasn't 100% sure that powf(1.0, 0.5f * log2(10.0f)) wouldn't be > 1.0f + epsilon (for epsilon > 0). http://en.cppreference.com/w/c/numeric/math/pow "pow(±0, exponent), where exponent is positive non-integer or a positive even integer, returns +0" "pow(+1, exponent) returns 1 for any exponent, even when exponent is NaN"
https://codereview.chromium.org/2073223003/diff/1/chromecast/media/cma/backen... File chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.cc (right): https://codereview.chromium.org/2073223003/diff/1/chromecast/media/cma/backen... chromecast/media/cma/backend/alsa/stream_mixer_alsa_input_impl.cc:484: return std::max(0.0f, std::min(powf(volume_level, 0.5f * log2(10.0f)), 1.0f)); On 2016/06/17 21:59:54, kmackay wrote: > Is it necessary to clamp the result? If 0 <= a <= 1, then 0 <= a^n <= 1 anyway. Done.
lgtm
kmackay@chromium.org changed reviewers: + halliwell@chromium.org
On 2016/06/20 16:54:11, kmackay wrote: lgtm
The CQ bit was checked by jyw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2073223003/20001
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Use perceptually linear scaling for stream volumes SetVolumeMultiplier(0.5) should give audio that sounds half as loud as SetVolumeMultiplier(1.0). BUG=internal b/29253848 TEST=manual ========== to ========== [Chromecast] Use perceptually linear scaling for stream volumes SetVolumeMultiplier(0.5) should give audio that sounds half as loud as SetVolumeMultiplier(1.0). BUG=internal b/29253848 TEST=manual ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Use perceptually linear scaling for stream volumes SetVolumeMultiplier(0.5) should give audio that sounds half as loud as SetVolumeMultiplier(1.0). BUG=internal b/29253848 TEST=manual ========== to ========== [Chromecast] Use perceptually linear scaling for stream volumes SetVolumeMultiplier(0.5) should give audio that sounds half as loud as SetVolumeMultiplier(1.0). BUG=internal b/29253848 TEST=manual Committed: https://crrev.com/42e51772feb06a971921e4cfcd89ba7661cdf22a Cr-Commit-Position: refs/heads/master@{#401149} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/42e51772feb06a971921e4cfcd89ba7661cdf22a Cr-Commit-Position: refs/heads/master@{#401149}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2100733002/ by kmackay@chromium.org. The reason for reverting is: I think we should actually leave the per-stream volume multiplier as linear, and leave it up to whatever sets that multiplier to map correctly from user input to the desired volume curve. Otherwise, we tie the mixer implementation to a specific volume curve, which is looking less and less plausible (particularly since it is used with OEM devices that have an unknown volume curve).. |
