Bug 46560 - [LEAK] [GStreamer] Removing video element will not free assigned memory
Summary: [LEAK] [GStreamer] Removing video element will not free assigned memory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-09-25 02:37 PDT by Cedric de Saint Martin
Modified: 2014-11-04 01:34 PST (History)
15 users (show)

See Also:


Attachments
video memory leak use case (423 bytes, text/html)
2010-09-25 02:37 PDT, Cedric de Saint Martin
no flags Details
video memory leak use case 2 (461 bytes, text/html)
2011-04-25 08:43 PDT, Cedric de Saint Martin
no flags Details
Video element memory leak (2.14 KB, text/html)
2014-10-30 17:32 PDT, Patrick Ward
no flags Details
Test patch that helps a little with the memory leak (2.85 KB, patch)
2014-10-31 15:25 PDT, Patrick Ward
no flags Details | Formatted Diff | Diff
Patch (4.46 KB, patch)
2014-11-02 08:30 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (4.47 KB, patch)
2014-11-02 08:32 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (8.15 KB, patch)
2014-11-02 09:19 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (9.68 KB, patch)
2014-11-03 05:42 PST, Philippe Normand
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cedric de Saint Martin 2010-09-25 02:37:07 PDT
Created attachment 68819 [details]
video memory leak use case

Creating a <video> element will take a lot of memory.
But removing this element with javascript won't free this memory.

This memory won't be reused when another <video> element with same src is created.

Web application showing a lot of <video>, like the one I am developping, will suffer of this bug.

