WebAudioSourceProviderAVFObjC should not reconfigure for each data call
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 3 Jul 2017 21:01:00 +0000 (21:01 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 3 Jul 2017 21:01:00 +0000 (21:01 +0000)
https://bugs.webkit.org/show_bug.cgi?id=174101

Patch by Youenn Fablet <youenn@apple.com> on 2017-07-03
Reviewed by Eric Carlson.

Source/WebCore:

Covered by manual testing, in particular
https://webrtc.github.io/samples/src/content/peerconnection/webaudio-output/
and https://webrtc.github.io/samples/src/content/getusermedia/volume/.
Also improved LayoutTests web audio peer connection tests to make them more robust.

Before the patch, reconfiguration of the web audio provider was happening for every audioSamplesAvailable call.
It is now happening only when the format of the audio samples is changing.
Changed some member fields from uinque_ptr to optional as a minor improvement.

* platform/mediastream/mac/WebAudioSourceProviderAVFObjC.h:
* platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:
(WebCore::WebAudioSourceProviderAVFObjC::provideInput):
(WebCore::WebAudioSourceProviderAVFObjC::prepare):
(WebCore::WebAudioSourceProviderAVFObjC::unprepare):
(WebCore::WebAudioSourceProviderAVFObjC::audioSamplesAvailable):

LayoutTests:

* TestExpectations:
* webrtc/peer-connection-audio-mute2.html:
* webrtc/peer-connection-remote-audio-mute2.html:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@219090 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/TestExpectations
LayoutTests/webrtc/peer-connection-audio-mute2.html
LayoutTests/webrtc/peer-connection-remote-audio-mute2.html
Source/WebCore/ChangeLog
Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.h
Source/WebCore/platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm

index f8639ca..f699818 100644 (file)
@@ -1,3 +1,14 @@
+2017-07-03  Youenn Fablet  <youenn@apple.com>
+
+        WebAudioSourceProviderAVFObjC should not reconfigure for each data call
+        https://bugs.webkit.org/show_bug.cgi?id=174101
+
+        Reviewed by Eric Carlson.
+
+        * TestExpectations:
+        * webrtc/peer-connection-audio-mute2.html:
+        * webrtc/peer-connection-remote-audio-mute2.html:
+
 2017-07-03  Alex Christensen  <achristensen@webkit.org>
 
         Rebase test after r219024
index a94aeab..d83bd79 100644 (file)
@@ -774,8 +774,6 @@ webrtc/datachannel/multiple-connections.html [ Slow ]
 webrtc/negotiatedneeded-event-addStream.html [ Pass Crash ]
 webrtc/video-replace-track.html [ Pass Failure ]
 webrtc/video-getParameters.html [ Failure ]
-webrtc/peer-connection-audio-mute2.html [ Pass Failure ]
-webrtc/peer-connection-remote-audio-mute2.html [ Pass Failure ]
 webkit.org/b/170178 webrtc/video-replace-track-to-null.html [ Pass Failure ]
 fast/mediastream/RTCPeerConnection-closed-state.html [ Skip ]
 fast/mediastream/RTCPeerConnection-iceconnectionstatechange-event.html [ Skip ]
index 78db59f..9dda885 100644 (file)
@@ -2,7 +2,7 @@
 <html>
 <head>
     <meta charset="utf-8">
-    <title>Testing local audio capture playback causes "playing" event to fire</title>
+    <title>Testing web audio on peer connection audio tracks</title>
     <script src="../resources/testharness.js"></script>
     <script src="../resources/testharnessreport.js"></script>
 </head>
@@ -11,9 +11,6 @@
     <script>
     var context = new webkitAudioContext();
     promise_test((test) => {
-        if (window.testRunner)
-            testRunner.setUserMediaPermission(true);
-
         var localTrack;
         return navigator.mediaDevices.getUserMedia({audio: true}).then((localStream) => {
             localTrack = localStream.getAudioTracks()[0];
                     };
                 });
             }).then(() => {
-                return waitFor(500);
-            }).then(() => {
-                return analyseAudio(remoteStream, 500, context).then((results) => {
-                    assert_true(results.heardHum, "heard hum from remote enabled track");
-                });
+                return doHumAnalysis(remoteStream, true);
+            }).then((value) => {
+                assert_true(value, "heard hum from remote enabled track");
             }).then(() => {
                 localTrack.enabled = false;
-                return waitFor(500);
-            }).then(() => {
-                return analyseAudio(remoteStream, 500, context).then((results) => {
-                    assert_false(results.heardHum, "not heard hum from remote disabled track");
-                });
+                return doHumAnalysis(remoteStream, false);
+            }).then((value) => {
+                assert_true(value, "not heard hum from remote disabled track");
             }).then(() => {
                 localTrack.enabled = true;
-                return waitFor(500);
-            }).then(() => {
-                return analyseAudio(remoteStream, 500, context).then((results) => {
-                    assert_true(results.heardHum, "heard hum from remote reenabled track");
-                });
+                return doHumAnalysis(remoteStream, true);
+            }).then((value) => {
+                assert_true(value, "heard hum from remote reenabled track");
             }).then(() => {
                 return context.close();
             });
