Closed Bug 1020090 Opened 6 years ago Closed 6 years ago

Gecko Media Plugins: Use single Shmem buffer for multiple frames

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: jaas, Assigned: jesup)

References

Details

Attachments

(2 files, 6 obsolete files)

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: OpenH264
Blocks: 1012951
No longer blocks: OpenH264
Duplicate of this bug: 1024039
Duplicate of this bug: 1024046
Assignee: nobody → rjesup
Target Milestone: --- → mozilla33
Attached patch wip_shmem.patch (obsolete) — Splinter Review
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.
Attached patch wip_shmem.patch (obsolete) — Splinter Review
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)
Blocks: 1032492
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.
Attachment #8448578 - Attachment is obsolete: true
Attachment #8448757 - Attachment is obsolete: true
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.
Cleaned up for review - and the plugin failure cutout has been tested and works (killing the plugin container while in use)
Attachment #8448908 - Attachment is obsolete: true
Attachment #8448046 - Attachment is obsolete: true
Attachment #8448046 - Flags: feedback?(jld)
Attachment #8448046 - Flags: feedback?(bent.mozilla)
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+
Attachment #8454221 - Attachment is obsolete: true
Attachment #8454221 - Flags: review?(rjesup)
Attachment #8454222 - Flags: review?(rjesup)
Attachment #8454222 - Flags: review?(rjesup) → review+
You need to log in before you can comment on or make changes to this bug.