I included with this bug report a html file containing a use case of such memory leak.
Comment 1 Alexey Proskuryakov 2010-09-27 11:37:46 PDT
Could you please describe your measurement technique in more detail? Are you looking at Activity Monitor data, or something different?
Comment 2 Cedric de Saint Martin 2010-09-27 21:17:27 PDT
(In reply to comment #1)
> Could you please describe your measurement technique in more detail? Are you looking at Activity Monitor data, or something different?

I looked at Activity Monitor, for private memory (RPRVT). A simple look at it shows a visible increase between two loops.
I am using latest nightlies.
Comment 3 Cedric de Saint Martin 2011-04-01 09:57:20 PDT
Hi,
Can anybody reproduce this huge leak?
It seems that nobody is working on it, but 6 months after having added this bug, this is still not resolved.

Regards,
Cedric
Comment 4 Cedric de Saint Martin 2011-04-25 08:42:01 PDT
Hi,
It seems like the attachment was misleading. Of course the "chrome.mp4" link should points to any readable video file.
I uploaded a new attachment with a video.src pointing to a real video file on the web. Depending on your connection and/or server load, you may want to change it to point to a local file (or increase the timeouts), because we want to have loaded at least a part of the file.

Regards,
Cedric
Comment 5 Cedric de Saint Martin 2011-04-25 08:43:46 PDT
Created attachment 90918 [details]
video memory leak use case 2
Comment 6 David Kilzer (:ddkilzer) 2011-12-06 09:31:23 PST
<rdar://problem/10297586>
Comment 7 Patrick Ward 2014-10-30 17:30:56 PDT
I would like to confirm that this is still a problem. I can reliably recreate the problem with the attachment in this comment. Right now, the test has a hardcoded for the local path "/home/patrick/Downloads/big_buck_bunny.mp4", with the same file copied at "/home/patrick/Downloads/big_buck_bunny2.mp4".

There are three buttons in the test. I ran "top" in a terminal and kept checking the memory consumption of WebKit while running this test in the Epiphany browser.

The "Toggle Video" button will flip the video's "src" attribute between the "big_buck_bunny.mp4" and "big_buck_bunny2.mp4" files. Note that no memory leak occurs.

The "Add/Remove Video" button will create and add the video node to the DOM, then remove it the next time the button is clicked. A memory leak occurs when clicking this repeatedly.

The "Add/Remove Src Video" button will create and add the video node to the DOM, then remove the node the next time the button is clicked. However, a memory leak DOES NOT occur. This is because the video node's "src" attribute is set to the empty string BEFORE the video node is removed.

As a side note, if one creates an HTML string with a video node and a valid, non-empty src attribute and uses JQuery to operate on it, the memory leak will still occur as in the following:

var videoElementString = '<div><video src="file:///home/patrick/Downloads/big_buck_bunny.mp4" /></div>'
$(videoElementString)

The JQuery call will parse the video element string, and the memory leak will still occur.
Comment 8 Patrick Ward 2014-10-30 17:32:27 PDT
Created attachment 240716 [details]
Video element memory leak

Shows three different buttons. Clicking "Add/Remove Video" repeatedly will cause a memory leak. Clicking "Add/Remove Src Video" will not cause a memory leak.
Comment 9 Jer Noble 2014-10-30 17:59:26 PDT
(In reply to comment #7)
> I would like to confirm that this is still a problem. I can reliably
> recreate the problem with the attachment in this comment. Right now, the
> test has a hardcoded for the local path
> "/home/patrick/Downloads/big_buck_bunny.mp4", with the same file copied at
> "/home/patrick/Downloads/big_buck_bunny2.mp4".
> 
> There are three buttons in the test. I ran "top" in a terminal and kept
> checking the memory consumption of WebKit while running this test in the
> Epiphany browser.
> 
> The "Toggle Video" button will flip the video's "src" attribute between the
> "big_buck_bunny.mp4" and "big_buck_bunny2.mp4" files. Note that no memory
> leak occurs.
> 
> The "Add/Remove Video" button will create and add the video node to the DOM,
> then remove it the next time the button is clicked. A memory leak occurs
> when clicking this repeatedly.

Removing the element from the DOM does not immediately free the element, it simply makes it available to the garbage collector. Until the GC specifically destroys that element, memory used by that element will not be freed. This is not a memory leak.

If you force GC to be run (either through your browser's developer tools, or by creating lots of high-memory objects), the video element will eventually be destroyed by GC.

However, there are things WebKit could change to make GC more likely, and to ensure that the video element is destroyed when GC is run. MediaPlayerPrivate exposes a method called "extraMemoryCost()" to be implemented by media engines, which reports the extra memory cost used by the media engine to load, decode, and display the media resource. It sounds like whatever port is being used by the Epiphany browser does not implement this method, and the GC doesn't know how much memory is being used by the video element.

It does appear, however, that you have discovered a good workaround: set src="" and call load() before removing the element from the DOM.
Comment 10 Jer Noble 2014-10-30 18:03:24 PDT
(In reply to comment #9)
> (In reply to comment #7)
> > I would like to confirm that this is still a problem. I can reliably
> > recreate the problem with the attachment in this comment. Right now, the
> > test has a hardcoded for the local path
> > "/home/patrick/Downloads/big_buck_bunny.mp4", with the same file copied at
> > "/home/patrick/Downloads/big_buck_bunny2.mp4".
> > 
> > There are three buttons in the test. I ran "top" in a terminal and kept
> > checking the memory consumption of WebKit while running this test in the
> > Epiphany browser.
> > 
> > The "Toggle Video" button will flip the video's "src" attribute between the
> > "big_buck_bunny.mp4" and "big_buck_bunny2.mp4" files. Note that no memory
> > leak occurs.
> > 
> > The "Add/Remove Video" button will create and add the video node to the DOM,
> > then remove it the next time the button is clicked. A memory leak occurs
> > when clicking this repeatedly.
> 
> Removing the element from the DOM does not immediately free the element, it
> simply makes it available to the garbage collector. Until the GC
> specifically destroys that element, memory used by that element will not be
> freed. This is not a memory leak.
> 
> If you force GC to be run (either through your browser's developer tools, or
> by creating lots of high-memory objects), the video element will eventually
> be destroyed by GC.
> 
> However, there are things WebKit could change to make GC more likely, and to
> ensure that the video element is destroyed when GC is run.
> MediaPlayerPrivate exposes a method called "extraMemoryCost()" to be
> implemented by media engines, which reports the extra memory cost used by
> the media engine to load, decode, and display the media resource. It sounds
> like whatever port is being used by the Epiphany browser does not implement
> this method, and the GC doesn't know how much memory is being used by the
> video element.
> 
> It does appear, however, that you have discovered a good workaround: set
> src="" and call load() before removing the element from the DOM.

Looks like this is a bug in the GStreamer port; they don't implement MediaPlayerPrivate::extraMemoryCost(). Labelling as such.
Comment 11 Philippe Normand 2014-10-31 00:54:48 PDT
Thanks for the heads-up Jer, I'll work on a patch for this.
Comment 12 Patrick Ward 2014-10-31 15:25:12 PDT
Created attachment 240760 [details]
Test patch that helps a little with the memory leak

Attached is a simple patch that ports the extraMemoryCost() function from the AVFoundation implementation to the gstreamer implementation. Running the test with the patch still results in an increasingly growing memory leak.

With and without the patch, I see the memory fluctuating somewhat, but I still get a significant memory leak that eventually crashes the WebKit process. With the patch, however, it takes a much longer time for the WebKit process to crash.
Comment 13 Philippe Normand 2014-11-01 02:11:12 PDT
I'd rather move this to the MediaPlayerPrivateInterface directly to avoid code duplication. ::totalBytes() could also be declared there.

I have a similar patch here and it works but not in the best way :) I can see the GC is triggered after many add/remove steps. But that default implementation is not the best because it depends on the media file size. So if you use a large file the GC will be triggered faster. Another option would be to force a smaller size for the heap (with the JSC_maxHeapSize env var IIRC).

Ideally we'd need to know how much memory is used by the gst libs but it's not a trivial task...

Another workaround would be to call clearMediaPlayer() within HTMLMediaElement after it was removed from its container but I'm not sure such patch would be approved :)
Comment 14 Eric Carlson 2014-11-01 12:08:44 PDT
(In reply to comment #13)
> Another workaround would be to call clearMediaPlayer() within
> HTMLMediaElement after it was removed from its container but I'm not sure
> such patch would be approved :)

Indeed, see https://bugs.webkit.org/show_bug.cgi?id=137025.
Comment 15 Philippe Normand 2014-11-02 08:30:48 PST
Created attachment 240809 [details]
Patch
Comment 16 WebKit Commit Bot 2014-11-02 08:31:56 PST
Attachment 240809 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/MediaPlayerPrivate.h:244:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Philippe Normand 2014-11-02 08:32:32 PST
Created attachment 240810 [details]
Patch
Comment 18 Philippe Normand 2014-11-02 09:05:28 PST
Thanks for the review Eric. Looks like the patch doesn't build on mac/win... I'll fix it before landing.
Comment 19 Philippe Normand 2014-11-02 09:19:39 PST
Created attachment 240812 [details]
Patch
Comment 20 Philippe Normand 2014-11-03 05:42:16 PST
Created attachment 240842 [details]
Patch
Comment 21 Philippe Normand 2014-11-03 05:43:38 PST
Hopefully this time it will build on Mac too. Can you please do another review Eric? I had to change the signature of ::totalBytes in the QTKit and AVF players.
Comment 22 Eric Carlson 2014-11-03 14:19:45 PST
Comment on attachment 240842 [details]
Patch

Thanks Philippe!
Comment 23 Philippe Normand 2014-11-04 01:31:14 PST
Committed r175526: <http://trac.webkit.org/changeset/175526>
Comment 24 Philippe Normand 2014-11-04 01:34:52 PST
I opened bug 138354 as a follow-up to improve the ::extraMemoryCost() implementation for the GStreamer backend.