index 518ec19..1ba5d33 100644 (file)
@@ -2,7 +2,7 @@
 <html>
 <head>
     <meta charset="utf-8">
-    <title>Testing local audio capture playback causes "playing" event to fire</title>
+    <title>Testing web audio on peer connection audio tracks</title>
     <script src="../resources/testharness.js"></script>
     <script src="../resources/testharnessreport.js"></script>
 </head>
@@ -11,9 +11,6 @@
     <script>
     var context = new webkitAudioContext();
     promise_test((test) => {
-        if (window.testRunner)
-            testRunner.setUserMediaPermission(true);
-
         return navigator.mediaDevices.getUserMedia({audio: true}).then((localStream) => {
             var remoteTrack;
             var remoteStream;
                     };
                 });
             }).then(() => {
-                return analyseAudio(remoteStream, 500, context).then((results) => {
-                    assert_true(results.heardHum, "heard hum from remote enabled track");
-                });
+                return doHumAnalysis(remoteStream, true);
+            }).then((value) => {
+                assert_true(value, "heard hum from remote enabled track");
             }).then(() => {
                 remoteTrack.enabled = false;
-                return waitFor(100);
-            }).then(() => {
-                return analyseAudio(remoteStream, 500, context).then((results) => {
-                    assert_false(results.heardHum, "not heard hum from remote disabled track");
-                });
+                return doHumAnalysis(remoteStream, false);
+            }).then((value) => {
+                assert_true(value, "not heard hum from remote disabled track");
             }).then(() => {
                 remoteTrack.enabled = true;
-                return waitFor(100);
-            }).then(() => {
-                return analyseAudio(remoteStream, 500, context).then((results) => {
-                    assert_true(results.heardHum, "heard hum from remote reenabled track");
-                });
+                return doHumAnalysis(remoteStream, true);
+            }).then((value) => {
+                assert_true(value, "heard hum from remote reenabled track");
             }).then(() => {
                 return context.close();
             });
index 651b209..3f2679b 100644 (file)
@@ -1,3 +1,26 @@
+2017-07-03  Youenn Fablet  <youenn@apple.com>
+
+        WebAudioSourceProviderAVFObjC should not reconfigure for each data call
+        https://bugs.webkit.org/show_bug.cgi?id=174101
+
+        Reviewed by Eric Carlson.
+
+        Covered by manual testing, in particular
+        https://webrtc.github.io/samples/src/content/peerconnection/webaudio-output/
+        and https://webrtc.github.io/samples/src/content/getusermedia/volume/.
+        Also improved LayoutTests web audio peer connection tests to make them more robust.
+
+        Before the patch, reconfiguration of the web audio provider was happening for every audioSamplesAvailable call.
+        It is now happening only when the format of the audio samples is changing.
+        Changed some member fields from uinque_ptr to optional as a minor improvement.
+
+        * platform/mediastream/mac/WebAudioSourceProviderAVFObjC.h:
+        * platform/mediastream/mac/WebAudioSourceProviderAVFObjC.mm:
+        (WebCore::WebAudioSourceProviderAVFObjC::provideInput):
+        (WebCore::WebAudioSourceProviderAVFObjC::prepare):
+        (WebCore::WebAudioSourceProviderAVFObjC::unprepare):
+        (WebCore::WebAudioSourceProviderAVFObjC::audioSamplesAvailable):
+
 2017-06-30  Alex Christensen  <achristensen@webkit.org>
 
         Stop using dispatch_async in ResourceHandleCFURLConnectionDelegateWithOperationQueue
index 9f6a236..2899ff2 100644 (file)
@@ -27,6 +27,7 @@
 
 #if ENABLE(WEB_AUDIO) && ENABLE(MEDIA_STREAM)
 
+#include "CAAudioStreamDescription.h"
 #include "MediaStreamTrackPrivate.h"
 #include "WebAudioSourceProvider.h"
 #include <CoreAudio/CoreAudioTypes.h>
@@ -50,7 +51,7 @@ public:
     static Ref<WebAudioSourceProviderAVFObjC> create(MediaStreamTrackPrivate&);
     virtual ~WebAudioSourceProviderAVFObjC();
 
-    void prepare(const AudioStreamBasicDescription*);
+    void prepare(const AudioStreamBasicDescription&);
     void unprepare();
 
 private:
