Bug 186307 - Incoming G722 doesn't work
Summary: Incoming G722 doesn't work
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: Safari 11
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL: http://appr.tc/r/18999?arc=g722/8000&...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-05 08:24 PDT by Nikita Petrov
Modified: 2018-06-22 14:37 PDT (History)
7 users (show)

See Also:


Attachments
Patch (8.62 KB, patch)
2018-06-22 08:22 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.52 MB, application/zip)
2018-06-22 09:18 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Petrov 2018-06-05 08:24:47 PDT
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
Comment 1 Radar WebKit Bug Importer 2018-06-05 09:03:54 PDT
<rdar://problem/40809745>
Comment 2 youenn fablet 2018-06-22 08:22:26 PDT
Created attachment 343325 [details]
Patch
Comment 3 EWS Watchlist 2018-06-22 09:18:37 PDT
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
Comment 4 EWS Watchlist 2018-06-22 09:18:39 PDT
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
Comment 5 youenn fablet 2018-06-22 09:30:42 PDT
Comment on attachment 343325 [details]
Patch

Mac failure is unrelated
Comment 6 youenn fablet 2018-06-22 14:09:25 PDT
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 @@
>+2018-06-22  Youenn Fablet  <youenn@apple.com>
>+
>+        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  <darin@apple.com>
> 
>         [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 @@
>+2018-06-22  Youenn Fablet  <youenn@apple.com>
>+
>+        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  <david_fenton@apple.com>
> 
>         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(() => {
Comment 7 WebKit Commit Bot 2018-06-22 14:37:00 PDT
Comment on attachment 343325 [details]
Patch

Clearing flags on attachment: 343325

Committed r233100: <https://trac.webkit.org/changeset/233100>
Comment 8 WebKit Commit Bot 2018-06-22 14:37:01 PDT
All reviewed patches have been landed.  Closing bug.