Bug 186307 – Incoming G722 doesn't work

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[Nikita Petrov]

Safari is unable to render incoming G722 stream. To try it out go to 

http://appr.tc/r/18999?arc=g722/8000&asc=g722/8000


Safari-to-Safari: no audio at all
Safari-to-Chrome: Chrome is able to render incoming audio, Safari is unable to render it.

What is curious, from the stats and the state of the audio element, it seems that the data is flowing and audio timestamps are advancing. But no playback.

I'm happy to provide any further data if it can help you with the investigation

[Radar WebKit Bug Importer]

<rdar://problem/40809745>

[youenn fablet]

Created attachment 343325 [details]
Patch

[EWS Watchlist]

Comment on attachment 343325 [details]
Patch

Attachment 343325 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8290883

New failing tests:
performance-api/performance-observer-no-document-leak.html

[EWS Watchlist]

Created attachment 343330 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[youenn fablet]

Comment on attachment 343325 [details]
Patch

Mac failure is unrelated

[youenn fablet]

Comment on attachment 343325 [details]
Patch

>Subversion Revision: 233079
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index b59540b0f63a819a0bb6c6ddf4ed862d28b9ac5d..e33e37ef6a2593ab6db3a8c792578d043b85f08d 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,25 @@
>[phone]  Youenn Fablet  <[e-mail]>
>+
>+        Incoming G722 doesn't work
>+        https://bugs.webkit.org/show_bug.cgi?id=186307
>+        <rdar://problem/40809745>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        WebRTC backends usually does the following:
>+        - Initially call RealtimeIncomingAudioSource with 16KHz data
>+        - Switch to 48KHz when actual data is decoded.
>+        We added a check that was discarding any 16KHz data, but in case of G722, the data remains as 16KHz and is then never read.
>+        The solution is to remove the check that discards 16KHz information.
>+        We then need to fix a bug in AudioTrackPrivateMediaStreamCocoa that was preventing proper handling of change of audio data configuration.
>+
>+        Test: webrtc/audio-peer-connection-g722.html
>+
>+        * platform/mediastream/mac/AudioTrackPrivateMediaStreamCocoa.cpp:
>+        (WebCore::AudioTrackPrivateMediaStreamCocoa::audioSamplesAvailable):
>+        * platform/mediastream/mac/RealtimeIncomingAudioSourceCocoa.cpp:
>+        (WebCore::RealtimeIncomingAudioSourceCocoa::OnData):
>+
> 2018-06-20  Darin Adler  <[e-mail]>
> 
>         [Cocoa] Use the isDirectory: variants of NSURL methods more to eliminate unnecessary file system activity
>diff --git a/Source/WebCore/platform/mediastream/mac/AudioTrackPrivateMediaStreamCocoa.cpp b/Source/WebCore/platform/mediastream/mac/AudioTrackPrivateMediaStreamCocoa.cpp
>index cbf88a462bb43dc608297d14658ebf1ef960daf1..87a65f26b1378a8f4714626739bf01f0a0394209 100644
>--- a/Source/WebCore/platform/mediastream/mac/AudioTrackPrivateMediaStreamCocoa.cpp
>+++ b/Source/WebCore/platform/mediastream/mac/AudioTrackPrivateMediaStreamCocoa.cpp
>@@ -194,14 +194,19 @@ void AudioTrackPrivateMediaStreamCocoa::audioSamplesAvailable(const MediaTime& s
>         m_inputDescription = std::make_unique<CAAudioStreamDescription>(inputDescription);
>         m_outputDescription = std::make_unique<CAAudioStreamDescription>(outputDescription);
> 
>-        if (!m_dataSource)
>-            m_dataSource = AudioSampleDataSource::create(description.sampleRate() * 2);
>+        m_dataSource = AudioSampleDataSource::create(description.sampleRate() * 2);
> 
>         if (m_dataSource->setInputFormat(inputDescription) || m_dataSource->setOutputFormat(outputDescription)) {
>             AudioComponentInstanceDispose(remoteIOUnit);
>             return;
>         }
> 
>+        if (m_isPlaying && AudioOutputUnitStart(remoteIOUnit)) {
>+            AudioComponentInstanceDispose(remoteIOUnit);
>+            m_inputDescription = nullptr;
>+            return;
>+        }
>+
>         m_dataSource->setVolume(m_volume);
>         m_remoteIOUnit = remoteIOUnit;
>     }
>diff --git a/Source/WebCore/platform/mediastream/mac/RealtimeIncomingAudioSourceCocoa.cpp b/Source/WebCore/platform/mediastream/mac/RealtimeIncomingAudioSourceCocoa.cpp
>index 119c9403dca1ea1de3a9190b3beb93307f192e89..22fb12561e8b9170f2f0ac9c47dfedcaaae5cb87 100644
>--- a/Source/WebCore/platform/mediastream/mac/RealtimeIncomingAudioSourceCocoa.cpp
>+++ b/Source/WebCore/platform/mediastream/mac/RealtimeIncomingAudioSourceCocoa.cpp
>@@ -68,15 +68,6 @@ static inline AudioStreamBasicDescription streamDescription(size_t sampleRate, s
> 
> void RealtimeIncomingAudioSourceCocoa::OnData(const void* audioData, int bitsPerSample, int sampleRate, size_t numberOfChannels, size_t numberOfFrames)
> {
>-    // We may receive OnData calls with empty sound data (mono, samples equal to zero and sampleRate equal to 16000) when starting the call.
>-    // FIXME: For the moment we skip them, we should find a better solution at libwebrtc level to not be called until getting some real data.
>-    if (sampleRate == 16000 && numberOfChannels == 1)
>-        return;
>-
>-    ASSERT(bitsPerSample == 16);
>-    ASSERT(numberOfChannels == 1 || numberOfChannels == 2);
>-    ASSERT(sampleRate == 48000);
>-
>     CMTime startTime = CMTimeMake(m_numberOfFrames, sampleRate);
>     auto mediaTime = PAL::toMediaTime(startTime);
>     m_numberOfFrames += numberOfFrames;
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index e37a4b213631544b4f0887c057420be5e3051e86..898fbe548da186568f9af4726854c81c96b53b4c 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,15 @@
>[phone]  Youenn Fablet  <[e-mail]>
>+
>+        Incoming G722 doesn't work
>+        https://bugs.webkit.org/show_bug.cgi?id=186307
>+        <rdar://problem/40809745>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * webrtc/audio-peer-connection-g722-expected.txt: Added.
>+        * webrtc/audio-peer-connection-g722.html: Added.
>+        * webrtc/routines.js:
>+
> 2018-06-21  David Fenton  <[e-mail]>
> 
>         Skip imported/w3c/web-platform-tests/css/css-display/display-contents-first-letter-002.html.
>diff --git a/LayoutTests/webrtc/audio-peer-connection-g722-expected.txt b/LayoutTests/webrtc/audio-peer-connection-g722-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..f263245bf45bc8a5ea4f72dd1b4f0f282e1f9100
>--- /dev/null
>+++ b/LayoutTests/webrtc/audio-peer-connection-g722-expected.txt
>@@ -0,0 +1,3 @@
>+
>+PASS Basic G722 audio playback through a peer connection 
>+
>diff --git a/LayoutTests/webrtc/audio-peer-connection-g722.html b/LayoutTests/webrtc/audio-peer-connection-g722.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..78a6d80bb6865966cf3b4d881ec7f2ebafa07211
>--- /dev/null
>+++ b/LayoutTests/webrtc/audio-peer-connection-g722.html
>@@ -0,0 +1,68 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+    <meta charset="utf-8">
>+    <title>Testing local audio capture playback causes "playing" event to fire</title>
>+    <script src="../resources/testharness.js"></script>
>+    <script src="../resources/testharnessreport.js"></script>
>+    <script src ="routines.js"></script>
>+    <script>
>+var context = new webkitAudioContext();
>+var remoteStream;
>+
>+async function checkForHumBipBop(stream, previousResults, counter)
>+{
>+    if (!previousResults)
>+        previousResults = {
>+            heardHum : false,
>+            heardBip : false,
>+            heardBop : false
>+    };
>+    if (!counter)
>+        counter = 1;
>+    else if (++counter > 4)
>+        return Promise.resolve(false);
>+    results = await analyseAudio(stream, 1000, context);
>+    previousResults.heardHum |= results.heardHum;
>+    previousResults.heardBip |= results.heardBip;
>+    previousResults.heardBop |= results.heardBop;
>+    if (previousResults.heardHum && previousResults.heardBip && previousResults.heardBop)
>+        return Promise.resolve(true);
>+    var results = await checkForHumBipBop(stream, previousResults, counter);
>+    return results;
>+}
>+
>+function setCodec(sdp, codec)
>+{
>+    return sdp.split('\r\n').filter(line => {
>+        return line.indexOf('a=fmtp') === -1 && line.indexOf('a=rtcp-fb') === -1 && (line.indexOf('a=rtpmap') === -1 || line.indexOf(codec) !== -1);
>+    }).join('\r\n');
>+}
>+
>+promise_test(async (test) => {
>+    if (window.testRunner)
>+        testRunner.setUserMediaPermission(true);
>+
>+    const stream = await navigator.mediaDevices.getUserMedia({audio: true});
>+    const remoteStream = new Promise((resolve, reject) => {
>+        createConnections((firstConnection) => {
>+            firstConnection.addTrack(stream.getAudioTracks()[0], stream);
>+        }, (secondConnection) => {
>+            secondConnection.ontrack = (event) => { resolve(event.streams[0]); };
>+        }, { observeOffer : (offer) => {
>+            offer.sdp = setCodec(offer.sdp, "722");
>+            return offer;
>+        }
>+        });
>+        setTimeout(() => reject("Test timed out"), 5000);
>+    });
>+
>+    const results = await checkForHumBipBop(stream);
>+    assert_true(results, "heard hum bip bop");
>+    context.close();
>+}, "Basic G722 audio playback through a peer connection");
>+    </script>
>+</head>
>+<body>
>+</body>
>+</html>
>diff --git a/LayoutTests/webrtc/routines.js b/LayoutTests/webrtc/routines.js
>index 8d8cc6365be7a22b7c711d7f0ab425e2eda20519..aebe26fe67685b1cfe6bd637d72e99e6d74a9d5e 100644
>--- a/LayoutTests/webrtc/routines.js
>+++ b/LayoutTests/webrtc/routines.js
>@@ -31,8 +31,11 @@ function onCreateSessionDescriptionError(error)
> 
> function gotDescription1(desc, options)
> {
>-    if (options.observeOffer)
>-        options.observeOffer(desc);
>+    if (options.observeOffer) {
>+        const result = options.observeOffer(desc);
>+        if (result)
>+            desc = result;
>+    }
> 
>     localConnection.setLocalDescription(desc);
>     remoteConnection.setRemoteDescription(desc).then(() => {

[WebKit Commit Bot]

Comment on attachment 343325 [details]
Patch

Clearing flags on attachment: 343325

Committed r233100: <https://trac.webkit.org/changeset/233100>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.