Attachment 372353 Details for Bug 197358 – Patch

[patch] Patch

bug-197358-20190618192838.patch (text/plain), 8.42 KB, created by Charlie Turner on 2019-06-18 11:28:39 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Charlie Turner

Created: 2019-06-18 11:28:39 PDT

Size: 8.42 KB

patch

obsolete

>Subversion Revision: 246536
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 681ccb6215f852bfb2669b8667e190a29895113c..e72310b504f1e2a93c9d0b1da1dee8bf1521b0ce 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,36 @@
>+2019-06-18  Charlie Turner  <cturner@igalia.com>
>+
>+        [GStreamer] Inconsistent volume scaling
>+        https://bugs.webkit.org/show_bug.cgi?id=197358
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        We were inconsistently using cubic volume scaling, which was
>+        casuing inappropriate volume changes at non-deterministic times.
>+
>+        Covered by existing volume tests.
>+
>+        * html/HTMLMediaElement.cpp:
>+        (WebCore::HTMLMediaElement::setVolume): Platform volume
>+        configuration methods are not used for anything, which extends to
>+        the m_volumeInitialized member.
>+        * html/HTMLMediaElement.h: Ditto.
>+        * platform/graphics/MediaPlayer.h: Ditto
>+        (WebCore::MediaPlayerClient::mediaPlayerPlay):
>+        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
>+        (WebCore::MediaPlayerPrivateGStreamer::paused const): Change the
>+        paused status logging to LOG level, since it's very spammy.
>+        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
>+        (WebCore::MediaPlayerPrivateGStreamerBase::setVolume): Switch to
>+        linear scaling. PulseAudio already applies cubic scaling on
>+        software volumes.
>+        (WebCore::MediaPlayerPrivateGStreamerBase::volume const): Ditto.
>+        (WebCore::MediaPlayerPrivateGStreamerBase::notifyPlayerOfVolumeChange): Ditto.
>+        (WebCore::MediaPlayerPrivateGStreamerBase::setStreamVolumeElement):
>+        Ditto, and also be consistent about using the right scaling
>+        function. This also removes more of the platform volume
>+        configuration as above.
>+
> 2019-06-18  Zan Dobersek  <zdobersek@igalia.com>
> 
>         [WebGL] Extensions3DOpenGLES::bindVertexArrayOES() should allow zero array object
>diff --git a/Source/WebCore/html/HTMLMediaElement.cpp b/Source/WebCore/html/HTMLMediaElement.cpp
>index 24c687e5b21c75ac4cb0a3b772a18c72dedc22a1..9959492fb799aa82c255fa23cb700f2e71e66793 100644
>--- a/Source/WebCore/html/HTMLMediaElement.cpp
>+++ b/Source/WebCore/html/HTMLMediaElement.cpp
>@@ -3746,7 +3746,6 @@ ExceptionOr<void> HTMLMediaElement::setVolume(double volume)
>         removeBehaviorRestrictionsAfterFirstUserGesture(MediaElementSession::AllRestrictions & ~MediaElementSession::RequireUserGestureToControlControlsManager);
> 
>     m_volume = volume;
>-    m_volumeInitialized = true;
>     updateVolume();
>     scheduleEvent(eventNames().volumechangeEvent);
> 
>@@ -7084,11 +7083,6 @@ void HTMLMediaElement::mediaPlayerPlay()
>     play();
> }
> 
>-bool HTMLMediaElement::mediaPlayerPlatformVolumeConfigurationRequired() const
>-{
>-    return !m_volumeInitialized;
>-}
>-
> bool HTMLMediaElement::mediaPlayerIsPaused() const
> {
>     return paused();
>diff --git a/Source/WebCore/html/HTMLMediaElement.h b/Source/WebCore/html/HTMLMediaElement.h
>index 2f7047efa3874467041ac89806090417f1781b4a..1ab45a2b313a70ab5eccc141055a73ec866cf073 100644
>--- a/Source/WebCore/html/HTMLMediaElement.h
>+++ b/Source/WebCore/html/HTMLMediaElement.h
>@@ -716,7 +716,6 @@ private:
>     void mediaPlayerSetSize(const IntSize&) override;
>     void mediaPlayerPause() override;
>     void mediaPlayerPlay() override;
>-    bool mediaPlayerPlatformVolumeConfigurationRequired() const override;
>     bool mediaPlayerIsPaused() const override;
>     bool mediaPlayerIsLooping() const override;
>     CachedResourceLoader* mediaPlayerCachedResourceLoader() override;
>@@ -1004,7 +1003,6 @@ private:
>     SeekType m_pendingSeekType { NoSeek };
> 
>     double m_volume { 1 };
>-    bool m_volumeInitialized { false };
>     MediaTime m_lastSeekTime;
>     
>     MonotonicTime m_previousProgressTime { MonotonicTime::infinity() };
>diff --git a/Source/WebCore/platform/graphics/MediaPlayer.h b/Source/WebCore/platform/graphics/MediaPlayer.h
>index e9df94587877f2032b17040368a4ddaf251159ef..ea93217b954e16b4a08289811d6de8ab936d1365 100644
>--- a/Source/WebCore/platform/graphics/MediaPlayer.h
>+++ b/Source/WebCore/platform/graphics/MediaPlayer.h
>@@ -189,7 +189,6 @@ public:
>     virtual void mediaPlayerSetSize(const IntSize&) { }
>     virtual void mediaPlayerPause() { }
>     virtual void mediaPlayerPlay() { }
>-    virtual bool mediaPlayerPlatformVolumeConfigurationRequired() const { return false; }
>     virtual bool mediaPlayerIsPaused() const { return true; }
>     virtual bool mediaPlayerIsLooping() const { return false; }
>     virtual CachedResourceLoader* mediaPlayerCachedResourceLoader() { return nullptr; }
>@@ -365,7 +364,6 @@ public:
> 
>     double volume() const;
>     void setVolume(double);
>-    bool platformVolumeConfigurationRequired() const { return client().mediaPlayerPlatformVolumeConfigurationRequired(); }
> 
>     bool muted() const;
>     void setMuted(bool);
>diff --git a/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp b/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
>index 0e0835b01f8acd9d03300520686f05a270e13b2d..e1d1b2debca8a51246ff98801e5c2d69f062d4b6 100644
>--- a/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
>+++ b/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
>@@ -663,7 +663,7 @@ bool MediaPlayerPrivateGStreamer::paused() const
>     GstState state;
>     gst_element_get_state(m_pipeline.get(), &state, nullptr, 0);
>     bool paused = state <= GST_STATE_PAUSED;
>-    GST_DEBUG_OBJECT(pipeline(), "Paused: %s", toString(paused).utf8().data());
>+    GST_LOG_OBJECT(pipeline(), "Paused: %s", toString(paused).utf8().data());
>     return paused;
> }
> 
>diff --git a/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp b/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp
>index 6ea36580f2d50e8b380f7d515c6a4017343aa8fd..e360e1186e236da9899ecfc4a0ebb58413ed5069 100644
>--- a/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp
>+++ b/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp
>@@ -573,7 +573,7 @@ void MediaPlayerPrivateGStreamerBase::setVolume(float volume)
>         return;
> 
>     GST_DEBUG_OBJECT(pipeline(), "Setting volume: %f", volume);
>-    gst_stream_volume_set_volume(m_volumeElement.get(), GST_STREAM_VOLUME_FORMAT_CUBIC, static_cast<double>(volume));
>+    gst_stream_volume_set_volume(m_volumeElement.get(), GST_STREAM_VOLUME_FORMAT_LINEAR, static_cast<double>(volume));
> }
> 
> float MediaPlayerPrivateGStreamerBase::volume() const
>@@ -581,7 +581,7 @@ float MediaPlayerPrivateGStreamerBase::volume() const
>     if (!m_volumeElement)
>         return 0;
> 
>-    return gst_stream_volume_get_volume(m_volumeElement.get(), GST_STREAM_VOLUME_FORMAT_CUBIC);
>+    return gst_stream_volume_get_volume(m_volumeElement.get(), GST_STREAM_VOLUME_FORMAT_LINEAR);
> }
> 
> 
>@@ -590,7 +590,7 @@ void MediaPlayerPrivateGStreamerBase::notifyPlayerOfVolumeChange()
>     if (!m_player || !m_volumeElement)
>         return;
>     double volume;
>-    volume = gst_stream_volume_get_volume(m_volumeElement.get(), GST_STREAM_VOLUME_FORMAT_CUBIC);
>+    volume = gst_stream_volume_get_volume(m_volumeElement.get(), GST_STREAM_VOLUME_FORMAT_LINEAR);
>     // get_volume() can return values superior to 1.0 if the user
>     // applies software user gain via third party application (GNOME
>     // volume control for instance).
>@@ -1228,13 +1228,8 @@ void MediaPlayerPrivateGStreamerBase::setStreamVolumeElement(GstStreamVolume* vo
>     ASSERT(!m_volumeElement);
>     m_volumeElement = volume;
> 
>-    // We don't set the initial volume because we trust the sink to keep it for us. See
>-    // https://bugs.webkit.org/show_bug.cgi?id=118974 for more information.
>-    if (!m_player->platformVolumeConfigurationRequired()) {
>-        GST_DEBUG_OBJECT(pipeline(), "Setting stream volume to %f", m_player->volume());
>-        g_object_set(m_volumeElement.get(), "volume", m_player->volume(), nullptr);
>-    } else
>-        GST_DEBUG_OBJECT(pipeline(), "Not setting stream volume, trusting system one");
>+    GST_DEBUG_OBJECT(pipeline(), "Setting stream volume to %f", m_player->volume());
>+    gst_stream_volume_set_volume(m_volumeElement.get(), GST_STREAM_VOLUME_FORMAT_LINEAR, static_cast<double>(m_player->volume()));
> 
>     GST_DEBUG_OBJECT(pipeline(), "Setting stream muted %s", toString(m_player->muted()).utf8().data());
>     g_object_set(m_volumeElement.get(), "mute", m_player->muted(), nullptr);

Actions: View | Formatted Diff | Diff

Attachments on bug 197358: 372339 | 372353 | 372365 | 372555 | 372741