1257777 - Move Android audio/video decoder out of application process

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Audio/Video, defect, P2)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

If we moved Android video decoding to inside a GeckoMediaPlugin, when the decoder crashed it would not crash Firefox.

We will need to figure out a way to pass a video frame in GPU memory across from the GMP process to the content process.

Comment 1

[Kilik Kuo [:kikuo] (inactive)]

As James and I investigated on fennec EME, currently we found no JNI environment can be obtained in GMP (decryptor) child process. In that case, we won't be able to use the wrapper APIs which are generated by AnnotationProcessors and SDKProcessors according these android APIs [1].

[1] https://dxr.mozilla.org/mozilla-central/source/widget/android/bindings/

Comment 2

[Kilik Kuo [:kikuo] (inactive)]

Should add, for EME decryption / decoder cases.
When App starts to playback, MediaCodec::configure() is called, and a MediaCrypto object is passed into it as parameter for decryption. So at the time decoding is happening in mediaserver process, the decryption is also processed in mediaserver.

IMHO, it seems unnecessary to have (decrypt)/decode flow as following,

Process 1   |   Process 2                                               |  Process 3
Fennec App --> GMPChild (calling JNI wrapper API to android framework) --> mediaserver (decrypt/decode/render)

Comment 3

[John Lin [:jhlin][:jolin]]

To run decoder in GMP on Android, there are 2 approaches that I can think of:
 a. Run Android JVM in GMP process and implement |PGMPxxxDecoder| with existing PDM/|AndroidDecoderModule|. The JVM is necessary for the PDM to utilize Java classes through JNI.
 b. Rewrite PDM using NDK, and implement GMP decoders with that. Other than developing a new PDM implementation, build system needs to upgrade the Android platform version Fennec currently built against (from android-9 [1] to android-21).

 But passing GMP process the Android GPU/HW memory (graphic buffer) *through IPDL* won't work because the |Surface| class needed by |MediaCodec| can only be passed using Android binder: at first I thought |Parcel.marshall()|[3] can be used to serialize |Surface| and then pass those raw bytes using IPDL, but it turns out |Surface| contains remotely callable object, and marshalling rejects it. I guess |MediaCrypto| (needed by EME) will have same problem too.

 Bottom line is, we need Android binder for off app process video decoders to get HW buffers.

 Now, my proposal for addressing the 'crashing decoder takes down Firefox app with it' problem on Android: instead of moving decoder to GMP process, we create an (off app process) Android service to host remote |MediaCodec| object, and let |AndroidDecoderModule| call the binder. This essentially makes PDM proxy of remote codec.

 Benefits and advantages of this solution:
 - decoder crash in service process can be handled by callback in app process
 - it uses only public Android API & technology: executing JVM belongs to Android internals and I doubt that it will ever be exposed in SDK or NDK.
 - it could be easier for EME decryptor to work with decoder: Java |MediaCryto| can be used directly
 
 A prototype[4] of said service has been developed as proof of concept.

[1] https://dxr.mozilla.org/mozilla-central/source/old-configure#116
[2] https://developer.android.com/reference/android/view/Surface.html
[3] https://developer.android.com/reference/android/os/Parcel.html#marshall()
[4] https://github.com/jhlin/remote-decoding-test

Comment 4

[Blake Wu [:bwu][:blakewu]]

ni cpearce and snorp for more feedbacks.

Comment 5

[Blake Wu [:bwu][:blakewu]]

London work week could be a good time for us to go through more details and in-depth design reviews.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Thanks John! I agree that a separate Service process is the best way to go, since we need the JVM. Using the NDK stuff would be great, but we have to support down to API 15 right now.

Do you want to use Binder in the GMP plugin to talk to the Service or ipdl?

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Using a non-GMP out-of-process decoding service sounds fine to me.

Comment 8

