We landed a workaround for resolution changes to the VP8 encoder causing distortions in bug 1027100 and it's leaking memory because it is now re-initialising the encoder on such resolution changes. This throws away a raw pointer without destroying it. See `vpx_codec_enc_init_ver()` in vpx_encoder.c [1]. The essential parts: > ctx->iface = iface; > ctx->name = iface->name; > ctx->priv = NULL; > ctx->init_flags = flags; > ctx->config.enc = cfg; > res = ctx->iface->init(ctx, NULL); ctx->priv (raw pointer, see [2]) was inited last time around but is now thrown away. ctx->iface->init() will create ctx->priv anew per [3] along with a bunch of other things. STR: 1. Use a DMD build, then `mach run --dmd` 2. Open a peerconnection in a tab 3. Force the load manager to change the resolution of the sent video, you can monitor the video element videoWidth property (or "resize" event) on the receiving side to see when it kicks in. 4. Close the tab 5. Go to about:memory and save DMD data Expected: Nothing about webrtc's encoders or decoders Actual: Lot's of stuff alloced by webrtc's VP8Encoder, e.g.: > Unreported { > 8 blocks in heap block record 5 of 3,061 > 7,143,424 bytes (7,115,064 requested / 28,360 slop) > Individual block sizes: 1,208,320 x 4; 577,536 x 4 > 2.32% of the heap (25.04% cumulative) > 4.57% of unreported (49.37% cumulative) > Allocated at { > #01: replace_malloc (DMD.cpp:1233, in libdmd.dylib) > #02: malloc_zone_malloc (in libsystem_malloc.dylib) + 71 > #03: malloc (in libsystem_malloc.dylib) + 42 > #04: vpx_memalign (vpx_mem.c:25, in XUL) > #05: vp8_yv12_realloc_frame_buffer (yv12config.c:63, in XUL) > #06: vp8_alloc_frame_buffers (alloccommon.c:66, in XUL) > #07: vp8_alloc_compressor_data (onyx_if.c:1227, in XUL) > #08: vp8_change_config (onyx_if.c:1816, in XUL) > #09: vp8_create_compressor (onyx_if.c:1439, in XUL) > #10: vp8e_init (vp8_cx_iface.c:704, in XUL) > #11: vpx_codec_enc_init_ver (vpx_encoder.c:54, in XUL) > #12: webrtc::VP8EncoderImpl::UpdateCodecFrameSize(webrtc::I420VideoFrame const&) (vp8_impl.cc:446, in XUL) > #13: webrtc::VP8EncoderImpl::Encode(webrtc::I420VideoFrame const&, webrtc::CodecSpecificInfo const*, std::vector<webrtc::VideoFrameType, std::allocator<webrtc::VideoFrameType> > const*) (vp8_impl.cc:361, in XUL) > #14: webrtc::VCMGenericEncoder::Encode(webrtc::I420VideoFrame const&, webrtc::CodecSpecificInfo const*, std::vector<webrtc::FrameType, std::allocator<webrtc::FrameType> > const&) (generic_encoder.cc:117, in XUL) > #15: webrtc::vcm::VideoSender::AddVideoFrame(webrtc::I420VideoFrame const&, webrtc::VideoContentMetrics const*, webrtc::CodecSpecificInfo const*) (video_sender.cc:370, in XUL) > #16: webrtc::ViEEncoder::DeliverFrame(int, webrtc::I420VideoFrame*, int, unsigned int const*) (vie_encoder.cc:554, in XUL) > #17: webrtc::ViEFrameProviderBase::DeliverFrame(webrtc::I420VideoFrame*, int, unsigned int const*) (vie_frame_provider_base.cc:60, in XUL) > #18: webrtc::ViECapturer::DeliverI420Frame(webrtc::I420VideoFrame*) (vie_capturer.cc:553, in XUL) > #19: webrtc::ViECapturer::ViECaptureProcess() (scoped_ptr.h:270, in XUL) > #20: webrtc::ViECapturer::ViECaptureThreadFunction(void*) (vie_capturer.cc:465, in XUL) > #21: webrtc::ThreadPosix::Run() (thread_posix.cc:365, in XUL) > #22: webrtc::StartThread(void*) (thread_posix.cc:106, in XUL) > #23: _pthread_body (in libsystem_pthread.dylib) + 131 > #24: _pthread_body (in libsystem_pthread.dylib) + 0 > } > } [1] https://dxr.mozilla.org/mozilla-central/source/media/libvpx/vpx/src/vpx_encoder.c#51 [2] https://dxr.mozilla.org/mozilla-central/source/media/libvpx/vpx/vpx_codec.h#212 [3] https://dxr.mozilla.org/mozilla-central/source/media/libvpx/vp8/vp8_cx_iface.c?case=true&from=vp8e_init#655

If you have a couple of peer connections doing all audio and video, you can see heap-unclassified in about:memory quickly rise to hundreds of MBs. A couple of hours straight and you have GBs.

Bug 1198107 - Destroy VP8 encoder context before re-initing to avoid leaking memory. r=pkerr

Comment on attachment 8652179 [details] MozReview Request: Bug 1198107 - Destroy VP8 encoder context before re-initing to avoid leaking memory. r=jesup https://reviewboard.mozilla.org/r/17101/#review15199 r+; I'm good either way on delete. Please also try and see if the weird horizontal noise is gone with the define flipped (force reduction by overloading the CPU), since that would be even better. ::: media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:438 (Diff revision 1) > if (vpx_codec_enc_init(encoder_, vpx_codec_vp8_cx(), config_, flags)) { Would it be safer to delete encoder_; encoder = new vpx_codec_ctx_t;? Perhaps not; likely this is just fine, but that would guarantee it's the same as a new encoder, which may be slightly safer for uplift (and minimal performance hit).

(In reply to Randell Jesup [:jesup] from comment #3) > Comment on attachment 8652179 [details] > MozReview Request: Bug 1198107 - Destroy VP8 encoder context before > re-initing to avoid leaking memory. r=pkerr > > https://reviewboard.mozilla.org/r/17101/#review15199 > > r+; I'm good either way on delete. Please also try and see if the weird > horizontal noise is gone with the define flipped (force reduction by > overloading the CPU), since that would be even better. I can't see any noise, but I think I'll land and uplift this fix to the workaround and let pkerr verify and remove the ifdef in bug 1030324. At least warrants safer uplifts since libvpx was updated so recently. > ::: media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:438 > (Diff revision 1) > > if (vpx_codec_enc_init(encoder_, vpx_codec_vp8_cx(), config_, flags)) { > > Would it be safer to delete encoder_; encoder = new vpx_codec_ctx_t;? > Perhaps not; likely this is just fine, but that would guarantee it's the > same as a new encoder, which may be slightly safer for uplift (and minimal > performance hit). Sure. Sounds reasonable in this low-level code. :-)

Comment on attachment 8652179 [details] MozReview Request: Bug 1198107 - Destroy VP8 encoder context before re-initing to avoid leaking memory. r=jesup Bug 1198107 - Destroy VP8 encoder context before re-initing to avoid leaking memory. r=jesup

Comment on attachment 8652179 [details] MozReview Request: Bug 1198107 - Destroy VP8 encoder context before re-initing to avoid leaking memory. r=jesup Carrying forward r=jesup.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf039aa88203

Comment on attachment 8652179 [details] MozReview Request: Bug 1198107 - Destroy VP8 encoder context before re-initing to avoid leaking memory. r=jesup Approval Request Comment [Feature/regressing bug #]: Bug 1027100 [User impact if declined]: A user in a WebRTC call will suffer memory leaks. It is triggered by high CPU load so in a situation where CPU usage is high (several peers in the call, for instance), Firefox's memory usage will quickly rise, eventually swapping to disk. The only remedy is a restart of Firefox. [Describe test coverage new/current, TreeHerder]: Manually tested. On m-c but probably not well-covered by automation. [Risks and why]: Low. Small, straight-forward and non-platform-specific change that is not subject to any race conditions. [String/UUID change made/needed]: None

Comment on attachment 8652179 [details] MozReview Request: Bug 1198107 - Destroy VP8 encoder context before re-initing to avoid leaking memory. r=jesup We don't like memory leaks, taking it. However, this does not match the esr criteria, not taking it for esr 38 (and as Hello is not available in esr, the impact is even lower).

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #13) > https://hg.mozilla.org/releases/mozilla-beta/rev/f5080b93ad6f Ryan, is there a way to port that on 2_2r directly or do i need to via the 2_2 branch for uplifting here )given https://hg.mozilla.org/hgcustom/version-control-tools/file/default/pylib/mozautomation/mozautomation/repository.py has no 2_2r ?

Assuming you're on your v2.2r clone already, this should just be a matter of grafting/transplanting the rev from beta (the nearest gecko rev) on to it?

Comment on attachment 8652179 [details] MozReview Request: Bug 1198107 - Destroy VP8 encoder context before re-initing to avoid leaking memory. r=jesup Not maintaining FxOS 2.2 anymore