Bug 168898 – [WebRTC] Fix remote audio rendering

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[WebRTC] Fix remote audio rendering

[Jer Noble]

Created attachment 302825 [details]
Patch

[youenn fablet]

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.

[Jer Noble]

(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.

[Jer Noble]

Created attachment 302844 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 302844 [details]
Patch for landing

Clearing flags on attachment: 302844

Committed r213080: <http://trac.webkit.org/changeset/213080>