[WebRTC] Fix remote audio rendering
Created attachment 302825 [details] Patch
Comment on attachment 302825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302825&action=review > Source/WebCore/ChangeLog:8 > + Test: webrtc/audio-peer-connection-webaudio.html Great test! > Source/WebCore/platform/mediastream/mac/RealtimeIncomingAudioSource.cpp:95 > + m_audioSourceProvider->prepare(&m_streamFormat); OnData is called on one of webrtc threads (probably the network one). audioSourceProvider getter method will probably be called on the main thread. Shouldn't we execute this code in the main thread? > LayoutTests/webrtc/audio-peer-connection-webaudio.html:11 > + internals.useMockRTCPeerConnectionFactory("TwoRealPeerConnections"); Could you move this within the test block, just before the call to createConnections? That may help if we add future tests in this file. Btw, if one doesn't like async_test, promise_test can be used instead. One would need to return the getUserMedia promise and create a new promise inside it. The small benefit here would be the automatic handling of gum promise being rejected. Not probably worth changing the test here.
(In reply to comment #2) > Comment on attachment 302825 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=302825&action=review > > > Source/WebCore/ChangeLog:8 > > + Test: webrtc/audio-peer-connection-webaudio.html > > Great test! > > > Source/WebCore/platform/mediastream/mac/RealtimeIncomingAudioSource.cpp:95 > > + m_audioSourceProvider->prepare(&m_streamFormat); > > OnData is called on one of webrtc threads (probably the network one). > audioSourceProvider getter method will probably be called on the main thread. > Shouldn't we execute this code in the main thread? The getter will probably be called on the WebAudio thread, but WebAudioSourceProviderAVFObjC has a lock which protects against prepare() and provideInput() being called simultaneously, so this should be fine. (The provideInput() side is a try-lock.) > > LayoutTests/webrtc/audio-peer-connection-webaudio.html:11 > > + internals.useMockRTCPeerConnectionFactory("TwoRealPeerConnections"); > > Could you move this within the test block, just before the call to > createConnections? > That may help if we add future tests in this file. Sure. > Btw, if one doesn't like async_test, promise_test can be used instead. > One would need to return the getUserMedia promise and create a new promise > inside it. > The small benefit here would be the automatic handling of gum promise being > rejected. > Not probably worth changing the test here. Yeah, if I were to re-write this test, I'd probably use the promise_test path.
Created attachment 302844 [details] Patch for landing
Comment on attachment 302844 [details] Patch for landing Clearing flags on attachment: 302844 Committed r213080: <http://trac.webkit.org/changeset/213080>