Closed Bug 1293794 Opened 3 years ago Closed 3 years ago
Frame volatile buffers to be freed on all platforms
Right now there are three cases after a call to imgFrame::Optimize(): (1) We created an optimized SourceSurface. (i.e., OptimizeSourceSurface() returned a different surface than our original DataSourceSurface) In this case we totally free the volatile buffer that we originally decoded the image into. (2) We couldn't optimize our DataSourceSurface. We keep both the DataSourceSurface and the underlying volatile buffer around. Because we keep the DataSourceSurface, which has a strong reference to the volatile buffer's memory, the operating system is never allowed to free the volatile buffer. (3) We couldn't optimize our DataSourceSurface, as in case 2, but we free it, leaving only the underlying volatile buffer. This allows the operating system to free the volatile buffer if it's running low on memory. Right now case 3 is Android-only, and case 1 and 2 are the only options everywhere else. This is because of this #ifdef: https://dxr.mozilla.org/mozilla-central/source/image/imgFrame.cpp?q=imgFrame.cpp&redirect_type=direct#500 There's not actually a good reason for this, though, because there's no (remaining?) backend where it's expensive to wrap a raw memory buffer (i.e., the volatile buffer) with a DataSourceSurface. So let's rip this out so volatile buffers can work on platforms other than Android.
So ultimately, after this bug, only case 1 and case 3 will remain, on all platforms. Someone with better knowledge of our graphics backends may be correct me, but to the best of my knowledge this is the effect that this will have on all tier 1 platforms: Windows: OptimizeSourceSurface() does a GPU upload and returns a different SourceSurface, so we started in case 1 and we stay in case 1. The status quo is maintained. OS X: We use Skia for content, so OptimizeSourceSurface() does nothing. Because volatile buffers are supported on this platform, we were in case 2 before this bug and now we'll be in case 3. This means that image data can be freed by the OS on OS X in low memory conditions, which should make us more resistant to OOM. Linux: We use Skia for content, same as OS X, so we're in case 3, except that volatile buffers don't work on this platform. That means that practically speaking, the status quo is maintained. Android: We also use Skia for content here, but the #ifdef enabled case 3 on this platform, so the status quo is maintained here too. Thus, this change primarily benefits OS X users. And, of course, our developers, who have one less platform-specific thing to worry about.
Here's the patch. I shuffled things around slightly and added comments so that anyone who maintains this code in the future will hopefully understand what the heck is going on. The current structure confuses me for a second every time I look at it.
Attachment #8779530 - Flags: review?(jmuizelaar)
Try job: remote: Follow the progress of your build on Treeherder: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae0bde12b8c5 remote: remote: It looks like this try push has talos jobs. Compare performance against a baseline revision: remote: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=ae0bde12b8c5
Comment on attachment 8779530 [details] [diff] [review] Allow imgFrame volatile buffers to be freed on all platforms. Review of attachment 8779530 [details] [diff] [review]: ----------------------------------------------------------------- Yes please.
Attachment #8779530 - Flags: review?(jmuizelaar) → review+
Thanks for the review! Things look OK on try. (That one XPCShell failure gave me pause, but it looks like it's an existing crash.)
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/e1e30e96ea5a Allow imgFrame volatile buffers to be freed on all platforms. r=jrmuizel
You need to log in before you can comment on or make changes to this bug.