@@ -68,8 +69,8 @@ private:
     void trackEnabledChanged(MediaStreamTrackPrivate&) final { }
 
     size_t m_listBufferSize { 0 };
-    std::unique_ptr<CAAudioStreamDescription> m_inputDescription;
-    std::unique_ptr<CAAudioStreamDescription> m_outputDescription;
+    std::optional<CAAudioStreamDescription> m_inputDescription;
+    std::optional<CAAudioStreamDescription> m_outputDescription;
     RefPtr<AudioSampleDataSource> m_dataSource;
 
     uint64_t m_writeCount { 0 };
@@ -78,7 +79,6 @@ private:
     MediaStreamTrackPrivate* m_captureSource { nullptr };
     Lock m_mutex;
     bool m_connected { false };
-    AudioStreamBasicDescription m_streamFormat;
 };
 
 }
index f9eb276..7e516df 100644 (file)
@@ -79,7 +79,7 @@ void WebAudioSourceProviderAVFObjC::provideInput(AudioBus* bus, size_t framesToP
         return;
     }
 
-    WebAudioBufferList list { *m_outputDescription };
+    WebAudioBufferList list { m_outputDescription.value() };
     for (unsigned i = 0; i < bus->numberOfChannels(); ++i) {
         AudioChannel& channel = *bus->channel(i);
         if (i >= list.bufferCount()) {
@@ -115,15 +115,15 @@ void WebAudioSourceProviderAVFObjC::setClient(AudioSourceProviderClient* client)
     }
 }
 
-void WebAudioSourceProviderAVFObjC::prepare(const AudioStreamBasicDescription* format)
+void WebAudioSourceProviderAVFObjC::prepare(const AudioStreamBasicDescription& format)
 {
     std::lock_guard<Lock> lock(m_mutex);
 
     LOG(Media, "WebAudioSourceProviderAVFObjC::prepare(%p)", this);
 
-    m_inputDescription = std::make_unique<CAAudioStreamDescription>(*format);
-    int numberOfChannels = format->mChannelsPerFrame;
-    double sampleRate = format->mSampleRate;
+    m_inputDescription = CAAudioStreamDescription(format);
+    int numberOfChannels = format.mChannelsPerFrame;
+    double sampleRate = format.mSampleRate;
     ASSERT(sampleRate >= 0);
 
     const int bytesPerFloat = sizeof(Float32);
@@ -133,12 +133,12 @@ void WebAudioSourceProviderAVFObjC::prepare(const AudioStreamBasicDescription* f
     const bool isNonInterleaved = true;
     AudioStreamBasicDescription outputDescription { };
     FillOutASBDForLPCM(outputDescription, sampleRate, numberOfChannels, bitsPerByte * bytesPerFloat, bitsPerByte * bytesPerFloat, isFloat, isBigEndian, isNonInterleaved);
-    m_outputDescription = std::make_unique<CAAudioStreamDescription>(outputDescription);
+    m_outputDescription = CAAudioStreamDescription(outputDescription);
 
     if (!m_dataSource)
         m_dataSource = AudioSampleDataSource::create(kRingBufferDuration * sampleRate);
-    m_dataSource->setInputFormat(*m_inputDescription);
-    m_dataSource->setOutputFormat(*m_outputDescription);
+    m_dataSource->setInputFormat(m_inputDescription.value());
+    m_dataSource->setOutputFormat(m_outputDescription.value());
 
     callOnMainThread([protectedThis = makeRef(*this), numberOfChannels, sampleRate] {
         if (protectedThis->m_client)
@@ -150,8 +150,8 @@ void WebAudioSourceProviderAVFObjC::unprepare()
 {
     std::lock_guard<Lock> lock(m_mutex);
 
-    m_inputDescription = nullptr;
-    m_outputDescription = nullptr;
+    m_inputDescription = std::nullopt;
+    m_outputDescription = std::nullopt;
     m_dataSource = nullptr;
     m_listBufferSize = 0;
     if (m_captureSource) {
@@ -162,11 +162,10 @@ void WebAudioSourceProviderAVFObjC::unprepare()
 
 void WebAudioSourceProviderAVFObjC::audioSamplesAvailable(MediaStreamTrackPrivate&, const MediaTime&, const PlatformAudioData& data, const AudioStreamDescription& description, size_t frameCount)
 {
-    // FIXME: We should try to call prepare based on trackSettingsChanged callback.
     ASSERT(description.platformDescription().type == PlatformDescription::CAAudioStreamBasicType);
     auto& basicDescription = *WTF::get<const AudioStreamBasicDescription*>(description.platformDescription().description);
-    if (m_streamFormat != basicDescription)
-        prepare(&basicDescription);
+    if (!m_inputDescription || m_inputDescription->streamDescription() != basicDescription)
+        prepare(basicDescription);
 
     if (!m_dataSource)
         return;