Bug 199559 - [GStreamer] Ensure that “volume = foo; assert(volume == foo)” always succeeds
Summary: [GStreamer] Ensure that “volume = foo; assert(volume == foo)” always succeeds
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-07-07 02:58 PDT by Charlie Turner
Modified: 2019-07-10 08:42 PDT (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Charlie Turner 2019-07-07 02:58:49 PDT
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.
Comment 1 Charlie Turner 2019-07-07 02:59:52 PDT
The upstream PA bug is at https://gitlab.freedesktop.org/pulseaudio/pulseaudio/issues/697
Comment 2 Xabier Rodríguez Calvar 2019-07-08 00:09:22 PDT
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.
Comment 3 Philippe Normand 2019-07-09 09:56:08 PDT
In MediaPlayerPrivateGStreamerBase::notifyPlayerOfVolumeChange() why do we cast the value to float? m_player->volumeChanged() expects a double.
Comment 4 Philippe Normand 2019-07-09 09:58:17 PDT
Also, I wonder if we should override MediaPlayerPrivate::setVolumeDouble(double) instead of MediaPlayer::setVolume(float).
Comment 5 Charlie Turner 2019-07-10 08:42:41 PDT
(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.