Last Comment Bug 1014614 - (mediacodec) Use android.media.MediaCodec for decoding/encoding on recent Android
(mediacodec)
: Use android.media.MediaCodec for decoding/encoding on recent Android
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: unspecified
: All Mac OS X
-- normal with 1 vote (vote)
: mozilla36
Assigned To: James Willcox (:snorp) (jwillcox@mozilla.com)
:
: Maire Reavy [:mreavy] Please needinfo me
Mentors:
: 941301 (view as bug list)
Depends on: 1037147 1089261 1089262 1089423 1089654 1090300 1091597 1093641 1100126 1101330 1106108 1133645
Blocks: 1005436 1052888 1084456 1084514 1084607 1085432 1085744 1086693 1089872 1091599 1097276 1107377 1109624
  Show dependency treegraph
 
Reported: 2014-05-22 08:21 PDT by James Willcox (:snorp) (jwillcox@mozilla.com)
Modified: 2015-02-16 21:54 PST (History)
22 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
+
fixed
35+


Attachments
MediaCodec PlatfromDecoderModule (292.08 KB, patch)
2014-07-18 12:44 PDT, Andrew Martin McDonough (:mmcdonough)
no flags Details | Diff | Splinter Review
MediaCodec PDM with H264 and AAC working (362.47 KB, patch)
2014-08-06 16:47 PDT, Andrew Martin McDonough (:mmcdonough)
no flags Details | Diff | Splinter Review
MediaCodec PDM with H264 and AAC (151.09 KB, patch)
2014-08-12 15:43 PDT, Andrew Martin McDonough (:mmcdonough)
no flags Details | Diff | Splinter Review
PDM using Android MediaCodec (132.02 KB, patch)
2014-08-21 16:13 PDT, Andrew Martin McDonough (:mmcdonough)
cpearce: review-
Details | Diff | Splinter Review
mediacodec-14-09-18.patch (129.45 KB, patch)
2014-09-18 13:59 PDT, Andrew Martin McDonough (:mmcdonough)
cpearce: feedback+
Details | Diff | Splinter Review
0001-Bug-1014614-Rename-nsSurfaceTexture-to-AndroidSurfac.patch (31.88 KB, patch)
2014-10-02 12:49 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
jgilbert: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review
0002-Bug-1014614-Expose-Android-native-window-via-Android.patch (12.32 KB, patch)
2014-10-02 12:51 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
blassey.bugs: review+
Details | Diff | Splinter Review
0003-Bug-1014614-Expose-more-SurfaceTexture-API-in-Androi.patch (3.62 KB, patch)
2014-10-02 12:51 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
blassey.bugs: review+
Details | Diff | Splinter Review
0004-Bug-1014614-Do-not-try-to-use-a-temporary-texture-fo.patch (1.19 KB, patch)
2014-10-02 12:52 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
jgilbert: review-
Details | Diff | Splinter Review
0005-Bug-1014614-Fix-JNI-wrapper-for-registering-SurfaceT.patch (1.65 KB, patch)
2014-10-02 12:53 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
blassey.bugs: review+
Details | Diff | Splinter Review
0006-Bug-1014614-Use-Android-MediaCodec-for-decoding-H264.patch (104.07 KB, patch)
2014-10-02 13:16 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
cpearce: review+
edwin.bugs: review+
Details | Diff | Splinter Review
0007-Bug-1014614-Fix-readback-of-SurfaceTexture-backed-vi.patch (13.01 KB, patch)
2014-10-03 13:27 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
no flags Details | Diff | Splinter Review
0007-Bug-1014614-Fix-readback-of-SurfaceTexture-backed-vi.patch (13.35 KB, patch)
2014-10-03 14:03 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
jgilbert: review-
Details | Diff | Splinter Review
0006-Bug-1014614-Support-attach-detach-of-GLContext-to-An.patch (10.76 KB, patch)
2014-10-14 11:34 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
jgilbert: review+
Details | Diff | Splinter Review
0007-Bug-1014614-Use-Android-MediaCodec-for-decoding-H264.patch (104.05 KB, patch)
2014-10-14 11:34 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
snorp: review+
Details | Diff | Splinter Review
0008-Bug-1014614-Add-GLBlitHelper-BlitImageToFramebuffer-.patch (11.23 KB, patch)
2014-10-14 11:35 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
jgilbert: review+
Details | Diff | Splinter Review
0009-Bug-1014614-Fix-readback-of-SurfaceTextureImage.patch (3.41 KB, patch)
2014-10-14 11:36 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
jgilbert: review+
Details | Diff | Splinter Review
Remove mutex in GLImages.cpp (988 bytes, patch)
2014-10-20 18:21 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
jgilbert: review+
Details | Diff | Splinter Review

Description User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-05-22 08:21:01 PDT
Android, as of JellyBean, has a blessed Java API for decoding/encoding media. In theory, relying on this where we can will be much more reliable.

http://developer.android.com/reference/android/media/MediaCodec.html
Comment 1 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-06-05 19:19:02 PDT
We'll also need to use MediaExtractor for demux: http://developer.android.com/reference/android/media/MediaExtractor.html

Our intern Martin is working on generating automatic JNI bindings for those classes, so I'll assign this to him.
Comment 2 User image Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin] 2014-06-05 19:24:26 PDT
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #1)
> We'll also need to use MediaExtractor for demux:
> http://developer.android.com/reference/android/media/MediaExtractor.html

We've forked stagefright's demuxer for use on all platforms. See bug 908503.

All that needs to be done here is adding a MediaCodec implementation of the PlatformDecoderModule interface.
Comment 3 User image Anthony Jones (:kentuckyfriedtakahe, :k17e) 2014-06-05 20:05:25 PDT
Can we close this as duplicate of bug 941301?
Comment 4 User image Andrew Martin McDonough (:mmcdonough) 2014-07-18 12:44:26 PDT
Created attachment 8458890 [details] [diff] [review]
MediaCodec PlatfromDecoderModule

This patch demonstrates using MediaCodec in a PDM. It will crash on decoding AAC, so it _must_ be run on MP4 files that _only_ have video streams.
Comment 5 User image Anthony Jones (:kentuckyfriedtakahe, :k17e) 2014-07-20 15:26:50 PDT
Land it with the AAC decoding disabled then land the AAC separately.
Comment 6 User image Anthony Jones (:kentuckyfriedtakahe, :k17e) 2014-07-20 15:29:07 PDT
*** Bug 941301 has been marked as a duplicate of this bug. ***
Comment 7 User image Andrew Martin McDonough (:mmcdonough) 2014-07-22 15:12:30 PDT
An APK with this patch applied can be found here:
http://flyingjester.site11.com/extra/fennec-33.0a1.en-US.android-arm.apk

I've hosted a couple of doctored videos (no audio streams) here:
http://flyingjester.site11.com/extra
Comment 8 User image Andrew Martin McDonough (:mmcdonough) 2014-08-06 16:47:58 PDT
Created attachment 8468896 [details] [diff] [review]
MediaCodec PDM with H264 and AAC working

This new patch adds the code to make AAC audio decoding work. It also makes the output properly task-based instead of basically polling (input loading was always properly task-based). It also makes seeking work much better, as well as reducing overhead, particularly when multiple streams are open.
Comment 9 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-08-08 11:56:55 PDT
Comment on attachment 8468896 [details] [diff] [review]
MediaCodec PDM with H264 and AAC working

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

Are there somehow two (or three) copies of each file in this patch? The review software seems to think so, at least.

Lets get the java bindings stuff split into another patch.

::: content/media/fmp4/androidmc/AndroidDecoderModule.cpp
@@ +23,4 @@
>  
>  namespace mp4_demuxer{
>  class Adts{
>  public:

Why can't you include that?

@@ +38,2 @@
>  #include <cstdio>
>  

I assume this the contents of Adts.h? Naughty!

@@ +123,5 @@
> +      PR_AtomicSet(&mSleeping, 1);
> +    }
> +
> +    if(PR_AtomicAdd(&mSleeping, 0)!=0){
> +

Why do you need a scope here?

@@ +124,5 @@
> +    }
> +
> +    if(PR_AtomicAdd(&mSleeping, 0)!=0){
> +
> +      mTaskQueue->AwaitIdle();

just 'unloader' is fine

@@ +127,5 @@
> +
> +      mTaskQueue->AwaitIdle();
> +
> +      PR_AtomicSet(&mSleeping, 0);
> +

lData -> data or even inputData

@@ +134,5 @@
>                                         mDecoder, mCallback, mConfig,
>                                         mImageContainer, mFormat, mVideoFormat,
>                                         mSynchro, mTaskQueue);
>  
>        RefPtr<nsIRunnable> rout(rLoader);

You don't need the separate variable here, you can assign to the RefPtr when it's declared:

RefPtr<AndroidInputTask> unloader = new AndroidInputTask();

@@ +142,3 @@
>  
>      return NS_OK;
>    }