[John Lin [:jhlin][:jolin]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #6)
> Thanks John! I agree that a separate Service process is the best way to go,
> since we need the JVM. Using the NDK stuff would be great, but we have to
> support down to API 15 right now.
> 
> Do you want to use Binder in the GMP plugin to talk to the Service or ipdl?

 My proposal doesn't involve GMP/IPDL at all. The service and the remote codec it creates are all binders, implemented using AIDL[1]. A thin proxy class (in Java) will use these binders. |MediaCodecDataDecoder| can be rewrite to use this proxy class, or a new |MediaDataDecoder| subclass can be developed.

[1] https://developer.android.com/guide/components/aidl.html

Comment 9

[John Lin [:jhlin][:jolin]]

Attachment: Remote media codec.pdf

Diagram depicting the proposal.

Comment 10

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1257777 - Part 1: AIDL interfaces for remote codec and manager service binders.

Review commit: https://reviewboard.mozilla.org/r/68262/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68262/

Comment 11

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1257777 - Part 2: Implement async codec using API level 16 MediaCodec.

Asynchronous mode was not available until API level 21. To make it transparent to the caller, introduce a wrapper interface that mimics the new API.

Review commit: https://reviewboard.mozilla.org/r/68264/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68264/

Comment 12

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1257777 - Part 3: Implement remote codec, manager service, and parcelables.

Review commit: https://reviewboard.mozilla.org/r/68266/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68266/

Comment 13

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1257777 - Part 4: Implement remote codec proxy.

Review commit: https://reviewboard.mozilla.org/r/68268/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68268/

Comment 14

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1257777 - Part 5: Seperate PDM and data decoders into different files.

Review commit: https://reviewboard.mozilla.org/r/68270/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68270/

Comment 15

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1257777 - Part 6: Implement remote data decoders and enable/disable them with pref.

Review commit: https://reviewboard.mozilla.org/r/68272/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68272/

Comment 16

[John Lin [:jhlin][:jolin]]

Comment on attachment 8776493 [details]
Bug 1257777 - Part 2: Implement async codec using API level 16 MediaCodec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68264/diff/1-2/

Comment 17

[John Lin [:jhlin][:jolin]]

Comment on attachment 8776494 [details]
Bug 1257777 - Part 3: Implement remote codec, manager service, and parcelables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68266/diff/1-2/

Comment 18

[John Lin [:jhlin][:jolin]]

Comment on attachment 8776495 [details]
Bug 1257777 - Part 4: Implement remote codec proxy.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68268/diff/1-2/

Comment 19

[John Lin [:jhlin][:jolin]]

Comment on attachment 8776496 [details]
Bug 1257777 - Part 5: Seperate PDM and data decoders into different files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68270/diff/1-2/

Comment 20

[John Lin [:jhlin][:jolin]]

Comment on attachment 8776497 [details]
Bug 1257777 - Part 6: Implement remote data decoders and enable/disable them with pref.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68272/diff/1-2/

Comment 21

[John Lin [:jhlin][:jolin]]

Comment 15-20 are just for triming trailing white spaces in previous patches.

Comment 22

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8776492 [details]
Bug 1257777 - Part 1: AIDL interfaces for remote codec and manager service binders.

https://reviewboard.mozilla.org/r/68262/#review65344

::: mobile/android/base/Makefile.in:547
(Diff revision 1)
>  endif
>  
>  libs:: classes.dex
>  	$(INSTALL) classes.dex $(FINAL_TARGET)
> +
> +# Generate Java binder interfaces from AIDL files.

Please get Nick Alexander or a build peer to review this. It seems ok to me, but I'm not sure if we want to do it another way.

Eventually we want to build with gradle so this will be a lot easier.

::: mobile/android/base/aidl/org/mozilla/gecko/media/ICodec.aidl:22
(Diff revision 1)
> +    oneway void start();
> +    oneway void stop();
> +    oneway void flush();
> +    oneway void release();
> +
> +    oneway void inputSample(in Sample sample);

Maybe 'queueInput' would be a better name, as it matches the MediaCodec terminology.

Comment 23

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://reviewboard.mozilla.org/r/68264/#review65346

Please have Eugen Sawin review this, as he is the person most familiar with the current AndroidDecoderModule

Comment 24

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8776494 [details]
Bug 1257777 - Part 3: Implement remote codec, manager service, and parcelables.

https://reviewboard.mozilla.org/r/68266/#review65348

Looks mostly ok. Can you have Jim Chen review this too?

::: mobile/android/base/AndroidManifest.xml.in:427
(Diff revision 2)
>  
> +        <service
> +            android:name="org.mozilla.gecko.media.CodecManager"
> +            android:enabled="true"
> +            android:exported="false"
> +            android:process=":mediasvc"

Just :media

::: mobile/android/base/java/org/mozilla/gecko/media/Codec.java:37
(Diff revision 2)
> +    public enum Error {
> +        DECODE, FATAL
> +    };
> +
> +    private final class ImplWorker extends Handler {
> +        private AsyncCodec mImpl;

mImpl (and similar namings) don't seem very good to me. It's an implementation of a Codec, so just call it mCodec? Then createImpl -> createCodec, etc. I think that's more readable, at least.

::: mobile/android/base/java/org/mozilla/gecko/media/Codec.java:48
(Diff revision 2)
> +        }
> +
> +        public void config(final String codecName, final MediaFormat format, final Surface output, final int flags) {
> +            final CountDownLatch lock = new CountDownLatch(1);
> +
> +            post(new Runnable() {

All of this post/latch stuff is so clunky. There has to be a better way.

Actually, what is the point of running everything in a different thread if we're just going to synchronize with it like this?

::: mobile/android/base/java/org/mozilla/gecko/media/Codec.java:63
(Diff revision 2)
> +            });
> +
> +            try {
> +                lock.await();
> +            } catch (InterruptedException e) {
> +                e.printStackTrace();

Need to do something else here, right? Something failed and need to pass that along to the caller.

Comment 25

[John Lin [:jhlin][:jolin]]

Thanks a lot for your prompt review and comments. Will update the patch ASAP.

Comment 26

[John Lin [:jhlin][:jolin]]

https://reviewboard.mozilla.org/r/68266/#review65348

> All of this post/latch stuff is so clunky. There has to be a better way.
> 
> Actually, what is the point of running everything in a different thread if we're just going to synchronize with it like this?

The worker thread plays similar role as MediaCodecDataDecoder.mThread. But on second thought, it should be AsyncCodec's responsibility instead of Codec's. Will move it to JellyBeanAsyncCodec.

Comment 27

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8776497 [details]
Bug 1257777 - Part 6: Implement remote data decoders and enable/disable them with pref.

https://reviewboard.mozilla.org/r/68272/#review66008

Comment 28

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8776495 [details]
Bug 1257777 - Part 4: Implement remote codec proxy.

https://reviewboard.mozilla.org/r/68268/#review66004

Comment 29

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8776496 [details]
Bug 1257777 - Part 5: Seperate PDM and data decoders into different files.

https://reviewboard.mozilla.org/r/68270/#review66006

Comment 30

[John Lin [:jhlin][:jolin]]

Comment on attachment 8776492 [details]
Bug 1257777 - Part 1: AIDL interfaces for remote codec and manager service binders.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68262/diff/1-2/

Comment 31

[John Lin [:jhlin][:jolin]]

Comment on attachment 8776493 [details]
Bug 1257777 - Part 2: Implement async codec using API level 16 MediaCodec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68264/diff/2-3/

Comment 32

[John Lin [:jhlin][:jolin]]

Comment on attachment 8776494 [details]
Bug 1257777 - Part 3: Implement remote codec, manager service, and parcelables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68266/diff/2-3/

Comment 33

[John Lin [:jhlin][:jolin]]

Comment on attachment 8776495 [details]
Bug 1257777 - Part 4: Implement remote codec proxy.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68268/diff/2-3/

Comment 34

[John Lin [:jhlin][:jolin]]

Comment on attachment 8776496 [details]
Bug 1257777 - Part 5: Seperate PDM and data decoders into different files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68270/diff/2-3/

Comment 35

[John Lin [:jhlin][:jolin]]

Comment on attachment 8776497 [details]
Bug 1257777 - Part 6: Implement remote data decoders and enable/disable them with pref.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68272/diff/2-3/

Comment 36

[John Lin [:jhlin][:jolin]]

https://reviewboard.mozilla.org/r/68266/#review65348

> The worker thread plays similar role as MediaCodecDataDecoder.mThread. But on second thought, it should be AsyncCodec's responsibility instead of Codec's. Will move it to JellyBeanAsyncCodec.

In latest patch, Codec doesn't have its own worker thread anymore. Instead, JellyBeanAsyncCodec now have poller thread that internally handles buffer pollling jobs.

> Need to do something else here, right? Something failed and need to pass that along to the caller.

The latest patch got rid of the latch.

Comment 37

[John Lin [:jhlin][:jolin]]

Comment on attachment 8776493 [details]
Bug 1257777 - Part 2: Implement async codec using API level 16 MediaCodec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68264/diff/3-4/

Comment 38

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1257777 - Part 1: AIDL interfaces for remote codec and manager service binders.

Review commit: https://reviewboard.mozilla.org/r/69544/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69544/

Comment 39

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1257777 - Part 3: Implement remote codec, manager service, and parcelables.

Review commit: https://reviewboard.mozilla.org/r/69546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69546/

Comment 40

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1257777 - Part 4: Implement remote codec proxy.

Review commit: https://reviewboard.mozilla.org/r/69548/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69548/

Comment 41

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1257777 - Part 5: Seperate PDM and data decoders into different files.

Review commit: https://reviewboard.mozilla.org/r/69550/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69550/

Comment 42

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1257777 - Part 6: Implement remote data decoders and enable/disable them with pref.

Review commit: https://reviewboard.mozilla.org/r/69552/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69552/

Comment 43

[John Lin [:jhlin][:jolin]]

Comment on attachment 8778204 [details]
Bug 1257777 - Part 4: Implement remote codec proxy.

https://reviewboard.mozilla.org/r/69548/#review66678

Put back snorp's r+ removed by my mistake.

Comment 44

[John Lin [:jhlin][:jolin]]

Comment on attachment 8778205 [details]
Bug 1257777 - Part 5: Seperate PDM and data decoders into different files.

https://reviewboard.mozilla.org/r/69550/#review66680

Restore snorp's r+ removed by mistake.

Comment 45

[John Lin [:jhlin][:jolin]]

Comment on attachment 8778206 [details]
Bug 1257777 - Part 6: Implement remote data decoders and enable/disable them with pref.

https://reviewboard.mozilla.org/r/69552/#review66682

Restore snorp's r+ removed by mistake.

Comment 46

[John Lin [:jhlin][:jolin]]

Shouldn't have pushed only updated patch to MozReview. It reset states of all others. Comments above now looks like a mess. :$

Comment 47

[Eugen Sawin [:esawin]]

Comment on attachment 8776493 [details]
Bug 1257777 - Part 2: Implement async codec using API level 16 MediaCodec.

https://reviewboard.mozilla.org/r/68264/#review66712

This is looking great so far!

It would be nice to have some comments on the polling mechanics.

::: mobile/android/base/java/org/mozilla/gecko/media/AsyncCodec.java:15
(Diff revision 4)
> +import android.view.Surface;
> +
> +import java.nio.ByteBuffer;
> +
> +// A wrapper interface that mimics the new {@link android.media.MediaCodec}
> +// asynchronse mode API in Lollipop.

asynchronous or just async

::: mobile/android/base/java/org/mozilla/gecko/media/JellyBeanAsyncCodec.java:21
(Diff revision 4)
> +import java.io.IOException;
> +import java.nio.ByteBuffer;
> +
> +// Implement async API using MediaCodec sync mode.
> +final class JellyBeanAsyncCodec implements AsyncCodec {
> +    private static final String LOG_TAG = JellyBeanAsyncCodec.class.getSimpleName();

It's a close call (1457 vs 1415 occurrences), but let's go with LOGTAG.

As a human, I would prefer a readable string here for quick log filtering.

Generally (out of scope of this bug), I find that our native MOZ_LOG mechanics are more convenient when combined with \_\_func\_\_ compared to our logging in Java. I'm convinced this doesn't need to be the case.

::: mobile/android/base/java/org/mozilla/gecko/media/JellyBeanAsyncCodec.java:126
(Diff revision 4)
> +                case MSG_OUTPUT_FORMAT_CHANGE: // obj: output format.
> +                    mCallbacks.onOutputFormatChanged(JellyBeanAsyncCodec.this,
> +                                                     (MediaFormat)msg.obj);
> +                    break;
> +                case MSG_ERROR: // arg1: error code.
> +                    mCallbacks.onError(JellyBeanAsyncCodec.this, msg.arg1);

We need to make sure that errors are logged somewhere with this LOGTAG, too, independent from the callbacks.

::: mobile/android/base/java/org/mozilla/gecko/media/JellyBeanAsyncCodec.java:136
(Diff revision 4)
> +
> +            return true;
> +        }
> +    }
> +
> +    private final class BufferPoller extends CancelableHandler {

This class could use a description.

::: mobile/android/base/java/org/mozilla/gecko/media/JellyBeanAsyncCodec.java:197
(Diff revision 4)
> +                mCallbackSender.notifyError(result);
> +            }
> +        }
> +
> +        private void pollOutputBuffer() {
> +            boolean more = true;

Could use a more descriptive name.

::: mobile/android/base/java/org/mozilla/gecko/media/JellyBeanAsyncCodec.java:356
(Diff revision 4)
> +
> +    @Override
> +    public void release() {
> +        assertCallbacks();
> +
> +        cancelPendingTasks();

Should we release mCallbackSender here to make sure assertCallbacks fails?

::: mobile/android/base/java/org/mozilla/gecko/media/JellyBeanAsyncCodec.java:361
(Diff revision 4)
> +        cancelPendingTasks();
> +        mCodec.release();
> +        deinitBufferPoller();
> +    }
> +
> +    private void deinitBufferPoller() {

Maybe better {release|stop}BufferPoller?

::: mobile/android/base/java/org/mozilla/gecko/media/JellyBeanAsyncCodec.java:368
(Diff revision 4)
> +            Log.e(LOG_TAG, "no initialized poller.");
> +            return;
> +        }
> +
> +        mBufferPoller.getLooper().quit();
> +        mBufferPoller = null;

I assume releasing the HandlerThread reference will cleanly shut it down (via quit)?

Comment 48

[(inactive) Jim Chen [:jchen] [:darchons]]

https://reviewboard.mozilla.org/r/69546/#review66754

::: mobile/android/base/java/org/mozilla/gecko/media/Codec.java:65
(Diff revision 1)
> +                e.printStackTrace();
> +            }
> +            mCodec.releaseOutputBuffer(index, true);
> +            boolean eos = (info.flags & MediaCodec.BUFFER_FLAG_END_OF_STREAM) != 0;
> +            if (eos) {
> +                Log.d(LOG_TAG, "output EOS");

Add `private static final boolean DEBUG = false;` to this class, and log here only when `DEBUG` is true.

::: mobile/android/base/java/org/mozilla/gecko/media/Codec.java:71
(Diff revision 1)
> +            }
> +        }
> +
> +        private void outputDummy(MediaCodec.BufferInfo info) {
> +            try {
> +                Log.d(LOG_TAG, "return dummy sample");

Same here and other places.

Comment 49

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8778203 [details]
Bug 1257777 - Part 3: Implement remote codec, manager service, and parcelables.

The service looks good, but I think someone else should review the logic in Codec, etc.

Comment 50

[Nick Alexander :nalexander [he/him]]

https://reviewboard.mozilla.org/r/69544/#review66776

Comment 51

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8778204 [details]
Bug 1257777 - Part 4: Implement remote codec proxy.

https://reviewboard.mozilla.org/r/69548/#review66822

Comment 52

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8778205 [details]
Bug 1257777 - Part 5: Seperate PDM and data decoders into different files.

https://reviewboard.mozilla.org/r/69550/#review66824

Comment 53

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8778206 [details]
Bug 1257777 - Part 6: Implement remote data decoders and enable/disable them with pref.

https://reviewboard.mozilla.org/r/69552/#review66826

Comment 54

[John Lin [:jhlin][:jolin]]

Comment on attachment 8776493 [details]
Bug 1257777 - Part 2: Implement async codec using API level 16 MediaCodec.

https://reviewboard.mozilla.org/r/68264/#review66712

Thanks a lot for your comments. Will update the patch accordingly before landing.

> It's a close call (1457 vs 1415 occurrences), but let's go with LOGTAG.
> 
> As a human, I would prefer a readable string here for quick log filtering.
> 
> Generally (out of scope of this bug), I find that our native MOZ_LOG mechanics are more convenient when combined with \_\_func\_\_ compared to our logging in Java. I'm convinced this doesn't need to be the case.

> As a human, I would prefer a readable string here for quick log filtering.

 I'm really bad at naming... How about 'GeckoAsyncCodecAPIv16'?

> I assume releasing the HandlerThread reference will cleanly shut it down (via quit)?

Yes. Looper.quit() will end Looper.loop() and quit the thread. HandlerThread object should be GC'ed along with the Handler and Looper.

Comment 55

[Eugen Sawin [:esawin]]

(In reply to John Lin [:jolin][:jhlin] from comment #54)
>  I'm really bad at naming... How about 'GeckoAsyncCodecAPIv16'?

Sounds good to me.

Comment 56

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8778202 [details]
Bug 1257777 - Part 1: AIDL interfaces for remote codec and manager service binders.

https://reviewboard.mozilla.org/r/69544/#review67208

I'll follow on to my earlier questions: just land this as you see fit if it doesn't break the Gradle build.  If it does, mcomella and sebastian can help get that working.  I'm going on PTO in a day; I don't want to hold this up until I get back.  We can update things for GeckoView, etc, later if we need to.

Comment 57

[John Lin [:jhlin][:jolin]]

Comment on attachment 8778202 [details]
Bug 1257777 - Part 1: AIDL interfaces for remote codec and manager service binders.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69544/diff/1-2/

Comment 58

[John Lin [:jhlin][:jolin]]

Comment on attachment 8776493 [details]
Bug 1257777 - Part 2: Implement async codec using API level 16 MediaCodec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68264/diff/4-5/

Comment 59

[John Lin [:jhlin][:jolin]]

Comment on attachment 8778203 [details]
Bug 1257777 - Part 3: Implement remote codec, manager service, and parcelables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69546/diff/1-2/

Comment 60

[John Lin [:jhlin][:jolin]]

Comment on attachment 8778204 [details]
Bug 1257777 - Part 4: Implement remote codec proxy.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69548/diff/1-2/

Comment 61

[John Lin [:jhlin][:jolin]]

Comment on attachment 8778205 [details]
Bug 1257777 - Part 5: Seperate PDM and data decoders into different files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69550/diff/1-2/

Comment 62

[John Lin [:jhlin][:jolin]]

Comment on attachment 8778206 [details]
Bug 1257777 - Part 6: Implement remote data decoders and enable/disable them with pref.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69552/diff/1-2/

Comment 63

[John Lin [:jhlin][:jolin]]

Comment on attachment 8778202 [details]
Bug 1257777 - Part 1: AIDL interfaces for remote codec and manager service binders.

https://reviewboard.mozilla.org/r/69544/#review67208

Thanks a lot. Gradle build break is fixed by adding the AIDL path to gradle buid file.

Comment 64

[John Lin [:jhlin][:jolin]]

Comment on attachment 8776493 [details]
Bug 1257777 - Part 2: Implement async codec using API level 16 MediaCodec.

https://reviewboard.mozilla.org/r/68264/#review66712

Patch v5 addessed review comments and some bugs:
- fix a problem that mInputEnded not reset when new input is queued
- fix crash when playing m4v files by catching IllegalStateException for dequeue & queue buffer methods

Comment 65

[John Lin [:jhlin][:jolin]]

Comment on attachment 8778203 [details]
Bug 1257777 - Part 3: Implement remote codec, manager service, and parcelables.

https://reviewboard.mozilla.org/r/69546/#review66754

v2 addressed review comments and fixed a bug that Sample.byteArrayFromBuffer() didn't work when offset is non-zero.

Comment 66

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8778203 [details]
Bug 1257777 - Part 3: Implement remote codec, manager service, and parcelables.

https://reviewboard.mozilla.org/r/69546/#review69206

Comment 67

[John Lin [:jhlin][:jolin]]

Comment on attachment 8778202 [details]
Bug 1257777 - Part 1: AIDL interfaces for remote codec and manager service binders.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69544/diff/2-3/

Comment 68

[John Lin [:jhlin][:jolin]]

Comment on attachment 8776493 [details]
Bug 1257777 - Part 2: Implement async codec using API level 16 MediaCodec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68264/diff/5-6/

Comment 69

[John Lin [:jhlin][:jolin]]

Comment on attachment 8778203 [details]
Bug 1257777 - Part 3: Implement remote codec, manager service, and parcelables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69546/diff/2-3/

Comment 70

[John Lin [:jhlin][:jolin]]

Comment on attachment 8778204 [details]
Bug 1257777 - Part 4: Implement remote codec proxy.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69548/diff/2-3/

Comment 71

[John Lin [:jhlin][:jolin]]

Comment on attachment 8778205 [details]
Bug 1257777 - Part 5: Seperate PDM and data decoders into different files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69550/diff/2-3/

Comment 72

[John Lin [:jhlin][:jolin]]

Comment on attachment 8778206 [details]
Bug 1257777 - Part 6: Implement remote data decoders and enable/disable them with pref.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69552/diff/2-3/

Comment 73

[John Lin [:jhlin][:jolin]]

Comment 70 rebased patch part 4 for bug 1292323.

Comment 74

[John Lin [:jhlin][:jolin]]

Comment on attachment 8778202 [details]
Bug 1257777 - Part 1: AIDL interfaces for remote codec and manager service binders.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69544/diff/3-4/

Comment 75

[John Lin [:jhlin][:jolin]]

Comment on attachment 8776493 [details]
Bug 1257777 - Part 2: Implement async codec using API level 16 MediaCodec.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68264/diff/6-7/

Comment 76

[John Lin [:jhlin][:jolin]]

Comment on attachment 8778203 [details]
Bug 1257777 - Part 3: Implement remote codec, manager service, and parcelables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69546/diff/3-4/

Comment 77

[John Lin [:jhlin][:jolin]]

Comment on attachment 8778204 [details]
Bug 1257777 - Part 4: Implement remote codec proxy.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69548/diff/3-4/

Comment 78

[John Lin [:jhlin][:jolin]]

Comment on attachment 8778205 [details]
Bug 1257777 - Part 5: Seperate PDM and data decoders into different files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69550/diff/3-4/

Comment 79

[John Lin [:jhlin][:jolin]]

Comment on attachment 8778206 [details]
Bug 1257777 - Part 6: Implement remote data decoders and enable/disable them with pref.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69552/diff/3-4/

Comment 80

[John Lin [:jhlin][:jolin]]

Comment 79 changed the way to access preference value:
- define the default pref value in mobile.js instead of all.js
- use MediaPrefs rather than Preferences method, which requires to be run only on main thread.

Thanks to JamesCheng's question that makes me notice the problem.

Comment 81

[John Lin [:jhlin][:jolin]]

Comment on attachment 8778206 [details]
Bug 1257777 - Part 6: Implement remote data decoders and enable/disable them with pref.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69552/diff/4-5/

Comment 82

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d7c03e732b9
Part 1: AIDL interfaces for remote codec and manager service binders. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/1d86193c7b9a
Part 2: Implement async codec using API level 16 MediaCodec. r=esawin
https://hg.mozilla.org/integration/autoland/rev/8c1fffb47bc8
Part 3: Implement remote codec, manager service, and parcelables. r=jchen
https://hg.mozilla.org/integration/autoland/rev/ec903c5415a2
Part 4: Implement remote codec proxy. r=snorp
https://hg.mozilla.org/integration/autoland/rev/b264b46cbb45
Part 5: Seperate PDM and data decoders into different files. r=snorp
https://hg.mozilla.org/integration/autoland/rev/bfe241c22c27
Part 6: Implement remote data decoders and enable/disable them with pref. r=snorp

Comment 83

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0d7c03e732b9
https://hg.mozilla.org/mozilla-central/rev/1d86193c7b9a
https://hg.mozilla.org/mozilla-central/rev/8c1fffb47bc8
https://hg.mozilla.org/mozilla-central/rev/ec903c5415a2
https://hg.mozilla.org/mozilla-central/rev/b264b46cbb45
https://hg.mozilla.org/mozilla-central/rev/bfe241c22c27

Comment 84

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8778202 [details]
Bug 1257777 - Part 1: AIDL interfaces for remote codec and manager service binders.

https://reviewboard.mozilla.org/r/69544/#review71094

Comment 85

[Rachelle Yang [:ryang][[e-mail]]]

Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:

Comment 86

[Bogdan Surd, QA [:bsurd, NI]]

Devices:
 - Xiaomi Mi Pad 2 (Android 5.1)

Hello,

I've verified this issue, and it seems to be working just fine. I've documented the process that I used with steps here: https://goo.gl/vSqrKe. The fix seems to be working just fine. The only bump along the way was that I had to restart the app in order for the preference to take effect.

Comment 87

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx53 Aurora release notes.

Comment 88

[Julien Cristau [:jcristau]]

This was disabled for 53 in bug 1350209, and moved to the 54 relnotes.