+++ This bug was initially created as a clone of Bug #1250356 +++ 57 is the last rev before another breaking API change (audio engine API being removed). Also note that in 56 gyp was replaced by gn, so that makes life fun going forward. We'll port-forward gyp support for this landing, but not again if we can help it. Initial import I'm working with is from stable branch 57; commit 71394677e4dc343ca5c0f996037207a9bd9616c9

Comment on attachment 8839460 [details] [diff] [review] Add gyp files from rel 55 Which BSD bits need fixing is unclear until there's a green Try build but the following looks wrong. > + # A flag for BSD platforms > + ['OS=="freebsd" or OS=="openbsd"', { > + 'os_bsd%': 1, > + }, { > + 'os_bsd%': 0, > + }], Don't drop support for OS=="dragonfly" or OS=="netbsd". http://searchfox.org/mozilla-central/rev/9c1c7106eef1/media/webrtc/trunk/build/common.gypi#85 > + ['os_bsd==1', { > + 'defines': [ > + 'BSD', > + ], > + }], > + ['OS=="openbsd"', { > + 'defines': [ > + 'OPENBSD', > + ], > + }], > + ['OS=="freebsd"', { > + 'defines': [ > + 'FREEBSD', > + ], > + }], This invites namespace pollution since -DBSD is used by ICU/nICEr while -DFREEBSD/-DOPENBSD are used by NSPR/NSS. Better restore WEBRTC_BSD while sparingly use __DragonFly__, __FreeBSD__, __NetBSD__ or __OpenBSD__. http://searchfox.org/mozilla-central/rev/9c1c7106eef1/media/webrtc/trunk/webrtc/build/common.gypi#362 http://searchfox.org/mozilla-central/rev/9c1c7106eef1/media/webrtc/trunk/webrtc/base/platform_thread.cc#50 > + ['OS!="linux" and OS!="openbsd" and OS!="freebsd" or >(nacl_untrusted_build)==1', { > + 'sources/': [ > + ['exclude', '_linux(_unittest|_test)?\\.(h|cc)$'], > + ['exclude', '(^|/)linux/'], > + ], > + }], [...] > + ['OS!="linux" and OS!="openbsd" and OS!="freebsd" or >(nacl_untrusted_build)==1', { > + 'sources/': [ > + ['exclude', '_linux(_unittest|_test)?\\.(h|cc)$'], > + ['exclude', '(^|/)linux/'], > + ], > + }], May require backporting bug 1285501. http://searchfox.org/mozilla-central/rev/9c1c7106eef1/media/webrtc/trunk/build/filename_rules.gypi#32 > + # NSS usage. > + ['(OS=="linux" or OS=="freebsd" or OS=="openbsd" or OS=="solaris")', { Replace every OS=="freebsd" or OS=="openbsd" instance with os_bsd==1. http://searchfox.org/mozilla-central/rev/9c1c7106eef1/media/webrtc/trunk/build/common.gypi#424 > + ['OS=="linux"', { > + 'sources': [ > + 'dbus.cc', > + 'dbus.h', > + 'libdbusglibsymboltable.cc', > + 'libdbusglibsymboltable.h', DBus monitor should be under use_dbus==1 but Gecko doesn't use, and upstream later removed it. https://chromium.googlesource.com/external/webrtc/+/01af3a352c24%5E!/ > + ['OS=="linux" and use_gtk==1', { Replace OS=="linux" with desktop_linux==1 but I doubt Gecko would use the code. > media/webrtc/trunk/webrtc/modules/audio_device/audio_device.gypi The file lacks Gecko changes. I guess, they're going to be added later.

Thanks Jan!! Dan, can you do the changes he indicated and add them as a new patch (just the deltas, as a new patch). thanks