Same

@@ +156,5 @@
>    MCAudioDataDecoder(const mp4_demuxer::AudioDecoderConfig& aConfig,
>                       MediaFormat *aFormat, MediaDataDecoderCallback* aCallback,
>                       MediaTaskQueue* aAudioTaskQueue)
>    : MCDataDecoder(MediaData::Type::AUDIO_SAMPLES, aConfig.mime_type, aFormat,
>                    aCallback, aAudioTaskQueue)

Are you sure you're supposed to delete this? It seems weird to free data that was passed in here.

@@ +165,5 @@
>  
>    virtual nsresult Input(mp4_demuxer::MP4Sample* aSample){
>  
> +     mp4_demuxer::Adts::ConvertEsdsToAdts(mConfig.channel_count,
> +                                          mConfig.frequency_index,

In general I'd prefer names like MediaCodecAudioDecoder

@@ +267,2 @@
>    mcdecode::Format *lFormat = MediaFormat::Wrap(lJFormat);
>  

mimeType

@@ +267,5 @@
>    mcdecode::Format *lFormat = MediaFormat::Wrap(lJFormat);
>  
>    if(lFormat == nullptr)
>      return nullptr;
>  

Just jFormat or format

@@ +327,5 @@
>  
>        FantasyFaeryPixieHorses[0] = aConfig.audio_specific_config[0];
>        FantasyFaeryPixieHorses[1] = aConfig.audio_specific_config[1];
>  
>        jobject lBuffer = lEnv->NewDirectByteBuffer(FantasyFaeryPixieHorses, 2);

wat

@@ +438,5 @@
>    return NS_OK;
>  
>  }
>  
>    // Inserts a sample into the decoder's decode pipeline. The decoder must

So can you nuke all of this?
Comment 10 User image Andrew Martin McDonough (:mmcdonough) 2014-08-12 15:43:48 PDT
Created attachment 8471934 [details] [diff] [review]
MediaCodec PDM with H264 and AAC

Similar to last patch, but separates out the JNI generator Java code, removes unnecessary calls for ESDS/ADTS conversion, and has better naming.
Comment 11 User image Andrew Martin McDonough (:mmcdonough) 2014-08-15 11:38:27 PDT
A demo APK (that has this patch applied) can be found at 
http://flyingjester.site11.com/extra/fennec-34.0a1.en-US.android-arm.apk

Some test videos and files can be found at:

http://pearce.org.nz/video/h264.html
http://flyingjester.site11.com/extra/
Comment 12 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-08-19 14:40:09 PDT
Aaron can you run that APK through some of your devices and report back? We care about MP4/H264/AAC.
Comment 13 User image Aaron Train [:aaronmt] 2014-08-20 06:52:58 PDT
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #12)
> Aaron can you run that APK through some of your devices and report back? We
> care about MP4/H264/AAC.

Grim.

Devices on me today:

Nexus 5 (4.4.4) + Galaxy S4 (4.4.2, GPE) + Nexus 4 (4.4.2) + HTC One (4.4.2, M7) + Note II (4.4.2) + Galaxy S5 (4.4.4) + Nexus 7 (4.4.4, 2013)

MP4/H.264/AAC - All fail, "No video with supported format and MIME type found". Also fails with force-stagefright enabled
Comment 14 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-08-20 07:24:21 PDT
Bummer. I wonder if there is a pref you need to set? Martin uses a Nexus 5 himself so that one should've worked at least.
Comment 15 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-08-20 07:26:39 PDT
Glancing at the patch, it looks like you may need this:

media.fragmented-mp4.exposed = true
media.fragmented-mp4.use-blank-decoder = true

That second one is probably just a bug

Also you don't want to force stagefright, as that's related to the old omx-plugin stuff.
Comment 16 User image Andrew Martin McDonough (:mmcdonough) 2014-08-20 08:36:52 PDT
Sorry, I forgot!
Yes, there is a pref(s). You will need:

media.fragmented-mp4.exposed = true
media.fragmented-mp4.android-media-codec.enabled = true
media.fragmented-mp4.android-media-codec.preferred = true

I don't think that 

media.fragmented-mp4.use-blank-decoder = true

is necessary, but I do have it set on my device.
Comment 17 User image Aaron Train [:aaronmt] 2014-08-20 08:44:56 PDT
Mind posting an updated APK with these preferences set?
Comment 18 User image Andrew Martin McDonough (:mmcdonough) 2014-08-20 10:40:04 PDT
No problem. The APK at
http://flyingjester.site11.com/extra/fennec-34.0a1.en-US.android-arm.apk
has been updated, and it should have these preferences enabled by default.
Comment 19 User image Aaron Train [:aaronmt] 2014-08-20 11:57:20 PDT
Thanks. Round II

Using the content on my media page (only looking for playback problems) with the devices I have currently:

http://people.mozilla.org/~atrain/mobile/tests/media.html

* Samsung Galaxy S5 (Android 4.4.4, Touchwiz) & LG Nexus 5 (Android 4.4.4)
** H.264/MP4/AAC, no problems in playback

* Samsung Galaxy Note II (Android 4.4.2)
** H.264/MP4, very bad colour conversion, video content has green and blue overlays (video is broken on trunk on this device bug 1005436), videos are unwatchable but they do start playing with this APK
** AAC, no problems

* Samsung Galaxy S4 (Android 4.4.2, Google Play Edition) & Nexus 4 (Android 4.4.4) & HTC One (Android 4.4.2, M7)
** H.264/MP4, video content is all white (works fine on trunk Nightly), video playback does start. Unwatchable.
** AAC, no problems
Comment 20 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-08-20 12:11:25 PDT
Aaron that Note II with KK doesn't work at all with Nightly, right?

The corrupted image issues are almost certainly a YUV handling problem. Martin is aware that there are cases that we don't handle yet.
Comment 21 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-08-20 13:08:05 PDT
I tried it on my Galaxy Nexus and got no video output. Output format was TI_FormatYUV420PackedSemiPlanar.
Comment 22 User image Aaron Train [:aaronmt] 2014-08-20 13:20:05 PDT
HTC One (M7)/Galaxy S4/Nexus 4 ->  0x7fa30c03
Comment 23 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-08-20 13:33:32 PDT
Comment on attachment 8471934 [details] [diff] [review]
MediaCodec PDM with H264 and AAC

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

Really coming along. The locking/messaging in the AndroidDecoderModule seems a little weird to me. I'm going to have a deeper look later.

::: configure.in
@@ +5153,5 @@
>  dnl = Built-in fragmented MP4 support.
>  dnl ========================================================
> +
> +if test "$OS_TARGET" = Android; then
> +    dnl Testing FMP4 for Android using MediaCodec

s/Testing/Enable/

::: content/media/DecoderTraits.cpp
@@ +400,5 @@
> +      nsDependentCString(aMIMEType).EqualsASCII("video/mp4")){
> +    if(IsMP4SupportedType(nsDependentCString(aMIMEType)))
> +      return CANPLAY_YES;
> +    if(Preferences::GetBool("media.fragmented-mp4.exposed", false) &&
> +       Preferences::GetBool("media.fragmented-mp4.use-blank-decoder", false))

We don't actually need use-blank-decoder right?

::: content/media/fmp4/MP4Decoder.cpp
@@ +138,5 @@
>  #ifdef XP_WIN
>           // We have H.264/AAC platform decoders on Windows Vista and up.
>           IsVistaOrLater() ||
> +#elif defined(MOZ_ANDROIDMC)
> +true ||

We need to check the API version here, so add IsJellyBeanOrLater() which does that. You can use AndroidBridge::Bridge()->GetAPIVersion() there.

::: content/media/fmp4/androidmc/AndroidDecoderModule.cpp
@@ +150,5 @@
> +  }
> +
> +  virtual nsresult Input(mp4_demuxer::MP4Sample* aSample){
> +/*
> +     mp4_demuxer::Adts::ConvertEsdsToAdts(mConfig.channel_count,

Nuke this comment

@@ +420,5 @@
> +  PR_AtomicSet(&mShouldDie, 1);
> +  mTaskQueue->Flush();
> +  mVideoInputTaskQueue->Flush();
> +
> +  while(!PR_AtomicAdd(&mDidDie, 0)){

You should just wait on a semaphore here I think?

::: content/media/fmp4/androidmc/AndroidDecoderUtils.cpp
@@ +427,5 @@
> +  void *lDirectBuffer = aEnv->GetDirectBufferAddress(vOutputBuffers[aOutputBufferIndex]);
> +
> +  bool lInterlacedCbCr = false;
> +
> +  if( !(false

Get rid of that, not necessary

@@ +428,5 @@
> +
> +  bool lInterlacedCbCr = false;
> +
> +  if( !(false
> +     || (mVideoFormat.mColorFormat==mcdecode::QCOM_FormatYUV420PackedSemiPlanar32m)

Put the || at the end of the lines

@@ +438,5 @@
> +    printf_stderr("Media Codec: Format 0x%x\n", mVideoFormat.mColorFormat);
> +    MOZ_CRASH("Unknown Format.");
> +  }
> +
> +  if( false

Same as above

@@ +447,5 @@
> +  {
> +    lInterlacedCbCr = true;
> +  }
> +
> +  if( true // Enumerates formats that have specifically been tested.

Same again

@@ +500,5 @@
> +
> +
> +    mCallback->Output(rData);
> +
> +    //printf_stderr("Media Codec: Ending Format. There are %i frames that haven't produced output yet. Timestamp %lld\t Duration %lld.\n", mSynchro->mQueue.size(), lMetaData->time, lMetaData->duration);

Remove

@@ +508,5 @@
> +    mSynchro->mQueue.pop();
> +
> +  }
> +
> +  //mCallback->InputExhausted();

Remove

::: widget/android/AndroidJavaWrappers.cpp
@@ +10,5 @@
>  #include "nsIWidget.h"
>  #include "mozilla/BasicEvents.h"
>  #include "mozilla/TimeStamp.h"
>  #include "mozilla/TouchEvents.h"
> +#include "GeneratedJNIWrappers_2.h"

Lets rename to GeneratedSDKWrappers maybe?

@@ +121,5 @@
>      AndroidRect::InitRectClass(jEnv);
>      AndroidRectF::InitRectFClass(jEnv);
>      AndroidLayerRendererFrame::InitLayerRendererFrameClass(jEnv);
>      InitStubs(jEnv);
> +    InitStubs2(jEnv);

InitSDKStubs?
Comment 24 User image Andrew Martin McDonough (:mmcdonough) 2014-08-20 16:27:28 PDT
A new APK (just rebased) is up at

http://flyingjester.site11.com/extra/fennec-34.0a1.en-US.android-arm.apk

There's also more and better logging when hitting unknown YUV formats (and known ones), which should help to get it working right on more devices.
Comment 25 User image Anthony Jones (:kentuckyfriedtakahe, :k17e) 2014-08-20 17:02:19 PDT
> media.fragmented-mp4.use-blank-decoder = true

This will give you green and beep which is not what you want.
Comment 26 User image Chris Pearce (:cpearce) 2014-08-21 03:40:57 PDT
Comment on attachment 8471934 [details] [diff] [review]
MediaCodec PDM with H264 and AAC

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

The formatting of this patch is not very consistent. Can you run `./mach clang-format` over this patch, or at least clean up the indentation and so forth manually? Your code should conform to our style guide: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

::: content/media/fmp4/androidmc/AndroidDecoderModule.cpp
@@ +30,5 @@
> +namespace mozilla {
> +
> +using mozilla::mcdecode::Decoder;
> +
> +AndroidMCSynchro::AndroidMCSynchro(SampleMetaQueue &aQueue, PRLock *aQueueLock,

Don't use PRLock manually, use mozilla::Monitor, mozilla::Mutex or mozilla::ReentrantMonitor instead.

::: content/media/fmp4/androidmc/AndroidDecoderModule.h
@@ +33,5 @@
> +  PRLock *mBufferLock;
> +
> +  AndroidMCSynchro(SampleMetaQueue &aQueue, PRLock *aQueueLock,
> +                  PRLock *aBufferLock, PRInt32 &aShouldFlush,
> +                  PRInt32 &aDidFlush, PRInt32 &aShouldDie, PRInt32 &aDidDie,

int32_t, don't PRInt32 and other PR types.

@@ +47,5 @@
> +  PRInt32 &mSleeping;
> +public:
> +
> +  inline void SetShouldDie(bool aShould){
> +    PR_AtomicSet(&mShouldDie, (aShould)?1:0);

In general, atomics are a bad idea, as they're very, very hard to get right. When you access an atomic value, there's nothing to stop some other thread immediately changing the values you just read atomically, invalidating the decision you just made.

Whereas if you take a lock, you can at least be sure that your view of the shared data is stable while you hold the lock.

I'd feel better if you just required this object to be locked while accessing and making decisions based on its shared state, like CDMCapabilities for example.

Though, in general you're better off communicating using tasks, rather than with raw atomic signals, but it's getting later here so I'll have to look over your code more tomorrow to see if that'd work.
Comment 27 User image Aaron Train [:aaronmt] 2014-08-21 07:32:32 PDT
Couple other devices with new APK

* Nexus 7 (2012, 4.4.2)
** Crashes on playback attempt of H.264/MP4
** AAC works

* Galaxy SII (4.1.2)
** H.264/MP4 video playback works, bad colour conversion (green/blue). Unwatchable.
***  Media Codec: Format 0x15
** AAC works
Comment 28 User image Andrew Martin McDonough (:mmcdonough) 2014-08-21 16:13:53 PDT
Created attachment 8477047 [details] [diff] [review]
PDM using Android MediaCodec

Updated patch. I've renamed a lot of classes to be more consistent, replaced the PRLocks with mozilla::mutexes, made the indentation and open-bracket formatting more consistent, and renamed the GeneratedJNIWrappers_2 files to GenerateSDKWrappers.
Comment 29 User image Chris Pearce (:cpearce) 2014-08-27 21:22:53 PDT
Comment on attachment 8477047 [details] [diff] [review]
PDM using Android MediaCodec

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

Firstly, it's great that you got this working. My review comments below notwithstanding, I'm impressed that you got this far.

From a thread safety standpoint, we need to make some improvements here before I'm comfortable r+ing this.

I am not r+ing this if it's using atomics. Change it to using mozilla::Monitors with Wait()/NotifyAll() pairs. Those are known to be correct, whereas bespoke waiting/signalling has to have every permutation of load/stor that could possibly happen on all threads checked, and no human being is smart enough to get that right.

I haven't checked whether the implementation of MediaDataDecoder/PlatformDecoderModule makes sense, because it's not obvious the threading is correct. Once we've got the threading/locking sorted out, I'll check the MediaDataDecoder/PlatformDecoderModule implementation.

Also, the indentation and whitespace is not consistent. Please run `./mach clang-format` with your patch applied to fix the whitespace mistakes.

::: content/media/DecoderTraits.cpp
@@ +393,5 @@
>      codecList = MediaDecoder::IsOpusEnabled() ? gOggCodecsWithOpus : gOggCodecs;
>      result = CANPLAY_MAYBE;
>    }
> +#ifdef MOZ_FMP4
> +  if (nsDependentCString(aMIMEType).EqualsASCII("audio/mp4") ||

I've fixed the CanHandleMediaType code for the MP4Reader in bug 1059523. So remove this change.

::: content/media/fmp4/MP4Decoder.cpp
@@ +148,5 @@
>  #ifdef XP_WIN
>           // We have H.264/AAC platform decoders on Windows Vista and up.
>           IsVistaOrLater() ||
> +#elif defined(MOZ_ANDROIDMC)
> +true || // TODO Check actual Android version, return true for JellyBean or greater.

We need to do this TODO before we can enable this.

You also need to check the pref media.fragmented-mp4.android-media-codec.* to ensure it's enabled and preferred here too.

::: content/media/fmp4/androidmc/AndroidDecoderModule.cpp
@@ +106,5 @@
> +  }
> +
> +  virtual nsresult PumpOutput(){
> +
> +    if(mTaskQueue->IsEmpty()){

No way I'm r+ing this.

You should be using a mozilla::Monitor, a Java-style monitor.

When you wait you do:

{
  MonitorAutoEnter mon(mMonitor);
  while (!condition) {
    mon.Wait();
  }
}

Then you have another function that does:

{
  MonitorAutoEnter mon(mMonitor);
  condition = true; // or equivalent
  mon.NotifyAll();
}

This means you may have to have a pointer from your other objects back to this object, so the other objects can call the appropriate functions to awaken the thread waiting here.

@@ +338,5 @@
> +  return NS_OK;
> +
> +}
> +
> +nsresult MediaCodecDataDecoder::Input(mp4_demuxer::MP4Sample* aSample){

You should not override this, and override it in the subclass and instanitate the subclass.

@@ +349,5 @@
> +
> +
> +  JNIEnv *lEnv = GetJNIForThread();
> +
> +  for(unsigned i = 0; i< vOutputBuffers.size(); i++)

for (...) {
  ...
}

Here and everywhere else.

@@ +383,5 @@
> +
> +nsresult MediaCodecDataDecoder::Drain(){
> +
> +  // Signal for a flush, wait for ack.
> +  PR_AtomicSet(&mShouldFlush, 1);

You should be waiting on mozilla::Monitors here.

@@ +405,5 @@
> +  PR_AtomicSet(&mShouldDie, 1);
> +  mTaskQueue->Flush();
> +  mVideoInputTaskQueue->Flush();
> +
> +  while(!PR_AtomicAdd(&mDidDie, 0)){

No. This will busy-loop the CPU testing this condition. Do not do this. Wait on a monitor, or sync dispatch a task to your queue to do that shutdown on your queues.

::: content/media/fmp4/androidmc/AndroidDecoderModule.h
@@ +24,5 @@
> +};
> +
> +typedef std::queue<SampleMetaData *> SampleMetaQueue;
> +
> +class MediaCodecSynchro {

You should use mozilla::Atomic<int32_t> instead of using PR_AtomicSet. In fact you should not use atomics at all. Delete this class.

Move all its functions into your MediaDataDecoder. Your MediaDataDecoder should have a monitor which it exposes, and the things that are calling these functions should take that monitor before they call these functions, and for as long as they need to ensure that the state their basing their decisions is consistent. These functions should call monitor.AssertCurrentThreadOwns() before doing their thing to enforce that their callers are following taking the lock.

You can either expose the monitor directly, or have a Lock() and Unlock() functions on this class, and write an AutoLock class that calls your MediaDataDecoder's Lock() in its constructor and Unlock() in its destructor.

@@ +71,5 @@
> +    return (PR_AtomicAdd(&mShouldDie, 0)==1);
> +  }
> +
> +  inline bool GetShouldFlush(void){
> +    return (PR_AtomicAdd(&mShouldFlush, 0)==1);

You realise that there's no guarantee that when GetShouldFlush() returns that mShouldFlush wasn't changed on another thread and is now the opposite of the value your thread thinks it should be? You should be locking your shared state while you make decisions based on the state.

@@ +152,5 @@
> +
> +  };
> +
> +}
> +

There are copy-pasted comments here that are don't add any value. Delete them.

@@ +285,5 @@
> +  virtual void ResetOutputBuffers();
> +
> +};
> +
> +} // namwspace mozilla

namespace

::: content/media/fmp4/androidmc/AndroidDecoderUtils.cpp
@@ +59,5 @@
> +    int lInputIndex = mDecoder->DequeueInputBuffer(0);
> +
> +    if(lInputIndex<0){
> +      mSynchro->mBufferLock.Unlock();
> +      continue;

Why the while(true){continue} here? Does this busy loop?

@@ +72,5 @@
> +    void *lDirectBuffer = lEnv->GetDirectBufferAddress(lBuffer);
> +
> +    MOZ_ASSERT(lDirectBuffer!=nullptr);
> +
> +    if((jsize)mData->mSize<lCap)

if (...) {
  ...
}

Here and everywhere else.

@@ +188,5 @@
> +  if(mSynchro->GetShouldDie()){
> +    return NS_OK;
> +  }
> +
> +  JNIEnv *lEnv = GetJNIForThread();

What happens if something sets that state that GetShouldDie() checks atomically right after you checked called GetShouldDie()? Atomics are bad.

@@ +220,5 @@
> +    mSynchro->SetDidFlush(true);
> +
> +  }
> +
> +  mSynchro->mBufferLock.Lock();

What if something sets the should flush flag right now, right after you checked whether you should flush?

@@ +320,5 @@
> +  if(mSynchro->GetShouldDie()){
> +    mSynchro->SetDidDie(true);
> +  }
> +  else if(mSynchro->GetSleeping()){
> +    mCallback->InputExhausted();

Why would you call InputExhausted here?

In general, if there's something non-obvious happening in the code, you should add a comment explaining *why* you're doing something.

@@ +418,5 @@
> +        (mVideoFormat.mColorFormat==mcdecode::SEC_FormatNV12Tiled) ||
> +        (mVideoFormat.mColorFormat==mcdecode::FormatYUV420SemiPlanar) ||
> +        (mVideoFormat.mColorFormat==mcdecode::FormatYUV420Planar)))
> +  {
> +    printf_stderr("Media Codec: Unknown Format Number 0x%x\n", mVideoFormat.mColorFormat);

You should not leave raw printfs in release code.

::: content/media/fmp4/androidmc/AndroidDecoderUtils.h
@@ +30,5 @@
> +
> +namespace mozilla {
> +
> +// Make up for what the jarClassProcessor still needs:
> +/*

Don't leave commented out code in your code.
Comment 30 User image Kevin Brosnan [:kbrosnan] 2014-09-09 21:00:16 PDT
What are we doing to get this committed?
Comment 31 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-09-10 07:39:29 PDT
(In reply to Kevin Brosnan [:kbrosnan] from comment #30)
> What are we doing to get this committed?

We need to get some things fixed up. Martin was going to try to stay involved, but not sure if that's happening, as school has started back up. I should be able to finish it up maybe next week if he can't.
Comment 32 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-09-10 13:56:52 PDT
So I get a format of QCOM_FormatYUV420PackedSemiPlanar64x32Tile2m8ka on the Nexus 4. We try to handle this in omx-plugin right now by using Android's ColorConverter class. It's going to be really terrible if we still have to do that.
Comment 33 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-09-10 15:04:26 PDT
Actually, at a glance, the ColorConverter class looks unchanged from 4.1 (which is where we can first use MediaCodec) to 4.4, so maybe it won't be too bad to dlopen?
Comment 34 User image Andrew Martin McDonough (:mmcdonough) 2014-09-18 13:59:23 PDT
Created attachment 8491801 [details] [diff] [review]
mediacodec-14-09-18.patch

I've replaced the interlocking atomics with mozilla::Monitors, added API level checks, and removed the MediaCodecSynchro object, and pulled the reference comments from the PDM classes.

I haven't run clang-format on it yet, because it seg faults on my machine :(
Comment 35 User image John Lin [:jolin][:jhlin] 2014-09-24 20:22:11 PDT
(In reply to Andrew Martin McDonough (:mmcdonough) from comment #34)
> 
> I haven't run clang-format on it yet, because it seg faults on my machine :(

 Bug 1000527?
Comment 36 User image Chris Pearce (:cpearce) 2014-09-28 02:16:05 PDT
Comment on attachment 8491801 [details] [diff] [review]
mediacodec-14-09-18.patch

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

Looks much better.

::: content/media/fmp4/PlatformDecoderModule.h
@@ +33,5 @@
>  class MediaTaskQueue;
>  class CDMProxy;
>  typedef int64_t Microseconds;
>  
> +already_AddRefed<MediaTaskQueue> CreateTaskQueue();

You need a comment here explaining that this task queue runs on the decode thread pool.

::: content/media/fmp4/androidmc/AndroidDecoderModule.cpp
@@ +99,5 @@
> +    }
> +
> +    if(mSleeping){
> +
> +      nsRunnable *loader = new AndroidVideoOutput(vInputBuffers, vOutputBuffers,

RefPtr<nsIRunnable> loader(new ...);

And below...

@@ +231,5 @@
> +
> +  JNIEnv *lEnv = GetJNIForThread();
> +
> +  if(lFormat->GetByteBuffer(NS_LITERAL_STRING("csd-0"))==nullptr){
> +      uint8_t *csd_0 = new uint8_t[2];

You don't delete csd_0. But why do you need to allocate it? Can't you pass aConfig.audio_specific_config.begin() to NewDirectByteBuffer()?

@@ +311,5 @@
> +
> +  JNIEnv *lEnv = GetJNIForThread();
> +
> +  for(unsigned i = 0; i< vOutputBuffers.size(); i++)
> +    lEnv->DeleteGlobalRef(vOutputBuffers[i]);

For loops and if statements should always have {}, even (especially!) single line for/if statements...

It can lead to costly mistakes:
https://www.imperialviolet.org/2014/02/22/applebug.html

@@ +334,5 @@
> +}
> +
> +nsresult MediaCodecDataDecoder::Flush(){
> +
> +  Drain();

Flush() isn't the same as Drain(). Flush aborts everything in the pipeline, whereas Drain() waits for everything pending to output. If your Flush() can't() flush properly, please add a comment why.

@@ +375,5 @@
> +      lock.Wait();
> +    }
> +  }
> +
> +  // Wait for all pending output ot complete.

s/ot/to/

::: content/media/fmp4/androidmc/AndroidDecoderUtils.cpp
@@ +48,5 @@
> +{
> +
> +  while(true){
> +    mDataDecoder->mBufferLock.Lock();
> +    int lInputIndex = mDecoder->DequeueInputBuffer(0);

Does DequeueInputBuffer() block? If not, you could be busy-looping here when you `continue` below.

@@ +249,5 @@
> +    FormatChanged();
> +
> +  } else{
> +    MonitorAutoLock lock(mDataDecoder->mMonitor);
> +    mDataDecoder->SetSleeping(true);

Why do you need this "sleeping" state?

::: mobile/android/app/mobile.js
@@ +601,5 @@
>  
> +// Enable the MediaCodec PlatformDecoderModule by default.
> +pref("media.fragmented-mp4.exposed", true);
> +pref("media.fragmented-mp4.android-media-codec.enabled", true);
> +pref("media.fragmented-mp4.android-media-codec.preferred", true);

How sure are you sure that this is ready to set on by default? ;)
Comment 37 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-02 12:49:57 PDT
Created attachment 8499104 [details] [diff] [review]
0001-Bug-1014614-Rename-nsSurfaceTexture-to-AndroidSurfac.patch

This simply renames nsSurfaceTexture to the far more sensible AndroidSurfaceTexture, and puts it in gfx/gl
Comment 38 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-02 12:51:02 PDT
Created attachment 8499105 [details] [diff] [review]
0002-Bug-1014614-Expose-Android-native-window-via-Android.patch

This adds a Android native window, providing a way to draw into the SurfaceTexture
Comment 39 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-02 12:51:54 PDT
Created attachment 8499106 [details] [diff] [review]
0003-Bug-1014614-Expose-more-SurfaceTexture-API-in-Androi.patch

This adds the setDefaultBufferSize() method from SurfaceTexture, and exposes the underlying Java Surface.
Comment 40 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-02 12:52:58 PDT
Created attachment 8499108 [details] [diff] [review]
0004-Bug-1014614-Do-not-try-to-use-a-temporary-texture-fo.patch

SurfaceTexture always binds to the texture you created it with when the updateTexImage() method is called. It's confusing and wrong to bind to something else beforehand.
Comment 41 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-02 12:53:44 PDT
Created attachment 8499109 [details] [diff] [review]
0005-Bug-1014614-Fix-JNI-wrapper-for-registering-SurfaceT.patch

This fixes the JNI wrapper for registerSurfaceTextureFrameListener() such that it can be called from any thread.
Comment 42 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-02 13:16:21 PDT
Created attachment 8499126 [details] [diff] [review]
0006-Bug-1014614-Use-Android-MediaCodec-for-decoding-H264.patch

This is a refactored and reworked version of Martin's patch. Besides cleanup, the main changes are:

1) Use a single thread for driving the MediaCodec. This is a lot simpler, and is the way it's expected to be used.

2) Use SurfaceTexture to get stuff onto the screen. It should perform better than passing around bit buffers, but the main advantage we get here is that we don't have to deal with the crazy-ass YUV formats that various devices output. You are guaranteed to be able to composite it via OpenGL. However...

3) This will have problems when we try to draw the video element into a canvas, or any other places where we need to read the video frame into a byte buffer. The plan for that is to add some stuff to the compositor that will draw a Image into an offscreen FBO, read it back, and return them over ipdl. This is kinda terrible, but something I'm willing to live with. I'd even be relatively ok with pushing this without that fixed yet.

This works on every device I've tried it on. There was a small issue on Android L, which I fixed, otherwise no problems so far.
Comment 43 User image Jeff Gilbert [:jgilbert] 2014-10-02 13:39:41 PDT
Comment on attachment 8499104 [details] [diff] [review]
0001-Bug-1014614-Rename-nsSurfaceTexture-to-AndroidSurfac.patch

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

::: gfx/gl/AndroidSurfaceTexture.cpp
@@ +253,5 @@
> +{
> +  if (mFrameAvailableCallback) {
> +    // Proxy to main thread if we aren't on it
> +    if (!NS_IsMainThread()) {
> +      // Proxy to main thread 

Boo EOL whitespace.

::: gfx/gl/AndroidSurfaceTexture.h
@@ +40,5 @@
> +
> +  // Returns with reasonable certainty whether or not we'll
> +  // be able to create and use a SurfaceTexture
> +  static bool Check();
> +  

Boo EOL whitespace.

@@ +70,5 @@
> +
> +  int mID;
> +  nsRefPtr<nsIRunnable> mFrameAvailableCallback;
> +};
> +  

Boo EOL whitespace.

::: widget/android/AndroidJNI.cpp
@@ +50,4 @@
>  using namespace mozilla::dom::mobilemessage;
>  using namespace mozilla::layers;
>  using namespace mozilla::widget::android;
> +using namespace mozilla::gl;

I think it uses more characters to do `using namespace` here than to just properly specify the namespace path in the two places this is needed.

We should be avoiding `using namespace` where we can, so try to avoid it here.
Comment 44 User image Jeff Gilbert [:jgilbert] 2014-10-02 13:51:17 PDT
Comment on attachment 8499108 [details] [diff] [review]
0004-Bug-1014614-Do-not-try-to-use-a-temporary-texture-fo.patch

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

What if no texture is bound? (Assert that the texture binding is non-zero?)
Whether or not this is valid depends on the state guarantees of the GL compositor.

Looking at code around this, I tend toward thinking this is not valid, and will overwrite textures left bound by other TextureHosts. For example, this seems like it would break badly if called after GLTextureSource::BindTexture.

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +634,4 @@
>  SurfaceTextureSource::GetTextureTransform()
>  {
>    gfx::Matrix4x4 ret;
> + 

Boo EOL whitespace.
Comment 45 User image Brad Lassey [:blassey] (use needinfo?) 2014-10-02 15:56:06 PDT
Comment on attachment 8499105 [details] [diff] [review]
0002-Bug-1014614-Expose-Android-native-window-via-Android.patch

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

::: gfx/gl/AndroidNativeWindow.h
@@ +58,5 @@
> +  virtual ~AndroidNativeWindow();
> +
> +  void* mWindow;
> +};
> +  

ws
Comment 46 User image Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin] 2014-10-02 18:45:27 PDT
Comment on attachment 8499126 [details] [diff] [review]
0006-Bug-1014614-Use-Android-MediaCodec-for-decoding-H264.patch

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

Nice! Just minor things, mostly nits.

::: content/media/fmp4/android/AndroidDecoderModule.cpp
@@ +26,5 @@
> +using namespace mozilla::widget::android;
> +
> +static MediaCodec* CreateDecoder(JNIEnv* aEnv, const char* aMimeType)
> +{
> +  if(aMimeType == nullptr) {

nit: if (

@@ +66,5 @@
> +
> +    return InitDecoder(mSurfaceTexture->JavaSurface());
> +  }
> +
> +  nsresult Input(mp4_demuxer::MP4Sample* aSample) {

nit: virtual Method() MOZ_OVERRIDE; here and through the rest of the file.

@@ +96,5 @@
> +                                                 img, isSync,
> +                                                 aInfo->getPresentationTimeUs(),
> +                                                 gfx::IntRect(0, 0,
> +                                                  mConfig.display_width,
> +                                                  mConfig.display_height)));

nit: indent

@@ +103,5 @@
> +
> +protected:
> +  layers::ImageContainer* mImageContainer;
> +  const mp4_demuxer::VideoDecoderConfig& mConfig;
> +  RefPtr<mozilla::gl::AndroidSurfaceTexture> mSurfaceTexture;

nit: nsRefPtr outside of gfx code.

@@ +122,5 @@
> +
> +    uint32_t numChannels = mConfig.channel_count;
> +    uint32_t numFrames = (aInfo->getSize() / numChannels) / 2;
> +
> +    AudioDataValue* audio = new AudioDataValue[numFrames * numChannels];

Consider using AudioCompactor here.

@@ +129,5 @@
> +    for (uint32_t frame = 0; frame < numFrames; frame++) {
> +      for (uint32_t channel = 0; channel < numChannels; channel++) {
> +        *tmp++ = *data++;
> +      }
> +    }

this can be a memcpy.

@@ +160,5 @@
> +                                layers::ImageContainer* aImageContainer,
> +                                MediaTaskQueue* aVideoTaskQueue,
> +                                MediaDataDecoderCallback* aCallback)
> +{
> +

nit: empty line

@@ +169,5 @@
> +                                                   aConfig.display_width,
> +                                                   aConfig.display_height);
> +
> +  if (jFormat == nullptr)
> +    return nullptr;

nit: braces; here and below.

@@ +200,5 @@
> +    return nullptr;
> +
> +  MediaFormat* format = MediaFormat::Wrap(jFormat);
> +
> +  if(format == nullptr)

nit: "if (", and braces here and above.

@@ +206,5 @@
> +
> +  JNIEnv* env = GetJNIForThread();
> +
> +  if(format->GetByteBuffer(NS_LITERAL_STRING("csd-0")) == nullptr) {
> +      uint8_t* csd0 = new uint8_t[2];

nit: "if (" and 2-space indent.

::: content/media/fmp4/android/AndroidDecoderModule.h
@@ +35,5 @@
> +  CreateH264Decoder(const mp4_demuxer::VideoDecoderConfig& aConfig,
> +                    layers::LayersBackend aLayersBackend,
> +                    layers::ImageContainer* aImageContainer,
> +                    MediaTaskQueue* aVideoTaskQueue,
> +                    MediaDataDecoderCallback* aCallback);

nit: MOZ_OVERRIDE; here and in the rest of the file.
Comment 47 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-03 08:58:13 PDT
(In reply to Jeff Gilbert [:jgilbert] from comment #44)
> Comment on attachment 8499108 [details] [diff] [review]
> 0004-Bug-1014614-Do-not-try-to-use-a-temporary-texture-fo.patch
> 
> Review of attachment 8499108 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What if no texture is bound? (Assert that the texture binding is non-zero?)
> Whether or not this is valid depends on the state guarantees of the GL
> compositor.

I don't think I'm following. SurfaceTexture.updateTexImage() always binds the texture that it is holding[0]. It is impossible to create a SurfaceTexture without passing a texture name, so the worst case is that it binds to an invalid one. I don't see how we can stomp on anything. The binding that occurred before the UpdateTexImage() call was just getting blown away, so removing it should be fine.

[0] http://developer.android.com/reference/android/graphics/SurfaceTexture.html#updateTexImage%28%29

> 
> Looking at code around this, I tend toward thinking this is not valid, and
> will overwrite textures left bound by other TextureHosts. For example, this
> seems like it would break badly if called after GLTextureSource::BindTexture.
> 
> ::: gfx/layers/opengl/TextureHostOGL.cpp
> @@ +634,4 @@
> >  SurfaceTextureSource::GetTextureTransform()
> >  {
> >    gfx::Matrix4x4 ret;
> > + 
> 
> Boo EOL whitespace.
Comment 48 User image Jeff Gilbert [:jgilbert] 2014-10-03 12:53:55 PDT
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #47)
> (In reply to Jeff Gilbert [:jgilbert] from comment #44)
> > Comment on attachment 8499108 [details] [diff] [review]
> > 0004-Bug-1014614-Do-not-try-to-use-a-temporary-texture-fo.patch
> > 
> > Review of attachment 8499108 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > What if no texture is bound? (Assert that the texture binding is non-zero?)
> > Whether or not this is valid depends on the state guarantees of the GL
> > compositor.
> 
> I don't think I'm following. SurfaceTexture.updateTexImage() always binds
> the texture that it is holding[0]. It is impossible to create a
> SurfaceTexture without passing a texture name, so the worst case is that it
> binds to an invalid one. I don't see how we can stomp on anything. The
> binding that occurred before the UpdateTexImage() call was just getting
> blown away, so removing it should be fine.
> 
> [0]
> http://developer.android.com/reference/android/graphics/SurfaceTexture.
> html#updateTexImage%28%29

Alright, fix the documentation then. It should be that this is the case.
In particular, at least fix the comment on UpdateTexImage:
  // This attaches the updated data to the TEXTURE_EXTERNAL target
Should be like:
  // This updates and binds its texture the TEXTURE_EXTERNAL bind point.

Most functions like this just replace the contents of the currently bound texture, so that this one changes the binding is notable.
Comment 49 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-03 13:27:41 PDT
Created attachment 8499792 [details] [diff] [review]
0007-Bug-1014614-Fix-readback-of-SurfaceTexture-backed-vi.patch

This fixes drawing of the video element into a canvas, amongst other things. Basically anything that uses nsLayoutUtils::SurfaceFromElement.

I'm not sure this is totally right, and it seems like it could be more generalized.

:nical/:jgilbert you may want to look at the prior patch for context.
Comment 50 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-03 14:03:49 PDT
Created attachment 8499819 [details] [diff] [review]
0007-Bug-1014614-Fix-readback-of-SurfaceTexture-backed-vi.patch

This one at least has a prayer of building on other platforms
Comment 51 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-03 14:04:36 PDT
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #50)
> Created attachment 8499819 [details] [diff] [review]
> 0007-Bug-1014614-Fix-readback-of-SurfaceTexture-backed-vi.patch
> 
> This one at least has a prayer of building on other platforms

So it seems like what I really want is to somehow use ImageHost/Client here, but I'm not sure that is really wired for this kind of thing.
Comment 52 User image Jeff Gilbert [:jgilbert] 2014-10-03 14:12:29 PDT
Comment on attachment 8499819 [details] [diff] [review]
0007-Bug-1014614-Fix-readback-of-SurfaceTexture-backed-vi.patch

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

::: gfx/layers/ipc/PCompositor.ipdl
@@ +84,5 @@
>    sync MakeSnapshot(SurfaceDescriptor inSnapshot, nsIntRect dirtyRect);
>  
> +  // Composites the input surface in isolation and reads back the result. Useful
> +  // for certain surface types (SurfaceTexture) that cannot be read any other way.
> +  sync DrawAndReadback(SurfaceDescriptor inSurface, SurfaceDescriptor outSurface);

Why does this need to be done in the Compositor, much less synchronously. Can't we just use something similar to the existing BlitTextureToFramebuffer functions, if we just want to do readback?
Comment 53 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-03 18:45:40 PDT
(In reply to Jeff Gilbert [:jgilbert] from comment #52)
> Comment on attachment 8499819 [details] [diff] [review]
> 0007-Bug-1014614-Fix-readback-of-SurfaceTexture-backed-vi.patch
> 
> Review of attachment 8499819 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/PCompositor.ipdl
> @@ +84,5 @@
> >    sync MakeSnapshot(SurfaceDescriptor inSnapshot, nsIntRect dirtyRect);
> >  
> > +  // Composites the input surface in isolation and reads back the result. Useful
> > +  // for certain surface types (SurfaceTexture) that cannot be read any other way.
> > +  sync DrawAndReadback(SurfaceDescriptor inSurface, SurfaceDescriptor outSurface);
> 
> Why does this need to be done in the Compositor

That's where the texture we are reading from is valid. We can detach it from there and reattach another context (and texture), but the detach must be done on the attached thread, so we'd still need synchronous IPC. And some additional bookkeeping to avoid trying to attach it to two contexts simultaneously.

> , much less synchronously.

We're doing a synchronous canvas call when we need this information, so I'm not sure that can be avoided.

> Can't we just use something similar to the existing BlitTextureToFramebuffer
> functions, if we just want to do readback?

Yeah, I had forgotten about those. I guess we'd need to add some stuff to support external targets, but should be straightforward? Will all of that machinery work on an external target?

I was sort of hoping to have more code reuse around the "composite this SurfaceDescriptor" work, but I don't know if that's easy or not. Need :nical wisdom there.
Comment 54 User image Nicolas Silva [:nical] 2014-10-06 01:34:34 PDT
Comment on attachment 8499819 [details] [diff] [review]
0007-Bug-1014614-Fix-readback-of-SurfaceTexture-backed-vi.patch

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

I agree with Jeff, not found of adding this ipdl message.
Comment 55 User image Chris Pearce (:cpearce) 2014-10-08 15:04:15 PDT
Comment on attachment 8499126 [details] [diff] [review]
0006-Bug-1014614-Use-Android-MediaCodec-for-decoding-H264.patch

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

r=cpearce with the H.264 duration issue fixed.

It looks really good. Nice work.

::: configure.in
@@ +5198,5 @@
>  dnl ========================================================
>  dnl = Built-in fragmented MP4 support.
>  dnl ========================================================
> +
> +if test "$OS_TARGET" = Android; then

It feels to me like we should have a smarter way to know that we're building for Android.

It also seems like we don't need MOZ_ANDROID_FMP4 here, why can't you use MOZ_WIDGET_ANDROID?

Can this just be:

if test "$OS_TARGET" = Android; then
   Testing FMP4 for Android using MediaCodec
   MOZ_FMP4=1
fi

And then you use MOZ_WIDGET_ANDROID instead of MOZ_ANDROID_FMP4?

::: content/media/fmp4/MP4Decoder.cpp
@@ +23,4 @@
>  #ifdef MOZ_APPLEMEDIA
>  #include "apple/AppleDecoderModule.h"
>  #endif
> +#ifdef MOZ_ANDROID_FMP4

MOZ_WIDGET_ANDROID?

@@ +171,4 @@
>           // We have H.264/AAC platform decoders on Windows Vista and up.
>           IsVistaOrLater() ||
>  #endif
> +#ifdef MOZ_ANDROID_FMP4

Why can't you use MOZ_WIDGET_ANDROID? This code only gets compiled if MOZ_FMP4 is defined.

::: content/media/fmp4/android/AndroidDecoderModule.cpp
@@ +58,5 @@
> +    }
> +
> +    mSurfaceTexture = mozilla::gl::AndroidSurfaceTexture::Create(tex);
> +    if (!mSurfaceTexture) {
> +      printf_stderr("failed to create SurfaceTexture for video decode\n");

Did you mean to leave this printf here? I'm not familiar with the convention for Fennec, but it's not supposed to happen on desktop.

@@ +59,5 @@
> +
> +    mSurfaceTexture = mozilla::gl::AndroidSurfaceTexture::Create(tex);
> +    if (!mSurfaceTexture) {
> +      printf_stderr("failed to create SurfaceTexture for video decode\n");
> +      // FIXME: free the texture

Probably should free the texture here.

@@ +91,5 @@
> +    typedImg->SetData(data);
> +
> +    mCallback->Output(VideoData::CreateFromImage(videoInfo, mImageContainer, aInfo->getOffset(),
> +                                                 aInfo->getPresentationTimeUs(),
> +                                                 0,

You should have a duration here. Input MP4Samples have a duration from the demuxer, so you can plumb that through somehow.

I think H.264 is constant frame rate, so you can probably just get away with saving the first non-zero duration MP4Sample you get, and using its duration as the duration for all subsequent files.

@@ +214,5 @@
> +
> +      jobject buffer = env->NewDirectByteBuffer(csd0, 2);
> +      format->SetByteBuffer(NS_LITERAL_STRING("csd-0"), buffer);
> +
> +      // TODO: free byte buffer?

Uh yeah, shouldn't you free that byte buffer?

@@ +217,5 @@
> +
> +      // TODO: free byte buffer?
> +  }
> +
> +  // TODO: only do this for AAC?

Yes, only do that if mimeType.EqualsLiteral("audio/mp4a-latm").

@@ +230,5 @@
> +
> +
> +nsresult AndroidDecoderModule::Shutdown()
> +{
> +  // Ummmmm

Probably don't need to do anything here; MediaDataDecoder::Shutdown() is called on all active decoders first, and it looks like that's where all the state is.

@@ +349,5 @@
> +
> +        MOZ_ASSERT(env->GetDirectBufferCapacity(buffer) >= sample->size,
> +          "Decoder buffer is not large enough for sample");
> +
> +        memcpy(directBuffer, sample->data, sample->size);

Apparently the cool kids are using PodCopy these days.

::: content/media/fmp4/android/AndroidDecoderModule.h
@@ +49,5 @@
> +
> +  virtual bool SupportsAudioMimeType(const char* aMimeType);
> +};
> +
> +class MediaCodecDataDecoderCallback : public MediaDataDecoder {

This class, MediaCodecDataDecoderCallback, is unused, and confusingly it's named as if it inherits from MediaDataDecoderCallback, and it declares overrides for MediaDataDecoderCallback's functions, but it in fact inherits from MediaDataDecoder and declares overrides for none of that classes functions. If you labelled these with MOZ_OVERRIDE, this would have emitted an error I'd wager.
Comment 56 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-13 08:05:34 PDT
(In reply to Chris Pearce (:cpearce) from comment #55)

> @@ +171,4 @@
> >           // We have H.264/AAC platform decoders on Windows Vista and up.
> >           IsVistaOrLater() ||
> >  #endif
> > +#ifdef MOZ_ANDROID_FMP4
> 
> Why can't you use MOZ_WIDGET_ANDROID? This code only gets compiled if
> MOZ_FMP4 is defined.

Yeah, I did as you suggested. Much nicer.

> 
> ::: content/media/fmp4/android/AndroidDecoderModule.cpp
> @@ +58,5 @@
> > +    }
> > +
> > +    mSurfaceTexture = mozilla::gl::AndroidSurfaceTexture::Create(tex);
> > +    if (!mSurfaceTexture) {
> > +      printf_stderr("failed to create SurfaceTexture for video decode\n");
> 
> Did you mean to leave this printf here? I'm not familiar with the convention
> for Fennec, but it's not supposed to happen on desktop.

We do printf_stderr sometimes which goes to the android logcat. I left this one in on purpose because it's one of the most likely failure points and it gives us a clue in the log.

> 
> @@ +59,5 @@
> > +
> > +    mSurfaceTexture = mozilla::gl::AndroidSurfaceTexture::Create(tex);
> > +    if (!mSurfaceTexture) {
> > +      printf_stderr("failed to create SurfaceTexture for video decode\n");
> > +      // FIXME: free the texture
> 
> Probably should free the texture here.

Yeah, sadly there isn't an easy way to do that right now, it seems. I'm fixing that.

> 
> @@ +91,5 @@
> > +    typedImg->SetData(data);
> > +
> > +    mCallback->Output(VideoData::CreateFromImage(videoInfo, mImageContainer, aInfo->getOffset(),
> > +                                                 aInfo->getPresentationTimeUs(),
> > +                                                 0,
> 
> You should have a duration here. Input MP4Samples have a duration from the
> demuxer, so you can plumb that through somehow.
> 
> I think H.264 is constant frame rate, so you can probably just get away with
> saving the first non-zero duration MP4Sample you get, and using its duration
> as the duration for all subsequent files.

Ok, sounds reasonable.

> 
> @@ +214,5 @@
> > +
> > +      jobject buffer = env->NewDirectByteBuffer(csd0, 2);
> > +      format->SetByteBuffer(NS_LITERAL_STRING("csd-0"), buffer);
> > +
> > +      // TODO: free byte buffer?
> 
> Uh yeah, shouldn't you free that byte buffer?

Yeah, forgot about that :)

> 
> @@ +217,5 @@
> > +
> > +      // TODO: free byte buffer?
> > +  }
> > +
> > +  // TODO: only do this for AAC?
> 
> Yes, only do that if mimeType.EqualsLiteral("audio/mp4a-latm").

Fixed, thanks.

> 
> @@ +230,5 @@
> > +
> > +
> > +nsresult AndroidDecoderModule::Shutdown()
> > +{
> > +  // Ummmmm
> 
> Probably don't need to do anything here; MediaDataDecoder::Shutdown() is
> called on all active decoders first, and it looks like that's where all the
> state is.
> 
> @@ +349,5 @@
> > +
> > +        MOZ_ASSERT(env->GetDirectBufferCapacity(buffer) >= sample->size,
> > +          "Decoder buffer is not large enough for sample");
> > +
> > +        memcpy(directBuffer, sample->data, sample->size);
> 
> Apparently the cool kids are using PodCopy these days.

Yeah, appears to be so. I changed it because I want to be cool.

> 
> ::: content/media/fmp4/android/AndroidDecoderModule.h
> @@ +49,5 @@
> > +
> > +  virtual bool SupportsAudioMimeType(const char* aMimeType);
> > +};
> > +
> > +class MediaCodecDataDecoderCallback : public MediaDataDecoder {
> 
> This class, MediaCodecDataDecoderCallback, is unused, and confusingly it's
> named as if it inherits from MediaDataDecoderCallback, and it declares
> overrides for MediaDataDecoderCallback's functions, but it in fact inherits
> from MediaDataDecoder and declares overrides for none of that classes
> functions. If you labelled these with MOZ_OVERRIDE, this would have emitted
> an error I'd wager.

Yeah, not sure why that's there. Nuked.
Comment 57 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-14 09:00:04 PDT
(In reply to Chris Pearce (:cpearce) from comment #55)

> @@ +91,5 @@
> > +    typedImg->SetData(data);
> > +
> > +    mCallback->Output(VideoData::CreateFromImage(videoInfo, mImageContainer, aInfo->getOffset(),
> > +                                                 aInfo->getPresentationTimeUs(),
> > +                                                 0,
> 
> You should have a duration here. Input MP4Samples have a duration from the
> demuxer, so you can plumb that through somehow.
> 
> I think H.264 is constant frame rate, so you can probably just get away with
> saving the first non-zero duration MP4Sample you get, and using its duration
> as the duration for all subsequent files.

Alright, I definitely get varying durations here. Annoying. Can't this just be inferred from the presentation time? Surely duration = presentationNextFrame - presentationCurrentFrame?
Comment 58 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-14 09:49:42 PDT
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #57)
> (In reply to Chris Pearce (:cpearce) from comment #55)
> 
> > @@ +91,5 @@
> > > +    typedImg->SetData(data);
> > > +
> > > +    mCallback->Output(VideoData::CreateFromImage(videoInfo, mImageContainer, aInfo->getOffset(),
> > > +                                                 aInfo->getPresentationTimeUs(),
> > > +                                                 0,
> > 
> > You should have a duration here. Input MP4Samples have a duration from the
> > demuxer, so you can plumb that through somehow.
> > 
> > I think H.264 is constant frame rate, so you can probably just get away with
> > saving the first non-zero duration MP4Sample you get, and using its duration
> > as the duration for all subsequent files.
> 
> Alright, I definitely get varying durations here. Annoying. Can't this just
> be inferred from the presentation time? Surely duration =
> presentationNextFrame - presentationCurrentFrame?

Eh, it looks like one sample in equals one sample out, so I'm just queuing up the duration at input time and dequeue in output. Seems to work alright. I think Martin's original patch did this too.
Comment 59 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-14 11:34:16 PDT
Created attachment 8504916 [details] [diff] [review]
0006-Bug-1014614-Support-attach-detach-of-GLContext-to-An.patch
Comment 60 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-14 11:34:54 PDT
Created attachment 8504918 [details] [diff] [review]
0007-Bug-1014614-Use-Android-MediaCodec-for-decoding-H264.patch
Comment 61 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-14 11:35:25 PDT
Created attachment 8504919 [details] [diff] [review]
0008-Bug-1014614-Add-GLBlitHelper-BlitImageToFramebuffer-.patch
Comment 62 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-14 11:36:47 PDT
Created attachment 8504922 [details] [diff] [review]
0009-Bug-1014614-Fix-readback-of-SurfaceTextureImage.patch

This version avoids all of the IPC mess and instead constructs a single GLContext that we can use for reading the SurfaceTexture on the main thread.
Comment 63 User image Jeff Gilbert [:jgilbert] 2014-10-15 12:32:58 PDT
Comment on attachment 8504916 [details] [diff] [review]
0006-Bug-1014614-Support-attach-detach-of-GLContext-to-An.patch

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

What are we going to do for ICS?
Comment 64 User image Jeff Gilbert [:jgilbert] 2014-10-15 12:36:39 PDT
Comment on attachment 8504919 [details] [diff] [review]
0008-Bug-1014614-Add-GLBlitHelper-BlitImageToFramebuffer-.patch

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

::: gfx/gl/GLBlitHelper.cpp
@@ +243,4 @@
>                                           vTexCoord * uTexCoordMult);  \n\
>          }                                                             \n\
>      ";
> +#ifdef ANDROID /* MOZ_WIDGET_ANDROID || MOZ_WIDGET_GONK */

Just use the ANDROID || GONK part.

@@ +728,5 @@
> +
> +    int oldBinding = 0;
> +    mGL->fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_EXTERNAL, &oldBinding);
> +
> +    surfaceTexture->UpdateTexImage();

This assumes that we're attached? Can you comment on this near the decl?
Comment 65 User image Jeff Gilbert [:jgilbert] 2014-10-15 12:38:44 PDT
Comment on attachment 8504922 [details] [diff] [review]
0009-Bug-1014614-Fix-readback-of-SurfaceTextureImage.patch

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

::: gfx/layers/GLImages.cpp
@@ +46,5 @@
> +  mData.mSurfTex->Attach(sSnapshotContext);
> +  helper.BlitImageToFramebuffer(this, mData.mSize, fb.FB(), false);
> +  mData.mSurfTex->Detach();
> +
> +  sSnapshotContext->fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, 0);

Why do you call this immediately before binding to a different FB?
Comment 66 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-15 12:51:14 PDT
(In reply to Jeff Gilbert [:jgilbert] from comment #65)
> Comment on attachment 8504922 [details] [diff] [review]
> 0009-Bug-1014614-Fix-readback-of-SurfaceTextureImage.patch
> 
> Review of attachment 8504922 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/GLImages.cpp
> @@ +46,5 @@
> > +  mData.mSurfTex->Attach(sSnapshotContext);
> > +  helper.BlitImageToFramebuffer(this, mData.mSize, fb.FB(), false);
> > +  mData.mSurfTex->Detach();
> > +
> > +  sSnapshotContext->fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, 0);
> 
> Why do you call this immediately before binding to a different FB?

