[LEAK] [GStreamer] Removing video element will not free assigned memory

• Component: Media
• Status: RESOLVED FIXED

[Cedric de Saint Martin]

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.

[Alexey Proskuryakov]

Could you please describe your measurement technique in more detail? Are you looking at Activity Monitor data, or something different?

[Cedric de Saint Martin]

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

[Cedric de Saint Martin]

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

[Cedric de Saint Martin]

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

[Cedric de Saint Martin]

Created attachment 90918 [details]
video memory leak use case 2

[David Kilzer (:ddkilzer)]

<rdar://problem/10297586>

[Patrick Ward]

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.

[Patrick Ward]

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.

[Jer Noble]

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

[Jer Noble]

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

[Philippe Normand]

Thanks for the heads-up Jer, I'll work on a patch for this.

[Patrick Ward]

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.

[Philippe Normand]

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

[Eric Carlson]

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

[Philippe Normand]

Created attachment 240809 [details]
Patch

[WebKit Commit Bot]

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.

[Philippe Normand]

Created attachment 240810 [details]
Patch

[Philippe Normand]

Thanks for the review Eric. Looks like the patch doesn't build on mac/win... I'll fix it before landing.

[Philippe Normand]

Created attachment 240812 [details]
Patch

[Philippe Normand]

Created attachment 240842 [details]
Patch

[Philippe Normand]

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.

[Eric Carlson]

Comment on attachment 240842 [details]
Patch

Thanks Philippe!

[Philippe Normand]

Committed r175526: <http://trac.webkit.org/changeset/175526>

[Philippe Normand]

I opened bug 138354 as a follow-up to improve the ::extraMemoryCost() implementation for the GStreamer backend.