Closed Bug 1014614 (mediacodec) Opened 10 years ago Closed 10 years ago

Use android.media.MediaCodec for decoding/encoding on recent Android

Categories

(Core :: Audio/Video, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 + fixed
firefox35 + fixed
firefox36 + fixed
fennec 35+ ---

People

(Reporter: snorp, Assigned: snorp)

References

Details

Attachments

(12 files, 6 obsolete files)

132.02 KB, patch
cpearce
: review-
Details | Diff | Splinter Review
129.45 KB, patch
cpearce
: feedback+
Details | Diff | Splinter Review
31.88 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
12.32 KB, patch
blassey
: review+
Details | Diff | Splinter Review
3.62 KB, patch
blassey
: review+
Details | Diff | Splinter Review
1.19 KB, patch
jgilbert
: review-
Details | Diff | Splinter Review
1.65 KB, patch
blassey
: review+
Details | Diff | Splinter Review
10.76 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
104.05 KB, patch
snorp
: review+
Details | Diff | Splinter Review
11.23 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
3.41 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
988 bytes, patch
jgilbert
: review+
Details | Diff | Splinter Review
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
See Also: → 1009410
tracking-fennec: --- → 33+
Hardware: x86 → All
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.
Assignee: nobody → mmcdonough
(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.
Can we close this as duplicate of bug 941301?
Attached patch MediaCodec PlatfromDecoderModule (obsolete) — — Splinter Review
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.
Land it with the AAC decoding disabled then land the AAC separately.
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
Attached patch MediaCodec PDM with H264 and AAC working (obsolete) — — Splinter Review
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.
Attachment #8458890 - Attachment is obsolete: true
Attachment #8468896 - Flags: feedback?(snorp)
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?
Attachment #8468896 - Flags: feedback?(snorp)
Attached patch MediaCodec PDM with H264 and AAC (obsolete) — — Splinter Review
Similar to last patch, but separates out the JNI generator Java code, removes unnecessary calls for ESDS/ADTS conversion, and has better naming.
Attachment #8468896 - Attachment is obsolete: true
Attachment #8471934 - Flags: feedback?(snorp)
Aaron can you run that APK through some of your devices and report back? We care about MP4/H264/AAC.
Flags: needinfo?(aaron.train)
Attachment #8471934 - Flags: review?(cpearce)
(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
Flags: needinfo?(aaron.train)
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.
Flags: needinfo?(mmcdonough)
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.
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.
Flags: needinfo?(mmcdonough)
Mind posting an updated APK with these preferences set?
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.
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
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.
I tried it on my Galaxy Nexus and got no video output. Output format was TI_FormatYUV420PackedSemiPlanar.
HTC One (M7)/Galaxy S4/Nexus 4 ->  0x7fa30c03
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?
Attachment #8471934 - Flags: feedback?(snorp)
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.
> media.fragmented-mp4.use-blank-decoder = true

This will give you green and beep which is not what you want.
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.
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
Attached patch PDM using Android MediaCodec — — Splinter Review
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.
Attachment #8471934 - Attachment is obsolete: true
Attachment #8471934 - Flags: review?(cpearce)
Attachment #8477047 - Flags: review?(cpearce)
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.
Attachment #8477047 - Flags: review?(cpearce) → review-
What are we doing to get this committed?
Flags: needinfo?(snorp)
(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.
Flags: needinfo?(snorp)
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.
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?
tracking-fennec: 33+ → 35+
Attached patch mediacodec-14-09-18.patch — — Splinter Review
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 :(
Attachment #8491801 - Flags: feedback?(cpearce)
(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 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? ;)
Attachment #8491801 - Flags: feedback?(cpearce) → feedback+
Summary: Investigate using android.media.MediaCodec for decoding/encoding on recent Android → Use android.media.MediaCodec for decoding/encoding on recent Android
This simply renames nsSurfaceTexture to the far more sensible AndroidSurfaceTexture, and puts it in gfx/gl
Attachment #8499104 - Flags: review?(bjacob)
This adds a Android native window, providing a way to draw into the SurfaceTexture
Attachment #8499105 - Flags: review?(blassey.bugs)
This adds the setDefaultBufferSize() method from SurfaceTexture, and exposes the underlying Java Surface.
Attachment #8499106 - Flags: review?(blassey.bugs)
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.
Attachment #8499108 - Flags: review?(jgilbert)
This fixes the JNI wrapper for registerSurfaceTextureFrameListener() such that it can be called from any thread.
Attachment #8499109 - Flags: review?(blassey.bugs)
Attachment #8499104 - Flags: review?(bjacob) → review?(jgilbert)
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.
Attachment #8499126 - Flags: review?(edwin)
Attachment #8499126 - Flags: review?(cpearce)
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.
Attachment #8499104 - Flags: review?(jgilbert) → review+
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.
Attachment #8499108 - Flags: review?(jgilbert) → review-
Attachment #8499106 - Flags: review?(blassey.bugs) → review+
Attachment #8499109 - Flags: review?(blassey.bugs) → review+
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
Attachment #8499105 - Flags: review?(blassey.bugs) → review+
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.
Attachment #8499126 - Flags: review?(edwin) → review+
(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.
(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.
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.
Assignee: foolkingcrown → snorp
Status: NEW → ASSIGNED
Attachment #8499792 - Flags: review?(nical.bugzilla)
Attachment #8499792 - Flags: review?(jgilbert)
This one at least has a prayer of building on other platforms
Attachment #8499792 - Attachment is obsolete: true
Attachment #8499792 - Flags: review?(nical.bugzilla)
Attachment #8499792 - Flags: review?(jgilbert)
Attachment #8499819 - Flags: review?(nical.bugzilla)
Attachment #8499819 - Flags: review?(jgilbert)
(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 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?
Attachment #8499819 - Flags: review?(jgilbert) → review-
(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 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.
Attachment #8499819 - Flags: review?(nical.bugzilla)
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.
Attachment #8499126 - Flags: review?(cpearce) → review+
(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.
(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?
(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.
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.
Attachment #8499819 - Attachment is obsolete: true
Attachment #8504922 - Flags: review?(jgilbert)
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?
Attachment #8504916 - Flags: review?(jgilbert) → review+
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?
Attachment #8504919 - Flags: review?(jgilbert) → review+
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?
Attachment #8504922 - Flags: review?(jgilbert) → review+
(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.
(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.
(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.
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.
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.
Blocks: 1084514
Blocks: 1084607
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.
Flags: needinfo?(snorp)
Attached patch Remove mutex in GLImages.cpp — — Splinter Review
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.
Attachment #8508390 - Flags: review?(jgilbert)
Attachment #8508390 - Flags: review?(jgilbert) → review+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #78)
> Victory!

\o/ Rockstar!
Depends on: 1089262
Depends on: 1089261
Depends on: 1089423
Depends on: 1089654
Depends on: 1090300
Depends on: 1091597
Depends on: 1091599
Depends on: 1093641
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
Attachment #8499104 - Flags: approval-mozilla-beta?
Attachment #8499104 - Flags: approval-mozilla-aurora?
Attachment #8499104 - Flags: approval-mozilla-beta?
Attachment #8499104 - Flags: approval-mozilla-beta+
Attachment #8499104 - Flags: approval-mozilla-aurora?
Attachment #8499104 - Flags: approval-mozilla-aurora+
Rebasing this for Aurora and Beta is all yours, snorp.
Flags: needinfo?(snorp)
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.
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
Blocks: 1091599
No longer depends on: 1091599
Alias: mediacodec
Depends on: 1100126
Depends on: 1101330
Depends on: 1106108
Blocks: 1107377
Blocks: 1109624
Depends on: 1133645
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: