Closed
Bug 1257777
Opened 9 years ago
Closed 8 years ago
Move Android audio/video decoder out of application process
Categories
(Firefox for Android Graveyard :: Audio/Video, defect, P2)
Firefox for Android Graveyard
Audio/Video
Tracking
(relnote-firefox 54+, firefox51 verified)
RESOLVED
FIXED
Firefox 52
People
(Reporter: cpearce, Assigned: jhlin)
References
Details
(Keywords: feature)
Attachments
(7 files, 5 obsolete files)
(deleted),
application/pdf
|
Details | |
(deleted),
text/x-review-board-request
|
esawin
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
nalexander
:
review+
snorp
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jchen
:
review+
snorp
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
snorp
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
snorp
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
snorp
:
review+
|
Details |
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.
Updated•8 years ago
|
Assignee: nobody → jolin
Updated•8 years ago
|
Priority: -- → P2
Updated•8 years ago
|
Severity: normal → major
Comment 1•8 years ago
|
||
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•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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
Updated•8 years ago
|
Flags: needinfo?(snorp)
Comment 5•8 years ago
|
||
London work week could be a good time for us to go through more details and in-depth design reviews.
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?
Flags: needinfo?(snorp) → needinfo?(jolin)
Reporter | ||
Comment 7•8 years ago
|
||
Using a non-GMP out-of-process decoding service sounds fine to me.
Flags: needinfo?(cpearce)
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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
Flags: needinfo?(jolin)
Assignee | ||
Comment 9•8 years ago
|
||
Diagram depicting the proposal.
Assignee | ||
Updated•8 years ago
|
Summary: Move Android audio/video decoder to GMP → Move Android audio/video decoder out of application process
Assignee | ||
Comment 10•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/68262/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68262/
Attachment #8776492 -
Flags: review?(snorp)
Attachment #8776493 -
Flags: review?(snorp)
Attachment #8776494 -
Flags: review?(snorp)
Attachment #8776495 -
Flags: review?(snorp)
Attachment #8776496 -
Flags: review?(snorp)
Attachment #8776497 -
Flags: review?(snorp)
Assignee | ||
Comment 11•8 years ago
|
||
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/
Assignee | ||
Comment 12•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/68266/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68266/
Assignee | ||
Comment 13•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/68268/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68268/
Assignee | ||
Comment 14•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/68270/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68270/
Assignee | ||
Comment 15•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/68272/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68272/
Assignee | ||
Comment 16•8 years ago
|
||
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/
Assignee | ||
Comment 17•8 years ago
|
||
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/
Assignee | ||
Comment 18•8 years ago
|
||
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/
Assignee | ||
Comment 19•8 years ago
|
||
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/
Assignee | ||
Comment 20•8 years ago
|
||
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/
Assignee | ||
Comment 21•8 years ago
|
||
Comment 15-20 are just for triming trailing white spaces in previous patches.
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.
Attachment #8776492 -
Flags: review?(snorp) → review+
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 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.
Attachment #8776494 -
Flags: review?(snorp) → review+
Assignee | ||
Comment 25•8 years ago
|
||
Thanks a lot for your prompt review and comments. Will update the patch ASAP.
Assignee | ||
Comment 26•8 years ago
|
||
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.
Attachment #8776497 -
Flags: review?(snorp) → review+
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 on attachment 8776495 [details]
Bug 1257777 - Part 4: Implement remote codec proxy.
https://reviewboard.mozilla.org/r/68268/#review66004
Attachment #8776495 -
Flags: review?(snorp) → review+
Comment on attachment 8776496 [details]
Bug 1257777 - Part 5: Seperate PDM and data decoders into different files.
https://reviewboard.mozilla.org/r/68270/#review66006
Attachment #8776496 -
Flags: review?(snorp) → review+
Assignee | ||
Comment 30•8 years ago
|
||
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/
Attachment #8776492 -
Flags: review+ → review?(nalexander)
Attachment #8776493 -
Flags: review?(snorp) → review?(esawin)
Attachment #8776494 -
Flags: review+ → review?(nchen)
Assignee | ||
Comment 31•8 years ago
|
||
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/
Assignee | ||
Comment 32•8 years ago
|
||
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/
Assignee | ||
Comment 33•8 years ago
|
||
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/
Assignee | ||
Comment 34•8 years ago
|
||
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/
Assignee | ||
Comment 35•8 years ago
|
||
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/
Assignee | ||
Comment 36•8 years ago
|
||
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.
Assignee | ||
Comment 37•8 years ago
|
||
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/
Assignee | ||
Updated•8 years ago
|
Attachment #8776492 -
Attachment is obsolete: true
Attachment #8776492 -
Flags: review?(nalexander)
Assignee | ||
Updated•8 years ago
|
Attachment #8776494 -
Attachment is obsolete: true
Attachment #8776494 -
Flags: review?(nchen)
Assignee | ||
Updated•8 years ago
|
Attachment #8776495 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8776496 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8776497 -
Attachment is obsolete: true
Assignee | ||
Comment 38•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/69544/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69544/
Attachment #8778202 -
Flags: review?(nalexander)
Attachment #8778203 -
Flags: review?(nchen)
Attachment #8778204 -
Flags: review?(snorp)
Attachment #8778205 -
Flags: review?(snorp)
Attachment #8778206 -
Flags: review?(snorp)
Assignee | ||
Comment 39•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/69546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69546/
Assignee | ||
Comment 40•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/69548/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69548/
Assignee | ||
Comment 41•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/69550/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69550/
Assignee | ||
Comment 42•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/69552/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69552/
Assignee | ||
Comment 43•8 years ago
|
||
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.
Attachment #8778204 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8778205 -
Flags: review+
Assignee | ||
Comment 44•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Attachment #8778206 -
Flags: review+
Assignee | ||
Comment 45•8 years ago
|
||
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.
Assignee | ||
Comment 46•8 years ago
|
||
Shouldn't have pushed only updated patch to MozReview. It reset states of all others. Comments above now looks like a mess. :$
Comment 47•8 years ago
|
||
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)?
Attachment #8776493 -
Flags: review?(esawin) → review+
Comment 48•8 years ago
|
||
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•8 years ago
|
||
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.
Attachment #8778203 -
Flags: review?(nchen) → review+
Comment 50•8 years ago
|
||
Attachment #8778204 -
Flags: review?(snorp) → review+
Comment on attachment 8778204 [details]
Bug 1257777 - Part 4: Implement remote codec proxy.
https://reviewboard.mozilla.org/r/69548/#review66822
Comment on attachment 8778205 [details]
Bug 1257777 - Part 5: Seperate PDM and data decoders into different files.
https://reviewboard.mozilla.org/r/69550/#review66824
Attachment #8778205 -
Flags: review?(snorp) → review+
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
Attachment #8778206 -
Flags: review?(snorp) → review+
Assignee | ||
Comment 54•8 years ago
|
||
mozreview-review-reply |
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•8 years ago
|
||
(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•8 years ago
|
||
mozreview-review |
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.
Attachment #8778202 -
Flags: review?(nalexander) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 63•8 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 64•8 years ago
|
||
mozreview-review-reply |
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
Assignee | ||
Comment 65•8 years ago
|
||
mozreview-review-reply |
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•8 years ago
|
||
mozreview-review |
Comment on attachment 8778203 [details]
Bug 1257777 - Part 3: Implement remote codec, manager service, and parcelables.
https://reviewboard.mozilla.org/r/69546/#review69206
Attachment #8778203 -
Flags: review?(nchen) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 73•8 years ago
|
||
Comment 70 rebased patch part 4 for bug 1292323.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 80•8 years ago
|
||
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 hidden (mozreview-request) |
Comment 82•8 years ago
|
||
Pushed by jolin@mozilla.com:
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•8 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Comment 84•8 years ago
|
||
mozreview-review |
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
Attachment #8778202 -
Flags: review?(snorp) → review+
Attachment #8778203 -
Flags: review?(snorp) → review+
Comment 85•8 years ago
|
||
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:
relnote-firefox:
--- → ?
Updated•8 years ago
|
relnote-firefox:
? → ---
Updated•8 years ago
|
Target Milestone: Firefox 51 → Firefox 52
Updated•8 years ago
|
Blocks: Widevine_on_Fennec
Comment 86•8 years ago
|
||
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.
Added to Fx53 Aurora release notes.
relnote-firefox:
--- → 53+
Comment 88•7 years ago
|
||
This was disabled for 53 in bug 1350209, and moved to the 54 relnotes.
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
Updated•3 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•