Bug 167572

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.