1129298 - Memory keeps increasing during video playback with developer-pref set

Status: RESOLVED WORKSFORME

(Core :: Audio/Video, defect)

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Affects Firefox 36b6 but does not affect aurora or nightly.

Steps to reproduce:

* Add a pref for media.mediasource.decoder-per-segment=true
* Go to https://www.youtube.com/watch?v=jI-kpVh6e1U
* Select 1080p
* Wait two minutes

Expected results:

Memory climbs to 300MB and levels out

Actual results:

Memory climbs up to 700MB and keeps climbing until it runs out of memory.

Comment 1

[cajbir (:cajbir)]

Bugs on Aurora related to MSE that aren't on Beta:

Bug 1114840
Bug 1100803
Bug 1108960

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=a3559f0f4a13&tochange=533ac462e0e5

Comment 3

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6bfc0e1c4b29&tochange=29b05d283b00

Comment 4

[cmtalbert]

Matt Woodrow: It looks like you landed this preference in the code. Can you verify if my interpretation of this is accurate?

The preference essentially forces us to use a new decoder object for every segment of media that we are decoding. We get more segments of media every time we seek and request a range of bytes for decoding.

The preference value is used in only one place in the code in this if statement: https://dxr.mozilla.org/mozilla-central/source/dom/media/mediasource/TrackBuffer.cpp#167

Which as I read it, it is basically saying "if we cannot use an existing decoder to satisfy the media required from this create a new one; otherwise, append this new segment to an existing decoder."

So, setting this preference to true causes us to always create new decoders and new arrays of bytes to append to them. 

The reason Anthony says this proves that memory issues are fixed is that on nightly and aurora with this pref set to true (which means we are allocating memory like crazy) we are remaining a constant memory load. Therefore, we are freeing memory properly on those builds. However,if memory grows without bound on beta with this pref set to true, then we still have memory issues on beta.

While I do understand that decoders are the most significant use of memory in the MSE system, I don't see any indication that they are our only source of leaked memory. Furthermore, I don't see anything to indicate to me that all the downstream graphics allocations that are made as a result of these decoders' work are properly freed either unless we know that all those are made as a result of the decoder work alone. In other words, is there a way to leak graphics surfaces through other avenues of the codebase? 

In short, I could agree that this could solve most of our leaks, but I'm not convinced it solves all of them. And I'm not convinced it would have any impact on non-OOM crashes, which we have not yet focused on. I am still leaning toward recommending we should move MSE to 37.

Matt, NI you to see if my read of the code is correct.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Yes, that's all correct.

It definitely proves that we've fixed one memory leak, but without beta feedback we can't know what % of our crashes this will fix.