Summary: | [WebRTC] Add a libwebrtc AudioModule specific to WebKit | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||
Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, agouaillard, commit-queue, eric.carlson, jer.noble | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 143211 | ||||||||||
Attachments: |
|
Description
youenn fablet
2017-01-29 21:38:01 PST
Created attachment 300086 [details]
Patch
Created attachment 300171 [details]
Patch
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. > 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. Created attachment 300222 [details]
Patch
Comment on attachment 300222 [details] Patch Clearing flags on attachment: 300222 Committed r211439: <http://trac.webkit.org/changeset/211439> All reviewed patches have been landed. Closing bug. |