Closed Bug 1198107 Opened 4 years ago Closed 4 years ago

Memory leak when WebRTC LoadManager adjusts resolution for VP8 encoder

Categories

(Core :: WebRTC, defect, P1)

33 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox40 --- wontfix
firefox41 --- fixed
firefox42 --- fixed
firefox43 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- wontfix
b2g-v2.1 --- wontfix
b2g-v2.1S --- wontfix
b2g-v2.2 --- affected
b2g-v2.2r --- fixed
b2g-master --- fixed
Blocking Flags:

People

(Reporter: pehrsons, Assigned: pehrsons)

References

Details

Attachments

(2 files)

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
Attachment #8652179 - Flags: review?(pkerr)
backlog: --- → webrtc/webaudio+
Rank: 5
Component: WebRTC: Audio/Video → WebRTC
Priority: -- → P1
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).
Attachment #8652179 - Flags: review+
(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. :-)
See Also: → 1030324
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
Attachment #8652179 - Attachment description: MozReview Request: Bug 1198107 - Destroy VP8 encoder context before re-initing to avoid leaking memory. r=pkerr → MozReview Request: Bug 1198107 - Destroy VP8 encoder context before re-initing to avoid leaking memory. r=jesup
Attachment #8652179 - Flags: review?(rjesup)
Attachment #8652179 - Flags: review?(pkerr)
Attachment #8652179 - Flags: review+
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.
Attachment #8652179 - Flags: review?(rjesup) → review+
Assignee: nobody → pehrsons
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5ff14e03b5e8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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
Attachment #8652179 - Flags: approval‑mozilla‑b2g37_v2_2r?
Attachment #8652179 - Flags: approval-mozilla-esr38?
Attachment #8652179 - Flags: approval-mozilla-beta?
Attachment #8652179 - Flags: approval-mozilla-b2g37?
Attachment #8652179 - Flags: approval-mozilla-aurora?
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).
Attachment #8652179 - Flags: approval-mozilla-esr38?
Attachment #8652179 - Flags: approval-mozilla-esr38-
Attachment #8652179 - Flags: approval-mozilla-beta?
Attachment #8652179 - Flags: approval-mozilla-beta+
Attachment #8652179 - Flags: approval-mozilla-aurora?
Attachment #8652179 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 1074310
Attachment #8652179 - Flags: approval‑mozilla‑b2g37_v2_2r? → approval‑mozilla‑b2g37_v2_2r+
(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 ?
Flags: needinfo?(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?
Flags: needinfo?(ryanvm)
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
Attachment #8652179 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37-
You need to log in before you can comment on or make changes to this bug.