Gecko Media Plugins: Use single Shmem buffer for multiple frames

RESOLVED FIXED in mozilla33

Status

()

Core
Audio/Video
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Josh Aas, Assigned: jesup)

Tracking

Trunk
mozilla33
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

4 years ago
Right now our Gecko Media Plugins implementation uses up to three Shmem objects to back a single frame. The problem with this is that it leads to us hitting file descriptor limits (~10k) and crashing the parent process when we queue to many frames for processing. There are some other downsides as well, including non-trivial IPC/allocation overhead.

One solution is to use a single Shmem object for multiple frames. This bug is about pursuing such a solution.
Blocks: 948160
Blocks: 1012951
No longer blocks: 948160
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1024039
Duplicate of this bug: 1024046
(Assignee)

Updated

3 years ago
Assignee: nobody → rjesup
Target Milestone: --- → mozilla33
(Assignee)

Comment 3

3 years ago
Created attachment 8448022 [details] [diff] [review]
wip_shmem.patch

WIP patch to recycle YUV buffer and encoded-data buffers back and forth across the interface.  Has a limit on the number that can accumulate on one side; I could (if it matters) send them in IPC messages to the other side if there's an inbalance (and then free there if it has "enough" too).  Right now if there are 10 in a pool on one side; it will delete to keep the limit at 10.  

Note that one-way calls will be totally imbalanced and all buffers would get deleted and not recycled, so we may want to include the return messages (there's some earlier work towards a version of that in this patch which should be cleaned up/reworked to do that).

IN a ~15 minute call, I saw ~280K recycles of buffers, and ~1500 allocations (most of the time it was sitting at ~500), and then I ran out of something and failed a shmem alloc.  Memory used in the pools was a couple of meg and appeared stable.
Attachment #8448022 - Flags: feedback?(bent.mozilla)
Comment on attachment 8448022 [details] [diff] [review]
wip_shmem.patch

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

::: content/media/gmp/moz.build
@@ +47,5 @@
>      'GMPPlatform.cpp',
>      'GMPProcessChild.cpp',
>      'GMPProcessParent.cpp',
>      'GMPService.cpp',
> +    'GMPSharedMemManager.cpp',

This file doesn't exist.
(Assignee)

Comment 5

3 years ago
Created attachment 8448046 [details] [diff] [review]
wip_shmem.patch

Forgot hg add for GMPSharedMemManager.cpp
Attachment #8448022 - Attachment is obsolete: true
Attachment #8448022 - Flags: feedback?(bent.mozilla)
Attachment #8448046 - Flags: feedback?(jld)
Attachment #8448046 - Flags: feedback?(bent.mozilla)
(Assignee)

Updated

3 years ago
Blocks: 1032492
(Assignee)

Comment 6

3 years ago
Created attachment 8448578 [details] [diff] [review]
WIP patch to use Shmem pools and proxy all Shmem create/delete to parent

WIP and attempts to proxy everything to the parent.  Builds cleanly, but when we call SendNeedShmem(size, &mem), it runs and seems to return an Shmem in the parent, but the child fails with Error deserializing 'Shmem'.  Very close I believe.
(Assignee)

Comment 7

3 years ago
Created attachment 8448757 [details] [diff] [review]
WIP patch to use Shmem pools and proxy all Shmem create/delete to parent
(Assignee)

Updated

3 years ago
Attachment #8448578 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
Created attachment 8448908 [details] [diff] [review]
WIP patch to use Shmem pools and proxy all Shmem create/delete to parent
(Assignee)

Updated

3 years ago
Attachment #8448757 - Attachment is obsolete: true

Comment 9

3 years ago
The shmem pools patch worked solidly for me on OSX running for 20mins or so.  The plugin process still leaks some memory though.  Perhaps I will do some more frame counting.
(Assignee)

Comment 10

3 years ago
Created attachment 8449120 [details] [diff] [review]
Proxy all GMP Shmem create/delete to parent and reduce allocation traffic

Cleaned up for review - and the plugin failure cutout has been tested and works (killing the plugin container while in use)
(Assignee)

Updated

3 years ago
Attachment #8448908 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8448046 - Attachment is obsolete: true
Attachment #8448046 - Flags: feedback?(jld)
Attachment #8448046 - Flags: feedback?(bent.mozilla)
(Assignee)

Updated

3 years ago
Attachment #8449120 - Flags: review?(bent.mozilla)
Comment on attachment 8449120 [details] [diff] [review]
Proxy all GMP Shmem create/delete to parent and reduce allocation traffic

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

Ben is busy so I'll do the review instead
Attachment #8449120 - Flags: review?(bent.mozilla) → review?(nical.bugzilla)
Comment on attachment 8449120 [details] [diff] [review]
Proxy all GMP Shmem create/delete to parent and reduce allocation traffic

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

Looks good. The whole thing is based on the idea that using intr protocols solves the problem of synchronously allocating a shmem on the parent and returning to the child side. I don't know how intr actually works so I am assuming it does work :) (and I am genuinely interested in it working because some gfx ipc code will most likely need to use the same trick at some point).
Attachment #8449120 - Flags: review?(nical.bugzilla) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/96aa9d33a1f5
Something in this push was causing leaks. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/70f3b01cb736

https://tbpl.mozilla.org/php/getParsedLog.php?id=43566548&tree=Mozilla-Inbound
Created attachment 8454221 [details] [diff] [review]
Patch: Store GMP free list in a StaticAutoPtr
Attachment #8454221 - Flags: review?(rjesup)
Created attachment 8454222 [details] [diff] [review]
Patch: Store GMP free list in a StaticAutoPtr
Attachment #8454221 - Attachment is obsolete: true
Attachment #8454221 - Flags: review?(rjesup)
Attachment #8454222 - Flags: review?(rjesup)
(Assignee)

Updated

3 years ago
Attachment #8454222 - Flags: review?(rjesup) → review+
Landed with fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c928106052fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1ba940ea14b
https://hg.mozilla.org/mozilla-central/rev/c928106052fe
https://hg.mozilla.org/mozilla-central/rev/b1ba940ea14b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.