879743 - Large memory leak when giving getUserMedia the fake: true constraint

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Florian Quèze [:florian]]

Attachment: DMD output

When using the current code of talkilla and just adding fake: true in the getUserMedia constraints, the browsers (a current Firefox nightly) on both ends of a call take an increasingly large amount of memory. Within minutes, both browsers take more than 1GB.

Unfortunately I don't have a simple test case (http://mozilla.github.io/webrtc-landing/pc_test.html doesn't reproduce the leak).

about:memory reports 90+% of the memory in heap-unclassified. A local build with DMD shows that the memory is allocated with this stack:

Unreported: 2,468 blocks in stack trace record 1 of 2,358
 1,142,308,864 bytes (1,137,254,400 requested / 5,054,464 slop)
 91,43% of the heap (91,43% cumulative);  93,94% of unreported (93,94% cumulative)
 Allocated at
   replace_malloc (DMD.cpp:1226, in libdmd.dylib) 0x100008951
   _asl_msg_test_expression (in libsystem_c.dylib) + 83 0x7fff8ccad153
   _asl_msg_new_key_val_op (in libsystem_c.dylib) + 571 0x7fff8ccadba7
   moz_xmalloc (mozalloc.cpp:54, in libmozalloc.dylib) 0x1000d48ce
   mozilla::layers::BufferRecycleBin::GetBuffer(unsigned int) (mozalloc.h:213, in XUL) 0x102706425
   mozilla::layers::PlanarYCbCrImage::CopyData(mozilla::layers::PlanarYCbCrImage::Data const&) (ImageContainer.cpp:463, in XUL) 0x102707027
   mozilla::MediaEngineDefaultVideoSource::Notify(nsITimer*) (MediaEngineDefault.cpp:237, in XUL) 0x101b33234
   nsTimerImpl::Fire() (nsTimerImpl.cpp:546, in XUL) 0x102658fc9
   nsTimerEvent::Run() (nsTimerImpl.cpp:627, in XUL) 0x1026590d8
   nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:627, in XUL) 0x102656761
   NS_ProcessNextEvent(nsIThread*, bool) (nsThreadUtils.cpp:238, in XUL) 0x102617d00
   nsThread::ThreadFunc(void*) (nsThread.h:63, in XUL) 0x102655bdd
   _pt_root (in libnss3.dylib) + 203 0x10111751b
   acl_create_entry_np (in libsystem_c.dylib) + 91 0x7fff8cc94742

I'm attaching the full DMD output, just in case looking at smaller allocations could be helpful.

Comment 1

[Florian Quèze [:florian]]

Attachment: DMD output

attachment 758534 [details] is garbage, I don't know what happened when I attached the file...

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 758543 [details]
DMD output

Still the same garbage. I uploaded a correct copy of the file at http://queze.net/goinfre/fake-leak-dmd.txt.gz

Comment 3

[Jason Smith [:jsmith]]

fake:true isn't supposed to be used for production level use - it's for testing only. Can you reproduce this without fake: true?

Comment 4

[Florian Quèze [:florian]]

