Bug 1257777 copies input and output data when sending them between app and decoder process that uses both space and CPU resources. Shared memory should be used to reduce memory footprint and CPU/power usage.

Hi Blake and John, I propose this bug targeting 52. Do you have any concern or comments on this ? Would you please kindly share your view on the bug? To be noted,it might better to set pref-on in Firefox52 after Bug 1295106 is resolved, and it's better to make this happen at least 2-3 weeks before the final merge day of Firefox53 (2016/11/07). Then we could have buffer time for softvision to plan and complete the test and verification to target 52. QA Test Plan fyi: https://wiki.mozilla.org/QA/Fennec/Move_Android_decoder_out_of_app_process Thank you very much !

(In reply to Rachelle Yang [:ryang][ryang@mozilla.com] from comment #1) > Hi Blake and John, I propose this bug targeting 52. > > Do you have any concern or comments on this ? Would you please kindly share > your view on the bug? No concerns. We are thinking the same way. Thanks for your reminder.

Comment on attachment 8798755 [details] Bug 1295106 - Part 1: abstract payload in Sample to support both Java byte array and shared memory. https://reviewboard.mozilla.org/r/84162/#review82818

Comment on attachment 8798756 [details] Bug 1295106 - Part 2: add dequeueInput() and releaseOutput() to codec interface. https://reviewboard.mozilla.org/r/84164/#review82820

Comment on attachment 8798758 [details] Bug 1295106 - Part 4: implement JNI methods for shared memory buffers. https://reviewboard.mozilla.org/r/84168/#review82826 I would very much like to avoid adding another Gecko binary library (see note), so: explain to me why it is necessary. Explain why it should be loaded with the special Gecko linker and not the standard Android linker (like mozglue is). Explain to me which process the library load is happening in: main process or codec process? Note: why avoid another binary? Some packaging assumes a small fixed set. There's a small cost to loading more libraries. Each new JNI thing is a chance to fail in a new way. Generally trying to keep things consistent/resist change. Specific note: I'm not thrilled with adding a cross-directory flag to `mobile/android` to build some part of `dom/media`. We should find a way to make that build as part of `dom/media`, or as part of the existing Fennec embedding binaries. r- simply to discuss further.

Comment on attachment 8798757 [details] Bug 1295106 - Part 3: introduce shared memory implemented with MemoryFile and support it in Sample. https://reviewboard.mozilla.org/r/84166/#review82854 The hoops you have to go through to get the MemoryFile across binder is truly amazing. They could've made this so easy in the SDK!

Comment on attachment 8798759 [details] Bug 1295106 - Part 5: allocate samples from a pool. https://reviewboard.mozilla.org/r/84170/#review82856

(In reply to Nick Alexander :nalexander from comment #10) > Comment on attachment 8798758 [details] > Bug 1295106 - Part 4: implement JNI methods for shared memory buffers. > > https://reviewboard.mozilla.org/r/84168/#review82826 > > I would very much like to avoid adding another Gecko binary library (see > note), so: explain to me why it is necessary. Explain why it should be > loaded with the special Gecko linker and not the standard Android linker > (like mozglue is). Explain to me which process the library load is > happening in: main process or codec process? Thanks a lot for the review. The JNI code will be loaded in both main process and codec process. I don't really have any compelling reason for adding a new lib. Just don't want to put it in xul. :) Does including these JNI methods in mozglue sound good to you? It seems address both your 'another lib' and 'gecko linker' concerns. > > Note: why avoid another binary? Some packaging assumes a small fixed set. > There's a small cost to loading more libraries. Each new JNI thing is a > chance to fail in a new way. Generally trying to keep things > consistent/resist change. > > Specific note: I'm not thrilled with adding a cross-directory flag to > `mobile/android` to build some part of `dom/media`. We should find a way to > make that build as part of `dom/media`, or as part of the existing Fennec > embedding binaries. Agreed. Again, it seems putting it in mozglue also fix this? > > r- simply to discuss further.

Ni nalexander for comment 13.

Comment on attachment 8798757 [details] Bug 1295106 - Part 3: introduce shared memory implemented with MemoryFile and support it in Sample. https://reviewboard.mozilla.org/r/84166/#review83498 ::: mobile/android/base/java/org/mozilla/gecko/media/SharedMemory.java:34 (Diff revision 1) > + } catch (NoSuchMethodException e) { > + e.printStackTrace(); > + } > + } > + > + private static final class NativePointer { I don't think it's necessary to have a separate NativePointer class. You should put all the code directly inside SharedMemory class. ::: mobile/android/base/java/org/mozilla/gecko/media/SharedMemory.java:40 (Diff revision 1) > + public static final NativePointer Null = new NativePointer(0); > + /* package */ static final NativePointer Unmapped = new NativePointer(0); > + > + private NativePointer(long handle) { mHandle = handle; } > + > + private boolean isMapped() { public ::: mobile/android/base/java/org/mozilla/gecko/media/SharedMemory.java:43 (Diff revision 1) > + private NativePointer(long handle) { mHandle = handle; } > + > + private boolean isMapped() { > + return (this != Null) && (this != Unmapped); > + } > + private final long mHandle; public ::: mobile/android/base/java/org/mozilla/gecko/media/SharedMemory.java:80 (Diff revision 1) > + dest.writeInt(mId); > + } > + > + /* package */ SharedMemory(int id, int size) throws IOException { > + if (sGetFDMethod == null) { > + throw new IOException("MemoryFile.getFileDescriptor() doesn't exist."); We will crash if a new Android version changes the `getFileDescriptor` method. We should fail gracefully. ::: mobile/android/base/java/org/mozilla/gecko/media/SharedMemory.java:82 (Diff revision 1) > + > + /* package */ SharedMemory(int id, int size) throws IOException { > + if (sGetFDMethod == null) { > + throw new IOException("MemoryFile.getFileDescriptor() doesn't exist."); > + } > + mBackedFile = new MemoryFile(null, size); Alternatively, consider using native code directly instead of using `MemoryFile`.

Comment on attachment 8798758 [details] Bug 1295106 - Part 4: implement JNI methods for shared memory buffers. https://reviewboard.mozilla.org/r/84168/#review83500 Yes, please move the `SharedMemBuffer` class to `org.mozilla.gecko.mozglue` and put the native code in libmozglue.

(In reply to Blake Wu [:bwu][:blakewu] from comment #14) > Ni nalexander for comment 13. Per #c16, I'm fine with having it in mozglue. Roll on!

Comment on attachment 8798757 [details] Bug 1295106 - Part 3: introduce shared memory implemented with MemoryFile and support it in Sample. https://reviewboard.mozilla.org/r/84166/#review83498 > public It seems not neccessary to have a method for this after NativePointer class is gone. Replaced with a boolean flag. > We will crash if a new Android version changes the `getFileDescriptor` method. We should fail gracefully. You're right. Will change it to throw `NoSuchMethodException`. `SamplePool` will catch that and fall back to use `ArrayBuffer` when `SharedMemory` cannot be created. > Alternatively, consider using native code directly instead of using `MemoryFile`. Keep MemoryFile for now and will investigate using native code directly.

Comment on attachment 8798758 [details] Bug 1295106 - Part 4: implement JNI methods for shared memory buffers. https://reviewboard.mozilla.org/r/84168/#review83846 I have one significant comment, which I think needs to be addressed, but I'm happy with the (simple!) build system changes here and willing to have you finish the library loading with the platform group (jchen and snorp). Looking good from my side! ::: mobile/android/base/java/org/mozilla/gecko/media/Codec.java:26 (Diff revision 2) > > /* package */ final class Codec extends ICodec.Stub implements IBinder.DeathRecipient { > private static final String LOGTAG = "GeckoRemoteCodec"; > private static final boolean DEBUG = false; > > + static { This doesn't seem right. We shouldn't be having the Java classloader choosing when to load this native library, we should be managing its lifecycle ourselves. See https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/PasswordsProvider.java#105 for a similar situation, and https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/background/nativecode/NativeCrypto.java#16 for a cautious example intended to be loaded by a background service (which might be distinct from the Gecko lifecycle, which I think is the situation when the codec is loaded in a separate process). If you're absolutely convinced this is correct, make sure you get that jchen and snorp agree.

Comment on attachment 8798758 [details] Bug 1295106 - Part 4: implement JNI methods for shared memory buffers. https://reviewboard.mozilla.org/r/84168/#review84388 ::: mobile/android/base/java/org/mozilla/gecko/media/Codec.java:26 (Diff revisions 1 - 2) > + static { > + System.loadLibrary("mozglue"); > + } I agree with nalexander that you want to load mozglue elsewhere, probably when the Service is started. You also should keep using `GeckoLoader.doLoadLibrary`.

Pushed by jolin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0feabcb02231 Part 1: abstract payload in Sample to support both Java byte array and shared memory. r=snorp https://hg.mozilla.org/integration/autoland/rev/e64c7bf82474 Part 2: add dequeueInput() and releaseOutput() to codec interface. r=snorp https://hg.mozilla.org/integration/autoland/rev/b52117c93d5f Part 3: introduce shared memory implemented with MemoryFile and support it in Sample. r=jchen,snorp https://hg.mozilla.org/integration/autoland/rev/ccba0d65789e Part 4: implement JNI methods for shared memory buffers. r=nalexander https://hg.mozilla.org/integration/autoland/rev/ef80aab8018b Part 5: allocate samples from a pool. r=snorp

https://hg.mozilla.org/mozilla-central/rev/0feabcb02231 https://hg.mozilla.org/mozilla-central/rev/e64c7bf82474 https://hg.mozilla.org/mozilla-central/rev/b52117c93d5f https://hg.mozilla.org/mozilla-central/rev/ccba0d65789e https://hg.mozilla.org/mozilla-central/rev/ef80aab8018b