Bug 167572 - [WebRTC] Add a libwebrtc AudioModule specific to WebKit
Summary: [WebRTC] Add a libwebrtc AudioModule specific to WebKit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks: 143211
  Show dependency treegraph
 
Reported: 2017-01-29 21:38 PST by youenn fablet
Modified: 2017-01-31 16:20 PST (History)
5 users (show)

See Also:


Attachments
Patch (23.87 KB, patch)
2017-01-29 21:53 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (23.91 KB, patch)
2017-01-30 17:51 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (24.02 KB, patch)
2017-01-31 09:12 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-01-29 21:38:01 PST
Add a libwebrtc AudioModule specific to WebKit
Comment 1 youenn fablet 2017-01-29 21:53:04 PST
Created attachment 300086 [details]
Patch
Comment 2 youenn fablet 2017-01-30 17:51:27 PST
Created attachment 300171 [details]
Patch
Comment 3 Alex Christensen 2017-01-30 21:12:01 PST
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.
Comment 4 youenn fablet 2017-01-31 09:00:55 PST
> 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.
Comment 5 youenn fablet 2017-01-31 09:12:18 PST
Created attachment 300222 [details]
Patch
Comment 6 WebKit Commit Bot 2017-01-31 10:59:21 PST
Comment on attachment 300222 [details]
Patch

Clearing flags on attachment: 300222

Committed r211439: <http://trac.webkit.org/changeset/211439>
Comment 7 WebKit Commit Bot 2017-01-31 10:59:25 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Alex Christensen 2017-01-31 16:20:03 PST
r211458