941302 - PlatformDecoderModule for FirefoxOS/B2G

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

We need a PlatformDecoderModule created for FirefoxOS/B2G before we can playback fragmented MP4 in MSE on Android.

Comment 1

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

Oops, I made a typo, comment 0 should read:

We need a PlatformDecoderModule created for FirefoxOS/B2G before we can playback fragmented MP4 in MSE on *B2G*.

Comment 2

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

See PlatformDecoderModule for interface that needs to be implemented:
http://mxr.mozilla.org/mozilla-central/source/content/media/fmp4/PlatformDecoderModule.h

And for an example, the Windows platform decoder module code is here:
http://mxr.mozilla.org/mozilla-central/source/content/media/fmp4/wmf/

Comment 3

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

Attachment: mp4 decoding blockdiagram.ppt

Attach a file to show a big picture of this implementation which mostly bases on the decoder module of windows version. 
The main difference will lie in inputting encoded frame and outputting decoded frame. And since the current design for mp4reader is async, MediaCodec will be applied to hook up.

Comment 4

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

+ More information. The bug number of windows version is Bug 886196. 
(In reply to Blake Wu [:bwu] from comment #3)
> Created attachment 8374685 [details]
> mp4 decoding blockdiagram.ppt
> 
> Attach a file to show a big picture of this implementation which mostly
> bases on the decoder module of windows version. 
> The main difference will lie in inputting encoded frame and outputting
> decoded frame. And since the current design for mp4reader is async,
> MediaCodec will be applied to hook up.

Comment 5

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

For developing this you can use the following page as a test case:

http://people.mozilla.org/~cpearce/fmp4/

You'll need to set the pref media.fragmented-mp4.exposed to true, otherwise you'll be using the normal MediaDecoderReader for your platform, and not the MP4Reader.

Comment 6

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

(In reply to Blake Wu [:bwu] from comment #3)

> The main difference will lie in inputting encoded frame and outputting
> decoded frame. And since the current design for mp4reader is async,
> MediaCodec will be applied to hook up.
Two more tasks to do.
1. Resource management 
   B2G uses HW codec which resource is limited. Need to integrate the resource managment currently used in OMXCodecProxy.
2. MediaExtractor replacement
   Replace original MediaExtractor with current mp4_demuxer used in windows version as well.

Comment 7

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

Why do you need to replace the MediaExtractor? The MP4Reader includes a demuxer, and the PlatformDecoderModule is only used by the MP4Reader.

Incidentally, I've been thinking that perhaps we should switch to use Android's MP4 demuxer, rather than adding non-fragmented MP4 demuxing and seeking to the current mp4_demuxer. I haven't had a chance to look into it in detail yet however.

Comment 8

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

(In reply to Chris Pearce (:cpearce) from comment #7)
> Why do you need to replace the MediaExtractor? The MP4Reader includes a
> demuxer, and the PlatformDecoderModule is only used by the MP4Reader.
  I mean to replace Android's MediaExtractor(demuxer) with MP4Reader's demuxer. I thought we should use a generic demuxer on cross-platforms?  
> Incidentally, I've been thinking that perhaps we should switch to use
> Android's MP4 demuxer, rather than adding non-fragmented MP4 demuxing and
> seeking to the current mp4_demuxer. I haven't had a chance to look into it
> in detail yet however.
Why should we use Android's?

Comment 9

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

(In reply to Blake Wu [:bwu] from comment #8)
> (In reply to Chris Pearce (:cpearce) from comment #7)
> > Why do you need to replace the MediaExtractor? The MP4Reader includes a
> > demuxer, and the PlatformDecoderModule is only used by the MP4Reader.
>   I mean to replace Android's MediaExtractor(demuxer) with MP4Reader's
> demuxer. I thought we should use a generic demuxer on cross-platforms?  

Yes, it would be good if all platforms used the same demuxer. That is why we have the MP4Reader. Once the demuxer that MP4Reader uses supports fragmented MP4, we can use the MP4Reader for all MP4 playback on all platforms that have a PlatformDecoderModule.

So I don't understand what you mean by "replace Android's MediaExtractor". Do you want to replace the demuxer that the android backend (content/media/plugins) uses, or replace the demuxer that the OMX backend uses (content/media/omx)?

If we replaced the demuxer that either the MediaOmxReader uses, or the demuxer that the MediaPluginReader uses, then we'd just be duplicating the MP4Reader's functionality. We are better to just use the MP4Reader, then we only have to write the code once.


> > Incidentally, I've been thinking that perhaps we should switch to use
> > Android's MP4 demuxer, rather than adding non-fragmented MP4 demuxing and
> > seeking to the current mp4_demuxer. I haven't had a chance to look into it
> > in detail yet however.
> Why should we use Android's?

Our current mp4_demuxer does not support seeking, or non fragmented mp4. We need these features before we can enable the MP4Reader for non-MSE MP4 playback in <video>.

It will probably take less time to integrate an existing demuxer that supports the features we want, rather than to write and debug them.

Android's demuxer also supports other formats that we may want, like 3GP, which we need on mobile.

Comment 10

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

(In reply to Chris Pearce (:cpearce) from comment #9)
>
> Yes, it would be good if all platforms used the same demuxer. That is why we
> have the MP4Reader. Once the demuxer that MP4Reader uses supports fragmented
> MP4, we can use the MP4Reader for all MP4 playback on all platforms that
> have a PlatformDecoderModule.
Totally agree!
> So I don't understand what you mean by "replace Android's MediaExtractor".
> Do you want to replace the demuxer that the android backend
> (content/media/plugins) uses, or replace the demuxer that the OMX backend
> uses (content/media/omx)?

My mistake:) 
I don't mean to modify the existing codes to do replacement. What I am talking about is for B2G decoder module design. 
In this B2G decoder module, 
I am thinking to migrate the current omxDecoder with some resource management used in MediaOmxReader to fit this fmp4 design as windows version. Two replacements should be required. One is to replace OMXCodec with MediaCodec for async call flow. The other is to replace MediaExtractor with MP4 Demuxer for cross-platform purpose. 
 
> If we replaced the demuxer that either the MediaOmxReader uses, or the
> demuxer that the MediaPluginReader uses, then we'd just be duplicating the
> MP4Reader's functionality. We are better to just use the MP4Reader, then we
> only have to write the code once.
> 
> 
> > > Incidentally, I've been thinking that perhaps we should switch to use
> > > Android's MP4 demuxer, rather than adding non-fragmented MP4 demuxing and
> > > seeking to the current mp4_demuxer. I haven't had a chance to look into it
> > > in detail yet however.
> > Why should we use Android's?
> 
> Our current mp4_demuxer does not support seeking, or non fragmented mp4. We
> need these features before we can enable the MP4Reader for non-MSE MP4
> playback in <video>.
> 
> It will probably take less time to integrate an existing demuxer that
> supports the features we want, rather than to write and debug them.
> 
> Android's demuxer also supports other formats that we may want, like 3GP,
> which we need on mobile.
Got it. Maybe we can discuss this on the Meida work week.

Comment 11

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

(In reply to Blake Wu [:bwu] from comment #6)
> (In reply to Blake Wu [:bwu] from comment #3)
> 
> > The main difference will lie in inputting encoded frame and outputting
> > decoded frame. And since the current design for mp4reader is async,
> > MediaCodec will be applied to hook up.
> Two more tasks to do.
> 1. Resource management 
>    B2G uses HW codec which resource is limited. Need to integrate the
> resource managment currently used in OMXCodecProxy.

For this, you'd need to change MP4Reader to shutdown the MediaDataDecoder when the decoder becomes dormant, and re-create it when the decoder comes out of dormancy.

> Got it. Maybe we can discuss this on the Meida work week.

Yes, We should discuss MP4 demuxing at the next work week. :)

Comment 12

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

Originally, I planned to have a MediaCodecProxy to wrap MediaCodec with current codec resource management for async process. However there is no information related to graphicBuffer in current output interface (http://androidxref.com/4.3_r2.1/xref/frameworks/av/media/libstagefright/MediaCodec.cpp#275) which is required in B2G render pipeline. So we cannot use MediaCodec directly. 

Therefore I will copy MediaCodec and change the output interface(dequeueOutputBuffer) to expose the graphicbuffer and wrap it into VideoData as current OmxDecoder and MediaOmxReader do.

Comment 13

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

Since graphic buffer is created in ACodec, I am going to copy ACodec to B2G code base and change it to expose the graphic buffer to MediaCodec as well. 

Hi Sotaro, 
May I have your opinion here?

Comment 14

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

(In reply to Blake Wu [:bwu] from comment #13)
> Since graphic buffer is created in ACodec, I am going to copy ACodec to B2G
> code base and change it to expose the graphic buffer to MediaCodec as well. 
> 
> Hi Sotaro, 
> May I have your opinion here?

I feel that we should try to avoid copying ACodec as much as possible. This class could have a lot of hw vendor's modification. Even when we modified it, it should stay the same place. By ACodec::findBufferByID(), we could get GraphicBuffer from id. The problem is that ACodec::BufferInfo and ACodec::findBufferByID() is private. Can it be fixed like by adding a friend class to ACodec?

And if we are going to modify MediaCodec and  ACodec, we should get hw vendor's feedback before start implementation.

Comment 15

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

Anyways, we have to care about the following.
- get feedback from hw vendor partners before start implementation.
- modification should be minimum.

Comment 16

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

And not to forget to modify ACodec::allocateOutputBuffersFromNativeWindow() as not to return minimum undequeued buffers to the native window.

The following part.
----------------------------
    } else {
        // Return the required minimum undequeued buffers to the native window.
        cancelStart = bufferCount - minUndequeuedBuffers;
        cancelEnd = bufferCount;
    }

Comment 17

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

My thought is that if the following three conditions are met, it seems better to modify the class in place, not make copy in gecko.
- (1) Change is relatively small
- (2) Do not have direct dependence to gecko
- (3) Could have a lot of hw vendor's modifications

Comment 18

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

Hi Sotaro, 

Thanks for your feedback. 
The reason I want to have a copy in Gecko and modify it is trying to "decouple" Gecko and stagefright. And then we should have less porting efforts when Android has some changes in the new versions.
But what you mentioned is indeed another problem that how to take care of each HW partner's change in the same Gecko files or who should take care of it. 
Since "should we decouple Gecko and Android's Stagefright" is an open question and may require more discussions (maybe in next media work week or mailing list) and HW partner's feedback, I will first modify the codes in place as currently we do.

Comment 19

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

we current use OMXCodec as hw codec. I recognize the OMXCodec as same as hal level API, because it actually work as hal level API. It encapsulate hw's difference. If we want to copy a class from android to gecko, the class need to be vendor independent. Confirmation of current vendor's ACodec/OMXCodec modificaiton level seems important.

Comment 20

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

At least OMXCodec, caf OMXCodec is too different from aosp OMXCodec. We can not maintain such code in gecko.

Comment 21

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

I think this could be also a question of working model with HW partners. Should Mozilla have its own "XXXCodec" code as Android does and HW partners can have some changes on it for their HWs with Mozilla's help (or Mozilla should take responsibility to make B2G work on different HW platform?)? That's why I said it may require more discussions.

Comment 22

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

Might found a way to get graphic buffers from MediaCodec without modifying stagefright.
MediaCodec has a method renderOutputBufferAndRelease() that queues buffer back to native window and trigger GonkNativeWindowNewFrameCallback::OnNewFrame(). By implementing that callback, we can getCurrentBuffer() to get TextureClient for rendering. I tested it using HW H.264 decoder to do WebRTC video call and it seems working. Will upload a patch to get your feedback later.

Comment 23

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

Patch uploaded. Please check attachment 8410015 [details] [diff] [review] in bug 911046.

Comment 24

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

Hi John, 
Awesome! Thanks for your information. 
As you mentioned today, please also let us know your investigation if there is any performance impacted or not.

Comment 25

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

Hi Sotaro,

May I have your some feedback here? 
In order to get graphic buffer from decoded frames via MediaCodec/ACodec. There are two ways to do that. 
1. Modify current MediaCodec.cpp and ACodec.cpp to expose graphic buffer as we discussed previously in this bug
The change is not big. But we need to take care of merge when Google changes it. 
2. Enqueue buffer and dequeue it via a registered callback, OnNewFrame() as comment 22
No need to change Android's code. But not straightforward to me and not sure if there is any overhead caused.
Personally, I prefer the first one due to its simplicity, but would like to have more discussions here.

Comment 26

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

(In reply to Blake Wu [:bwu][:blakewu] from comment #25)
> Hi Sotaro,
> 
> May I have your some feedback here? 
> In order to get graphic buffer from decoded frames via MediaCodec/ACodec.
> There are two ways to do that. 
> 1. Modify current MediaCodec.cpp and ACodec.cpp to expose graphic buffer as
> we discussed previously in this bug
> The change is not big. But we need to take care of merge when Google changes
> it. 
> 2. Enqueue buffer and dequeue it via a registered callback, OnNewFrame() as
> comment 22
> No need to change Android's code. But not straightforward to me and not sure
> if there is any overhead caused.
> Personally, I prefer the first one due to its simplicity, but would like to
> have more discussions here.

 Currently WebRTC uses approach 2 and I found that ACodec could block at http://androidxref.com/4.4_r1/xref/frameworks/av/media/libstagefright/ACodec.cpp#3436

 It looks like that's because GonkNativeWindow doesn't have spare buffer so native_window_dequeue_buffer_and_wait() has to wait until consumer release a buffer back to buffer queue. :(

Comment 27

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

Maybe it is related to the modification Sotaro mentioned in comment 16.

Comment 28

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

I think [1] is reasonable way for media playback. From code maintenance point of view, [2] is clreaner than [1]. But [2] has a serious problem. The problem of [2] is already written in Comment 26. gecko's media frameworks request more buffers than MediaCodec/ACodec/GonkNativeWindow expected. We need to put off calling native_window_dequeue_buffer_and_wait() until Compositor's rendering complete in media playback use case.

In the past, when I tried to add Fence support to gonk JB, I once tried [2] by using OMXCodec in the past. The result is that the decoding thread got stacked at native_window_dequeue_buffer_and_wait() called under OMXCodec::signalBufferReturned().

Comment 29

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

Attachment: MediaCodec_ACodec_Change.patch

The attached is intended to expose graphicBuffer. 
Please let me know if you have any concern or not.

Comment 30

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

Comment on attachment 8419240 [details] [diff] [review]
MediaCodec_ACodec_Change.patch

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

My biggest concern would be introducing MediaBuffer to MediaCodec (although their names look pretty match :-)) and the lifecycle management code seems missing in this patch.

::: include/media/stagefright/MediaCodec.h
@@ +123,5 @@
>      status_t setParameters(const sp<AMessage> &params);
>  
> +    // This is used to get graphicBuffer with the index obtained from
> +    // calling dequeueOutputBuffer()
> +    status_t getOutputMediaBufferFromIndex(size_t index, MediaBuffer** graphicBuffer);

s/graphicBuffer/mediaBuffer/

Or returning GraphicBuffer instead of MediaBuffer:
 * IMHO, MediaBuffer looks more tightly coupled with OMXCodec API
 * it's more difficult to use. You have to call add_ref()/release() explicitly to control its lifecycle. On the other hand, sp<GraphicBuffer> takes care of it for you.

@@ +190,5 @@
>          void *mBufferID;
>          sp<ABuffer> mData;
>          sp<ABuffer> mEncryptedData;
> +        //Let life cycle be controled by ACodec
> +        MediaBuffer* mMediaBuffer;

Didn't find add_ref()/release() in this patch. Could you please explain how ACodec control the lifecycle?

Comment 31

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #29)
> Created attachment 8419240 [details] [diff] [review]
> MediaCodec_ACodec_Change.patch

I am not fun of adding MediaBuffer to ACodec. And we do not need to add members to ACodec, I think.
As in Comment 14, can't we get GraphicBuffer just by uisng ACodec::findBufferByID()?

- Get GraphicBuffer by uising index from MediaCodec.

Comment 32

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

Only thing we need to add is the following by minimum modification.

> - Get GraphicBuffer by uising index from MediaCodec.

Comment 33

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

You can create MediaBuffer in gecko siede. "index" can be added to MediaBuffer as meta data.

Comment 34

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

(In reply to John Lin[:jolin][:jhlin] from comment #30)
> Comment on attachment 8419240 [details] [diff] [review]
> MediaCodec_ACodec_Change.patch
> 
> Review of attachment 8419240 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> My biggest concern would be introducing MediaBuffer to MediaCodec (although
> their names look pretty match :-)) and the lifecycle management code seems
> missing in this patch.

The reason to use MediaBuffer is in this way we can leaverge the current design for "VideFrame" generation in OmxDecoder @http://dxr.mozilla.org/mozilla-central/source/content/media/omx/OmxDecoder.cpp?from=OmxDecoder.cpp&case=true#819. 
For lifecycle management. 
I should be more clear that I mean to let "graphicBuffer" be controlled by ACodec. And MediaBuffer indeed needs to be handled as well. Now I am thinking to create it outside MediaBuffer or inside, which lifecycle management will be handled differently.

Comment 35

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

(In reply to Sotaro Ikeda [:sotaro] from comment #31)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #29)
> > Created attachment 8419240 [details] [diff] [review]
> > MediaCodec_ACodec_Change.patch
> 
> I am not fun of adding MediaBuffer to ACodec. And we do not need to add
> members to ACodec, I think.
> As in Comment 14, can't we get GraphicBuffer just by uisng
> ACodec::findBufferByID()?

findBufferByID(uint32_t portIndex, IOMX::buffer_id bufferID, ssize_t *index = NULL);
This function requires to input bufferID. I think it is difficult for MediaCode to get this information.  
So a new API should be needed as below with making MediaCodec friend with ACodec. My current plan is to expose graphicBuffer via MediaCodec and people who use it can new a MediaBuffer with the exposed graphicBuffer and needs to take care of the lifecycle of this MediaBuffer. And the lifecycle of graphicBuffer is handled by ACodec as usual.  
 
+sp<GraphicBuffer> MediaCodec::getOutputGraphicBufferFromIndex(size_t index) {
+
+    if (mState != STARTED || index >= mPortBuffers[kPortIndexOutput].size()) {
+        return NULL;
+    }
+
+    return mCodec->mBuffers[kPortIndexOutput].editItemAt(index).mGraphicBuffer;
+}

Comment 36

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

Attachment: MediaCodec friends with ACodec to expose graphicBuffer

The version that MediaCodec friends with ACodec.

Comment 37

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

The reason that I did't prefer to let MediaCodec friend with ACodec is that would make MediaCodec coupled with Acodec. However MediaCodec should be one of users to use ACodec to access Codecs and no need to have a strong relation with it. But if for FFOS we will use MediaCodec very often to access Codecs via ACodec, then I think if is good to make them coupled.

Comment 38

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

Comment on attachment 8420022 [details] [diff] [review]
MediaCodec friends with ACodec to expose graphicBuffer

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

Looks good. It is less evil than the previous patch, I think. The change is minimum and clean.

Comment 39

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

Create a bug (bug 1009410) to expose graphicBuffer to MediaCodec which should be independent from this bug and could be leveraged for others cases/bugs.

Comment 40

[Bruce Sun [:brsun]]

(In reply to Sotaro Ikeda [:sotaro] from comment #28)
> I think [1] is reasonable way for media playback. From code maintenance
> point of view, [2] is clreaner than [1]. But [2] has a serious problem. The
> problem of [2] is already written in Comment 26. gecko's media frameworks
> request more buffers than MediaCodec/ACodec/GonkNativeWindow expected. We
> need to put off calling native_window_dequeue_buffer_and_wait() until
> Compositor's rendering complete in media playback use case.
> 
> In the past, when I tried to add Fence support to gonk JB, I once tried [2]
> by using OMXCodec in the past. The result is that the decoding thread got
> stacked at native_window_dequeue_buffer_and_wait() called under
> OMXCodec::signalBufferReturned().

Hi Sotaro,
By separating audio/video decoding into different threads, some buffer-starvation problem might be eased: https://bugzilla.mozilla.org/show_bug.cgi?id=974250#c43
It seems this task is on going now (thanks bug 979104), but I am not sure whether all buffer-starvation problems can be resolved by doing so. What do you think?

Comment 41

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

From android::OMXCodec user's point of view, buffer starvation is recognized that OMXCodec::read() does not return long time. Sometime it failed because of timeout.

bug 979104 is important and could mitigate the problem. It could minimize the bad effect of buffer starvation. But it does not fix the buffer starvation that happened at OMXCodec. 

The reason of the starvation can be categorized to the following.
-[1] output buffers do not returned to OMXCodec quickly.
  + we already return video frame as soon as possible.
  + media frame works need to hold some video buffers for smooth playback.
    And compositor is far away from an application.

-[2] input port's data read is pending.
  + [2-1] If it happens OMCCodec's mutex is held long time and blocks OMXCodec task processing. 
  + [2-2] Both Audio and Video decoder share same MediaResource instance.
          MediaResource is a blocking API, when video 's read is on going, audio's data read becomes wait.
  + [2-3] On b2g nsMediaCache size is set to 4MB. It is very small. And nsMediaCache holds discrete data.
          If cached data overflows the size limit, low priority data is dropped.
          On android, media cache is implemented in NuCachedSource2.
          It hold data in RAM and it caches continuous range of data.
          On b2g, in http video playback case, even when just playbacking video,
          MediaCache could face a cache miss often.
  + [2-4] During seeking video track, Audio track read is not blocked.
          On android, during video seek, audio track is set to pause.
          In HTTP video streaming case, this could affect to the seek performance.

Comment 42

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

(In reply to Sotaro Ikeda [:sotaro] from comment #41)
> From android::OMXCodec user's point of view, buffer starvation is recognized
> that OMXCodec::read() does not return long time. Sometime it failed because
> of timeout.

timeout problem can be fixed by using MediaCodec/ACodec.

About [1], we already did almost everything we can do :-( We can increase the number of buffers allocated by hw codecs by changing GonkBufferQueue's MaxAcquiredBufferCount. But it could cause ouf of memory problem in large video content.

[2-1] can be fixed by MediaCodec/ACodec.
[2-2], I talked a little bit about it with cpearce in NW media work week.
       I do not know if there is already a bug for it.
[2-3] One solution is use RAM as media cache if b2g device has a lot of RAM.
      It is Bug 963938, other solution could be possible.
[2-4] I do not know if there is a bug for it.

Comment 43

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

(In reply to Sotaro Ikeda [:sotaro] from comment #42)
> [2-2], I talked a little bit about it with cpearce in NW media work week.
>        I do not know if there is already a bug for it.

There is not a bug on file for this yet.

Comment 44

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

Attachment: mp4 decoding blockdiagram v2.pdf

Update the block diagram.

Comment 45

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

Attachment: Add_MediaCodecProxy.patch

WIP. 
Add MediaCodecProxy to have more generic interfaces to access codec via MediaCodec and integrate with current codec resource manager as well. 
For architecture's view, please refer to attachment 8423761 [details]. 

For "VideoFrame" construction, it will be handled in B2GVideoOutputSource.cpp which currently I am working on.

Comment 46

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

Attachment: Add_MediaCodecProxy.patch

Update for some missed changes and comments.

Comment 47

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

Attachment: Decoding block diagram v3.pdf

Update block diagram with Media Resource Manager.

Comment 48

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

Attachment: Add_MediaCodecProxy_v2.patch

Update to add mutex and let people (B2GVideoOutputSource) who uses MediaCodecProxy to create/new GonkNativeWindow.

Comment 49

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

Is it possible to share John Lin's code for WebRTC H264 via stagefright on B2G inside our PlatformDecoderModule for B2G? It would be good if we could avoid duplicating as much of John's work as possible here.

Comment 50

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

I think it should not be possible to share.
afaik, there are 3 differences between my current developing codes in this bug and John's WebRTC codes. 
1. More generic 
   My codes should be more generic and can be used for all kinds of application. 
   Current MediaCodec codes in WebRTC is integrated and designed for WebRTC only. Probably this can have better performance I think.    
2. Media Resource Management 
   it looks like current usage of MediaCodec in WebRTC didn't consider Media Resource Management which manages every access to HW codec. This can avoid resource conflict. 
3. The way to get grapicBuffer 
   Currently WebRTC use dequeue/enqueue buffer with registering a callback in native window as comment 26. My codes is intended to directly expose graphic buffer as discussed in bug 1009410. This should not have any performance problems. This is still under discussing.

NI jolin to sync and have more info.

Comment 51

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

(In reply to Blake Wu [:bwu][:blakewu] from comment #50)
> I think it should not be possible to share.
> afaik, there are 3 differences between my current developing codes in this
> bug and John's WebRTC codes. 
> 1. More generic 
>    My codes should be more generic and can be used for all kinds of
> application. 
>    Current MediaCodec codes in WebRTC is integrated and designed for WebRTC
> only. Probably this can have better performance I think.    
> 2. Media Resource Management 
>    it looks like current usage of MediaCodec in WebRTC didn't consider Media
> Resource Management which manages every access to HW codec. This can avoid
> resource conflict. 
> 3. The way to get grapicBuffer 
>    Currently WebRTC use dequeue/enqueue buffer with registering a callback
> in native window as comment 26. My codes is intended to directly expose
> graphic buffer as discussed in bug 1009410. This should not have any
> performance problems. This is still under discussing.
> 
> NI jolin to sync and have more info.

I don't think the code should be shared as is, either. But if it's about DRY, we can always think about how to refactor/generalize it to minimize code duplication.

1. The dependency to WebRTC should be easy to remove. But I'm not sure whether the filling/draining threading model fits in PlatformDecoderModule well. If it's just some small changes, then that should be fine too.
2. Resource management should be implemented eventually, although the desired behavior could be different. Bug 1003712 only needs a mechanism to know whether HW codec is in use or not, but IIUC, for playing back the 2nd video won't start until 1st video release HW decoder.

 Another DRY approach, as Alfredo demonstrated in bug 984223, is re-implementing WebRTC H.264 decoder using Blake's work through PlatformDecoderModule API.

Comment 52

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

> 2. Resource management should be implemented eventually, although the
> desired behavior could be different. Bug 1003712 only needs a mechanism to
> know whether HW codec is in use or not, but IIUC, for playing back the 2nd
> video won't start until 1st video release HW decoder.

One thing we need to remember is that in b2g, each application's status change happens asynchronously. Even if a foreground application try to free hw codec when the app change to background and a background application move to foreground and try to get hw codec, there is a cases that the background app try to get hw before the foreground app try to free hw codec.

In android, this application's status change always happen synchronously (with timeout).

Comment 53

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

Comment on attachment 8424723 [details] [diff] [review]
Add_MediaCodecProxy_v2.patch

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

As a first patch it seems good. The followings are some comments and questions to the patch. It seems better to have other classes to understand the class role correctly.

The patch has a lot of trailing tabs and white spaces, it might be better to remove this.

- How are we going to handle mp3?
  + OmxDecoder has some codes that handling mp3.
  + How is it going to be handled when mp3 is decoded by MediaCodec?
- Is there a reason why CodecType is defined in MediaCodecProxy?
  + Doesn't PlatformDecoderModule have such definition?
- Can I have informations of B2GVideoOutputSource and B2GMediaDataDecoder?
- Can we prevent memcopy from MediaCodecProxy::Input()?
  + FileMediaResource::Read() and ChannelMediaResource::Read() already do memcopy inside the functions.
- When we use Surface, we do not need to use GonkNativeWindowClient.
- Who is going to handle TextureClient?
- Which ojbect is going to handle TextureClient's recycle callback?
- ReleaseAllPendingVideoBuffersLocked() might block long time,
  if the function's caller does not like it, need to call PostReleaseVideoBuffer().
- About MediaCodecProxy::ReleaseMediaBuffer(), if the MediaBuffer's lifetime is controlled by TextureClient,
   We can not call MediaBuffer::release() independently from TextureClient. 
   We need to use MediaBuffer just for consistency to OmxDecoder.
   It might be better to remove MediaBuffer usage if possible.
   For media framework, MediaBuffer is not necessary.

Comment 54

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

(In reply to Sotaro Ikeda [:sotaro] from comment #53)
> - Can we prevent memcopy from MediaCodecProxy::Input()?
>   + FileMediaResource::Read() and ChannelMediaResource::Read() already do
> memcopy inside the functions.

The above functions are for input to parser. It is not related to MediaCodecProxy::Input(). Anyway, memcopy seems additional cost.

Comment 55

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

(In reply to Sotaro Ikeda [:sotaro] from comment #53)
> - ReleaseAllPendingVideoBuffersLocked() might block long time,
>   if the function's caller does not like it, need to call
> PostReleaseVideoBuffer().

This might not be a problem on MediaCodec.

Comment 56

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

Thanks for your feedback. 
Currently I am hooking up everything and having a build. 
(In reply to Sotaro Ikeda [:sotaro] from comment #53)
> Comment on attachment 8424723 [details] [diff] [review]
> Add_MediaCodecProxy_v2.patch
> 
> Review of attachment 8424723 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As a first patch it seems good. The followings are some comments and
> questions to the patch. It seems better to have other classes to understand
> the class role correctly.
> 
> The patch has a lot of trailing tabs and white spaces, it might be better to
> remove this.
> 
> - How are we going to handle mp3?
>   + OmxDecoder has some codes that handling mp3.
>   + How is it going to be handled when mp3 is decoded by MediaCodec?
The first target supported audio format will be AAC due to MSE. After this, I will add more to support mp3 and others. 
> - Is there a reason why CodecType is defined in MediaCodecProxy?
>   + Doesn't PlatformDecoderModule have such definition?
No. Each platform has its own define. 
> - Can I have informations of B2GVideoOutputSource and B2GMediaDataDecoder?
Will provide you once I finish the code hook-up and building.  
> - Can we prevent memcopy from MediaCodecProxy::Input()?
>   + FileMediaResource::Read() and ChannelMediaResource::Read() already do
> memcopy inside the functions.
Currently I don't have any ideas to avoid memcopy and pass data to IMOX. AFAIK currently OMXCodec also does memcopy when reading encoded data from MediaExtrator.  
> - When we use Surface, we do not need to use GonkNativeWindowClient.
Thanks! I will check it. 
> - Who is going to handle TextureClient?
B2GVideoOutputSource
> - Which ojbect is going to handle TextureClient's recycle callback?
B2GVideoOutputSource
> - ReleaseAllPendingVideoBuffersLocked() might block long time,
>   if the function's caller does not like it, need to call
> PostReleaseVideoBuffer().
> - About MediaCodecProxy::ReleaseMediaBuffer(), if the MediaBuffer's lifetime
> is controlled by TextureClient,
>    We can not call MediaBuffer::release() independently from TextureClient. 
>    We need to use MediaBuffer just for consistency to OmxDecoder.
>    It might be better to remove MediaBuffer usage if possible.
>    For media framework, MediaBuffer is not necessary.
Thanks! I will check it.

Comment 57

[Ivan Tsay (:ITsay)]

This feature will be moved to 2.1 due to the following reasons.
1. Need more time on implementation review and unittest
2. Per consult with media team, the MSE for MP4 will be available one version later than then Mozilla 32. even we finish this one, MSE for MP4 will be pref-off by default. No reason to sacrifice the quality to rush this one into 2.0. So move it to 2.1 and turn it on with MSE for MP4 together.

Comment 58

[Ivan Tsay (:ITsay)]

Move the ucid and functional team info to the new user story of MSE support. This bug is part of the engineering work on MSE feature.

Comment 59

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

Attachment: WIP patch for B2G Platform Decoder Module

Upload a WIP patch and update the current status. 

1. Video and audio can work with two-week-ago code base, but video will stop after 2 minutes. 
 
2. After moving to latest code base, video cannot work. Possible root cause is there is some demux codes changed. Need to have some modifications for those changes. Audio works fine. 

Note: 
This patch needs to work with the patch, attachment 8421562 [details] [diff] [review],on bug 1009410.

Comment 60

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

Attachment: 0001-Enable FMP4 for B2G

Enable MOZ_FMP4 for B2G

Comment 61

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

Attachment: 0002-Changes in MP4Reader/Decoder for B2G addition

1. Changes for adding B2G Decoder Module
2. Changes for codec resource management

Comment 62

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

Attachment: 0003- Add B2G Decoder Module and its Pref

1. Add B2G's Decoder Module
2. Add a Pref with default "false"

Comment 63

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

Attachment: 0004- Changes in demuxer to make aac_profile public

Make aac_profile "public", android's decoder requires this for configuration.

Comment 64

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

Attachment: 0005-Have a MediaCodecProxy to be easy to talk to MediaCodec with codec resource management

1.Add MediaCodecProxy to be a bridge between B2G decoder module and MediaCodec
2.Support codec resource management.

Comment 65

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

(In reply to Blake Wu [:bwu][:blakewu] from comment #64)
> Created attachment 8453523 [details] [diff] [review]
> 0005-Bug-941302-PlatformDecoderModule-for-FirefoxOS-B2G.patch
> 
> 1.Add MediaCodecProxy to be a bridge between B2G decoder module and
> MediaCodec
> 2.Support codec resource management.

This patch needs to work with the patch, attachment 8421562 [details] [diff] [review] [diff] [review],on bug 1009410.

Comment 66

[Mike Hommey [:glandium]]

Comment on attachment 8453516 [details] [diff] [review]
0001-Enable FMP4 for B2G

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

::: configure.in
@@ +3864,5 @@
>  MOZ_WMF=
> +if test -n "$MOZ_FMP4"; then
> +  MOZ_FMP4=1 
> +else
> +  MOZ_FMP4 = 

You have trailing whitespaces on both assignments.

Note that resetting MOZ_FMP4 when it's not set is not doing anything.

Comment 67

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #65)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #64)
> > Created attachment 8453523 [details] [diff] [review]
> > 0005-Bug-941302-PlatformDecoderModule-for-FirefoxOS-B2G.patch
> > 
> > 1.Add MediaCodecProxy to be a bridge between B2G decoder module and
> > MediaCodec
> > 2.Support codec resource management.
> 
> This patch needs to work with the patch, attachment 8421562 [details] [diff] [review]
> [review] [diff] [review],on bug 1009410.

Can the patch could be unified to Bug 904177? It seems that the some efforts are duplicated.

Comment 68

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

(In reply to Sotaro Ikeda [:sotaro] from comment #67)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #65)
> > (In reply to Blake Wu [:bwu][:blakewu] from comment #64)
> > > Created attachment 8453523 [details] [diff] [review]
> > > 0005-Bug-941302-PlatformDecoderModule-for-FirefoxOS-B2G.patch
> > > 
> > > 1.Add MediaCodecProxy to be a bridge between B2G decoder module and
> > > MediaCodec
> > > 2.Support codec resource management.
> > 
> > This patch needs to work with the patch, attachment 8421562 [details] [diff] [review]
> > [review] [diff] [review],on bug 1009410.
> 
> Can the patch could be unified to Bug 904177? It seems that the some efforts
> are duplicated.

MediaCodecProxy class is also going to be added in Bug 904177.

Comment 69

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

Bug 904177 divides remaining problems to its dependent bugs. Its way seems good for incremental development.

Comment 70

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

(In reply to Sotaro Ikeda [:sotaro] from comment #69)
> Bug 904177 divides remaining problems to its dependent bugs. Its way seems
> good for incremental development.

Thanks you for the reminder. 
Let me discuss with Bruce again to see if he can create a new bug to separate this MediaCodecProxy from Bug 904177 and merge our MediaCodecProxy to be commonly used. Otherwise there will be a dependency between this bug and bug 904177.

Comment 71

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

Comment on attachment 8453523 [details] [diff] [review]
0005-Have a MediaCodecProxy to be easy to talk to MediaCodec with codec resource management

Will integrate this patch with Bug 1037282.

Comment 72

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

Comment on attachment 8453518 [details] [diff] [review]
0003- Add B2G Decoder Module and its Pref

Need to have some changes related to those further changes mentioned in comment 71.
Will make corresponding changes.

Comment 73

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

Comment on attachment 8453517 [details] [diff] [review]
0002-Changes in MP4Reader/Decoder for B2G addition

Since decoder pipeline with MediaCodec will be changed, make this obsolete and will submit a new patch.

Comment 74

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

(In reply to Sotaro Ikeda [:sotaro] from comment #68)
> > > This patch needs to work with the patch, attachment 8421562 [details] [diff] [review]
> > > [review] [diff] [review],on bug 1009410.
> > 
> > Can the patch could be unified to Bug 904177? It seems that the some efforts
> > are duplicated.
> 
> MediaCodecProxy class is also going to be added in Bug 904177.
Sotaro, 
More information for you. 
I am going to base on MediaCodecProxy in the bug 1037282 to create a B2GCodec for responding to resource management and doing Mediabuffer lifetime control which will not be shared with or leverge the patches on Bug 904177 since the design of reader(MP4Reader) and interfaces are different. 
Please let me know if you have other thoughts or concerns.
Thanks!

Comment 75

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

Attachment: Add B2GCodec

This B2GCodec plays a "decoder" role in this platform decoder module to talk with MediaCodecProxy.

Comment 76

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

Attachment: Changes-in-MP4Reader.patch

1.Some changes for adding B2G Decoder Module 
2.Some Changes to support codec resource management

Comment 77

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

Attachment: Add-B2G-decoder-module and its Pref .patch

Update 
1. Add B2G's Decoder Module
2. Add a Pref with default "false"

Comment 78

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

(In reply to Blake Wu [:bwu][:blakewu] from comment #75)
> Created attachment 8456759 [details] [diff] [review]
> Add B2GCodec
> 
> This B2GCodec plays a "decoder" role in this platform decoder module to talk
> with MediaCodecProxy.

This needs to work with the patch attachment 8456705 [details] [diff] [review].  
Currently it still targets to have exposed grapic Buffer.

Comment 79

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

Attachment: 0003-Add-B2GCodec.patch

Remove some unnecessary codes in attachment 8456759 [details] [diff] [review]. 
This B2GCodec plays a "decoder" role in this platform decoder module to talk with MediaCodecProxy. 
1. It manly inputs encoded frames to MediaCodecProxy and gets decoded frames from MediaCodecProxy.
2. It controls the life cycle of MediaBuffer which contians graphicbuffer for render. The life cycle of graphicBuffer is controlled by ACodec.
3. It produces VideoData with MediaBuffer/GraphicBuffer.

Comment 80

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

(In reply to Blake Wu [:bwu][:blakewu] from comment #79)
> 3. It produces VideoData with MediaBuffer/GraphicBuffer.
Sorry. One correction. This part is done in B2GVideoOutputSource.cpp in attachment 8456763 [details] [diff] [review].

Comment 81

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #74)
> (In reply to Sotaro Ikeda [:sotaro] from comment #68)
> > > > This patch needs to work with the patch, attachment 8421562 [details] [diff] [review]
> > > > [review] [diff] [review],on bug 1009410.
> > > 
> > > Can the patch could be unified to Bug 904177? It seems that the some efforts
> > > are duplicated.
> > 
> > MediaCodecProxy class is also going to be added in Bug 904177.
> Sotaro, 
> More information for you. 
> I am going to base on MediaCodecProxy in the bug 1037282 to create a
> B2GCodec for responding to resource management and doing Mediabuffer
> lifetime control which will not be shared with or leverge the patches on Bug
> 904177 since the design of reader(MP4Reader) and interfaces are different. 
> Please let me know if you have other thoughts or concerns.
> Thanks!

I am not fun of having several classes directly uses android::MeduaCidec. But it is OK, both classes are just wrapper classes.

Comment 82

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

> > > 
> > > MediaCodecProxy class is also going to be added in Bug 904177.
> > Sotaro, 
> > More information for you. 
> > I am going to base on MediaCodecProxy in the bug 1037282 to create a
> > B2GCodec for responding to resource management and doing Mediabuffer
> > lifetime control which will not be shared with or leverge the patches on Bug
> > 904177 since the design of reader(MP4Reader) and interfaces are different. 
> > Please let me know if you have other thoughts or concerns.
> > Thanks!
> 
> I am not fun of having several classes directly uses android::MeduaCidec.
> But it is OK, both classes are just wrapper classes.

Sorry, comment was wrong. Is is OK to implement B2GCodec based on OMXCodecProxy.

Comment 83

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

Comment on attachment 8457808 [details] [diff] [review]
0003-Add-B2GCodec.patch

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

I could not understand well how this class is used only from this class's code. I have basic level questions about it. 

Some questions and comments.
- Which class is going to instantiate and use it? Is there already a such class in gecko?
    Without B2GCodec's user side's code, I could not understand well whether this level of abstraction is correct.
- In which use case is the class used?
- It seems better to avoid to use b2g as class name. B2G say nothing about the low level platform. For example, there is a b2g desktop.
  If we want to have a such name we use "gonk".
  And the class name seems too generic compare to other classes. It seems better that the class name says what the class is.
- Why does it use MediaBuffer as output buffer?
- Why B2G class creates two ALooper at most? It mean it creates two threads at most.
  It seems the responsibility of a class that creates the B2G instance.
- Why is there no source code that uses TextureClient? TextureClient is gecko's platform independent graphic buffer wrapper.

Comment 84

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

Last Tuesday, I re-think about the ways of how to disclose GraphicBuffer(TextureClient) from MediaCodec.

I write about it at Bug 1009420 Comment 14 and Bug 1009420 Comment 15. Current way that we used until recently is not ideal. It seems better to re-think about it.

Comment 85

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

(In reply to Sotaro Ikeda [:sotaro PTO July/25 - Aug/3] from comment #83)
>
>   It seems the responsibility of a class that creates the B2G instance.
> - Why is there no source code that uses TextureClient? TextureClient is
> gecko's platform independent graphic buffer wrapper.

gecko media frameworks uses Image class as platform independent video frame wrapper. Image class wraps TextureClient, TextureClient wraps GraphicBuffer on gonk.

Comment 86

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

Comment on attachment 8456763 [details] [diff] [review]
Add-B2G-decoder-module and its Pref .patch

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

Edwin, can you look over this first for me please? I will review after you.

Comment 87

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

(In reply to Sotaro Ikeda [:sotaro PTO July/25 - Aug/3] from comment #82)
> > > > 
> > > > MediaCodecProxy class is also going to be added in Bug 904177.
> > > Sotaro, 
> > > More information for you. 
> > > I am going to base on MediaCodecProxy in the bug 1037282 to create a
> > > B2GCodec for responding to resource management and doing Mediabuffer
> > > lifetime control which will not be shared with or leverge the patches on Bug
> > > 904177 since the design of reader(MP4Reader) and interfaces are different. 
> > > Please let me know if you have other thoughts or concerns.
> > > Thanks!
> > 
> > I am not fun of having several classes directly uses android::MeduaCidec.
> > But it is OK, both classes are just wrapper classes.
> 
> Sorry, comment was wrong. Is is OK to implement B2GCodec based on
> OMXCodecProxy.

The answer should be no since the function call (like read) in OMXCodecProxy is a blocking call and the pipeline of demuxing is different from the current design of platform decoder module which is illustrated in the attachment 8423761 [details] and attachment 8424717 [details]. It is better to use MediaCodecProxy.

Comment 88

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

 (In reply to Sotaro Ikeda [:sotaro PTO July/25 - Aug/3] from comment #83)
> Comment on attachment 8457808 [details] [diff] [review]
> 0003-Add-B2GCodec.patch
> 
> Review of attachment 8457808 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I could not understand well how this class is used only from this class's
> code. I have basic level questions about it. 
Sorry not to have clear explanations. This patch is used for a new design for Platform Decoder Module as Cpearce mentioned in comment 1 and comment 2. For more information, please refer to attachment 8423761 [details] and 
attachment 8424717 [details] or I can explain to you for more questions. 
> 
> Some questions and comments.
> - Which class is going to instantiate and use it? Is there already a such
> class in gecko?
>     Without B2GCodec's user side's code, I could not understand well whether
> this level of abstraction is correct.
> - In which use case is the class used?
All the information should be found in B2GVideoOutputSource.cpp in attachment 8456763 [details] [diff] [review]. 
> - It seems better to avoid to use b2g as class name. B2G say nothing about
> the low level platform. For example, there is a b2g desktop.
>   If we want to have a such name we use "gonk".
>   And the class name seems too generic compare to other classes. It seems
> better that the class name says what the class is.
Yes. we should have a more clear name. "gonk" probably sounds like too low-level, but this class doesn't do so too low-level thing. 
> - Why does it use MediaBuffer as output buffer?
I would like to use it in B2GVideoOutputSource.cpp. 
> - Why B2G class creates two ALooper at most? It mean it creates two threads
> at most.
One is for MediaCodecProxy and the other one is used by this class to 1) notify the upper layer that codec resource is reserved and 2) handle the buffer release process.  
>   It seems the responsibility of a class that creates the B2G instance.
> - Why is there no source code that uses TextureClient? TextureClient is
> gecko's platform independent graphic buffer wrapper.
I am doing the similar thing in OmxDecoder in B2GVideoOutputSource.cpp. Maybe you can find it in that file. Sorry I don't get your questions. Could you explain it more? Thanks.

Comment 89

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

(In reply to Blake Wu [:bwu][:blakewu] from comment #88)
>  (In reply to Sotaro Ikeda [:sotaro PTO July/25 - Aug/3] from comment #83)
> > Comment on attachment 8457808 [details] [diff] [review]
> > 0003-Add-B2GCodec.patch
> > 
> > Review of attachment 8457808 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >   It seems the responsibility of a class that creates the B2G instance.
> > - Why is there no source code that uses TextureClient? TextureClient is
> > gecko's platform independent graphic buffer wrapper.
Got your questions now. :)  TextureClient is used in B2GVideoOutputSource::CreateVideoFrame(). 
B2GVideoOutputSource creates VideoData with MediaBuffer which is produced in B2GCodec.

Comment 90

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

> > - It seems better to avoid to use b2g as class name. B2G say nothing about
> > the low level platform. For example, there is a b2g desktop.
> >   If we want to have a such name we use "gonk".
> >   And the class name seems too generic compare to other classes. It seems
> > better that the class name says what the class is.
> Yes. we should have a more clear name. "gonk" probably sounds like too
> low-level, but this class doesn't do so too low-level thing. 

I do not understand why gonk is too low-level. The code is gonk dependent code. The the code is used for b2g desktop? If they are used also on b2g desktop it is ok to use b2g. Otherwise, gonk should be used, other classes on gecko uses gonk if the code is gonk dependent.

Comment 91

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

B2GVideoOutputSource is depend on [1] in Bug 1009420 Comment 14. As I explained in Bug 1009420, I don't think it is a good idea to use [1] on new MediaCodec implementation.

Comment 92

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

I checked attachment 8456763 [details] [diff] [review] and attachment 8457808 [details] [diff] [review]. I can not find an enough reason to create B2GCodec. The class seems just What it did seems duplicate to what MediaCodecProxy, B2GVideoOutputSource and B2GAudioOutputSource.

For me, Implementation becomes more make sense if B2GVideoOutputSource and B2GAudioOutputSource create MediaCodecProxy directly.

Comment 93

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8456763 [details] [diff] [review]
Add-B2G-decoder-module and its Pref .patch

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

Looks good! A few small changes needed, and general styling issues; consider using clang-format[1].

Bug 1022501 will probably break a few things here.

I'd like to take another look after these comments have been addressed.

[1] https://groups.google.com/d/msg/mozilla.dev.platform/0GABa8sL4BA/LMiziRb2lCwJ

::: content/media/fmp4/b2g/B2GAudioOutputSource.cpp
@@ +77,5 @@
> +
> +android::sp<B2GCodec>
> +B2GAudioOutputSource::Init(MediaDataDecoderCallback* aCallback)
> +{
> +  mDecoder = B2GCodec::Create(B2GCodec::AAC_DEC,aCallback);

nit: comma should be followed by a space.

@@ +83,5 @@
> +  sp<AMessage> format = new AMessage;
> +  sp<GonkNativeWindow> nativeWindow = NULL;
> +  // Fixed values
> +  ALOG("Init Audio channel no:%d, sample-rate:%d", mAudioChannels,mAudioRate);
> +  format->setString("mime", "audio/mp4a-latm");

bug 1022501 adds MP3 in MP4 support, so the MIME type can now vary.

@@ +93,5 @@
> +  mDecoder->configure(format, nativeWindow);
> +
> +  status_t rtn = mDecoder->Input(mUserData.Elements(), mUserData.Length(), 0, android::MediaCodec::BUFFER_FLAG_CODECCONFIG);
> +  if (rtn == OK)
> +    return mDecoder;

nit: braces around conditionals, even if they're only one line.

@@ +95,5 @@
> +  status_t rtn = mDecoder->Input(mUserData.Elements(), mUserData.Length(), 0, android::MediaCodec::BUFFER_FLAG_CODECCONFIG);
> +  if (rtn == OK)
> +    return mDecoder;
> +  else {
> +    ALOG("Faile to input codec specific data!");

"Failed"

@@ +99,5 @@
> +    ALOG("Faile to input codec specific data!");
> +    return nullptr;
> +  }
> +}
> +nsresult

nit: one empty line between function definitions.

@@ +101,5 @@
> +  }
> +}
> +nsresult
> +B2GAudioOutputSource::CreateAudioFrame(MPAPI::AudioFrame *aFrame,
> +                                       int64_t aStreamOffset)

This function isn't necessary. It transforms mAudioBuffer into an MPAPI::AudioFrame, which just gets copied below into an AudioData. Best to not use MPAPI if we don't have to.

@@ +113,5 @@
> +    return NS_ERROR_UNEXPECTED;
> +  data = mAudioBuffer->data();
> +  dataOffset = mAudioBuffer->range_offset();
> +  size = mAudioBuffer->range_length();
> +  aFrame->Set(timeUs, static_cast<char *>(data) + dataOffset, size, mAudioChannels, mAudioRate);

Assert |aFrame| non-null, or pass by reference instead.

@@ +122,5 @@
> +
> +  MPAPI::AudioFrame aframe;
> +
> +  if (CreateAudioFrame(&aframe, aStreamOffset) != NS_OK)
> +     return NS_ERROR_UNEXPECTED;

nit: indent should be 2 spaces.

@@ +138,5 @@
> +  return NS_OK;
> +}
> +nsresult
> +B2GAudioOutputSource::Output(int64_t aStreamOffset,
> +                        nsAutoPtr<MediaData>& aOutData)

nit: alignment.

@@ +152,5 @@
> +    case OK:
> +    {
> +      if (mAudioBuffer && mAudioBuffer->range_length() != 0) {
> +      int64_t timeUs;
> +      if (!mAudioBuffer->meta_data()->findInt64(kKeyTime, &timeUs))

Conditional blocks should be indented.

::: content/media/fmp4/b2g/B2GAudioOutputSource.h
@@ +26,5 @@
> +
> +  virtual nsresult Input(mp4_demuxer::MP4Sample* aSample) MOZ_OVERRIDE;
> +
> +  virtual nsresult Output(int64_t aStreamOffset,
> +                         nsAutoPtr<MediaData>& aOutput) MOZ_OVERRIDE;

nit: alignment.

@@ +32,5 @@
> +
> +  bool SetAudioFormat();
> +
> +  nsresult CreateAudioFrame(MPAPI::AudioFrame *aFrame,
> +                                int64_t aStreamOffset);

nit: alignment, here and below.

@@ +59,5 @@
> +  // from the next audio packet we decode. This happens after a seek, since
> +  // WMF doesn't mark a stream as having a discontinuity after a seek(0).
> +  bool mMustRecaptureAudioPosition;
> +
> +  android::MediaBuffer* mAudioBuffer;

Can this be an sp<MediaBuffer> instead, so we don't have to release it manually?

::: content/media/fmp4/b2g/B2GMediaDataDecoder.cpp
@@ +98,5 @@
> +      mCallback->Output(output.forget());
> +      continue;
> +    }
> +    else
> +      break;

} else {
  break;
}

@@ +122,5 @@
> +  // flushing.
> +  mTaskQueue->Flush();
> +
> +  status_t err = mDecoder->Flush();
> +  if(err == OK)

nit: Space after `if'.

::: content/media/fmp4/b2g/B2GMediaDataDecoder.h
@@ +31,5 @@
> +  // than NS_ERROR_NOT_AVAILABLE, an error will be reported to the
> +  // MP4Reader.
> +  virtual nsresult Input(mp4_demuxer::MP4Sample* aSample) = 0;
> +  virtual nsresult Output(int64_t aStreamOffset,
> +                         nsAutoPtr<MediaData>& aOutput) = 0;

nit: alignment.

::: content/media/fmp4/b2g/B2GVideoOutputSource.cpp
@@ +95,5 @@
> +
> +nsresult
> +B2GVideoOutputSource::CreateVideoFrame(MPAPI::VideoFrame *aFrame,
> +                                       int64_t aStreamOffset)
> +{

B2GDecoderModule should make MPAPI obsolete on B2G, so we shouldn't be using any of it. This method can be merged into CreateVideoData() and the use of MPAPI::VideoFrame can be removed.

@@ +139,5 @@
> +
> +  return NS_OK;
> +}
> +nsresult
> +B2GVideoOutputSource::CreateVideoData(int64_t aStreamOffset, VideoData **v) {

nit: Asterisks on the type side, rather than the variable side, please.

@@ +143,5 @@
> +B2GVideoOutputSource::CreateVideoData(int64_t aStreamOffset, VideoData **v) {
> +
> +  MPAPI::VideoFrame frame;
> +
> +  *v = nullptr;

Assert that |v| is non-null before using it.

@@ +145,5 @@
> +  MPAPI::VideoFrame frame;
> +
> +  *v = nullptr;
> +  if (CreateVideoFrame(&frame, aStreamOffset) != NS_OK)
> +     return NS_ERROR_UNEXPECTED;

nit: 2 space indents, please, and braces around the return.

@@ +185,5 @@
> +    b.mPlanes[2].mOffset = frame.Cr.mOffset;
> +    b.mPlanes[2].mSkip = frame.Cr.mSkip;
> +    *v = VideoData::Create(mInfo.mVideo,
> +                          mImageContainer,
> +                          aStreamOffset,

nit: alignment.

@@ +205,5 @@
> +                          picture);
> +  }
> +  return NS_OK;
> +}
> +// Blocks until decoded sample is produced by the deoder.

nit: newline between function definitions.

@@ +208,5 @@
> +}
> +// Blocks until decoded sample is produced by the deoder.
> +nsresult
> +B2GVideoOutputSource::Output(int64_t aStreamOffset,
> +                        nsAutoPtr<MediaData>& aOutData)

nit: alignment.

@@ +237,5 @@
> +      if (!SetVideoFormat()) {
> +        return NS_ERROR_UNEXPECTED;
> +      }
> +      else
> +        return Output(aStreamOffset, aOutData);

`else' should be on the same line as the brace. Further, the return statement should be in braces.

::: content/media/fmp4/b2g/B2GVideoOutputSource.h
@@ +35,5 @@
> +
> +  virtual nsresult Input(mp4_demuxer::MP4Sample* aSample) MOZ_OVERRIDE;
> +
> +  virtual nsresult Output(int64_t aStreamOffset,
> +                         nsAutoPtr<MediaData>& aOutput) MOZ_OVERRIDE;

nit: alignment.

@@ +44,5 @@
> +
> +  bool SetVideoFormat();
> +
> +  nsresult CreateVideoFrame(MPAPI::VideoFrame *aFrame,
> +                                int64_t aStreamOffset);

nit: alignment, here and below.

Comment 94

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

(In reply to Sotaro Ikeda [:sotaro PTO July/25 - Aug/3] from comment #90)
> > > - It seems better to avoid to use b2g as class name. B2G say nothing about
> > > the low level platform. For example, there is a b2g desktop.
> > >   If we want to have a such name we use "gonk".
> > >   And the class name seems too generic compare to other classes. It seems
> > > better that the class name says what the class is.
> > Yes. we should have a more clear name. "gonk" probably sounds like too
> > low-level, but this class doesn't do so too low-level thing. 
> 
> I do not understand why gonk is too low-level. The code is gonk dependent
> code. The the code is used for b2g desktop? If they are used also on b2g
> desktop it is ok to use b2g. Otherwise, gonk should be used, other classes
> on gecko uses gonk if the code is gonk dependent.
ok. I am fine with "gonk". Just wonder it there is other better names. For b2g desktop, maybe we can just directly name it b2gDesktop. Anyway, I will change the name to be GonkCodec. I don't have concern on the name.

Comment 95

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

(In reply to Sotaro Ikeda [:sotaro PTO July/25 - Aug/3] from comment #91)
> B2GVideoOutputSource is depend on [1] in Bug 1009420 Comment 14. As I
> explained in Bug 1009420, I don't think it is a good idea to use [1] on new
> MediaCodec implementation.
To me, currently it is a workable solution we can use and the design is simple as mentioned in Bug 1009420 Comment 24 and 16. If possible, maybe [1] could be a near-term solution first since too many bugs are blocked by it and cannot move on :(. Lets have more discussion on bug 1009420.

Comment 96

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

(In reply to Sotaro Ikeda [:sotaro PTO July/25 - Aug/3] from comment #92)
> I checked attachment 8456763 [details] [diff] [review] and attachment
> 8457808 [details] [diff] [review]. I can not find an enough reason to create
> B2GCodec. The class seems just What it did seems duplicate to what
> MediaCodecProxy, B2GVideoOutputSource and B2GAudioOutputSource.
> 
> For me, Implementation becomes more make sense if B2GVideoOutputSource and
> B2GAudioOutputSource create MediaCodecProxy directly.
Yes. They can create MediaCodecProxy directly. But I would like to have a B2GCodec to separate the jobs or simply play an role as abstraction layer. 
As you mentioned in comment 90, B2GCodec is doing many gonk-dependent things. That's one reason I create this file to handle all the things related to codec. And B2GVideoOutputSource/B2GAudioOutputSource (gecko layer) are responsible for doing "post-process" (create Video/Audio data) decoded frames obtained from B2GCodec and feedback to the upper layer (MP4Reader).

Comment 97

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

Comment on attachment 8456763 [details] [diff] [review]
Add-B2G-decoder-module and its Pref .patch

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

::: content/media/fmp4/b2g/B2GAudioOutputSource.cpp
@@ +126,5 @@
> +     return NS_ERROR_UNEXPECTED;
> +
> +  nsAutoArrayPtr<AudioDataValue> buffer(new AudioDataValue[aframe.mSize/2] );
> +  memcpy(buffer.get(), aframe.mData, aframe.mSize);
> +  uint32_t frames = aframe.mSize / (2 * aframe.mAudioChannels);

If aframe.mAudioChannels==0 here, you will crash. You need to be sure you can't crash here somehow.

::: content/media/fmp4/b2g/B2GAudioOutputSource.h
@@ +29,5 @@
> +  virtual nsresult Output(int64_t aStreamOffset,
> +                         nsAutoPtr<MediaData>& aOutput) MOZ_OVERRIDE;
> +private:
> +
> +  bool SetAudioFormat();

This function is unused, delete it.

@@ +49,5 @@
> +  nsTArray<uint8_t> mUserData;
> +
> +  // The offset, in audio frames, at which playback started since the
> +  // last discontinuity.
> +  int64_t mAudioFrameOffset;

You don't use mAudioFrameOffset, delete it.

@@ +52,5 @@
> +  // last discontinuity.
> +  int64_t mAudioFrameOffset;
> +  // The number of audio frames that we've played since the last
> +  // discontinuity.
> +  int64_t mAudioFrameSum;

You don't use mAudioFrameSum, delete it.

@@ +57,5 @@
> +
> +  // True if we need to re-initialize mAudioFrameOffset and mAudioFrameSum
> +  // from the next audio packet we decode. This happens after a seek, since
> +  // WMF doesn't mark a stream as having a discontinuity after a seek(0).
> +  bool mMustRecaptureAudioPosition;

You don't use MustRecaptureAudioPosition either, delete it.

::: content/media/fmp4/b2g/B2GDecoderModule.cpp
@@ +11,5 @@
> +#include "B2GMediaDataDecoder.h"
> +
> +namespace mozilla {
> +
> +bool B2GDecoderModule::sIsB2GEnabled = true;

you should read a pref to determine whether this is enabled.

You can create a cache of the pref's value, so that you can read it off-main thread, like we do here:
http://mxr.mozilla.org/mozilla-central/source/content/media/fmp4/PlatformDecoderModule.cpp#34

::: content/media/fmp4/b2g/B2GMediaDataDecoder.cpp
@@ +90,5 @@
> +void
> +B2GMediaDataDecoder::ProcessOutput()
> +{
> +  nsAutoPtr<MediaData> output;
> +  nsresult rtn;

The Gecko convention is "nsresult rv". Sometimes "nsresult res", but "rv" is what we're supposed to use.

::: content/media/fmp4/b2g/B2GVideoOutputSource.cpp
@@ +127,5 @@
> +     // Set recycle callback for TextureClient
> +     textureClient->SetRecycleCallback(B2GVideoOutputSource::RecycleCallback, this);
> +
> +     aFrame->mGraphicBuffer = textureClient;
> +     aFrame->mRotation = mVideoRotation;

mVideoRotation is always 0. Why do you need it?

@@ +231,5 @@
> +      return NS_OK;
> +    }
> +    case android::INFO_FORMAT_CHANGED:
> +    {
> +      // If the format changed, update our cached info.

If the format of the video changed, you need to call IsValidVideoRegion again to check that the picture is still a supported size.

@@ +241,5 @@
> +        return Output(aStreamOffset, aOutData);
> +    }
> +    case -EAGAIN:
> +    {
> +      //ALOG("may need more input");

Delete commented out line.

@@ +270,5 @@
> +    mVideoBuffer->release();
> +    mVideoBuffer = nullptr;
> +  }
> +}
> +/* static */ void

nit: newline between function definitions.

@@ +278,5 @@
> +  GrallocTextureClientOGL* client = static_cast<GrallocTextureClientOGL*>(aClient);
> +
> +  aClient->ClearRecycleCallback();
> +  outputSource->mDecoder->PostReleaseVideoBuffer(client->GetMediaBuffer(), client->GetReleaseFenceHandle());
> +}

SetVideoFormat() doesn't do anything. Why do you have it?

@@ +279,5 @@
> +
> +  aClient->ClearRecycleCallback();
> +  outputSource->mDecoder->PostReleaseVideoBuffer(client->GetMediaBuffer(), client->GetReleaseFenceHandle());
> +}
> +bool B2GVideoOutputSource::SetVideoFormat() {

nit: newline between function definitions.

@@ +282,5 @@
> +}
> +bool B2GVideoOutputSource::SetVideoFormat() {
> +  return true;
> +}
> +nsresult

nit: newline between function definitions.

@@ +293,5 @@
> +  const uint8_t* data = reinterpret_cast<const uint8_t*>(aSample->data);
> +
> +  uint32_t length = aSample->size;
> +  status_t rtn = mDecoder->Input(data, length, aSample->composition_timestamp, TIMEOUT_WAIT_FOREVER);
> +  if (rtn == OK)

if (rtn == OK) {
  return NS_OK;
} else {
  return NS_ERROR_UNEXPECTED;
}

Or:
 return (rtn == OK) ? NS_OK : NS_ERROR_UNEXPECTED;

Always put {} around one line if statement blocks.

::: content/media/fmp4/b2g/B2GVideoOutputSource.h
@@ +50,5 @@
> +  nsresult CreateVideoData(int64_t aStreamOffset,
> +                              VideoData** aOutData);
> +  void ReleaseVideoBuffer();
> +  // Video frame geometry.
> +  VideoInfo mVideoInfo;

mVideoInfo is unused, delete it.

@@ +62,5 @@
> +  nsIntRect mPicture;
> +  nsIntSize mInitialFrame;
> +
> +  android::sp<B2GCodec> mDecoder;
> +  RefPtr<layers::ImageContainer> mImageContainer;

If possible, use nsRefPtr in new code. We're supposed to stop using mozilla::RefPtr, and use nsRefPtr in future.

@@ +67,5 @@
> +  RefPtr<MediaTaskQueue> mTaskQueue;
> +  MediaDataDecoderCallback* mCallback;
> +
> +  const layers::LayersBackend mLayersBackend;
> +  bool mUseHwAccel;

mUseHwAccel and mLayersBackend are unused? Delete them.

Comment 98

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #96)
> (In reply to Sotaro Ikeda [:sotaro PTO July/25 - Aug/3] from comment #92)
> > I checked attachment 8456763 [details] [diff] [review] and attachment
> > 8457808 [details] [diff] [review]. I can not find an enough reason to create
> > B2GCodec. The class seems just What it did seems duplicate to what
> > MediaCodecProxy, B2GVideoOutputSource and B2GAudioOutputSource.
> > 
> > For me, Implementation becomes more make sense if B2GVideoOutputSource and
> > B2GAudioOutputSource create MediaCodecProxy directly.
> Yes. They can create MediaCodecProxy directly. But I would like to have a
> B2GCodec to separate the jobs or simply play an role as abstraction layer. 
> As you mentioned in comment 90, B2GCodec is doing many gonk-dependent
> things. That's one reason I create this file to handle all the things
> related to codec. And B2GVideoOutputSource/B2GAudioOutputSource (gecko
> layer) are responsible for doing "post-process" (create Video/Audio data)
> decoded frames obtained from B2GCodec and feedback to the upper layer
> (MP4Reader).

Anyway, B2GVideoOutputSource/B2GAudioOutputSource do gonk dependent task. Therefore, there name also should not have b2g, instead it should have gonk. It it what other gecko's code is doing. Consistency to other code is more important.

B2GCodec is very ugly from architecture point of view. It's implementation should be part of B2GVideoOutputSource/B2GAudioOutputSource.

Comment 99

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

> > > For me, Implementation becomes more make sense if B2GVideoOutputSource and
> > > B2GAudioOutputSource create MediaCodecProxy directly.
> > Yes. They can create MediaCodecProxy directly. But I would like to have a
> > B2GCodec to separate the jobs or simply play an role as abstraction layer. 
> > As you mentioned in comment 90, B2GCodec is doing many gonk-dependent
> > things. That's one reason I create this file to handle all the things
> > related to codec. And B2GVideoOutputSource/B2GAudioOutputSource (gecko
> > layer) are responsible for doing "post-process" (create Video/Audio data)
> > decoded frames obtained from B2GCodec and feedback to the upper layer
> > (MP4Reader).
> 
> Anyway, B2GVideoOutputSource/B2GAudioOutputSource do gonk dependent task.
> Therefore, there name also should not have b2g, instead it should have gonk.
> It it what other gecko's code is doing. Consistency to other code is more
> important.
> 
> B2GCodec is very ugly from architecture point of view. It's implementation
> should be part of B2GVideoOutputSource/B2GAudioOutputSource.

I do not think B2G codec have enough reason and volume to be a independent class. Instead, it just seems to add unnecessary redundancy.

Comment 100

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

Hi Sotaro, 
Thanks for your feedback. 
I don't think that B2GCodec is redundant. For me, it makes codes more simple and easily readable as comment 96. It is just like usually we will not put so many stuffs into one function, instead we create some new functions and move some stuffs there and call them in the main function. That would be easier to understand the main function. That is my purpose to have this class. Would you mind letting me know more specific details about what you meant "very ugly from architecture point of view"?

Hi Edwin, 
May I have your opinion for the naming and B2GCodec?

Comment 101

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

Comment on attachment 8457808 [details] [diff] [review]
0003-Add-B2GCodec.patch

Hi Edwin, 
From PDM architecture perspective, may I have your some feedback about this patch as well? Do you prefer to merge this to B2GVideoOutputSource/B2GAudioOutputSource? 
Thanks!

Comment 102

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #100)
> Hi Sotaro, 
> Thanks for your feedback. 
> I don't think that B2GCodec is redundant. For me, it makes codes more simple
> and easily readable as comment 96. It is just like usually we will not put
> so many stuffs into one function, instead we create some new functions and
> move some stuffs there and call them in the main function. That would be
> easier to understand the main function. That is my purpose to have this
> class. Would you mind letting me know more specific details about what you
> meant "very ugly from architecture point of view"?

If you want just to add readability, it seems better to create a super class of B2GVideoOutputSource and B2GAudioOutputSource. B2GCodec does not provide enough wrapping to a capability. It does not provide a clear role. Some capabilities are divided between B2GCodec and B2GVideoOutputSource. One example of it is usage of MediaBuffer. There is no necessity to use MediaBuffer here. If a capability is wrapped correctly, we should not have a necessity to use MediaBuffer. It show one capability is divided to two classes incorrectly.

And if we choose [3] in Bug 1009420 Comment 14, we do not need to get android::GraphicBuffer from MediaCodec. We do not even need to touch GraphicBuffer in these classes. In this case, we got just TexutreClient from GonkNativeWindow. From this, abstraction by B2GCodec is fragile. B2GCodec is not resistant to choose [1] or [3] in Bug 1009420 Comment 14. It also says that abstraction is not correct.

Comment 103

[Sotaro Ikeda [:sotaro PTO July/5 - July/11]]

And it seems better to clearly recognize that B2GCodec, B2GVideoOutputSource and B2GAudioOutputSource are handling gonk dependent code. They can not be used for all b2g hardware platform. They can be used only for gonk. Therefore, Include "b2g" in class name and folder name is not correct.

Comment 104

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8457808 [details] [diff] [review]
0003-Add-B2GCodec.patch

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

IIUC, this is a wrapper class around MediaCodecProxy, which is a wrapper class around MediaCodec. Seems rather redundant.

I think most of the code here can be split between (not necessarily all of):
 - MediaCodecProxy;
 - B2GMediaDataDecoder; or
 - a helper class used by B2G*OutputSource.

And I agree with Sotaro here on renaming B2G* to Gonk*.

Comment 105

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

(In reply to Sotaro Ikeda [:sotaro PTO July/25 - Aug/3] from comment #102)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #100)
> If you want just to add readability, it seems better to create a super class
> of B2GVideoOutputSource and B2GAudioOutputSource. B2GCodec does not provide
> enough wrapping to a capability. It does not provide a clear role. Some
> capabilities are divided between B2GCodec and B2GVideoOutputSource. One
> example of it is usage of MediaBuffer. There is no necessity to use
> MediaBuffer here. If a capability is wrapped correctly, we should not have a
> necessity to use MediaBuffer. It show one capability is divided to two
> classes incorrectly.
> 
> And if we choose [3] in Bug 1009420 Comment 14, we do not need to get
> android::GraphicBuffer from MediaCodec. We do not even need to touch
> GraphicBuffer in these classes. In this case, we got just TexutreClient from
> GonkNativeWindow. From this, abstraction by B2GCodec is fragile. B2GCodec is
> not resistant to choose [1] or [3] in Bug 1009420 Comment 14. It also says
> that abstraction is not correct.
May bad. I should not have used the word "abstraction" in comment 96 which makes my explanation confusing. I should say B2GCodec is a wrapper or helper class which handles all the communications/actions (dequeueInputBuffer, dequeueOutputBuffer, queueInputBuffer, getInputBuffers, and etc) with Codec and provides more easy interfaces (configure, Input, Output, and etc) to the upper layer (those interfaces are similar to other platform decoder modules). That is my purpose to add readability.

Comment 106

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #104)
> I think most of the code here can be split between (not necessarily all of):
>  - MediaCodecProxy;
>  - B2GMediaDataDecoder; or

Oops, I had meant to say B2GOutputSource here.

Comment 107

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

Update the current status. 
Since so far there is no clear/locked-down solution to GraphicBuffer problem, I have created the a follow-up bug, bug 1043274,  to fix it once having the solution. In addition, making this bug not block MSE integration. 
The following is what I am going to do. 
1. Replace the name "b2g" with "gonk". 
2. Remove B2GCodec 
Without using GraphicBuffer, Gonk*OutputSource will communicate with MediaCodecProxy directly as current MediaCodecReader does.

Comment 108

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

Comment on attachment 8456760 [details] [diff] [review]
Changes-in-MP4Reader.patch

Remove review first for the coming file name change.

Comment 109

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

Attachment: 0002-Changes-in-MediaCodecProxy.patch

Add more friendly-used interfaces to MediaCodecProxy for Gonk*DecoderManager's use which was originally created in attachment 8457808 [details] [diff] [review].

Comment 110

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

Attachment: 0003-Changes-for-Adding-Gonk-Decode-Module.patch

Main changes for 
1. Add gonk decoder module.
2. Add new interfaces for codec resource management.

Comment 111

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

Comment on attachment 8456760 [details] [diff] [review]
Changes-in-MP4Reader.patch

Replace this with attachment 8463916 [details] [diff] [review].

Comment 112

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

Attachment: 0005-Add-Gonk-Decoder-Module.patch

Add Gonk Decoder Module.

Comment 113

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

Comment on attachment 8420022 [details] [diff] [review]
MediaCodec friends with ACodec to expose graphicBuffer

This patch is discussed in bug 1009410. 
Set it obsolete.

Comment 114

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

(In reply to Blake Wu [:bwu][:blakewu] from comment #109)
> Created attachment 8463915 [details] [diff] [review]
> 0002-Changes-in-MediaCodecProxy.patch
> 
> Add more friendly-used interfaces to MediaCodecProxy for
> Gonk*DecoderManager's use which was originally created in attachment 8457808 [details] [diff] [review]
> [details] [diff] [review].
Hi Edwin, 
Since sotaro is during PTO, could you help review this patch?

Comment 115

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

(In reply to Blake Wu [:bwu][:blakewu] from comment #112)
> Created attachment 8463917 [details] [diff] [review]
> 0005-Add-Gonk-Decoder-Module.patch
> 
> Add Gonk Decoder Module.

More information for this patch.
This patch doesn't use GraphicBuffer as comment 107. For using GraphicBuffer, that would be added in bug 1043274.

Comment 116

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8463915 [details] [diff] [review]
0002-Changes-in-MediaCodecProxy.patch

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

Looks great!

r+ with a little bit of style stuff.

::: content/media/omx/MediaCodecProxy.cpp
@@ +4,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +//#define LOG_NDEBUG 0

This isn't used; remove it.

@@ +395,4 @@
>    }
>  }
>  
> +bool MediaCodecProxy::prepare() {

Opening brace goes on the next line, here and in all the methods below.

@@ +414,5 @@
> +  return true;
> +}
> +
> +status_t MediaCodecProxy::input(const uint8_t* aData, uint32_t aDataSize,
> +                            int64_t aTimestampUsecs, uint64_t flags) {

|int64_t| should be aligned with |const| on the previous line. |flags| should be |aFlags|.

@@ +453,5 @@
> +  size_t size = 0;
> +  int64_t timeUs = 0;
> +  uint32_t flags = 0;
> +
> +  *aBuffer = NULL;

nullptr.

@@ +456,5 @@
> +
> +  *aBuffer = NULL;
> +
> +  status_t err = dequeueOutputBuffer(&index, &offset, &size,
> +                                      &timeUs, &flags, aTimeoutUs);

alignment.

@@ +478,5 @@
> +  return mCodec == nullptr;
> +}
> +
> +bool MediaCodecProxy::isDormantNeeded() {
> +  return mCodecLooper.get()? true:false;

Spaces either side of the "?" and ":" -- "get() ? true : false;"

::: content/media/omx/MediaCodecProxy.h
@@ +20,5 @@
> +//management. Basically user can use it like MediaCodec, but need to handle
> +//the listener when Codec is reserved for Async case. A good example is
> +//MediaCodecReader.cpp. Another useage is to use configure(), prepare(),
> +//input(), and output(). It is used in GonkVideoDecoderManager.cpp which
> +//doesn't need to handle the buffers for codec.

There should be a space between "//" and the comment text:
"// comment", not "//comment"

@@ +119,5 @@
> +
> +  bool prepare();
> +  bool isWaitingResources();
> +  bool isDormantNeeded();
> +  void releaseMediaResources();

Additional methods that aren't part of the MediaCodec interface should start with an upper-case letter.

Comment 117

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

Edwin, 
Thanks for your review! One thing is not sure to me. 
(In reply to Edwin Flores [:eflores] [:edwin] from comment #116)
> Comment on attachment 8463915 [details] [diff] [review]
> 0002-Changes-in-MediaCodecProxy.patch
> 
> Review of attachment 8463915 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great!
> 
> r+ with a little bit of style stuff.
> 
> @@ +119,5 @@
> > +
> > +  bool prepare();
> > +  bool isWaitingResources();
> > +  bool isDormantNeeded();
> > +  void releaseMediaResources();
> 
> Additional methods that aren't part of the MediaCodec interface should start
> with an upper-case letter.
OK. I thought for static methods, we should start with an upper-case letter!? or both with those that aren't part of the MediaCodec interfaces?

Comment 118

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #117)
> OK. I thought for static methods, we should start with an upper-case
> letter!? or both with those that aren't part of the MediaCodec interfaces?

Android style is lower-case for member methods and upper-case for static methods, whereas Gecko style is upper-case everywhere.

Making the methods upper-case makes it clear to callers that these methods are Gecko additions.

Comment 119

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

Understood. Thanks!

Comment 120

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

Attachment: 0002-Changes-in-MediaCodecProxy-Final.patch

Add changes according to comment 116 and carry r+ from edwin.

Comment 121

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

Attachment: 0003-Changes-for-Adding-Gonk-Decode-Module-V2.patch

Update a new one for new interface names change in MediaCodecProxy in attachment 8464444 [details] [diff] [review].

Comment 122

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

Attachment: 0005-Add-Gonk-Decoder-Module-V2.patch

Update a new one for the interface names changed in MediaCodecProxy in attachment 8464444 [details] [diff] [review] [diff] [review].

Comment 123

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

Comment on attachment 8464446 [details] [diff] [review]
0003-Changes-for-Adding-Gonk-Decode-Module-V2.patch

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

Almost an r+.  :)

::: content/media/fmp4/MP4Decoder.cpp
@@ +85,5 @@
>  
>  static bool
> +IsGonkMP4DecoderAvailable()
> +{
> +  if (!Preferences::GetBool("media.fragmented-mp4.gonk.enabled", false)) {

return Preferences::GetBool("media.fragmented-mp4.gonk.enabled", false);

You don't need to "if (false) return false; else return true;" here

@@ +101,4 @@
>           IsVistaOrLater() ||
>  #endif
>           IsFFmpegAvailable() ||
> +	       IsGonkMP4DecoderAvailable() ||

Indentation is wrong here; you have a tab character instead of spaces here.

::: content/media/fmp4/MP4Reader.cpp
@@ +173,5 @@
>  MP4Reader::Init(MediaDecoderReader* aCloneDonor)
>  {
>    MOZ_ASSERT(NS_IsMainThread(), "Must be on main thread.");
> +
> +  if (mState == INIT_STATE){

Init() should not be called more than once, you should not need this change. Please remove it.

@@ +199,4 @@
>  MP4Reader::ReadMetadata(MediaInfo* aInfo,
>                          MetadataTags** aTags)
>  {
> +  if (mState == READ_METADATA_STATE) {

I added MP4Reader::mDemuxerInitialized yesterday. You can use that here instead of creating a MP4Reader::mState data field.

@@ +599,4 @@
>    return NS_OK;
>  }
>  
> +bool MP4Reader::IsWaitingMediaResources()

I landed a MP4Reader::IsWaitingMediaResources() yesterday. You will need to rebase. Sorry!

@@ +631,5 @@
> +  if (container) {
> +    container->ClearCurrentFrame();
> +  }
> +  if (mVideo.mDecoder) {
> +    mVideo.mDecoder->ReleaseMediaResources();

I took a while to review your patch because I was trying to think of a better way to do this. I cannot.

::: content/media/fmp4/MP4Reader.h
@@ +55,4 @@
>    virtual nsresult GetBuffered(dom::TimeRanges* aBuffered,
>                                 int64_t aStartTime) MOZ_OVERRIDE;
>  
> +  /*For Codec Resource Management*/

// For Codec Resource Management.

(Match prevailing style when editing existing code).

@@ +55,5 @@
>    virtual nsresult GetBuffered(dom::TimeRanges* aBuffered,
>                                 int64_t aStartTime) MOZ_OVERRIDE;
>  
> +  /*For Codec Resource Management*/
> +  virtual bool IsWaitingMediaResources();

These functions should be MOZ_OVERRIDE.

@@ +105,4 @@
>      virtual void DrainComplete() MOZ_OVERRIDE {
>        mReader->DrainComplete(mType);
>      }
> +    virtual void NotifyResourcesStatusChanged()MOZ_OVERRIDE {

virtual void NotifyResourcesStatusChanged() MOZ_OVERRIDE

(You should have a space between "()" and "MOZ_OVERRIDE".

@@ +169,4 @@
>  
>    layers::LayersBackend mLayersBackendType;
>  
> +  enum State{

Delete "enum State" and mState. Use MP4Reader::mDemuxerInitialized instead.

You never used the SEEK_STATE, but it was still here.

::: content/media/fmp4/PlatformDecoderModule.cpp
@@ +39,4 @@
>                                 "media.fragmented-mp4.use-blank-decoder");
>    Preferences::AddBoolVarCache(&sFFmpegDecoderEnabled,
>                                 "media.fragmented-mp4.ffmpeg.enabled", false);
> +  Preferences::AddBoolVarCache(&sGonkDecoderEnabled,

#ifdef MOZ_GONK_MEDIACODEC
  Preferences::AddBoolVarCache(&sGonkDecoderEnabled,
                               "media.fragmented-mp4.gonk.enabled", false);
#endif

::: content/media/fmp4/PlatformDecoderModule.h
@@ +132,5 @@
>    virtual void InputExhausted() = 0;
>  
>    virtual void DrainComplete() = 0;
> +
> +  virtual void NotifyResourcesStatusChanged() = 0;

virtual void NotifyResourcesStatusChanged() {}

virtual void ReleaseMediaResources() {}


Otherwise you'll get compile errors in all other PDMs, right?

@@ +201,5 @@
>    // returned.
>    virtual nsresult Shutdown() = 0;
>  
> + /*For Codec Resource Management*/
> +  virtual bool IsWaitingMediaResources() {

// For Codec Resource Management.

(Match the prevailing style in the file in which you're editing)

::: modules/libpref/src/init/all.js
@@ +243,4 @@
>  #ifdef MOZ_FMP4
>  pref("media.fragmented-mp4.enabled", true);
>  pref("media.fragmented-mp4.ffmpeg.enabled", false);
> +pref("media.fragmented-mp4.gonk.enabled", false);

Put this in b2g/app/b2g.js. It doesn't make sense on any other platform.

Comment 124

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

Comment on attachment 8464446 [details] [diff] [review]
0003-Changes-for-Adding-Gonk-Decode-Module-V2.patch

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

Almost an r+.  :)

::: content/media/fmp4/MP4Decoder.cpp
@@ +85,5 @@
>  
>  static bool
> +IsGonkMP4DecoderAvailable()
> +{
> +  if (!Preferences::GetBool("media.fragmented-mp4.gonk.enabled", false)) {

return Preferences::GetBool("media.fragmented-mp4.gonk.enabled", false);

You don't need to "if (false) return false; else return true;" here

@@ +101,4 @@
>           IsVistaOrLater() ||
>  #endif
>           IsFFmpegAvailable() ||
> +	       IsGonkMP4DecoderAvailable() ||

Indentation is wrong here; you have a tab character instead of spaces here.

::: content/media/fmp4/MP4Reader.cpp
@@ +173,5 @@
>  MP4Reader::Init(MediaDecoderReader* aCloneDonor)
>  {
>    MOZ_ASSERT(NS_IsMainThread(), "Must be on main thread.");
> +
> +  if (mState == INIT_STATE){

Init() should not be called more than once, you should not need this change. Please remove it.

@@ +199,4 @@
>  MP4Reader::ReadMetadata(MediaInfo* aInfo,
>                          MetadataTags** aTags)
>  {
> +  if (mState == READ_METADATA_STATE) {

I added MP4Reader::mDemuxerInitialized yesterday. You can use that here instead of creating a MP4Reader::mState data field.

@@ +599,4 @@
>    return NS_OK;
>  }
>  
> +bool MP4Reader::IsWaitingMediaResources()

I landed a MP4Reader::IsWaitingMediaResources() yesterday. You will need to rebase. Sorry!

@@ +631,5 @@
> +  if (container) {
> +    container->ClearCurrentFrame();
> +  }
> +  if (mVideo.mDecoder) {
> +    mVideo.mDecoder->ReleaseMediaResources();

I took a while to review your patch because I was trying to think of a better way to do this. I cannot.

::: content/media/fmp4/MP4Reader.h
@@ +55,4 @@
>    virtual nsresult GetBuffered(dom::TimeRanges* aBuffered,
>                                 int64_t aStartTime) MOZ_OVERRIDE;
>  
> +  /*For Codec Resource Management*/

// For Codec Resource Management.

(Match prevailing style when editing existing code).

@@ +55,5 @@
>    virtual nsresult GetBuffered(dom::TimeRanges* aBuffered,
>                                 int64_t aStartTime) MOZ_OVERRIDE;
>  
> +  /*For Codec Resource Management*/
> +  virtual bool IsWaitingMediaResources();

These functions should be MOZ_OVERRIDE.

@@ +105,4 @@
>      virtual void DrainComplete() MOZ_OVERRIDE {
>        mReader->DrainComplete(mType);
>      }
> +    virtual void NotifyResourcesStatusChanged()MOZ_OVERRIDE {

virtual void NotifyResourcesStatusChanged() MOZ_OVERRIDE

(You should have a space between "()" and "MOZ_OVERRIDE".

@@ +169,4 @@
>  
>    layers::LayersBackend mLayersBackendType;
>  
> +  enum State{

Delete "enum State" and mState. Use MP4Reader::mDemuxerInitialized instead.

You never used the SEEK_STATE, but it was still here.

::: content/media/fmp4/PlatformDecoderModule.cpp
@@ +39,4 @@
>                                 "media.fragmented-mp4.use-blank-decoder");
>    Preferences::AddBoolVarCache(&sFFmpegDecoderEnabled,
>                                 "media.fragmented-mp4.ffmpeg.enabled", false);
> +  Preferences::AddBoolVarCache(&sGonkDecoderEnabled,

#ifdef MOZ_GONK_MEDIACODEC
  Preferences::AddBoolVarCache(&sGonkDecoderEnabled,
                               "media.fragmented-mp4.gonk.enabled", false);
#endif

::: content/media/fmp4/PlatformDecoderModule.h
@@ +132,5 @@
>    virtual void InputExhausted() = 0;
>  
>    virtual void DrainComplete() = 0;
> +
> +  virtual void NotifyResourcesStatusChanged() = 0;

virtual void NotifyResourcesStatusChanged() {}

virtual void ReleaseMediaResources() {}


Otherwise you'll get compile errors in all other PDMs, right?

@@ +201,5 @@
>    // returned.
>    virtual nsresult Shutdown() = 0;
>  
> + /*For Codec Resource Management*/
> +  virtual bool IsWaitingMediaResources() {

// For Codec Resource Management.

(Match the prevailing style in the file in which you're editing)

::: content/media/fmp4/moz.build
@@ +40,4 @@
>        'ffmpeg',
>    ]
>  
> +if CONFIG['ANDROID_VERSION'] >= '18':

You should also check if MOZ_GONK_MEDIACODEC is defined here too, so that we don't try to build this code in Firefox for Android builds.

::: modules/libpref/src/init/all.js
@@ +243,4 @@
>  #ifdef MOZ_FMP4
>  pref("media.fragmented-mp4.enabled", true);
>  pref("media.fragmented-mp4.ffmpeg.enabled", false);
> +pref("media.fragmented-mp4.gonk.enabled", false);

Put this in b2g/app/b2g.js. It doesn't make sense on any other platform.

Comment 125

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

Weird. I add a comment on content/media/fmp4/moz.build, and it reposted all my other comments again. I did not mean to repost all my other comments again.

Comment 126

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

Comment on attachment 8464447 [details] [diff] [review]
0005-Add-Gonk-Decoder-Module-V2.patch

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

It looks ok, but lots of nits to fix.

::: content/media/fmp4/gonk/GonkAudioDecoderManager.cpp
@@ +58,5 @@
> +  , mAudioBuffer(nullptr)
> +{
> +  MOZ_COUNT_CTOR(GonkAudioDecoderManager);
> +  MOZ_ASSERT(mAudioChannels);
> +  AACAudioSpecificConfigToUserData(&aConfig.audio_specific_config[0],

mUserData.AppendElements(&aConfig.audio_specific_config[0],
                         aConfig.audio_specific_config.length());

It does not make sense to have a AACAudioSpecificConfigToUserData function here, as it does no conversion.

@@ +80,5 @@
> +  mLooper->setName("GonkAudioDecoderManager");
> +  mLooper->start();
> +
> +  mDecoder = MediaCodecProxy::CreateByType(mLooper, "audio/mp4a-latm", false, false, nullptr);
> +  if (!mDecoder.get()){

if (!mDecoder.get()) {

Space between ) and {

@@ +95,5 @@
> +  status_t err = mDecoder->configure(format, nullptr, nullptr, 0);
> +  if (err != OK || !mDecoder->Prepare()) {
> +    return nullptr;
> +  }
> +  status_t rv = mDecoder->Input(mUserData.Elements(), mUserData.Length(), 0,

status_t rv = mDecoder->Input(mUserData.Elements(), mUserData.Length(), 0,
                              android::MediaCodec::BUFFER_FLAG_CODECCONFIG);

Line up following line with '('.

@@ +100,5 @@
> +                android::MediaCodec::BUFFER_FLAG_CODECCONFIG);
> +
> +  if (rv == OK) {
> +    return mDecoder;
> +  }

} else {

@@ +108,5 @@
> +  }
> +}
> +
> +nsresult
> +GonkAudioDecoderManager::CreateAudioData(int64_t aStreamOffset, AudioData **v) {

nsresult
GonkAudioDecoderManager::CreateAudioData(int64_t aStreamOffset, AudioData **v)
{

@@ +121,5 @@
> +  }
> +  data = mAudioBuffer->data();
> +  dataOffset = mAudioBuffer->range_offset();
> +  size = mAudioBuffer->range_length();
> +

Can you check that memcpy won't write outside of buffer+size and return NS_ERROR_FAILURE if it will.

@@ +131,5 @@
> +  if (!duration.isValid()) {
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +  *v = new AudioData(aStreamOffset, timeUs, duration.value(), frames, buffer.forget(),
> +		                 mAudioChannels);

line up next line with  '(' on preceeding line.

@@ +138,5 @@
> +}
> +
> +nsresult
> +GonkAudioDecoderManager::Output(int64_t aStreamOffset,
> +                        nsAutoPtr<MediaData>& aOutData)

line up start of line with preceeding '('

@@ +150,5 @@
> +  switch (err) {
> +    case OK:
> +    {
> +      if (mAudioBuffer && mAudioBuffer->range_length() != 0) {
> +      int64_t timeUs;

Indent the code inside this "if" statement.

@@ +151,5 @@
> +    case OK:
> +    {
> +      if (mAudioBuffer && mAudioBuffer->range_length() != 0) {
> +      int64_t timeUs;
> +      if (!mAudioBuffer->meta_data()->findInt64(kKeyTime, &timeUs))

{} around if statment bodies.

This code reads differently when you have the {} there doesn't it?

@@ +155,5 @@
> +      if (!mAudioBuffer->meta_data()->findInt64(kKeyTime, &timeUs))
> +        return NS_ERROR_UNEXPECTED;
> +      }
> +      AudioData* data = nullptr;
> +      nsresult rv = CreateAudioData(aStreamOffset, &data);

Check rv here...

@@ +207,5 @@
> +GonkAudioDecoderManager::Input(mp4_demuxer::MP4Sample* aSample)
> +{
> +  const uint8_t* data = reinterpret_cast<const uint8_t*>(aSample->data);
> +  uint32_t length = aSample->size;
> +  status_t rv = mDecoder->Input(data, length, aSample->composition_timestamp, 0);

You shouldn't need to cast aSample->data to const here?

@@ +208,5 @@
> +{
> +  const uint8_t* data = reinterpret_cast<const uint8_t*>(aSample->data);
> +  uint32_t length = aSample->size;
> +  status_t rv = mDecoder->Input(data, length, aSample->composition_timestamp, 0);
> +  return rv == OK? NS_OK:NS_ERROR_UNEXPECTED;

You *must* delete aSample once you're finished using it, otherwise you'll leak memory.

Also:

return rv == OK ? NS_OK : NS_ERROR_UNEXPECTED;

(Spaces around operators (like ? and :)

::: content/media/fmp4/gonk/GonkAudioDecoderManager.h
@@ +25,5 @@
> +
> +  virtual nsresult Input(mp4_demuxer::MP4Sample* aSample) MOZ_OVERRIDE;
> +
> +  virtual nsresult Output(int64_t aStreamOffset,
> +                         nsAutoPtr<MediaData>& aOutput) MOZ_OVERRIDE;

Indentation.

::: content/media/fmp4/gonk/GonkDecoderModule.cpp
@@ +10,5 @@
> +#include "mozilla/DebugOnly.h"
> +#include "GonkMediaDataDecoder.h"
> +
> +namespace mozilla {
> +#ifdef MOZ_GONK_MEDIACODEC

delete GonkDecoderModule::sIsGonkEnabled.

Also, with the change to the content/media/fmp4/moz.build, this code should only be compiled when MOZ_GONK_MEDIACODEC is defined. So you don't need the "#ifdef MOZ_GONK_MEDIACODEC" here.

@@ +31,5 @@
> +  MOZ_ASSERT(NS_IsMainThread(), "Must be on main thread.");
> +}
> +
> +nsresult
> +GonkDecoderModule::Startup()

Your Startup() function does nothing. Delete it.

@@ +45,5 @@
> +GonkDecoderModule::Shutdown()
> +{
> +  MOZ_ASSERT(NS_IsMainThread(), "Must be on main thread.");
> +
> +  NS_ASSERTION(SUCCEEDED(hr), "MFShutdown failed");

You never compiled this in a debug build did you? This code won't compile. "hr" is not defined, and "MFShutdown" is a Windows function. Delete this line.

::: content/media/fmp4/gonk/GonkDecoderModule.h
@@ +39,5 @@
> +    MediaDataDecoderCallback* aCallback) MOZ_OVERRIDE;
> +
> +  static void Init();
> +private:
> +  static bool sIsGonkEnabled;

You already have PlatformDecoderModule::sGonkDecoderEnabled and is protected, so visible to GonkDecoderModule. You don't need both PlatformDecoderModule::sGonkDecoderEnabled and GonkDecoderModule::sIsGonkEnabled. Delete GonkDecoderModule::sIsGonkEnabled.

::: content/media/fmp4/gonk/GonkMediaDataDecoder.cpp
@@ +24,5 @@
> +
> +namespace mozilla {
> +
> +GonkMediaDataDecoder::GonkMediaDataDecoder(GonkDecoderManager* aManager,
> +                                         MediaTaskQueue* aTaskQueue,

These lines should line up with the '('

@@ +42,5 @@
> +nsresult
> +GonkMediaDataDecoder::Init()
> +{
> +  mDecoder = mManager->Init(mCallback);
> +  if (mDecoder.get())

{} around all single line "if" statements.

@@ +71,5 @@
> +
> +void
> +GonkMediaDataDecoder::ProcessDecode(mp4_demuxer::MP4Sample* aSample)
> +{
> +  const uint8_t* data = reinterpret_cast<const uint8_t*>(aSample->data);

data and length are unused. Delete them.

@@ +101,5 @@
> +    else
> +      break;
> +  }
> +
> +  if (rv == NS_ERROR_NOT_AVAILABLE){

if (rv == NS_ERROR_NOT_AVAILABLE) {

Space between ) and {

@@ +132,5 @@
> +void
> +GonkMediaDataDecoder::ProcessDrain()
> +{
> +  // Then extract all available output.
> +  ProcessOutput();

You probably need to do something to cause all samples in the decode pipeline to output.

Can you try playing http://people.mozilla.org/~cpearce/avatar.mp4 and at the end if you reach a black frame drain is working correctly. If you still see "December 18th" text, then you may not need to do anything else here.

::: content/media/fmp4/gonk/GonkMediaDataDecoder.h
@@ +13,5 @@
> +
> +namespace mozilla {
> +
> +// Encapsulates the initialization of the GonkDecoder appropriate for decoding
> +// a given stream, and the process of converting the IMFSample produced

I don't think a GonkDecoder produces IMFSamples. Fix this comment.

@@ +19,5 @@
> +class GonkDecoderManager {
> +public:
> +  virtual ~GonkDecoderManager() {}
> +
> +  // Creates an initializs the GonkDecoder.

"Creates and initializes"

@@ +23,5 @@
> +  // Creates an initializs the GonkDecoder.
> +  // Returns nullptr on failure.
> +  virtual android::sp<android::MediaCodecProxy> Init(MediaDataDecoderCallback* aCallback) = 0;
> +
> +  // Produces decoded output, if possible. Blocks until output can be produced,

The comment you copied from WMFMediaDataDecoder.h is wrong. Your MediaDataDecoder::Input() should not block. *Your* implementation in fact does not block. So this comment is wrong. Remove or fix this comment.

::: content/media/fmp4/gonk/GonkVideoDecoderManager.cpp
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#include <OMX_IVCommon.h>
> +#include <gui/Surface.h>
> +#include <ICrypto.h>

Why do you need ICrypto.h?

@@ +42,5 @@
> +  kNotifyCodecCanceled = 'coca',
> +};
> +
> +GonkVideoDecoderManager::GonkVideoDecoderManager(
> +                      mozilla::layers::ImageContainer* aImageContainer,

Inconsistent indentation.

@@ +65,5 @@
> +  // Validate the container-reported frame and pictureRect sizes. This ensures
> +  // that our video frame creation code doesn't overflow.
> +  nsIntSize frameSize(mVideoWidth, mVideoHeight);
> +  if (!IsValidVideoRegion(frameSize, pictureRect, displaySize)) {
> +    ALOG("It is not a valid region");

You should be checking whether the video region is valid inside Init(), and returning nullptr if it is not.

@@ +94,5 @@
> +  // Register AMessage handler to ALooper.
> +  mLooper->registerHandler(mHandler);
> +  // Start ALooper thread.
> +  if (mLooper->start() != OK) {
> +    return false;

return nullptr;

@@ +128,5 @@
> +    picture.width = (mFrameInfo.mWidth * mPicture.width) / mInitialFrame.width;
> +    picture.height = (mFrameInfo.mHeight * mPicture.height) / mInitialFrame.height;
> +  }
> +
> +  if (mVideoBuffer != nullptr && mVideoBuffer->size() > 0 && mVideoBuffer->data() != nullptr) {

if (!mVideoBuffer || mVideoBuffer->size() <= 0 || !mVideoBuffer->data()) {
  return NS_ERROR_UNEXPECTED;
}

Then you don't need to indent the code after this.

@@ +243,5 @@
> +}
> +// Blocks until decoded sample is produced by the deoder.
> +nsresult
> +GonkVideoDecoderManager::Output(int64_t aStreamOffset,
> +                        nsAutoPtr<MediaData>& aOutData)

Indentation.

@@ +327,5 @@
> +  if (mDecoder == nullptr) {
> +    ALOG("Decoder is not inited");
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +  status_t rv = mDecoder->Input(data, length, aSample->composition_timestamp, 0);

You *must* delete aSample when you're finished with it!

@@ +328,5 @@
> +    ALOG("Decoder is not inited");
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +  status_t rv = mDecoder->Input(data, length, aSample->composition_timestamp, 0);
> +  if (rv == OK)

either {} around if statement bodies, or

return (rv == OK) : NS_OK : NS_ERROR_FAILURE;

@@ +386,5 @@
> +      mReaderCallback->ReleaseMediaResources();
> +      break;
> +    }
> +
> +    // TODO

// TODO what? Delete this if there's no TODO.

::: content/media/fmp4/gonk/GonkVideoDecoderManager.h
@@ +20,5 @@
> +typedef android::MediaCodecProxy MediaCodecProxy;
> +
> +public:
> +  GonkVideoDecoderManager(mozilla::layers::ImageContainer* aImageContainer,
> +		       const mp4_demuxer::VideoDecoderConfig& aConfig);

Use only spaces, no tabs, in indentation.

@@ +29,5 @@
> +
> +  virtual nsresult Input(mp4_demuxer::MP4Sample* aSample) MOZ_OVERRIDE;
> +
> +  virtual nsresult Output(int64_t aStreamOffset,
> +                         nsAutoPtr<MediaData>& aOutput) MOZ_OVERRIDE;

Indentation.

@@ +68,5 @@
> +  public:
> +    VideoResourceListener(GonkVideoDecoderManager *aManager);
> +    ~VideoResourceListener();
> +
> +    virtual void codecReserved();

MOZ_OVERRIDE?

@@ +84,5 @@
> +
> +  bool SetVideoFormat();
> +
> +  nsresult CreateVideoData(int64_t aStreamOffset,
> +                              VideoData** aOutData);

Indentation.

@@ +88,5 @@
> +                              VideoData** aOutData);
> +  void ReleaseVideoBuffer();
> +  uint8_t* GetColorConverterBuffer(int32_t aWidth, int32_t aHeight);
> +
> +  //For codec resource management

// For

::: content/media/fmp4/gonk/moz.build
@@ +16,5 @@
> +    'GonkMediaDataDecoder.cpp',
> +    'GonkVideoDecoderManager.cpp',
> +]
> +LOCAL_INCLUDES += [
> +      '/content/media/omx/',

Indentation of '/content/media/omx/' doesn't match other exports.

Comment 127

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

(In reply to Chris Pearce (:cpearce) from comment #126)
> @@ +327,5 @@
> > +  if (mDecoder == nullptr) {
> > +    ALOG("Decoder is not inited");
> > +    return NS_ERROR_UNEXPECTED;
> > +  }
> > +  status_t rv = mDecoder->Input(data, length, aSample->composition_timestamp, 0);
> 
> You *must* delete aSample when you're finished with it!

Oops, I'm wrong, in GonkMediaDataDecoder::Input() you put it in an nsAutoPtr<>. My mistake.

Comment 128

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

Thanks for your review! 
(In reply to Chris Pearce (:cpearce) from comment #123)
> Comment on attachment 8464446 [details] [diff] [review]
> 0003-Changes-for-Adding-Gonk-Decode-Module-V2.patch
> 
> Review of attachment 8464446 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Almost an r+.  :)
> 
> ::: content/media/fmp4/MP4Reader.cpp
> @@ +173,5 @@
> >  MP4Reader::Init(MediaDecoderReader* aCloneDonor)
> >  {
> >    MOZ_ASSERT(NS_IsMainThread(), "Must be on main thread.");
> > +
> > +  if (mState == INIT_STATE){
> 
> Init() should not be called more than once, you should not need this change.
> Please remove it.
> 
> @@ +199,4 @@
> >  MP4Reader::ReadMetadata(MediaInfo* aInfo,
> >                          MetadataTags** aTags)
> >  {
> > +  if (mState == READ_METADATA_STATE) {
> 
> I added MP4Reader::mDemuxerInitialized yesterday. You can use that here
> instead of creating a MP4Reader::mState data field.
> 
> @@ +599,4 @@
> >    return NS_OK;
> >  }
> >  
> > +bool MP4Reader::IsWaitingMediaResources()
> 
> I landed a MP4Reader::IsWaitingMediaResources() yesterday. You will need to
> rebase. Sorry!
> 
This virtual function is actually used to wait for *codec* resource and MediaOmxReader already uses it as well. Since it may not make sense for B2G PDM to create another new function to do the same thing, would it be possible to create a new function name for your case? Or I should add a new virtual function, like IsWaitingOnCodecResouces(), and change MediaOmxReader to use this new one as well? How do you think?

Comment 129

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

IsWaitingMediaResources() is used to determine whether all the resources the Reader needs to decode are present.

If either the OMXCodec is waiting we cannot decode. If a CDMProxy is required and not present, we cannot decode. So I think it makes sense to merge your IsWaitingMediaResources() changes into the existing MP4Reader::IsWaitingMediaResources().

i.e. something like:

bool
MP4Reader::IsWaitingMediaResources()
{
#ifdef MOZ_GONK_MEDIACODEC
  if (mVideo.mDecoder && mVideo.mDecoder->IsWaitingMediaResources()) {
    return true;
  }
#endif

  // Existing MP4Reader::IsWaitingMediaResources() code to check
  // if CDMProxy required and caps are known.
  // ...
}

Comment 130

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

Great! I will merge it. So for FireFox OS, currently it will not have a CDMProxy, right?

Comment 131

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

The existing code in MP4Reader::IsWaitingMediaResources() needs to still be there in FirefoxOS. It should not be #define'd out. So there can be a CDMProxy in FirefoxOS.

Comment 132

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

Attachment: 0003-Changes-for-Adding-Gonk-Decode-Module-V3.patch

Chris,
Thanks for your review on comment 124. 
Please help review again. 
Thanks!

Comment 133

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

Comment on attachment 8466005 [details] [diff] [review]
0003-Changes-for-Adding-Gonk-Decode-Module-V3.patch

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

Getting very close now. :)

::: content/media/fmp4/MP4Reader.cpp
@@ +229,5 @@
> +  #ifdef MOZ_GONK_MEDIACODEC
> +  if (!mVideo.mDecoder) {
> +    return false;
> +  }
> +  return mVideo.mDecoder->IsWaitingMediaResources();

This is incorrect.

Here you always exit if we have a video decoder. But what if we're decoding EME content on B2G? Then we need to wait until we have a CDMProxy it's ready to decode before we return false here.

So here, please do:

if (mVideo.mDecoder && mVideo.mDecoder->IsWaitingMediaResources()) {
  return true;
}

@@ +295,5 @@
>      // an encrypted stream and we need to wait for a CDM to be set, we don't
>      // need to reinit the demuxer.
>      mDemuxerInitialized = true;
> +  } else {
> +    *aInfo = mInfo;

This doesn't look correct either. Here you are exiting early if the demuxer is initialized. But that does not imply that the PDM or its decoders or the CDMProxy are initialized.

Why do you need to return here? Are you returning here to avoid mPlatform and mVideo.mDecoder being re-created?

If you return here when mDemuxerInitialized==true, then if we were waiting on CDMProxy resources, we may end up exiting without a PDM created.

I think you should only return here if mPlatform is not null, and if !IsWaitingMediaResources(), i.e.:

} else if (mPlatform && !IsWaitingMediaResources()) {
  // PDM and CDMProxy no longer waiting for resource, ready to decode.
  *aInfo = mInfo;
  *aTags = nullptr;
  return NS_OK;
}

@@ +717,5 @@
>  }
>  
> +bool MP4Reader::IsDormantNeeded()
> +{
> +#ifdef MOZ_GONK_MEDIACODEC

return mVideo.mDecoder && mVideo.mDecoder->IsDormantNeeded();

@@ +729,5 @@
> +
> +void MP4Reader::ReleaseMediaResources()
> +{
> +#ifdef MOZ_GONK_MEDIACODEC
> +  ResetDecode();

MediaDecoderStateMachine calls ResetDecode() before calling ReleaseMediaResources. So you don't need to call it again here.

::: content/media/fmp4/MP4Reader.h
@@ +56,4 @@
>    virtual nsresult GetBuffered(dom::TimeRanges* aBuffered,
>                                 int64_t aStartTime) MOZ_OVERRIDE;
>  
> +  //For Media Resource Management

// For media resource management.

(Match prevailing style.)

::: content/media/fmp4/PlatformDecoderModule.h
@@ +213,4 @@
>    // returned.
>    virtual nsresult Shutdown() = 0;
>  
> + /*For Codec Resource Management*/

// For codec resource management.

(Match prevailing style.)

Comment 134

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

(In reply to Chris Pearce (:cpearce) from comment #133)
> Comment on attachment 8466005 [details] [diff] [review]
> 0003-Changes-for-Adding-Gonk-Decode-Module-V3.patch
> 
> Review of attachment 8466005 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Getting very close now. :)
> 
> ::: content/media/fmp4/MP4Reader.cpp
> @@ +229,5 @@
> > +  #ifdef MOZ_GONK_MEDIACODEC
> > +  if (!mVideo.mDecoder) {
> > +    return false;
> > +  }
> > +  return mVideo.mDecoder->IsWaitingMediaResources();
> 
> This is incorrect.
> 
> Here you always exit if we have a video decoder. But what if we're decoding
> EME content on B2G? Then we need to wait until we have a CDMProxy it's ready
> to decode before we return false here.
> 
> So here, please do:
> 
> if (mVideo.mDecoder && mVideo.mDecoder->IsWaitingMediaResources()) {
>   return true;
> }
Recap the discussions on IRC, this IsWaitingMediaResources needs to check both codec and CDM resource if them are reserved or not. 
I will create two new functions, IsWaitingOnCodecResource and IsWaitingOnCDMResource, and check the returned result of them in the original virtual function, IsWaitingMediaResources().  

> @@ +295,5 @@
> >      // an encrypted stream and we need to wait for a CDM to be set, we don't
> >      // need to reinit the demuxer.
> >      mDemuxerInitialized = true;
> > +  } else {
> > +    *aInfo = mInfo;
> 
> This doesn't look correct either. Here you are exiting early if the demuxer
> is initialized. But that does not imply that the PDM or its decoders or the
> CDMProxy are initialized.
> 
> Why do you need to return here? Are you returning here to avoid mPlatform
> and mVideo.mDecoder being re-created?
Yes. For waiting codec resource, this function will be called twice in MediaDecoderStateMachine. 
> 
> If you return here when mDemuxerInitialized==true, then if we were waiting
> on CDMProxy resources, we may end up exiting without a PDM created.
> 
> I think you should only return here if mPlatform is not null, and if
> !IsWaitingMediaResources(), i.e.:
> 
> } else if (mPlatform && !IsWaitingMediaResources()) {
>   // PDM and CDMProxy no longer waiting for resource, ready to decode.
>   *aInfo = mInfo;
>   *aTags = nullptr;
>   return NS_OK;
> }
Good suggestion! 
> 
> @@ +717,5 @@
> >  }
> >  
> > +bool MP4Reader::IsDormantNeeded()
> > +{
> > +#ifdef MOZ_GONK_MEDIACODEC
> 
> return mVideo.mDecoder && mVideo.mDecoder->IsDormantNeeded();
> 
> @@ +729,5 @@
> > +
> > +void MP4Reader::ReleaseMediaResources()
> > +{
> > +#ifdef MOZ_GONK_MEDIACODEC
> > +  ResetDecode();
> 
> MediaDecoderStateMachine calls ResetDecode() before calling
> ReleaseMediaResources. So you don't need to call it again here.
> 
> ::: content/media/fmp4/MP4Reader.h
> @@ +56,4 @@
> >    virtual nsresult GetBuffered(dom::TimeRanges* aBuffered,
> >                                 int64_t aStartTime) MOZ_OVERRIDE;
> >  
> > +  //For Media Resource Management
> 
> // For media resource management.
> 
> (Match prevailing style.)
> 
> ::: content/media/fmp4/PlatformDecoderModule.h
> @@ +213,4 @@
> >    // returned.
> >    virtual nsresult Shutdown() = 0;
> >  
> > + /*For Codec Resource Management*/
> 
> // For codec resource management.
> 
> (Match prevailing style.)

Comment 135

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

Attachment: 0003-Changes-for-Adding-Gonk-Decode-Module-V4.patch

Changes are done for comment 133 and comment 134.

Comment 136

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

Comment on attachment 8466940 [details] [diff] [review]
0003-Changes-for-Adding-Gonk-Decode-Module-V4.patch

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

::: content/media/fmp4/MP4Reader.cpp
@@ +254,5 @@
>  }
>  
> +bool MP4Reader::IsWaitingMediaResources()
> +{
> +  return (IsWaitingOnCodecResource() || IsWaitingOnCDMResource());

return IsWaitingOnCDMResource() || IsWaitingOnCodecResource();

IsWaitingOnCDMResource() *must* come first, because we don't know whether we can create a decoder until the CDM is initialized and it has told us whether *it* will decode, or whether we need to create a PDM to do the decoding.

Please add the above paragraph as a comment here too - it's important.

::: content/media/fmp4/moz.build
@@ +59,5 @@
>    ]
>  
> +if CONFIG['ANDROID_VERSION'] >= '18'and CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk':
> +  DEFINES['MOZ_GONK_MEDIACODEC'] = True
> +  DIRS += ['gonk']

Indentation of this file is 4 spaces, not 2. Match prevailing style.

Comment 137

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

Attachment: 0003-Changes-for-Adding-Gonk-Decode-Module-V5.patch

Made some changes according to comment 136.

Comment 138

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

Comment on attachment 8466970 [details] [diff] [review]
0003-Changes-for-Adding-Gonk-Decode-Module-V5.patch

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

Good. You'll need to rebase, MP4Reader changed yesterday again.

Comment 139

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

FYI, I rebased this patch and built with it in my queue, and EME still worked.

Comment 140

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

(In reply to Chris Pearce (:cpearce) from comment #139)
> FYI, I rebased this patch and built with it in my queue, and EME still
> worked.
Awesome! Thanks for the info.

Comment 141

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

Thanks for your detailed review! Sorry for a lots for nits. I will fix them in next version. 
(In reply to Chris Pearce (:cpearce) from comment #126)
> Comment on attachment 8464447 [details] [diff] [review]
> 0005-Add-Gonk-Decoder-Module-V2.patch
> 
> Review of attachment 8464447 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks ok, but lots of nits to fix.
> 
> ::: content/media/fmp4/gonk/GonkAudioDecoderManager.cpp
> 
> @@ +207,5 @@
> > +GonkAudioDecoderManager::Input(mp4_demuxer::MP4Sample* aSample)
> > +{
> > +  const uint8_t* data = reinterpret_cast<const uint8_t*>(aSample->data);
> > +  uint32_t length = aSample->size;
> > +  status_t rv = mDecoder->Input(data, length, aSample->composition_timestamp, 0);
> 
> You shouldn't need to cast aSample->data to const here?
In this decoder module, we are not going to change this data, so it should be ok to cast it as const. Any concerns? 
BTW, in WMF case, the data is also cast as const at 
http://dxr.mozilla.org/mozilla-central/source/content/media/fmp4/wmf/WMFAudioMFTManager.cpp#134 
> 
> @@ +132,5 @@
> > +void
> > +GonkMediaDataDecoder::ProcessDrain()
> > +{
> > +  // Then extract all available output.
> > +  ProcessOutput();
> 
> You probably need to do something to cause all samples in the decode
> pipeline to output.
> 
> Can you try playing http://people.mozilla.org/~cpearce/avatar.mp4 and at the
> end if you reach a black frame drain is working correctly. If you still see
> "December 18th" text, then you may not need to do anything else here.
Thanks for this info. I found my decoder moudle doesn't end well when EOS, I will creat another bug to further handle EOS case. 
> 
> ::: content/media/fmp4/gonk/GonkVideoDecoderManager.cpp
> @@ +4,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +#include <OMX_IVCommon.h>
> > +#include <gui/Surface.h>
> > +#include <ICrypto.h>
> 
> Why do you need ICrypto.h?
This is required for Android's MediaCodec.
http://androidxref.com/4.3_r2.1/xref/frameworks/av/media/libstagefright/MediaCodec.cpp#142 
Without including this header file, it will case build fail.

Comment 142

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

Comment on attachment 8419240 [details] [diff] [review]
MediaCodec_ACodec_Change.patch

Set this obsolete since it is discussed in bug 1009410.

Comment 143

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

Attachment: 0005-Add-Gonk-Decoder-Module-V3.patch

Fix nits to remove/correct unecessary variables, functions, and comments.

Comment 144

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

Comment on attachment 8467492 [details] [diff] [review]
0005-Add-Gonk-Decoder-Module-V3.patch

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

Very close. Just style nits to fix.

::: content/media/fmp4/gonk/GonkAudioDecoderManager.cpp
@@ +103,5 @@
> +  dataOffset = mAudioBuffer->range_offset();
> +  size = mAudioBuffer->range_length();
> +
> +  nsAutoArrayPtr<AudioDataValue> buffer(new AudioDataValue[size/2] );
> +  if ((size/2)*sizeof(AudioDataValue) < size) {

Looking at this again, I don't think this check makes sense. You can take it out. Sorry.

::: content/media/fmp4/gonk/GonkDecoderModule.cpp
@@ +42,5 @@
> +                                    MediaDataDecoderCallback* aCallback)
> +{
> +  nsRefPtr<MediaDataDecoder> decoder =
> +  new GonkMediaDataDecoder(new GonkVideoDecoderManager(aImageContainer,aConfig),
> +                                  aVideoTaskQueue, aCallback);

aVideoTaskQueue should line up with the '(' above

@@ +53,5 @@
> +                                   MediaDataDecoderCallback* aCallback)
> +{
> +  nsRefPtr<MediaDataDecoder> decoder =
> +  new GonkMediaDataDecoder(new GonkAudioDecoderManager(aConfig),
> +                                 aAudioTaskQueue,

aAudiTaskQueue should line up with the '(' above

::: content/media/fmp4/gonk/GonkDecoderModule.h
@@ +15,5 @@
> +public:
> +  GonkDecoderModule();
> +  virtual ~GonkDecoderModule();
> +
> +  // Called when the decoders have shutdown. Main thread only.

Delete "Main thread only." as since yesterday PDM::Shutdown() is is now called on the decode task queue.

::: content/media/fmp4/gonk/GonkMediaDataDecoder.cpp
@@ +91,5 @@
> +    if (rv == NS_OK) {
> +      mCallback->Output(output.forget());
> +      continue;
> +    }
> +    else

} else {
  break;
}

Always add the {}.

::: content/media/fmp4/gonk/GonkMediaDataDecoder.h
@@ +28,5 @@
> +  // code other than NS_ERROR_NOT_AVAILABLE, an error will be reported to the
> +  // MP4Reader.
> +  virtual nsresult Input(mp4_demuxer::MP4Sample* aSample) = 0;
> +  virtual nsresult Output(int64_t aStreamOffset,
> +                         nsAutoPtr<MediaData>& aOutput) = 0;

Indentation is incorrect.

@@ +40,5 @@
> +// type are handled by GonkDecoderManager and the GonkDecoder it creates.
> +class GonkMediaDataDecoder : public MediaDataDecoder {
> +public:
> +  GonkMediaDataDecoder(GonkDecoderManager* aDecoderManager,
> +                      MediaTaskQueue* aTaskQueue,

Indentation is incorrect.

::: content/media/fmp4/gonk/GonkVideoDecoderManager.cpp
@@ +245,5 @@
> +    }
> +  }
> +  return true;
> +}
> +// Blocks until decoded sample is produced by the deoder.

Empty line between function definitions.

Comment 145

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

(In reply to Chris Pearce (:cpearce) from comment #144)
> Comment on attachment 8467492 [details] [diff] [review]
> 0005-Add-Gonk-Decoder-Module-V3.patch
> 
> Review of attachment 8467492 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Very close. Just style nits to fix.
> 
Good eyes! Thanks! I will fix them.

Comment 146

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

Attachment: 0005-Add-Gonk-Decoder-Module-V4.patch

Fix nits according to comment 144.

Comment 147

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

Comment on attachment 8467518 [details] [diff] [review]
0005-Add-Gonk-Decoder-Module-V4.patch

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

挺好!

Comment 148

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

十分感謝！:D

Comment 149

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

Attachment: Part1 -Enable-FMP4-for-B2G-Final.patch

Carry r+ from glandium.

Comment 150

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

Attachment: Part2-Changes-in-MediaCodecProxy-Final.patch

Carry r+ from edwin.

Comment 151

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

Attachment: Part3-Changes-for-Adding-Gonk-Decode-Module-Final.patch

Carry r+ from cpearce.

Comment 152

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

Attachment: Part4-Changes-in-demuxer-to-make-aac_profile-public-Final.patch

Carry r+ from ajones.

Comment 153

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

Attachment: Part5-Add-Gonk-Decoder-Module-Final.patch

Carry r+ from cpearce.

Comment 154

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

Testing results: https://tbpl.mozilla.org/?tree=Try&rev=4bdbca72baed.

Comment 155

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/2bc3c5dfbe97
https://hg.mozilla.org/integration/b2g-inbound/rev/0ee6c16041e0
https://hg.mozilla.org/integration/b2g-inbound/rev/6cf06baf92e1
https://hg.mozilla.org/integration/b2g-inbound/rev/13efa442f3bd
https://hg.mozilla.org/integration/b2g-inbound/rev/73cfc50bcd37

Comment 156

[Ryan VanderMeulen [:RyanVM]]

Backed out for non-unified bustage on B2G JB. You should be able to reproduce by setting --disabled-unified-compilation.
https://hg.mozilla.org/integration/b2g-inbound/rev/41e5029a4b7d

https://tbpl.mozilla.org/php/getParsedLog.php?id=45342091&tree=B2g-Inbound

Comment 157

[Ryan VanderMeulen [:RyanVM]]

And KitKat

Comment 158

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

Hi Ryan,

May I know why this non-unified bustage could not be found on my TryServer building cases? 
By knowing that, I could add more test/build cases to avoid this again. 
Thanks!

Comment 159

[Ryan VanderMeulen [:RyanVM]]

Yeah, non-unified bustage is a pain because it's not easy to test on Try :(

To get non-unified builds on Try, you need to specify --disable-unified-compilation in mozconfig.common.override, similar to what is shown at the below link for PGO builds.
https://wiki.mozilla.org/ReleaseEngineering/TryChooser#What_if_I_want_PGO_for_my_build

With that in place, a JB/KK emulator run show confirm things:
try: -b o -p emulator-jb,emulator-kk -u none -t none

Sorry for the inconvenience. I know it's a big pain :(

Comment 160

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

Attachment: Bug-941302-Part5-Add-Gonk-Decoder-Module－Build.patch

Have some modifications for the non-unfilied bustage as comment 156. 
Compare to attachment 8468280 [details] [diff] [review], the following are the main differences 
1. Include Android's header files (ICrypto.h, Surface.h, and etc)
2. Remove direct including headers and add forward declaration
3. Add MOZ_EXPORT for some Android class/struct (AMessage, AString, and etc)

Comment 161

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #159)
> Yeah, non-unified bustage is a pain because it's not easy to test on Try :(
> 
> To get non-unified builds on Try, you need to specify
> --disable-unified-compilation in mozconfig.common.override, similar to what
> is shown at the below link for PGO builds.
> https://wiki.mozilla.org/ReleaseEngineering/
> TryChooser#What_if_I_want_PGO_for_my_build
> 
> With that in place, a JB/KK emulator run show confirm things:
> try: -b o -p emulator-jb,emulator-kk -u none -t none
> 
> Sorry for the inconvenience. I know it's a big pain :(

Thanks for your info!

Comment 162

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

Attachment: Bug-941302-Part1-Enable-FMP4-For-B2G-r-glandium.patch

Rebase and Carry r+ from glandium.

Comment 163

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

Attachment: Bug-941302-Part2-Changes-in-MediaCodecProxy-r-edwin.patch

Change the file name and carry r+ from edwin.

Comment 164

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

Attachment: Bug-941302-Part3-Changes-for-Adding-Gonk-Decode-Modu.patch

Rebase and carry r+ from cpearce.

Comment 165

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

Attachment: Bug-941302-Part4-Changes-in-demuxer-to-make-aac_prof.patch

Rebase and carry r+ from ajones.

Comment 166

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

Comment on attachment 8468280 [details] [diff] [review]
Part5-Add-Gonk-Decoder-Module-Final.patch

Replaced with attachment 8469802 [details] [diff] [review]

Comment 167

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #159)
> Yeah, non-unified bustage is a pain because it's not easy to test on Try :(
> 
> To get non-unified builds on Try, you need to specify
> --disable-unified-compilation in mozconfig.common.override, similar to what
> is shown at the below link for PGO builds.
> https://wiki.mozilla.org/ReleaseEngineering/
> TryChooser#What_if_I_want_PGO_for_my_build
I failed to set --disable-unified-compilation in mozconfig.common.override, so I directly modified configure.in (http://dxr.mozilla.org/mozilla-central/source/configure.in#3448) to make MOZ_DISABLE_UNIFIED_COMPILATION=1 and can reproduce that bustage. This modification of configure.in is also pushed to the try server as below.  
 
> With that in place, a JB/KK emulator run show confirm things:
> try: -b o -p emulator-jb,emulator-kk -u none -t none
> 
TBPL results:
https://tbpl.mozilla.org/?tree=Try&rev=087abe333e9b

Comment 168

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

Hi Ryan, 
May I have your help to check again? 
Thanks!

Comment 169

[Ryan VanderMeulen [:RyanVM]]

Of course! (but you don't need the needinfo for that :)...).  Sorry again for the inconvenience.

Comment 170

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/482148cfa36b
https://hg.mozilla.org/integration/mozilla-inbound/rev/da74ec8de2af
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b4b565cc2a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/538c8c8ceae9
https://hg.mozilla.org/integration/mozilla-inbound/rev/e27972249203

Comment 171

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

https://hg.mozilla.org/mozilla-central/rev/482148cfa36b
https://hg.mozilla.org/mozilla-central/rev/da74ec8de2af
https://hg.mozilla.org/mozilla-central/rev/1b4b565cc2a8
https://hg.mozilla.org/mozilla-central/rev/538c8c8ceae9
https://hg.mozilla.org/mozilla-central/rev/e27972249203

Comment 172

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

Attachment: Block Diagrams

Update the block diagram.