199559 2019-07-07 02:58:49 -0700 [GStreamer] Ensure that “volume = foo; assert(volume == foo)” always succeeds 2019-07-10 08:42:41 -0700 1 1 1 Unclassified WebKit Media WebKit Nightly Build Unspecified Unspecified NEW https://bugs.webkit.org/show_bug.cgi?id=199505 P2 Normal --- 1 cturner webkit-unassigned calvaris pnormand oldest_to_newest 1550698 0 cturner 2019-07-07 02:58:49 -0700 See https://bugs.webkit.org/show_bug.cgi?id=199505. It would be nice if for any media element E, E.volume = x does not undergo and transformations that cannot be reversed when reading the volume back. Currently in the pulseaudio backend, this is not the case due to the following PA code, // converting to PA integer volume code uint32_t pa_sw_volume_from_linear(double v) { if (v <= 0.0) return PA_VOLUME_MUTED; /* * We use a cubic mapping here, as suggested and discussed here: * * http://www.robotplanet.dk/audio/audio_gui_design/ * http://lists.linuxaudio.org/pipermail/linux-audio-dev/2009-May/thread.html#23151 * * We make sure that the conversion to linear and back yields the * same volume value! That's why we need the lround() below! */ // clamps between 0 and UINT32_MAX/2 return (uint32_t) PA_CLAMP_VOLUME((uint64_t) lround(cbrt(v) * PA_VOLUME_NORM)); } // converting back to double from PA integer volume code double pa_sw_volume_to_linear(uint32_t v) { double f; if (v <= PA_VOLUME_MUTED) return 0.0; if (v == PA_VOLUME_NORM) return 1.0; f = ((double) v / PA_VOLUME_NORM); return (f*f*f); } The lround there means it's lossy. I'm not sure how to fix that when the PA devs do not consider it a bug. 1550699 1 cturner 2019-07-07 02:59:52 -0700 The upstream PA bug is at https://gitlab.freedesktop.org/pulseaudio/pulseaudio/issues/697 1550764 2 calvaris 2019-07-08 00:09:22 -0700 If PA devs don't fix this, which is unlikely, I think we should cache the volume we set somehow and use this unless we get a volume change signal from GStreamer, which would invalidate that cache. 1551269 3 pnormand 2019-07-09 09:56:08 -0700 In MediaPlayerPrivateGStreamerBase::notifyPlayerOfVolumeChange() why do we cast the value to float? m_player->volumeChanged() expects a double. 1551272 4 pnormand 2019-07-09 09:58:17 -0700 Also, I wonder if we should override MediaPlayerPrivate::setVolumeDouble(double) instead of MediaPlayer::setVolume(float). 1551667 5 cturner 2019-07-10 08:42:41 -0700 (In reply to Philippe Normand from comment #3) > In MediaPlayerPrivateGStreamerBase::notifyPlayerOfVolumeChange() why do we > cast the value to float? m_player->volumeChanged() expects a double. It's old code, perhaps the MediaPlayer class changed since it was written. Regardless, it doesn't matter since the error from PA has already crept in before we start casting. Same goes for the setVolumeDouble, PA behavior still introduces this issue.