(In reply to Randell Jesup [:jesup] from comment #5) > Thanks Jan!! Dan, can you do the changes he indicated and add them as a new > patch (just the deltas, as a new patch). thanks Sure, will do. Jan, I didn't attempt to bring any of our local changes the gyp files forward when I imported these from branch 55, which is why some things you are expecting might be missing.

Aha. Right. Let me push my current merged repo, and jan please check that (though note I'm not done yet, but all our changes have been merged to the gyp files).

The full queue is at https://hg.mozilla.org/users/rjesup_wgate.com/webrtc_update_57 and applies on top of inbound rev 00aaf25b6e055dd3b557b0546a9c886865b5fff8 After the second patch is pushed, there will be conflicts. Just push the rest of the patches on; they resolve the conflicts.

Can you push to Try instead? Applying mq patches with incomplete headers via git-am is error-prone unlike checking out the whole repo. Without knowning whether it builds on Linux makes it confusing whether a BSD-specific fix is required. $ git cinnabar hg2git 00aaf25b6e055dd3b557b0546a9c886865b5fff8 7d5c4106705ac9fde3828ff85b8b0d7c2e1d1bf0 $ git checkout 7d5c4106705a $ for f ($(</path/to/webrtc_update_57/series)) if git am --patch-format hg /path/to/webrtc_update_57/$f; then echo "============== $f success"; else echo "=============== $f failed"; break; fi [...] $ ./mach build [...] In file included from dom/media/webrtc/MediaEngineWebRTC.cpp:17: In file included from dom/media/webrtc/MediaEngineWebRTC.h:53: media/webrtc/trunk/webrtc/modules/audio_processing/include/audio_processing.h:27:10: fatal error: 'webrtc/modules/audio_processing/include/config.h' file not found #include "webrtc/modules/audio_processing/include/config.h" ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated. $ find media/webrtc/trunk/webrtc/modules/audio_processing/include/ -type f media/webrtc/trunk/webrtc/modules/audio_processing/include/audio_processing.cc media/webrtc/trunk/webrtc/modules/audio_processing/include/config.cc media/webrtc/trunk/webrtc/modules/audio_processing/include/mock_audio_processing.h media/webrtc/trunk/webrtc/modules/audio_processing/include/audio_processing.h $ (cd /path/to/upstream/webrtc/57; find webrtc/modules/audio_processing/include/ -type f) webrtc/modules/audio_processing/include/mock_audio_processing.h webrtc/modules/audio_processing/include/config.h webrtc/modules/audio_processing/include/config.cc webrtc/modules/audio_processing/include/audio_processing.h webrtc/modules/audio_processing/include/audio_processing.cc

(In reply to Randell Jesup [:jesup] from comment #7) > note I'm not done yet I've tried to fill BSD-specific rebase gaps blindly. Not sure if there's any effect beyond noise. chromium_common.gypi seems to be unused by Gecko for now.

I don't have it compiling for linux yet. I have it maybe 80% compiling there are lots of unused gyp files in the import, yes.

https://hg.mozilla.org/try/pushloghtml?changeset=485241177583d2862792f25e722c23792e1dfff3

Comment on attachment 8870684 [details] [diff] [review] work around a VS2015 bug with empty base classes Review of attachment 8870684 [details] [diff] [review]: ----------------------------------------------------------------- The comment is a bit abbreviated (which is fine, I don't think we need to tell the whole story in that header) but I'd like to add a bit more context here in the bug for future archaeology: VoiceEngineImpl has many base classes: http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/media/webrtc/trunk/webrtc/voice_engine/voice_engine_impl.h#56-89 In the object layout, the inherited VoiceEngine piece of VoiceEngineImpl is invisible, because VoiceEngine has no data fields and no vtable. But if we add a void* mDummy to VoiceEngine, the bug becomes apparent: https://pastebin.mozilla.org/9022628 -- even though the VoiceEngine's mDummy is correctly listed between offsets 0xe4 and 0xe8, the compiler thinks that the member has offset 0x2020. This mistaken layout leads to the pointer math being incorrect in static_cast's between VoiceEngine* and VoiceEngineImpl*. If we give VoiceEngine a vtable, the member offsets come out correct and the pointer math does the right thing. Note that VoiceEngineImpl's size is just over 0x2000. Before the import of WebRTC 57 we were just under 0x2000. It could be that the compiler lays things out differently for large objects and crossing this threshold triggered the bug.

Comment on attachment 8871034 [details] [diff] [review] Clean up unannounced/changed SSRC handling in webrtc Review of attachment 8871034 [details] [diff] [review]: ----------------------------------------------------------------- Looks like a good improvement. Maybe even worth just uplifting this part? Just two questions as food for thoughts. ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +2135,2 @@ > bool queue = mRecvSSRCSetInProgress; > + if (queue || mRecvSSRC != ssrc) { Could we utilize mQueuedPackets.size() instead of mRecvSSRCSetInProgress and queue as the indicator for queuing? @@ +2149,5 @@ > + } > + // a new switch needs to be done > + // any queued packets are from a previous switch that hasn't completed > + // yet; drop them and only process the latest SSRC > + mQueuedPackets.Clear(); Is accessing mQueuedPackets thread safe? I'm wondering if this could cause some packet loss.

Brought back code from webrtc.org 49 to support 10.7 under ifdef

(In reply to Nils Ohlmeier [:drno] from comment #22) > Comment on attachment 8871034 [details] [diff] [review] > Clean up unannounced/changed SSRC handling in webrtc > > Review of attachment 8871034 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks like a good improvement. > Maybe even worth just uplifting this part? > > Just two questions as food for thoughts. > > ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp > @@ +2135,2 @@ > > bool queue = mRecvSSRCSetInProgress; > > + if (queue || mRecvSSRC != ssrc) { > > Could we utilize mQueuedPackets.size() instead of mRecvSSRCSetInProgress and > queue as the indicator for queuing? Perhaps; I'll look. It used to be Atomic, but isn't anymore. > > @@ +2149,5 @@ > > + } > > + // a new switch needs to be done > > + // any queued packets are from a previous switch that hasn't completed > > + // yet; drop them and only process the latest SSRC > > + mQueuedPackets.Clear(); > > Is accessing mQueuedPackets thread safe? > I'm wondering if this could cause some packet loss. We only touch mQueuedPackets on the packet reception thread. It does assume packet reception happens on one thread, but that's an existing assumption.

Realize you're away starting tomorrow... the webrtc queue is ready to commit; if you can give heads-up about BSD issues we can hit them before landing or right after. Or if someone else can do it Try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=63966ee31d1b804010b5c6647add95f4b1a02cab Current (cleaned-up) queue is: https://hg.mozilla.org/users/rjesup_wgate.com/webrtc_update_57

Note that videoadapter/videobroadcaster lets us handle scaling requests (due to load/bitrate) to the VideoFrameSource (from the VideoFrameSink) from the webrtc.org code, and also lets us support scaleResolutionBy. We'll only have one sink for the broadcaster, so it's a bit of a waste, but in practice doesn't cost much and avoids extra mods inside of the webrtc.org code, which is a win

patch by dminor

glandium: mostly for ipc/chromium/src/third_party/moz.build

Comment on attachment 8871173 [details] [diff] [review] webrtc gtest fixes for v57 import Review of attachment 8871173 [details] [diff] [review]: ----------------------------------------------------------------- Why is there a r? on yourself here?

jya - I don't expect you to be up on all the details of webrtc use of h264, but thought it would be useful to r? you on this patch, which has a (very) fake h264 codec used for testing (we plan to replace it with the real one; originally we avoided that due to patent concerns)

Comment on attachment 8870999 [details] [diff] [review] Add override statements to avoid errors in clang builds Review of attachment 8870999 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

Comment on attachment 8871003 [details] [diff] [review] Add lock around use of members marked with GUARDED_BY Review of attachment 8871003 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

Comment on attachment 8871004 [details] [diff] [review] Include changes for building under ASAN Review of attachment 8871004 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

Comment on attachment 8871005 [details] [diff] [review] small fixes to rollup code for Windows Review of attachment 8871005 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: build/gyp.mozbuild @@ +40,5 @@ > 'libevent_dir': '/ipc/chromium/src/third_party/libevent', > 'yuv_disable_avx2': 0 if CONFIG['HAVE_X86_AVX2'] else 1, > # don't use openssl > 'use_openssl': 0, > + #'enable_libevent': 0, default according to OS Why is this commented out instead of just set to 0?

Comment on attachment 8871022 [details] [diff] [review] Fix to upstream bug in webrtc.org 57 Review of attachment 8871022 [details] [diff] [review]: ----------------------------------------------------------------- Took me a few seconds to spot the comma :)

Comment on attachment 8871030 [details] [diff] [review] Fix crashes in call shutdown where the EventLog was destroyed too early Review of attachment 8871030 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

Comment on attachment 8871107 [details] [diff] [review] Support screen capture on mac when compiling with 10.7 sdk Review of attachment 8871107 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: media/webrtc/trunk/webrtc/modules/desktop_capture/screen_capturer_mac.mm @@ +68,5 @@ > namespace webrtc { > > namespace { > > +#if defined(MAC_OS_X_VERSION_10_8) Is this code only for 10.8 or for 10.8 or higher? If the latter, I think it would be more readable to use: #if !defined(MAC_OS_X_VERSION_10_7) here and below.

Comment on attachment 8871157 [details] [diff] [review] Adapt mac webrtc code to build with 10.7 SDK Review of attachment 8871157 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +28,5 @@ > #include "webrtc/modules/video_coding/codecs/vp8/include/vp8.h" > #include "webrtc/modules/video_coding/codecs/vp9/include/vp9.h" > #include "webrtc/common_video/include/video_frame_buffer.h" > +#include "webrtc/api/video/i420_buffer.h" > +#if 0 Please check the sdk version here rather than removing it completely.

Comment on attachment 8871158 [details] [diff] [review] Mac build fixes Review of attachment 8871158 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_mac.mm @@ +195,5 @@ > + const DesktopCaptureOptions& options) { > + > + std::unique_ptr<AppCapturerMac> capturer(new AppCapturerMac()); > + > + return std::unique_ptr<DesktopCapturer>(capturer.release()); can we use std::move here?

Comment on attachment 8871159 [details] [diff] [review] Build videoadapter and videobroadcaster Review of attachment 8871159 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

(In reply to Mike Hommey [:glandium] from comment #42) > Comment on attachment 8871173 [details] [diff] [review] > webrtc gtest fixes for v57 import > > Review of attachment 8871173 [details] [diff] [review]: > ----------------------------------------------------------------- > > Why is there a r? on yourself here? Because the patch was written by dminor, so I'll review it.

(In reply to Dan Minor [:dminor] from comment #47) > Comment on attachment 8871005 [details] [diff] [review] > small fixes to rollup code for Windows > > Review of attachment 8871005 [details] [diff] [review]: > ----------------------------------------------------------------- > > lgtm > > ::: build/gyp.mozbuild > @@ +40,5 @@ > > 'libevent_dir': '/ipc/chromium/src/third_party/libevent', > > 'yuv_disable_avx2': 0 if CONFIG['HAVE_X86_AVX2'] else 1, > > # don't use openssl > > 'use_openssl': 0, > > + #'enable_libevent': 0, default according to OS > > Why is this commented out instead of just set to 0? Basically as documentation of something one can set. Probably isn't needed

(In reply to Dan Minor [:dminor] from comment #50) > > +#if defined(MAC_OS_X_VERSION_10_8) > > Is this code only for 10.8 or for 10.8 or higher? If the latter, I think it > would be more readable to use: > #if !defined(MAC_OS_X_VERSION_10_7) > here and below. Good point, I was assuming 10_8 was defined on all 10_8 or newer.

Comment on attachment 8871166 [details] [diff] [review] Fix how VideoFrame buffer ownership is handled (in particular ensuring freeing of H264 shmem buffers). Also fixes a case where RIDs had garbage in them Review of attachment 8871166 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp @@ +268,5 @@ > + aWidth, aHeight, > + buffer_y, stride_y, > + buffer_u, stride_uv, > + buffer_v, stride_uv, > + rtc::Bind(&DeleteBuffer, aBuffer.release()))); Can you use a lambda here? That would be a bit nicer than having to add a static method just for this. @@ +271,5 @@ > + buffer_v, stride_uv, > + rtc::Bind(&DeleteBuffer, aBuffer.release()))); > + > + webrtc::VideoFrame video_frame(video_frame_buffer, aCaptureTime, > + aCaptureTime, webrtc::kVideoRotation_0); // XXX Please remove trailing // XXX

Comment on attachment 8871169 [details] [diff] [review] Fix windows build for screen capture and video capture Review of attachment 8871169 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: media/webrtc/trunk/webrtc/modules/desktop_capture/app_capturer_win.cc @@ +434,5 @@ > + const DesktopCaptureOptions& options) { > + > + std::unique_ptr<AppCapturerWin> capturer(new AppCapturerWin(options)); > + > + return std::unique_ptr<DesktopCapturer>(capturer.release()); Can you use std::move here? ::: media/webrtc/trunk/webrtc/modules/desktop_capture/screen_capturer_win.cc @@ +22,5 @@ > // static > std::unique_ptr<DesktopCapturer> DesktopCapturer::CreateRawScreenCapturer( > const DesktopCaptureOptions& options) { > std::unique_ptr<DesktopCapturer> capturer; > +#ifdef CAPTURE_ALLOW_DIRECTX Is this defined any where in tree? I'm having problems tracking it down. ::: media/webrtc/trunk/webrtc/modules/video_capture/windows/video_capture_factory_windows.cc @@ +23,4 @@ > } > > +rtc::scoped_refptr<VideoCaptureModule> VideoCaptureImpl::Create( > + const char* device_id) { I think this would be better combined with the previous line even if it is long, rather than having the weird indent here.

Comment on attachment 8871170 [details] [diff] [review] Random windows build fixes Review of attachment 8871170 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: media/webrtc/trunk/webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc @@ +20,5 @@ > #include "webrtc/modules/desktop_capture/mouse_cursor.h" > #include "webrtc/modules/desktop_capture/win/cursor.h" > #include "webrtc/modules/desktop_capture/win/window_capture_utils.h" > #include "webrtc/system_wrappers/include/logging.h" > +#include <windows.h> nit: please add a blank line between this include and the webrtc ones.

Comment on attachment 8871171 [details] [diff] [review] Backout changes to windows video capture code - new code relies on stuff in the MS SDK example they have in the tree with licenses we don't want to accept Review of attachment 8871171 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: media/webrtc/trunk/webrtc/modules/video_capture/windows/device_info_ds.cc @@ +12,5 @@ > > #include "webrtc/modules/video_capture/video_capture_config.h" > #include "webrtc/modules/video_capture/video_capture_delay.h" > #include "webrtc/modules/video_capture/windows/help_functions_ds.h" > +//#include "webrtc/base/refcountedobject.h" nit: please remove this completely ::: media/webrtc/trunk/webrtc/modules/video_capture/windows/sink_filter_ds.cc @@ +331,5 @@ > HANDLE handle= GetCurrentThread(); > SetThreadPriority(handle, THREAD_PRIORITY_HIGHEST); > _threadHandle = handle; > + // See http://msdn.microsoft.com/en-us/library/xcb2z8hs(VS.71).aspx for details on the code > + // in this function. Name od article is "Setting a Thread Name (Unmanaged)". nit: s/od/of/ @@ +347,5 @@ > + } > + __except (EXCEPTION_CONTINUE_EXECUTION) > + { > + } > + This is difficult to follow. Please add a comment, or remove if it's just leftover debugging code. ::: media/webrtc/trunk/webrtc/modules/video_capture/windows/video_capture_ds.cc @@ +247,5 @@ > + return SetCameraOutput(capability, capabilityIndex); > +} > + > +int32_t VideoCaptureDS::SetCameraOutput( > + const VideoCaptureCapability& capability, nit: join with line above

Comment on attachment 8871167 [details] [diff] [review] Logging fixes since most logs are now LOG(foo) << bar Review of attachment 8871167 [details] [diff] [review]: ----------------------------------------------------------------- R+ with question, do we want to keep the "nspr" reference?

written by dminor

also fixes a few random bugs that triggered link failures

Comment on attachment 8871427 [details] [diff] [review] Remove mozilla-specific GetRtt(); use call->GetStats().rtt_ms instead Review of attachment 8871427 [details] [diff] [review]: ----------------------------------------------------------------- This way all streams on the Call object will report the same RTT. Is this desirable? The patch itself looks fine, so if the answer is yes, then R+.

Comment on attachment 8871161 [details] [diff] [review] Use videoadapter/videobroadcaster to handle scaling requested from below and scaleResolutionBy Review of attachment 8871161 [details] [diff] [review]: ----------------------------------------------------------------- Couple of non-blocking questions/nits, but in general looks okay to me. ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +594,5 @@ > + // XXX webrtc.org code has a restriction on simulcast layers that each > + // layer must be 1/2 the dimension of the previous layer - not sure why. > + // This means we can't use scaleResolutionBy/scaleDownBy (yet), even if > + // the user specified it. The one exception is that we can apply it on > + // the full-resolution stream (which also happens to handle the Missing closing bracket. @@ +598,5 @@ > + // the full-resolution stream (which also happens to handle the > + // non-simulcast usage case. NOTE: we make an assumption here, not in the > + // spec, that the first stream is the full-resolution stream. > + auto& simulcastEncoding = mConduit->mCurSendCodecConfig->mSimulcastEncodings[0]; > +#if 0 This is confusing to me: does this '#if 0' mean we can't support scaleResolutionBy at all any more after the 57 merge? Or did you moved and commented this code before you added the OnScaleResolutionBy() support further below? @@ +1953,5 @@ > + if (!NS_IsMainThread()) { > + // This can be asynchronous > + RefPtr<WebrtcVideoConduit> self(this); > + NS_DispatchToMainThread(media::NewRunnableFrom([self, sink, wants]() { > + self->mVideoBroadcaster.AddOrUpdateSink(sink, wants); Wouldn't it be easier and avoid code duplication to just call WebrtcVideoConduit::AddOrUpdateSink() on the MainThread again and let the else part below handle what needs to be done? @@ +1984,5 @@ > > MediaConduitErrorCode > WebrtcVideoConduit::SendVideoFrame(webrtc::VideoFrame& frame) > { > + // XXX Google uses a "timestamp_aligner to translate timestamps from the Missing closing quotes. @@ +2040,5 @@ > + buffer = new rtc::RefCountedObject<CoreVideoFrameBuffer>( > + static_cast<CVPixelBufferRef>(frame.nativeHandle), adapted_width, adapted_height, > + crop_width, crop_height, crop_x, crop_y); > +#elif WEBRTC_WIN > + // XX FIX As this code appears to be copied from somewhere in webrtc.org it might be helpful to annotate from where it was copied to allow filling in the missing pieces once they become available in the webrtc.org code base. ::: media/webrtc/trunk/webrtc/media/base/videoadapter.cc @@ +241,5 @@ > + rtc::Optional<float> scale_resolution_by) { > + rtc::CritScope cs(&critical_section_); > + scale_resolution_by_ = scale_resolution_by.value_or(1.0); > + RTC_DCHECK_GE(scale_resolution_by_, 1.0); > + scale_ = static_cast<bool>(scale_resolution_by); Not very readable. Wish there would be a better way then a cast to test if an Optional contains a value.

Comment on attachment 8871175 [details] [diff] [review] Queue H264 GMP decode requests while we're initting Review of attachment 8871175 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp @@ +823,5 @@ > { > if (!mGMP) { > + if (mInitting) { > + // InitDone hasn't been called yet (race) > + GMPDecodeData *data = new GMPDecodeData(aInputImage, Who owns the uint8_t* that EncodedImage contains? Is it safe to queue the buffer here, or does it get freed before it's used? i.e. do you need to make a copy here if you're enqueuing the frame here?

Comment on attachment 8871568 [details] [diff] [review] Remove the rest of LoadManager Review of attachment 8871568 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

See also the r+'d update to this patch (which nails down the destruction order) https://bugzilla.mozilla.org/attachment.cgi?id=8871030&action=edit

Also, a later r+'d patch further tweaks the rid change in config.h

tweaked

Must of this is changed in later patches

Removes Renderer API in for VideoSinks; Lots a adaptations. Some bits are modified by later patches

Comment on attachment 8871479 [details] [diff] [review] VideoFrameUtils changes Review of attachment 8871479 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/systemservices/VideoFrameUtils.cpp @@ +16,5 @@ > { > + auto height = aVideoFrame.video_frame_buffer()->height(); > + return height * aVideoFrame.video_frame_buffer()->StrideY() + > + height * aVideoFrame.video_frame_buffer()->StrideU() + > + height * aVideoFrame.video_frame_buffer()->StrideV(); Is this allocating 150% as much space as necessary now? Should the U, and V planes be ceiling(size/2)? Or is this where your h264 color issues were coming from? @@ +21,3 @@ > } > > void VideoFrameUtils::InitFrameBufferProperties( Because the format is now assumed it could be added to the function sigs.

Comment on attachment 8871174 [details] [diff] [review] Add video_denoising support; basic fix some aspects of h264 use (cleanup later) and automatic resizing Review of attachment 8871174 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +546,5 @@ > webrtc::VideoEncoder::GetDefaultH264Settings(); > h264_settings.frameDroppingOn = frame_dropping; > + h264_settings.profile_byte = aConfig->mProfile; // XXX remove (replaced with profile) > + h264_settings.constraints = aConfig->mConstraints; // XXX remove (replaced with profile) > + h264_settings.level = aConfig->mLevel; // XXX is this used? copy to elsewhere? I'm assuming these are cleaned up in a subsequent patch.

Comment on attachment 8871466 [details] [diff] [review] Improve debug, makde apm_debug_dump overridable in gyp.mozbuild Review of attachment 8871466 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

Comment on attachment 8871467 [details] [diff] [review] Remove old unneeded controls for the AEC Review of attachment 8871467 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: dom/media/webrtc/MediaEngineWebRTC.h @@ +618,5 @@ > // gUM runnables can e.g. Enumerate from multiple threads > Mutex mMutex; > webrtc::VoiceEngine* mVoiceEngine; > // XXX REMOVE before checkin > //webrtc::Config mConfig; I assume this is removed in a subsequent patch.

This was the race condition I mentioned; you can have a callback runnable in-flight when the WebrtcGmpVideoCodec code calls Close and shuts down (and thus the callback points to UAF memory). It so happens we no longer have any need of the thread, it was due to Bug 1047442. The OnEncodedImage() method now supports callbacks from arbitrary threads in order to support HW encoders. Less bugs, and simpler!

good catch, thanks

Written by dminor MozReview-Commit-ID: BpeLMXfZgVn

Written by dminor MozReview-Commit-ID: 4XDdjjG10EH

Written by dminor

Comment on attachment 8871199 [details] [diff] [review] Improve fake OpenH264 GMP module and adapt to v57 usage. Has to handle insertion of SPS/PPS by packetization code Review of attachment 8871199 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: dom/media/gmp-plugin-openh264/gmp-fake-openh264.cpp @@ +121,5 @@ > + T* t = t_; > + t_ = nullptr; > + > + return t; > + } forget is unused (so far), please remove it unless a follow on patch will make use of it. ::: media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp @@ +956,5 @@ > + aDecodedFrame->Width(), aDecodedFrame->Height(), > + buffer_y, aDecodedFrame->Stride(kGMPYPlane), > + buffer_u, aDecodedFrame->Stride(kGMPUPlane), > + buffer_v, aDecodedFrame->Stride(kGMPVPlane), > + rtc::Bind(&DeleteBuffer, buffer.release()))); Could likely use a lambda here and avoid adding a static method, but I'm not certain it is worth making the change at this point.

Comment on attachment 8871474 [details] [diff] [review] Mixed bag of link fixes, especially adding things to gyp files Review of attachment 8871474 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

Comment on attachment 8871478 [details] [diff] [review] api.gyp changes Review of attachment 8871478 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

Comment on attachment 8871570 [details] [diff] [review] Update MediaConduitInterface for API changes and eventlog Review of attachment 8871570 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

Comment on attachment 8871574 [details] [diff] [review] VideoConduit.cpp build fixes. Cleanup of "REMOVE" etc is done in later patches Review of attachment 8871574 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +1760,5 @@ > { > mCodecMutex.AssertCurrentThreadOwns(); > > + // XXX REMOVE BEFORE CHECKIN > +#if 0 This can probably be removed now :) ::: media/webrtc/signaling/src/media-conduit/VideoConduit.h @@ +398,5 @@ > * Stores encoder configuration information and produces > * a VideoEncoderConfig from it. > */ > + class VideoStreamFactory; > + nit: please remove extra whitespace. @@ +421,5 @@ > size_t StreamCount(); > void ClearStreams(); > void ForEachStream( > const std::function<void(webrtc::VideoStream&, SimulcastStreamConfig&, const size_t index)> && f); > + Was the removal of "private:" here deliberate? ::: media/webrtc/trunk/webrtc/common_types.h @@ +645,5 @@ > + maxBitrate == other.maxBitrate && > + targetBitrate == other.targetBitrate && > + minBitrate == other.minBitrate && > + qpMax == other.qpMax && > + strcmp(rid, other.rid) == 0 && You could use a strncmp with kRIDSize here.

mistakenly assigned this to myself

Comment on attachment 8871575 [details] [diff] [review] move stream config into a VideoStreamFactory class, instead of passing down in a config struct Review of attachment 8871575 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +560,5 @@ > + // SetSendCodec() > + video_stream.width = width >> idx; > + video_stream.height = height >> idx; > + // We want to ensure this picks up the current framerate, so indirect > + video_stream.max_framerate = mConduit->mSendingFramerate;o Trailing 'o' here fixed in a later patch.

Comment on attachment 8871576 [details] [diff] [review] First wave of fixes and conflict resolutions for VideoConduit Review of attachment 8871576 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +503,5 @@ > + denoising = false; //parameters_.options.video_noise_reduction.value_or(false); > + } > + > + if (aName == "H264") { > + webrtc::VideoCodecH264 h264_settings = nit: trailing whitespace @@ +685,5 @@ > } > mEncoderConfig.AddStream(video_stream, simulcast_config); > } > > + // XXX parse the encoded SPS/PPS data and set spsData/spsLen/ppsData/ppsLen This does not seem to be addressed yet in the latest patch queue and should be fixed or a bug filed. @@ +1863,5 @@ > + callback_unused)); > + > + webrtc::VideoFrame video_frame(video_frame_buffer, capture_time, > + capture_time, webrtc::kVideoRotation_0); // XXX > + nit: remove XXX and extra whitespace. @@ +2289,5 @@ > || mCurSendCodecConfig->mName != newConfig.mName > || mCurSendCodecConfig->mType != newConfig.mType > || mCurSendCodecConfig->RtcpFbNackIsSet("") != newConfig.RtcpFbNackIsSet("") > || mCurSendCodecConfig->RtcpFbFECIsSet() != newConfig.RtcpFbFECIsSet() > +#if 0 This can be removed now, CompatibleH264Config has been removed in a subsequent patch.

Comment on attachment 8871581 [details] [diff] [review] Major work on updating Video and AudioConduit Review of attachment 8871581 [details] [diff] [review]: ----------------------------------------------------------------- I didn't look at LoadManager.* as it seems to be removed with the latest patches applied. Otherwise, lgtm. ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +361,5 @@ > > + webrtc::VideoCodecType encoder_type = > + SupportedCodecType(webrtc::PayloadNameToCodecType( > + mSendStreamConfig.encoder_settings.payload_name) > + .value_or(webrtc::VideoCodecType::kVideoCodecUnknown)); nit: I think this would be better if mSendStreamConfig lined up under webrtc:: or .value was indented to match mSendStreamConfig. @@ +379,5 @@ > "Each video substream must have a corresponding ssrc."); > > auto cfg = mEncoderConfig.GenerateConfig(); > + if (cfg.number_of_streams == 0) { > + MOZ_CRASH("mEncoderConfig - There are no configured streams!"); Could this be replaced with a MOZ_RELEASE_ASSERT? Also, is our only choice to crash the browser here? Could this be a MOZ_ASSERT and a return KMediaConduitVideoSendStreamError?

Comment on attachment 8871846 [details] [diff] [review] Mostly desktop_capture fixes Review of attachment 8871846 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: build/gyp.mozbuild @@ +45,1 @@ > 'enable_libevent': 1, Should be: Must match dom/systemservices/moz.build WEBRTC_BUILD_LIBEVENT It would be nice to keep these in sync automatically, but as we're replacing gyp soon anyway not worth spending extra time on. ::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc @@ +447,3 @@ > #endif > + started_(false) { > + // XXX Note that this won't capture drift! Maybe replace 'XXX' with 'TODO' if we're not planning to fix this prior to landing.

Comment on attachment 8871855 [details] [diff] [review] CamerasParent fixes and RTCP stats mods moved/fixed to match new upstream Review of attachment 8871855 [details] [diff] [review]: ----------------------------------------------------------------- lgtm, with one nit ::: media/webrtc/trunk/webrtc/modules/video_capture/video_capture_defines.h @@ +92,5 @@ > +// Callback class to be implemented by module user > +class VideoCaptureDataCallback > +{ > +public: > + virtual void OnIncomingCapturedFrame(const int32_t id, Weird indent

Comment on attachment 8871784 [details] [diff] [review] VideoFrameUtils changes Review of attachment 8871784 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

Comment on attachment 8871199 [details] [diff] [review] Improve fake OpenH264 GMP module and adapt to v57 usage. Has to handle insertion of SPS/PPS by packetization code Review of attachment 8871199 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/gmp-plugin-openh264/gmp-fake-openh264.cpp @@ +80,5 @@ > + > +#define GMPLOG(l, x) do { \ > + if (l <= g_log_level) { \ > + const char *log_string = "unknown"; \ > + if ((l >= 0) && (l <= 3)) { \ can we align the \ while at it? @@ +109,5 @@ > #define ENCODED_FRAME_MAGIC 0x4652414d > > +template <typename T> class SelfDestruct { > + public: > + explicit SelfDestruct (T* t) : t_ (t) {} { } with a space though braces should be place on a new line to follow coding style. but seeing it's all wrong in this code... @@ +156,5 @@ > int32_t numberOfCores, > uint32_t maxPayloadSize) override { > callback_ = callback; > > + const char *env = getenv("GMP_LOGGING"); Please use PR_GetEnv(...) const char* @@ +187,5 @@ > + eframe.timestamp_ = inputImage->Timestamp(); > + > + // Now return the encoded data back to the parent. > + GMPVideoFrame* ftmp; > + GMPErr err = host_->CreateFrame(kGMPEncodedVideoFrame, &ftmp); why not store this into SelfDestruct ? And have a get() member to retrieve its content so it can be used with the static_cast below @@ +202,5 @@ > + GMPLOG (GL_ERROR, "Error allocating frame data"); > + f->Destroy(); > + return; > + } > + memcpy(f->Buffer(), &eframe, sizeof(eframe)); can we assert on the allocated size here? too much size assumptions for my liking. Could easily miss something and have out of bound memory access @@ +228,5 @@ > + << "x" << eframe.height_); > + > + // Return the encoded frame. > + GMPCodecSpecificInfo info; > + memset (&info, 0, sizeof (info)); Please use PodZero @@ +339,5 @@ > GMPVideoDecoderCallback* callback, > int32_t coreCount) override { > GMPLOG (GL_INFO, "InitDecode"); > > + const char *env = getenv("GMP_LOGGING"); PR_GetEnv too @@ +391,5 @@ > } > + if (eframe->h264_compat_ != 5 && eframe->h264_compat_ != 1) { > + // only return video for iframes or pframes > + GMPLOG (GL_DEBUG, "Not a video frame: NAL type " << (int) eframe->h264_compat_); > + //return; please remove this comment ::: media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp @@ +391,5 @@ > } > GMPUniquePtr<GMPVideoi420Frame> frame(static_cast<GMPVideoi420Frame*>(ftmp)); > + rtc::scoped_refptr<webrtc::VideoFrameBuffer> input_image = aInputImage->video_frame_buffer(); > + // check for overflow of stride * height > + CheckedInt<int> ysize = CheckedInt<int>(input_image->StrideY()) * input_image->height(); use CheckedInt32, especially as that's what you use below. @@ +397,5 @@ > + // I will assume that if that doesn't overflow, the others case - YUV > + // 4:2:0 has U/V widths <= Y, even with alignment issues. > + err = frame->CreateFrame(ysize.value(), > + input_image->DataY(), > + input_image->StrideU() * (input_image->height()/2), + 1 / 2 for odd size maybe? And you round up later too... @@ +405,5 @@ > + input_image->width(), > + input_image->height(), > + input_image->StrideY(), > + input_image->StrideU(), > + input_image->StrideV()); I don't know what CreatFrame takes as argument, but if it's a dummy frame, can't we reduce the memory usage by having U and V plane point to the same thing? @@ +912,5 @@ > mInitting = false; > // Could now notify that it's dead > } > > +static void DeleteBuffer(uint8_t *data) uint8_t*

Comment on attachment 8871920 [details] [diff] [review] Remove GetReportBlockInfo() Review of attachment 8871920 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

Comment on attachment 8871741 [details] [diff] [review] GMPEncodedThread is no longer needed to avoid deadlocks; upstream code supports callbacks on arbitrary threads due to HW encoders Review of attachment 8871741 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

Comment on attachment 8871923 [details] [diff] [review] Make GetRTCPReceiverReport() use GetRTCPStatistics() and remove GetRemoteRTCPReceiverInfo() Review of attachment 8871923 [details] [diff] [review]: ----------------------------------------------------------------- r+ with notes ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp @@ +208,3 @@ > } > > bool WebrtcAudioConduit::GetRTCPReceiverReport(DOMHighResTimeStamp* timestamp, Is there any reason we don't pass the call_stats all the way up? We are the only consumers of WebrtcAudioConduit::GetRTCPReceiverReport. It would reduce code and allocations (albeit a small number of stack allocations). @@ +212,5 @@ > uint32_t* packetsReceived, > uint64_t* bytesReceived, > uint32_t* cumulativeLost, > int32_t* rttMs) { > + jitterMs is not being set here, but this is fixed in a later patch and the two will be folded ::: media/webrtc/trunk/webrtc/voice_engine/channel.cc @@ +2692,5 @@ > > int Channel::GetRTPStatistics(CallStatistics& stats) { > // --- RtcpStatistics > + // GetStatistics() grabs the stream_lock_ inside the object > + // rtp_receiver_->SSRC grabs a lock too. This does appear to be the case.

Comment on attachment 8871965 [details] [diff] [review] Fix jitter stats Review of attachment 8871965 [details] [diff] [review]: ----------------------------------------------------------------- lgtm, note: this is the patch that will be folded with "Make GetRTCPReceiverReport() use GetRTCPStatistics() and remove GetRemoteRTCPReceiverInfo()" ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp @@ +229,5 @@ > + unsigned int discardedPackets; > + unsigned int cumulative; > + mChannelProxy->GetRTPStatistics(averageJitterMs, maxJitterMs, discardedPackets, cumulative); > + *jitterMs = averageJitterMs; > + zap ws

Comment on attachment 8871972 [details] [diff] [review] Desktop_capture build fixes Review of attachment 8871972 [details] [diff] [review]: ----------------------------------------------------------------- Looks like there is probably a follow up patch to this. Once that is posted I will review them together. ::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_capture.gypi @@ +105,5 @@ > # 'window_capturer_mac.mm', > # 'window_capturer_win.cc', > "desktop_device_info.h", > "desktop_device_info.cc", > +# XXX REMOVE BEFORE CHECKIN Remove @@ +113,5 @@ > ], > 'conditions': [ > ['OS!="android"', { > 'sources': [ > +# XXX REMOVE BEFORE CHECKIN Remove ::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_capturer.cc @@ +60,5 @@ > +std::unique_ptr<DesktopCapturer> DesktopCapturer::CreateAppCapturer( > + const DesktopCaptureOptions& options) { > + // XXX REMOVE BEFORE CHECKIN!!! > + // XXX app capturing will need a lot of (minor?) updating > +#if 0 Remove @@ +66,5 @@ > + if (options.detect_updated_region()) { > + capturer.reset(new DesktopCapturerDifferWrapper(std::move(capturer))); > + } > +#else > + std::unique_ptr<DesktopCapturer> capturer; Always create an empty capturer? @@ +68,5 @@ > + } > +#else > + std::unique_ptr<DesktopCapturer> capturer; > +#endif > + return capturer; fold with 70

Comment on attachment 8871163 [details] [diff] [review] Clean up capturestream tests and make them less timing-sensitive. Also adds receiver Reset() support Review of attachment 8871163 [details] [diff] [review]: ----------------------------------------------------------------- LGTM with nits ::: dom/media/tests/mochitest/head.js @@ +933,5 @@ > let i = 0; > this._started = true; > this._intervalId = setInterval(() => { > try { > + info("draw " + (i ? "color1" : "color2")); nit: log message every 500ms seems a little excessive for production. ::: dom/media/tests/mochitest/pc.js @@ +1427,5 @@ > * @returns {Promise} > * Returns a promise which yields a StatsReport object with RTP stats. > */ > async waitForRtpFlow(track) { > + info("waitForRtpFlow("+track.id+")"); nit: doesn't the log message two lines below already tell you that this has been called including the track id? ::: dom/media/tests/mochitest/test_peerConnection_captureStream_canvas_2d.html @@ +31,5 @@ > function PC_LOCAL_CANVAS_CAPTURESTREAM(test) { > stream = canvas.captureStream(0); > test.pcLocal.attachLocalStream(stream); > + stream.requestFrame(); > + h.drawColor(canvas, h.green); nit: either drawing the second time green here is waste, or if drawing after stream.requestFrame() is required, then the whole PC_LOCAL_DRAW_INITIAL_LOCAL_GREEN() step appears to be useless. @@ +36,5 @@ > + var i = 1; > + return setInterval(function() { > + try { > + info("draw " + i ? "green" : "red"); > + h.drawColor(canvas, i ? h.green : h.red); nit: this initially draws green over green, because you start with 1 now. I guess it would make more sense to start with red here. ::: dom/media/tests/mochitest/test_peerConnection_captureStream_canvas_2d_noSSRC.html @@ +33,5 @@ > function PC_LOCAL_CANVAS_CAPTURESTREAM(test) { > stream = canvas.captureStream(0); > test.pcLocal.attachLocalStream(stream); > + stream.requestFrame(); > + h.drawColor(canvas, h.green); nit: same here - redundant drawing of green @@ +38,5 @@ > + var i = 1; > + return setInterval(function() { > + try { > + info("draw " + i ? "green" : "red"); > + h.drawColor(canvas, i ? h.green : h.red); nit: and again drawing green over green initially. ::: dom/media/tests/mochitest/test_peerConnection_captureStream_canvas_webgl.html @@ +84,5 @@ > + var i = 0; > + return setInterval(function() { > + try { > + info("draw " + i ? "green" : "red"); > + h.drawColor(canvas, i ? h.green : h.red); Looks strange that the test so far kept painting red over red every 500ms. ::: dom/media/tests/mochitest/test_peerConnection_multiple_captureStream_canvas_2d.html @@ +88,4 @@ > }, > + function WAIT_FOR_REMOTE2_GREEN() { > + return h.waitForPixelColor(vremote2, h.green, 128, > + "pcRemote's remote2 should become green"); I would collapse the two waitForPixelColor() calls into one Promise.all() call. ::: dom/media/tests/mochitest/test_peerConnection_verifyVideoAfterRenegotiation.html @@ +62,3 @@ > "pcRemote's remote should become green"); > }, > function DRAW_LOCAL_RED() { This DRAW_LOCAL_RED() function should no longer be needed - just inter-fears with the interval painting on the same canvas. ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +1942,5 @@ > > MediaConduitErrorCode > WebrtcVideoConduit::SendVideoFrame(webrtc::VideoFrame& frame) > { > + CSFLogVerbose(logTag, "%s (send SSRC %u (0x%x)", __FUNCTION__, Shouldn't this also be Debug instead of Verbose? @@ +2063,5 @@ > mQueuedPackets.AppendElement(Move(packet)); > return kMediaConduitNoError; > } > > + CSFLogVerbose(logTag, "%s: seq# %u, Len %d, SSRC %u (0x%x) ", __FUNCTION__, Debug? @@ +2078,5 @@ > > MediaConduitErrorCode > WebrtcVideoConduit::ReceivedRTCPPacket(const void* data, int len) > { > + CSFLogVerbose(logTag, " %s Len %d ", __FUNCTION__, len); Debug? @@ +2177,5 @@ > { > // XXX(pkerr) - PacketOptions possibly containing RTP extensions are ignored. > // The only field in it is the packet_id, which is used when the header > // extension for TransportSequenceNumber is being used, which we don't. > + CSFLogVerbose(logTag, "%s Sent RTP Packet seq %d, len %lu, SSRC %u (0x%x)", Debug? @@ +2198,5 @@ > // Called from multiple threads including webrtc Process thread > bool > WebrtcVideoConduit::SendRtcp(const uint8_t* packet, size_t length) > { > + CSFLogVerbose(logTag, "%s : len %lu ", __FUNCTION__, (unsigned long)length); Debug? Including the SSRC of the conduit might be helpful. @@ +2222,5 @@ > > void > WebrtcVideoConduit::OnFrame(const webrtc::VideoFrame& video_frame) > { > + CSFLogVerbose(logTag, "%s (recv SSRC %u (0x%x), size %ux%u", __FUNCTION__, Debug? Missing closing bracket in the message.

Comment on attachment 8871470 [details] [diff] [review] Bring back APMAnalyzeReverseStream so we can use it for feeding in speaker audio for the AEC Review of attachment 8871470 [details] [diff] [review]: ----------------------------------------------------------------- r+ with one issue ::: media/webrtc/trunk/webrtc/voice_engine/output_mixer.cc @@ +512,5 @@ > + // XXX Bug 1367510 -- convert to 48000? Or modify Processing to > + // support 44100 directly? (that's probably the best) > + AudioFrame *frame = &audioFrame; > + AudioFrame tempframe; > + if (frame->sample_rate_hz_ == AudioProcessing::NativeRate::kSampleRate44_1kHz) { Is this always 44.1kHz? Should this check for != 32kHz? If not an ASSERT is warranted here.

MozReview-Commit-ID: A8GSs50iImh

MozReview-Commit-ID: 1QGVP2pBtDe

MozReview-Commit-ID: In5OUxcqKF

Comment on attachment 8871970 [details] [diff] [review] build fixes - the #if 0's are resolved in later patches Review of attachment 8871970 [details] [diff] [review]: ----------------------------------------------------------------- LGTM

Comment on attachment 8871985 [details] [diff] [review] Lots of build fixes. Some things here (REMOVE etc) are handled in later patches Review of attachment 8871985 [details] [diff] [review]: ----------------------------------------------------------------- lgtm. There is a lot going on in this patch and it is a bit hard to follow as many things changed later. Non-exhaustive notes on what changed are included to make it a little easier to follow. ::: media/webrtc/moz.build @@ +34,5 @@ > 'trunk/webrtc/modules/audio_processing/echo_cancellation_impl.cc', # Because of name clash in the MapError function > 'trunk/webrtc/modules/audio_processing/echo_control_mobile_impl.cc', # Because of name clash in the MapError function > 'trunk/webrtc/modules/audio_processing/gain_control_impl.cc', # Because of name clash in the Handle typedef > 'trunk/webrtc/modules/audio_processing/noise_suppression_impl.cc', # Because of name clash in the Handle typedef > + 'trunk/webrtc/modules/audio_processing/rms_level.cc', # Because of name clash in the kMinLevel variable removed ::: media/webrtc/trunk/webrtc/voice_engine/channel.cc @@ +307,5 @@ > } > } > > void CNameChanged(const char* cname, uint32_t ssrc) override {} > zapped @@ +960,5 @@ > configuration.rtt_stats = &(*rtcp_rtt_stats_proxy_); > configuration.retransmission_rate_limiter = > retransmission_rate_limiter_.get(); > > +#if 0 removed @@ +967,5 @@ > audioproc_config.Set<ExtendedFilter>( > new ExtendedFilter(config.Get<ExtendedFilter>().enabled)); > audioproc_config.Set<DelayAgnostic>( > new DelayAgnostic(config.Get<DelayAgnostic>().enabled)); > +#endif removed, zapped ::: media/webrtc/trunk/webrtc/voice_engine/voe_rtp_rtcp_impl.cc @@ +215,5 @@ > return channelPtr->GetRemoteRTCP_CNAME(cName); > } > > +// XXX REMOVE THIS BEFORE CHECKIN > +// replace with calls to GetRTCPStatistics() removed, ∆ed as described @@ +233,2 @@ > _shared->SetLastError(VE_CHANNEL_NOT_VALID, kTraceError, > + // XXX FIX removed @@ +235,4 @@ > "GetRemoteRTCPReceiverInfo() failed to locate channel"); > return -1; > } > + NTPHigh = 0; // XXX ∆ed later

Comment on attachment 8871986 [details] [diff] [review] make the libevent code build Review of attachment 8871986 [details] [diff] [review]: ----------------------------------------------------------------- r+, there are no changes in this patch that exist by the end of the queue ::: build/gyp.mozbuild @@ +30,5 @@ > 'build_json': 0, > 'build_libjpeg': 0, > 'build_libyuv': 0, > 'build_libvpx': 0, > + 'build_libevent': 0, line removed @@ +36,5 @@ > 'build_json': 0, > 'build_icu': 0, > 'build_opus': 0, > 'libyuv_dir': '/media/libyuv/libyuv', > + 'libevent_dir': '/ipc/chromium/src/third_party/libevent', line removed @@ +41,4 @@ > 'yuv_disable_avx2': 0 if CONFIG['HAVE_X86_AVX2'] else 1, > # don't use openssl > 'use_openssl': 0, > + 'enable_libevent': 1, line removed ::: media/webrtc/signaling/signaling.gyp @@ +302,5 @@ > '_GNU_SOURCE', > 'LINUX', > 'GIPS_VER=3510', > 'SECLIB_OPENSSL', > + 'WEBRTC_BUILD_LIBEVENT', this line removed ::: media/webrtc/trunk/webrtc/base/base.gyp @@ +194,5 @@ > 'task_queue_posix.h', > ], > 'conditions': [ > #TODO: no libevent.gyp > + ['build_libevent==1', { this whole file is removed ::: media/webrtc/trunk/webrtc/base/task_queue_libevent.cc @@ +13,5 @@ > #include <fcntl.h> > #include <string.h> > #include <unistd.h> > > +#include "libevent/include/event.h" line reverted ::: media/webrtc/trunk/webrtc/build/common.gypi @@ +46,5 @@ > 'enable_libevent%': 0, > }, { > 'build_libevent%': 1, > 'enable_libevent%': 1, > + 'libevent_dir%': '<(DEPTH)/third_party/libevent', this whole file was removed

Comment on attachment 8871972 [details] [diff] [review] Desktop_capture build fixes Review of attachment 8871972 [details] [diff] [review]: ----------------------------------------------------------------- lgtm. ::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_capture.gypi @@ +101,1 @@ > # 'win/screen_capturer_win_magnifier.h', This whole file was removed ::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_capturer.cc @@ +60,5 @@ > +std::unique_ptr<DesktopCapturer> DesktopCapturer::CreateAppCapturer( > + const DesktopCaptureOptions& options) { > + // XXX REMOVE BEFORE CHECKIN!!! > + // XXX app capturing will need a lot of (minor?) updating > +#if 0 removed @@ +66,5 @@ > + if (options.detect_updated_region()) { > + capturer.reset(new DesktopCapturerDifferWrapper(std::move(capturer))); > + } > +#else > + std::unique_ptr<DesktopCapturer> capturer; fixed @@ +68,5 @@ > + } > +#else > + std::unique_ptr<DesktopCapturer> capturer; > +#endif > + return capturer; removed

Comment on attachment 8871175 [details] [diff] [review] Queue H264 GMP decode requests while we're initting Review of attachment 8871175 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the fixup patch.

Green with this test; I'll file a followup to look at startup error recovery in RTP for H264

Comment on attachment 8872176 [details] [diff] [review] force iframes every 10 frames in the fake H264 GMP encoder Adding jya as a possible reviewer (need only 1), since he looked at the earlier patch for gmp-fake-openh264.cpp https://treeherder.mozilla.org/#/jobs?repo=try&revision=5afe4457fa294267c45d2b35fedef833277947c4

Comment on attachment 8872176 [details] [diff] [review] force iframes every 10 frames in the fake H264 GMP encoder Review of attachment 8872176 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/gmp-plugin-openh264/gmp-fake-openh264.cpp @@ +268,5 @@ > GMPLOG (GL_ERROR, "no input image"); > return; > } > > + if (frame_type == kGMPKeyFrame || If you don't set frame_type=kGMPKeyFrame, you'll end up sending to the other side mislabeled. So you probably want to pass in kGMPKeyframe as a literal to all the SendFrame calls inside this branch.