Bug 167572 – [WebRTC] Add a libwebrtc AudioModule specific to WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Add a libwebrtc AudioModule specific to WebKit

[youenn fablet]

Created attachment 300086 [details]
Patch

[youenn fablet]

Created attachment 300171 [details]
Patch

[Alex Christensen]

Comment on attachment 300171 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=300171&action=review

Is there no better way than to do this than polling on a thread?  This code seems like it is specific to one particular configuration.

> Source/WebCore/ChangeLog:11
> +        Code inspired from Chrome AudioModule.

Can you give a link?

> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCAudioModule.h:43
> +    int16_t shouldNotBeCalled(int16_t value) const

This should be a template.  Not all values are 16-bit.

> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCAudioModule.h:59
> +    int64_t TimeUntilNextProcess() final { return 1000000; }

std::numeric_limits<int64_t>::max() ?

> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCUtils.cpp:100
> +    factoryAndThreads.factory = webrtc::CreatePeerConnectionFactory(factoryAndThreads.networkThread.get(), factoryAndThreads.networkThread.get(), factoryAndThreads.signalingThread.get(), factoryAndThreads.audioDeviceModule.get(), new webrtc::VideoToolboxVideoEncoderFactory() , new webrtc::VideoToolboxVideoDecoderFactory());

extra space before comma.
Do we really have to call new so often?  I know that's how libwebrtc works, but in WebKit it looks like it might be a memory leak because there's no corresponding delete.

[youenn fablet]

> Is there no better way than to do this than polling on a thread?  This code
> seems like it is specific to one particular configuration.

This code is inspired from:
https://chromium.googlesource.com/chromium/src/+/master/remoting/protocol/webrtc_audio_module.cc
https://chromium.googlesource.com/chromium/src/+/master/remoting/protocol/webrtc_audio_module.h

I would hope we can do better but did not dig into deep enough yet.

> > Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCAudioModule.h:43
> > +    int16_t shouldNotBeCalled(int16_t value) const
> 
> This should be a template.  Not all values are 16-bit.

OK

> > Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCAudioModule.h:59
> > +    int64_t TimeUntilNextProcess() final { return 1000000; }
> 
> std::numeric_limits<int64_t>::max() ?

OK

> > Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCUtils.cpp:100
> > +    factoryAndThreads.factory = webrtc::CreatePeerConnectionFactory(factoryAndThreads.networkThread.get(), factoryAndThreads.networkThread.get(), factoryAndThreads.signalingThread.get(), factoryAndThreads.audioDeviceModule.get(), new webrtc::VideoToolboxVideoEncoderFactory() , new webrtc::VideoToolboxVideoDecoderFactory());
> 
> extra space before comma.

OK

> Do we really have to call new so often?  I know that's how libwebrtc works,
> but in WebKit it looks like it might be a memory leak because there's no
> corresponding delete.

I did not want to change libwebrtc API in any patch, except wherever needed.
We can do those changes in our local libwebrtc copy.
I am unsure whether the upstream project would consider following that path.

[youenn fablet]

Created attachment 300222 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 300222 [details]
Patch

Clearing flags on attachment: 300222

Committed r211439: <http://trac.webkit.org/changeset/211439>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

r211458