For some reason I was thinking BlitImageToFramebuffer left some other FB bound, and I wanted my own ScopedBindFramebuffer to restore it to 0 when we were done. But it looks like BlitImageToFramebuffer is using a scoped bind so that's wrong. I'll remove that line.
Comment 67 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-15 12:54:12 PDT
(In reply to Jeff Gilbert [:jgilbert] from comment #64)
> @@ +728,5 @@
> > +
> > +    int oldBinding = 0;
> > +    mGL->fGetIntegerv(LOCAL_GL_TEXTURE_BINDING_EXTERNAL, &oldBinding);
> > +
> > +    surfaceTexture->UpdateTexImage();
> 
> This assumes that we're attached? Can you comment on this near the decl?

Oh, hmm. I actually think the helper should take care of attach/detach anyway. I'll change it to do that.
Comment 68 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-15 13:07:01 PDT
(In reply to Jeff Gilbert [:jgilbert] from comment #63)
> Comment on attachment 8504916 [details] [diff] [review]
> 0006-Bug-1014614-Support-attach-detach-of-GLContext-to-An.patch
> 
> Review of attachment 8504916 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What are we going to do for ICS?

As discussed on IRC, attach/detach will fail. That does mean that readback won't work on ICS, but we don't use it for video there, only Flash for now. If we decide that needs to work some day, we'd have to do the original hack I had planned -- readback on the compositor thread.
Comment 69 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-17 06:46:57 PDT
Green try run here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fb320f7dd674
Comment 70 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-17 06:47:37 PDT
We are not actually testing this code on Try right now because we have no 4.1+ devices. I'm going to run the mochitests locally to see if there is any badness before I push.
Comment 72 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-17 08:39:33 PDT
Local mochitests were mostly fine once I sorted out 1074635. test_can_play_type_mpeg.html needs updating, but I'll file a followup for that.
Comment 74 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-18 14:24:56 PDT
We don't even run any of this code on 4.0, so those failures are pretty fun. Also interesting is that I didn't get them in my Try run.
Comment 75 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-20 18:21:39 PDT
Created attachment 8508390 [details] [diff] [review]
Remove mutex in GLImages.cpp

Tests were crashing on shutdown because the Mutex would be destroyed there and XPCOM was not happy. We don't even need the mutex because we are supposed to be on the main thread anyway, so just nuke it. Once approved I'll merge this with the original patch.
Comment 78 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-10-21 12:46:52 PDT
Victory!
Comment 79 User image Anthony Jones (:kentuckyfriedtakahe, :k17e) 2014-10-21 18:24:09 PDT
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #78)
> Victory!

\o/ Rockstar!
Comment 80 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-11-04 14:11:59 PST
Comment on attachment 8499104 [details] [diff] [review]
0001-Bug-1014614-Rename-nsSurfaceTexture-to-AndroidSurfac.patch

I am requesting approval for all of the patches on this bug that landed, but since there are 10 or so I don't think we need a request for each one.

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: No MP4 video on Android L, somewhat unreliable video on other versions.
[Describe test coverage new/current, TBPL]: Baked on nightly for about a week
[Risks and why]: Moderate/high risk. Lots of new code. Benefit is high, however.
[String/UUID change made/needed]: None
Comment 81 User image Ryan VanderMeulen [:RyanVM] 2014-11-05 08:26:33 PST
Rebasing this for Aurora and Beta is all yours, snorp.
Comment 83 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-11-05 13:37:02 PST
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?changeset=5811de401315
Comment 84 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-11-05 17:41:53 PST
I backed this out of beta. It was crashing in test_telephonyPolicy.html for some reason. Locally, I see a familiar XPCOM problem:

I/Gecko   ( 5667): [5667] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /Users/snorp/source/gecko-dev/xpcom/base/nsTraceRefcnt.cpp, line 148

Looks like the problem is probably this line in GLImages.cpp:

static nsRefPtr<GLContext> sSnapshotContext;

I had a similar problem before with a mutex. My plan is to just add stuff to gfxPlatform to hold this GLContext and avoid the static nsRefPtr entirely. That way it can get properly cleaned up, too.
Comment 85 User image James Willcox (:snorp) (jwillcox@mozilla.com) 2014-11-06 07:41:44 PST
Relanded. Turned out that test is just busted on Android. The fix in comment 84 did not fix it.

https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?changeset=53692e16c248

Note You need to log in before you can comment on or make changes to this bug.