1198107 - Memory leak when WebRTC LoadManager adjusts resolution for VP8 encoder

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Andreas Pehrson [:pehrsons]]

Attachment: DMD report after having a PeerConnection open in one tab and closing it

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

Comment 1

[Andreas Pehrson [:pehrsons]]

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.

Comment 2

[Andreas Pehrson [:pehrsons]]

Attachment: 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=pkerr

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

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).

Comment 4

[Andreas Pehrson [:pehrsons]]

(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 5

[Andreas Pehrson [:pehrsons]]

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 6

[Andreas Pehrson [:pehrsons]]

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.

Comment 7

[Andreas Pehrson [:pehrsons]]

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

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ff14e03b5e8

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5ff14e03b5e8

Comment 10

[Andreas Pehrson [:pehrsons]]

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 11

[Sylvestre Ledru [:Sylvestre]]

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).

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cf044859a757

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f5080b93ad6f

Comment 15

[Carsten Book [:Tomcat]]

(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 ?

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/43b9dae36a17

Comment 18

[Josh Cheng [:josh]]

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