(In reply to Jason Smith [:jsmith] from comment #3)
> fake:true isn't supposed to be used for production level use - it's for
> testing only.

Sure. Still, I suspect this misbehavior could make testing of talkilla on build slaves that don't have devices very painful.

> Can you reproduce this without fake: true?

No.

Comment 5

[Jason Smith [:jsmith]]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> (In reply to Jason Smith [:jsmith] from comment #3)
> > fake:true isn't supposed to be used for production level use - it's for
> > testing only.
> 
> Sure. Still, I suspect this misbehavior could make testing of talkilla on
> build slaves that don't have devices very painful.

So using fake devices on build slaves isn't an option? Something like what Ted built in bug 815002?

Real devices I take it aren't an option either, right?

Comment 6

[Florian Quèze [:florian]]

(In reply to Jason Smith [:jsmith] from comment #5)

> So using fake devices on build slaves isn't an option? Something like what
> Ted built in bug 815002?
> 
> Real devices I take it aren't an option either, right?

Thanks for the bug number. We are using TravisCI to run our tests, so I'm not sure how much control we have on the build slaves, but I'm afraid it's not much.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

This seems likely to be a reasonably common use case for people that want to write automated tests for web apps/sites that use WebRTC.  Lowering the barrier to testing here seems likely to make Firefox more attractive to web-devs using WebRTC.

Comment 8

[Jason Smith [:jsmith]]

Note - this could be related to the issue seen in bug 875640.

Comment 9

[Florian Quèze [:florian]]

Maire, I just noticed I forgot to CC you when adding this to the Talkilla tracking bug. From what I heard today, this bug is currently preventing us from doing functional testing on the talkilla chat window (that uses WebRTC).

Comment 10

[Ethan Hugg [:ehugg]]

Attachment: pc_test with audio/video tracks and fake:true

This version of pc_test.html will repro the problem.  It's something about having audio and video tracks in the same stream and false:true.  This test makes my Nightly collect objects of type PlanarYCbCrImage.  I put a counter on it and they just keep increasing.  The original pc_test.html without audio and video together did not do this.

Still investigating why.

Comment 11

[Ethan Hugg [:ehugg]]

At first I assumed this was due to us trying to sync a/v, but commenting out this line in PeerConnectionMedia did not change the symptoms:

PeerConnectionMedia.cpp:486
      video_conduit->SyncTo(audio_conduit);

Then I figured out that Audio and Video were not being sent at the same rates.  Their callbacks are called based on the video rate which is defined twice and differently:

MediaEngineDefault.h:61
  static const int DEFAULT_VIDEO_FPS = 60;

MediaEngine.h:43
  static const int DEFAULT_VIDEO_FPS = 30;


Video was using one of these and audio the other.  I'm going to propose getting rid of the defs in MediaEngineDefault.h to avoid confusion.

However this still doesn't solve it because when we are loading the audio:

MediaEngineDefault.cpp:376
  segment.InsertNullDataAtStart(AUDIO_RATE/100); // 10ms of fake data

This is loading 10ms instead of the amount needed based on how often it's called.  I'll be changing it to this:

  segment.InsertNullDataAtStart(AUDIO_RATE/MediaEngine::DEFAULT_VIDEO_FPS);

Which seems to work.  Patch incoming.

Comment 12

[Ethan Hugg [:ehugg]]

Attachment: Test patch for spamming stdout with allocated image frame counts

Comment 13

[Ethan Hugg [:ehugg]]

Comment on attachment 763756 [details] [diff] [review]
Test patch for spamming stdout with allocated image frame counts


Only for test - marking as obsolete to avoid confusion.

Comment 14

[Ethan Hugg [:ehugg]]

Attachment: Fake audio should be sent at the same rate as fake video

Comment 15

[Ethan Hugg [:ehugg]]

Comment on attachment 763759 [details] [diff] [review]
Fake audio should be sent at the same rate as fake video

Review of attachment 763759 [details] [diff] [review]:
-----------------------------------------------------------------

This appears to output the same amount of audio and video so we don't have a backup of video frames.

Comment 16

[Ethan Hugg [:ehugg]]

Try run with Android OOM crashtest turned back on (bug 875640).

https://tbpl.mozilla.org/?tree=Try&rev=5e0de529c370

Comment 17

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

Comment on attachment 763759 [details] [diff] [review]
Fake audio should be sent at the same rate as fake video

Review of attachment 763759 [details] [diff] [review]:
-----------------------------------------------------------------

Easy fix - swap to 10ms callbacks and you have r+

::: content/media/webrtc/MediaEngineDefault.cpp
@@ +335,5 @@
>    // Remember TrackID so we can finish later
>    mTrackID = aID;
>  
>    // 1 Audio frame per Video frame
> +  mTimer->InitWithCallback(this, 1000 / MediaEngine::DEFAULT_VIDEO_FPS,

Instead of making it be 1 per default-video-frame-period, make it more realistic and generate one block per 10ms (which the Notify() code already assumed).

::: content/media/webrtc/MediaEngineDefault.h
@@ -60,5 @@
>  
> -  static const int DEFAULT_VIDEO_FPS = 60;
> -  static const int DEFAULT_VIDEO_MIN_FPS = 10;
> -  static const int DEFAULT_VIDEO_WIDTH = 640;
> -  static const int DEFAULT_VIDEO_HEIGHT = 480;

If these aren't used anywhere, they can go

Comment 18

[Ethan Hugg [:ehugg]]

>> -  static const int DEFAULT_VIDEO_FPS = 60;
>> -  static const int DEFAULT_VIDEO_MIN_FPS = 10;
>> -  static const int DEFAULT_VIDEO_WIDTH = 640;
>> -  static const int DEFAULT_VIDEO_HEIGHT = 480;

>If these aren't used anywhere, they can go

These are exactly copied in MediaEngine.h.  Every place except this audio ms computation uses them from there.  Having duplicate defs was part of the problem.

Comment 19

[Ethan Hugg [:ehugg]]

Attachment: Fake audio should be sent at the same rate as fake video

Comment 20

[Ethan Hugg [:ehugg]]

Comment on attachment 765447 [details] [diff] [review]
Fake audio should be sent at the same rate as fake video

Review of attachment 765447 [details] [diff] [review]:
-----------------------------------------------------------------

Created a new const for the audio timer and set it to 10ms.

Comment 21

[Ethan Hugg [:ehugg]]

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

Comment 22

[Ed Morley [:emorley]]

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