1009410 - Expose graphic Buffer to MediaCodec

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

Since current B2G render pipeline needs graphicBuffer and MediaCodec(used for Async callflow) cannot get graphicBuffer, we need to modify MediaCodec and ACodec to expose it.

Comment 1

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

Attachment: MediaCodec_Friend_With_ACodec.patch

MediaCodec friends with ACodec to expose graphicBuffer as previosuly discussed in bug 941302.

Comment 2

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8421562 [details] [diff] [review]
MediaCodec_Friend_With_ACodec.patch

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

LGTM.

Comment 3

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

Hi Michael, 

We need to modify MediaCodec.cpp and ACodec.cpp in stagefright to expose graphicBuffer for B2G rendering pipeline. Could you check it and see if it is ok to apply this patch to caf?

Comment 4

[Michael Vines [:m1] [:evilmachines]]

Why is this necessary?

Comment 5

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

More info for you. 
We plan to use MediaCodec/ACodec which operate in an async way and afaik Google added this for streaming case. Since B2G rendering pipeline requires graphicBuffer (the same way currently we get it from OMXCodec) but we cannot get it from MediaCodec/ACodec, it is necessary to modify codes to expose graphicBuffer. We tried to have some way around, like enqueue and dequeue the buffer (bug 941302 comment #25 and comment #28), but have chance to be blocked when dequeuing buffer. One method to solve that is to add more buffers, but the cost is high and not worth. 
Do you have other concerns?

Comment 6

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

More info on bug 1009420.

Comment 7

[Michael Vines [:m1] [:evilmachines]]

My main concern is that this will complicate porting of B2G, as now every device needs to modify their version of this file manually.  How many more buffers would need to be added to solve this that away?

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #7)
> My main concern is that this will complicate porting of B2G, as now every
> device needs to modify their version of this file manually.  How many more
> buffers would need to be added to solve this that away?

:m1, if caf's MediaCodec/ACodec does not have caf specific modification from aosp android, we can move MediaCodec/ACodec to gecko side. Is there the modifications to them? If there is no modification, we can move MediaCodec/ACodec to gecko. Does it make porting easier?

Comment 9

[Sotaro Ikeda [:sotaro]]

After MediaCodec/ACodec usage is enabled, we are going to stop using android::OMXCodec for video/audio decoding. It could reduce the porting cost.

Comment 10

[Michael Vines [:m1] [:evilmachines]]

Are you saying these changes are already in AOSP?  If so that totally changes the discussion.  Do you have a link to the corresponding AOSP commit?

Comment 11

[Sotaro Ikeda [:sotaro]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #10)
> Are you saying these changes are already in AOSP?

Sorry, my comment is not clear. I am not talking about it. If MediaCodec/ACodec does not have a dependency to some specific hardware, MediaCodec/ACodec could be moved to gecko side. If it is correct, it could reduce the porting cost like "modify their version of this file manually". How do you think about it?

Comment 12

[Sotaro Ikeda [:sotaro]]

Currently we use android::OMXCodec as a hw codec hal api. It is very high level API. stagefright provide more low api OMXNodeInstance and OpenMAX IL. This level of API seems enough for hw codec hal api. In this case, MediaCodec/ACodec are totally hardware independent and can be moved to gecko side. This could mitigate the porting tasks.

Comment 13

[Sotaro Ikeda [:sotaro]]

In Comment 12's case, change to MediaCodec/ACodec becomes just change to gecko.

Comment 14

[Sotaro Ikeda [:sotaro]]

The direction of talking each other seems different. I just commented to the way to reduce the hardware porting task. Because "modify their version of this file manually" was in Comment 7.

Comment 15

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

Recap the current discussions.
Since we want to modify the MediaCodec and ACodec and would like to reduce the porting effort as well, there are two ways as following. 
[1]Modify the MediaCodec/ACodec in AOSP. 
   The modifications need to be applied to each platform's code base, like caf. 
[2]Copy MediaCodec/ACodec to Gecko side and modify it. 
   If there is *no* any codes/changes related to HW platform or any dependency on HW, we can have our own MediaCodec/ACodec in Gecko. In this way there should be no porting efforts from platform to platform.   

So the current question is "Is there any dependency on HW platform in MediaCodec/ACodec?" 
If yes, we may need to take the approach[1]. Otherwise we can take [2].

Comment 16

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

(In reply to Blake Wu [:bwu][:blakewu] from comment #15)
> Recap the current discussions.
> Since we want to modify the MediaCodec and ACodec and would like to reduce
> the porting effort as well, there are two ways as following. 
> [1]Modify the MediaCodec/ACodec in AOSP. 
>    The modifications need to be applied to each platform's code base, like
> caf. 
> [2]Copy MediaCodec/ACodec to Gecko side and modify it. 
>    If there is *no* any codes/changes related to HW platform or any
> dependency on HW, we can have our own MediaCodec/ACodec in Gecko. In this
> way there should be no porting efforts from platform to platform.   
> 
> So the current question is "Is there any dependency on HW platform in
> MediaCodec/ACodec?" 
> If yes, we may need to take the approach[1]. Otherwise we can take [2].

 Is ExtendedCodec[1] considered HW dependency? Seems so to me because it's used in caf's ACodec[2] but not in aosp[3].

[1] https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/tree/include/media/stagefright/ExtendedCodec.h?h=kitkat
[2] https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/tree/media/libstagefright/ACodec.cpp?h=kitkat
[3] https://android.googlesource.com/platform/frameworks/av/+/master/media/libstagefright/ACodec.cpp

Comment 17

[Sotaro Ikeda [:sotaro]]

I forget to mention to the following. The following change is also necessary for ACodec that to be used in gecko.

> cancelStart = bufferCount - minUndequeuedBuffers;

http://androidxref.com/4.4_r1/xref/frameworks/av/media/libstagefright/ACodec.cpp#710

Comment 18

[Sotaro Ikeda [:sotaro]]

And need to confirm to codeaurora about if we could get low power without calling a function like pause(). On OMCCodec, we need to call OMXCodec::pause() to set the codec to low power state. The meaning of OMXCodec::pause() becomes different from android one.

Comment 19

[Diego Wilson [:diego]]

It appears on the WebRTC front they are using MediaCodec.h and are able to exchange graphic buffers without changing the interface. Perhaps this can be leveraged for media playback?

https://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp#212
https://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp#410

Comment 20

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

Before creating this bug, I have synced with :jolin who is responsible for that WebRTC codes. And as he described in bug 941302 comment #26 WebRTC hits some blocking problem.  so we think that it is required to modify codes to expose graphicBuffer.
NI :jolin to have more info.  
NI :diego to have further discussion.

Comment 21

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

(In reply to Blake Wu [:bwu][:blakewu] from comment #20)
> Before creating this bug, I have synced with :jolin who is responsible for
> that WebRTC codes. And as he described in bug 941302 comment #26 WebRTC hits
> some blocking problem.  so we think that it is required to modify codes to
> expose graphicBuffer.
> NI :jolin to have more info.  
> NI :diego to have further discussion.

 The code mentioned in comment 19 works but ACodec blocks (usually tens to a hundred ms or more) at [1] quite often during my tests. Increase the number of spare buffers helps. (Currently only 2 spare buffers, make it 10 and no blocking observed).

 [1] http://androidxref.com/4.4_r1/xref/frameworks/av/media/libstagefright/ACodec.cpp#3436

Diego,
 I suspect reducing decoder latency might also relief the symptom, could you please help check it with following patch applied on your test environment for bug 989945?

=== BEGIN ===
diff --git a/media/libstagefright/ACodec.cpp b/media/libstagefright/ACodec.cpp
index bfd727b..8fc2e00 100755
--- a/media/libstagefright/ACodec.cpp
+++ b/media/libstagefright/ACodec.cpp
@@ -3287,8 +3287,11 @@ void ACodec::BaseState::onOutputBufferDrained(const sp<AMessage> &msg) {
                 if (info->mStatus == BufferInfo::OWNED_BY_NATIVE_WINDOW) {
                     // We cannot resubmit the buffer we just rendered, dequeue
                     // the spare instead.
-
+                    int64_t now = ALooper::GetNowUs();
                     info = mCodec->dequeueBufferFromNativeWindow();
+                    ALOGE("[%s] dequeueBufferFromNativeWindow() took %lld ms",
+                          mCodec->mComponentName.c_str(),
+                          (ALooper::GetNowUs() - now)/1000);
                 }

                 if (info != NULL) {
=== END ===

Comment 22

[Diego Wilson [:diego]]

Hi jolin,

(In reply to John Lin[:jolin][:jhlin] from comment #21)
>  The code mentioned in comment 19 works but ACodec blocks (usually tens to a
> hundred ms or more) at [1] quite often during my tests. Increase the number
> of spare buffers helps. (Currently only 2 spare buffers, make it 10 and no
> blocking observed).

>  I suspect reducing decoder latency might also relief the symptom

Thanks for the analysis. You are correct, for media playback (non real time) the decoder needs at least 10 buffers as reference. This is the optimal configuration for media playback.

We are currently working on a solution to reduce the number of reference buffers needed for WebRTC (real time), but this solution will not apply to media playback.

Comment 23

[Diego Wilson [:diego]]

In other words, for media playback I suggest using ACodec with at least 10 output native window buffers.

Comment 24

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

Hi Diego,

Do you have further concerns for comment 20 and attachment 8421562 [details] [diff] [review]?

Comment 25

[Diego Wilson [:diego]]

I guess what I'm suggesting is to use the same approach as WebRTC but with 10 output buffers.

As Andreas' points out in bug 997567 comment 5, stagefright patches ought to be avoided as much as possible.

Comment 26

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

In order to avoid changing the stagefright codes, we need to allocate 8 more buffers which requires ~11 MB bytes (1280*720*1.5*8) for 720P case. For 1080P case, it needs more. I don't think it is worth doing this way especially the changes, attachment 8421562 [details] [diff] [review], is minor and straightforward.  
Moreover, what Andreas mean is ask vendor's help if changes are not avoidable :). IMO this case is unavoidable (unacceptable).
How do you think?

Comment 27

[Diego Wilson [:diego]]

I believe the 10 buffers are required for proper operation of the H.264 hardware codec. I think this is the way it OMXCodec works too. Reducing the number of buffers may result in unexpected behavior from the codec.

Comment 28

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

 (In reply to John Lin[:jolin][:jhlin] from comment #21)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #20)

>  The code mentioned in comment 19 works but ACodec blocks (usually tens to a
> hundred ms or more) at [1] quite often during my tests. Increase the number
> of spare buffers helps. (Currently only 2 spare buffers, make it 10 and no
> blocking observed).
AFAIK, the number of buffers John means is for *spare*(MIN_UNDEQUEUED_BUFFERS) and kept in GonkBufferQueue, not normally used for codec. It will be queried at http://androidxref.com/4.3_r2.1/xref/frameworks/av/media/libstagefright/OMXCodec.cpp#1775
In order to use WebRTC approach and avoid blocking problems, we need to increase the number of space buffers from 2 to 10. As comment 26, it is acceptable. Let me know if there is anything unclear.
May I have your help to review this patch to see if it can be uplifted or not?

Comment 29

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

(In reply to Blake Wu [:bwu][:blakewu] from comment #28)
> In order to use WebRTC approach and avoid blocking problems, we need to
> increase the number of space buffers from 2 to 10. As comment 26, it is
> acceptable. Let me know if there is anything unclear.
Opps. Sorry for the typo. I mean it should *not* be acceptable, since those additional 8 buffers are not required.

Comment 30

[Diego Wilson [:diego]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #28)
>  (In reply to John Lin[:jolin][:jhlin] from comment #21)
> > (In reply to Blake Wu [:bwu][:blakewu] from comment #20)
> 
> >  The code mentioned in comment 19 works but ACodec blocks (usually tens to a
> > hundred ms or more) at [1] quite often during my tests. Increase the number
> > of spare buffers helps. (Currently only 2 spare buffers, make it 10 and no
> > blocking observed).
> AFAIK, the number of buffers John means is for
> *spare*(MIN_UNDEQUEUED_BUFFERS) and kept in GonkBufferQueue, not normally
> used for codec. It will be queried at
> http://androidxref.com/4.3_r2.1/xref/frameworks/av/media/libstagefright/
> OMXCodec.cpp#1775
> In order to use WebRTC approach and avoid blocking problems, we need to
> increase the number of space buffers from 2 to 10. As comment 26, it is
> acceptable. Let me know if there is anything unclear.
> May I have your help to review this patch to see if it can be uplifted or
> not?

Actually we don't undequeue *any* buffers in FFOS [1]. So by increasing min undequeued buffers you are actually increasing the total number of buffers that are available for the decoder.

[1] https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/commit/?h=b2g_kk_3.5&id=20c8e958a17fbb8e612c887e4ee23a03585346da

Comment 31

[Diego Wilson [:diego]]

So technically you are doing the right thing here: you need to allocate 10 buffers as a minumum for the hardware decoder.

Comment 32

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

(In reply to Diego Wilson [:diego] from comment #30)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #28)
> >  (In reply to John Lin[:jolin][:jhlin] from comment #21)
> > > (In reply to Blake Wu [:bwu][:blakewu] from comment #20)
> > 
> > >  The code mentioned in comment 19 works but ACodec blocks (usually tens to a
> > > hundred ms or more) at [1] quite often during my tests. Increase the number
> > > of spare buffers helps. (Currently only 2 spare buffers, make it 10 and no
> > > blocking observed).
> > AFAIK, the number of buffers John means is for
> > *spare*(MIN_UNDEQUEUED_BUFFERS) and kept in GonkBufferQueue, not normally
> > used for codec. It will be queried at
> > http://androidxref.com/4.3_r2.1/xref/frameworks/av/media/libstagefright/
> > OMXCodec.cpp#1775
> > In order to use WebRTC approach and avoid blocking problems, we need to
> > increase the number of space buffers from 2 to 10. As comment 26, it is
> > acceptable. Let me know if there is anything unclear.
> > May I have your help to review this patch to see if it can be uplifted or
> > not?
> 
> Actually we don't undequeue *any* buffers in FFOS [1]. So by increasing min
> undequeued buffers you are actually increasing the total number of buffers
> that are available for the decoder.
> 
> [1]
> https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/commit/
> ?h=b2g_kk_3.5&id=20c8e958a17fbb8e612c887e4ee23a03585346da

In fact this change hasn't been applied to MediaCodec that WebRTC uses. In my experiments conducted on Flame, I saw total 20 buffers allocated for decoder output and ACodec canceled/queued 2 back to native window. So the decoder got only 18. AFAICT, these numbers are determined in [1].

[1] https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/tree/media/libstagefright/ACodec.cpp?h=b2g_kk_3.5#n589

Comment 33

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

So you increase the number of total buffers from 10 (2 for nativewindow) to 20 (2 for nativewindow?)and with this change no blocking in ACodec is observed, right?

Comment 34

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

(In reply to Blake Wu [:bwu][:blakewu] from comment #33)
> So you increase the number of total buffers from 10 (2 for nativewindow) to
> 20 (2 for nativewindow?)and with this change no blocking in ACodec is
> observed, right?

 Sorry, the numbers I gave in comment 32 are *before* increasing number of spare buffers. After increasing, the buffer counts went from 20(decoder:18 + native window:2) to 28(decoder:18 + native-window:10).

Comment 35

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

Let me recap the current discussion. 
[1] Discuss about MediaCodec/ACodec only
We are talking about MediaCodec/ACodec instead of OMXCodec. 
[2] Why use MediaCodec/ACodec
It operates in an async way which will better for many applications, especially for streaming case. 
[3] Why we need to modify MediaCodec/ACodec 
We need to expose the grapicBuffer which is required by B2G rendering pipeline. In OMXCodec case, we also use grapicBuffer.  
[4] Is there any way not to modify MediaCodec/ACodec.  
Yes. By enqueue buffer, register a callback to nativeWindow, and dequeue buffer once callback is called. But this hit a blocking problem in ACodec which can be solved by adding 8 more buffers. More info on comment 21, comment 26, comment 32 and comment 34.

[Proposal]
Modify the codes to make design simple and save unnecessary memory usage. 

Diego,  
From comment 43 as John mentioned, current WebRTC solution needs to add 8 more buffers. As mentioned in comment 26. It should not be acceptable. 
May I have your help to check again?

Comment 36

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

> 
> Diego,  
> From comment 43 as John mentioned, current WebRTC solution needs to add 8
I mean comment 34 :)

Comment 37

[Diego Wilson [:diego]]

(In reply to John Lin[:jolin][:jhlin] from comment #34)
>  Sorry, the numbers I gave in comment 32 are *before* increasing number of
> spare buffers. After increasing, the buffer counts went from 20(decoder:18 +
> native window:2) to 28(decoder:18 + native-window:10).

Where is the patch that does this? Has it landed already?

Comment 38

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

(In reply to Diego Wilson [:diego] from comment #37)
> (In reply to John Lin[:jolin][:jhlin] from comment #34)
> >  Sorry, the numbers I gave in comment 32 are *before* increasing number of
> > spare buffers. After increasing, the buffer counts went from 20(decoder:18 +
> > native window:2) to 28(decoder:18 + native-window:10).
> 
> Where is the patch that does this? Has it landed already?

 No, I didn't land it.
 Here is the patch that increases the number of spare buffers:

=== BEGIN ===
diff --git a/media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp b/media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
index b42a574..74bf0c1 100644
--- a/media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
+++ b/media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ -283,7 +283,10 @@ public:
     if (mNativeWindow.get()) {
       // listen to buffers queued by MediaCodec::RenderOutputBufferAndRelease().
       mNativeWindow->setNewFrameCallback(this);
-      surface = new Surface(mNativeWindow->getBufferQueue());
+      sp<GonkBufferQueue> bq = mNativeWindow->getBufferQueue();
+      // More spare buffers to avoid OMX decoder waiting for native window.
+      bq->setMaxAcquiredBufferCount(10);
+      surface = new Surface(bq);
     }
     status_t result = mCodec->configure(config, surface, nullptr, 0);
     if (result == OK) {
=== END ===

Comment 39

[Diego Wilson [:diego]]

John,

So Blake mentions the reason for this patch is to avoid blocking. Can you expand on this? What does this patch achieve. Better performance?

Comment 40

[Diego Wilson [:diego]]

Randell,

Are you aware of the blocking issue mentioned on comment 20?

Comment 41

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

(In reply to Diego Wilson [:diego] from comment #39)
> John,
> 
> So Blake mentions the reason for this patch is to avoid blocking. Can you
> expand on this? What does this patch achieve. Better performance?

All OMX callbacks from media server are handled by ACodec looper so any blocking on its thread means we can not respond to events such as OMX_EmptyBufferDone or OMX_FillBufferDone as soon as we can.

While Blake's patch makes it possible for us to avoid waiting on native window buffer and in theory can achieve better performance, I think it still needs your comment on how that could affect the behavior of QCOM codec. For example, currently H.264 decoder requests 18 output buffers. Is that a hard requirement? Will decoder pause/block when there are fewer (because some are currently in native window buffer queue and waiting for rendering)? If that's the case, then this patch won't have the benefit we want.

Comment 42

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

(In reply to John Lin[:jolin][:jhlin] from comment #41)
> currently in native window buffer queue and waiting for rendering)? If
> that's the case, then this patch won't have the benefit we want.
To be more clear, 
After discussing with John offline, this last sentence should not make much sense here, since this patch is not directly related and designed to/for the case John mentioned in comment 41. 
Please ignore it. Thanks.

Comment 43

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

One thing I am going to do is to finish bug 941302 (a simple video playback using MediaCodec) with the patch in this bug and then check if the same blocking problem happens with John's patch used in WebRTC. 
Then we can check if there is a blocking problem on the simple playback case and WebRTC case with two patches. 

John, 
I think it should be worth having a try to use my patch in WebRTC case to see if blocking problem is gone or not. I will use yours in my implementation in bug 941302 as well.

Comment 44

[Diego Wilson [:diego]]

Not sure what's the ni? for. If you need me input, can you please clarify and reset ni?

Comment 45

[Jason Smith [:jsmith]]

Ivan - Why is this flagged feature-b2g 2.0? The blocking bugs here all indicate that this needs to be resolved in 2.1, not 2.0.

Comment 46

[Ivan Tsay (:ITsay)]

(In reply to Jason Smith [:jsmith] from comment #45)
> Ivan - Why is this flagged feature-b2g 2.0? The blocking bugs here all
> indicate that this needs to be resolved in 2.1, not 2.0.

Hi Jason,

You are correct. I missed to move this one to 2.1 as well when moving its blocked bug 941302 to v2.1. Update the feature-b2g flag accordingly.

Comment 47

[Sotaro Ikeda [:sotaro]]

(In reply to John Lin[:jolin][:jhlin] from comment #38)
> (In reply to Diego Wilson [:diego] from comment #37)
> > (In reply to John Lin[:jolin][:jhlin] from comment #34)
> > >  Sorry, the numbers I gave in comment 32 are *before* increasing number of
> > > spare buffers. After increasing, the buffer counts went from 20(decoder:18 +
> > > native window:2) to 28(decoder:18 + native-window:10).
> > 
> > Where is the patch that does this? Has it landed already?
> 
>  No, I didn't land it.
>  Here is the patch that increases the number of spare buffers:
> 
> === BEGIN ===
> diff --git
> a/media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
> b/media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
> index b42a574..74bf0c1 100644
> --- a/media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
> +++ b/media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
> @@ -283,7 +283,10 @@ public:
>      if (mNativeWindow.get()) {
>        // listen to buffers queued by
> MediaCodec::RenderOutputBufferAndRelease().
>        mNativeWindow->setNewFrameCallback(this);
> -      surface = new Surface(mNativeWindow->getBufferQueue());
> +      sp<GonkBufferQueue> bq = mNativeWindow->getBufferQueue();
> +      // More spare buffers to avoid OMX decoder waiting for native window.
> +      bq->setMaxAcquiredBufferCount(10);
> +      surface = new Surface(bq);
>      }
>      status_t result = mCodec->configure(config, surface, nullptr, 0);
>      if (result == OK) {
> === END ===

It might be better to change the number of spare buffers by GonkNativeWindow's constructor. It is done at GonkCameraHardware.
http://mxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraHwMgr.cpp#178

Since android JB, android hw support 4 spare buffers on camera use case. If gecko needs more buffers, we need to get hw vendor's guarantee.

Comment 48

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

Comment on attachment 8421562 [details] [diff] [review]
MediaCodec_Friend_With_ACodec.patch

Hi Diego, 

According to a couple of reasons I recap as below, we need your help to review this patch and see if you have any concern. 

1. The necessity of this patch is described in comment 35. 
2. This bug blocks many bugs and on Bug 1009420 Sotaro also suggested to modify MediaCodec/ACodec. rjesup is also aware of this and increase the number of buffer as a near-term workaround on Bug 1031500.  
3. On Bug 941302, I have integrated and verified this patch. It works well.

Comment 49

[Marvin Khoo [:Marvin_Khoo]]

Hi Diego,
not sure what is blocking here, need your help to feedback on this.

BR,
Marvin

Comment 50

[Diego Wilson [:diego]]

Sotaro,

If in you believe the only way to unblock is by exposing the graphic buffers in MediaCodec than I'll take the hit and maintain the patch in gonk. WDYT?

Comment 51

[Sotaro Ikeda [:sotaro]]

Diego,

I thought about it more in Bug 1009420. There are three candidates for fixing the problem. Then, "exposing the graphic buffers in MediaCodec" seems best solution on JB and KK.

attachment 8421562 [details] [diff] [review] is just exposing the graphic buffers. It is not enough. Also need to modify undequeued buffers handling like attachment 8446689 [details] [diff] [review] in Bug 1009420.

Comment 52

[Diego Wilson [:diego]]

OK. Lets pull in attachment 8446689 [details] [diff] [review] into CAF gonk

Comment 53

[Sotaro Ikeda [:sotaro]]

(In reply to Diego Wilson [:diego] from comment #52)
> OK. Lets pull in attachment 8446689 [details] [diff] [review] into CAF gonk

It seems better to think about how to enable it in CAF. WebRTC code and attachment 8446689 [details] [diff] [review] is not compatible about how to handle "undequeued buffer handling".

One idea to mitigate this problem is that adding capability to enable/disable "undequeued buffer handling" change by a setting. MediaCodec::configure()'s argument could be used for it. AMessage can add any metadata. If the capability is exist in MediaCodec, MediaCodec could keep compatible to default MediaCodec. And WebRTC could continue to use current usage, even when video playback uses different "undequeued buffer handling" in a MediaCodec instance.

Comment 54

[Sotaro Ikeda [:sotaro]]

I am going to create a patch that implement Comment 53.

Comment 55

[Sotaro Ikeda [:sotaro]]

Attachment: patch - modification to MediaCodec

I did not tested the patch, though it seems to work.

Comment 56

[Sotaro Ikeda [:sotaro]]

attachment 8461906 [details] [diff] [review] seems to have a problem. I am going to update it.

Comment 57

[Sotaro Ikeda [:sotaro]]

Attachment: patch - modification to MediaCodec

Fix the problem.

Comment 58

[Sotaro Ikeda [:sotaro]]

By attachment 8462264 [details] [diff] [review], how to handle undequeued buffers by MediaCodec can be changed by client side. It could be done by adding the following metadata to a configuration.

> tmp->setInt32("moz-use-undequeued-bufs", 1);

Comment 59

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8462264 [details] [diff] [review]
patch - modification to MediaCodec

brsun, jolin and bwu, can you provide feedbacks to the patch?

By the way, I am in PTO for a week.

Comment 60

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

Comment on attachment 8462264 [details] [diff] [review]
patch - modification to MediaCodec

Looks good to me and Thanks!

Comment 61

[Sotaro Ikeda [:sotaro]]

To make gralloc buffer usage efficient, modification to GonkBufferQueue::dequeueBuffer() might becomes necessary. If so, it seems better to create a bug for it.

Comment 62

[Bruce Sun [:brsun]]

Comment on attachment 8462264 [details] [diff] [review]
patch - modification to MediaCodec

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

Looks good to me. Just curious about our strategy to avoid build fail caused by mismatching interface on different branch of frameworks/av.

::: media/libstagefright/ACodec.cpp
@@ +1144,5 @@
> +    if (msg->findInt32("moz-use-undequeued-bufs", &useUndequeuedBufs)) {
> +        mUseUndequeuedBufs = true;
> +    } else {
> +        mUseUndequeuedBufs = false;
> +    }

Does the value of |useUndequeuedBufs| need to affect the value of |mUseUndequeuedBufs|?

Comment 63

[Sotaro Ikeda [:sotaro]]

(In reply to Bruce Sun [:brsun] from comment #62)
> Comment on attachment 8462264 [details] [diff] [review]
> patch - modification to MediaCodec
> 
> Review of attachment 8462264 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me. Just curious about our strategy to avoid build fail caused
> by mismatching interface on different branch of frameworks/av.

Yes, this is a problem. Do you have an idea about it?

Comment 64

[Bruce Sun [:brsun]]

I don't have good ideas about it. There are some ways I could imagine to solve this problem:
 - [a] Sync the interface of MediaCodec on all branches frameworks/av.
       = This would need many communication efforts on all partners. It seems the scalability of this solution is also very limited, which required extra efforts on future branches of frameworks/av.
 - [b] Use compile flags in MediaCodecProxy to hide the interface difference of MediaCodec on different branches of frameworks/av.
       = This would introduce at least one additional compile flag into gonk. By doing this, MediaCodecProxy can only hide the interface difference, the behavior difference should still be handled by gecko itself to reflect the real abilities of underlying MediaCodec.

There might be some better ways to tackle this problem.

Comment 65

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

(In reply to Bruce Sun [:brsun] from comment #64)
> I don't have good ideas about it. There are some ways I could imagine to
> solve this problem:
>  - [a] Sync the interface of MediaCodec on all branches frameworks/av.
>        = This would need many communication efforts on all partners. It
> seems the scalability of this solution is also very limited, which required
> extra efforts on future branches of frameworks/av.
>  - [b] Use compile flags in MediaCodecProxy to hide the interface difference
> of MediaCodec on different branches of frameworks/av.
>        = This would introduce at least one additional compile flag into
> gonk. By doing this, MediaCodecProxy can only hide the interface difference,
> the behavior difference should still be handled by gecko itself to reflect
> the real abilities of underlying MediaCodec.
> 
[b]Sounds good! It would be better to check that flag to see if it is required to build the corresponding codes at gecko side or not. 

Hi Diego, 
May I have your opinion there? Thanks!

Comment 66

[Diego Wilson [:diego]]

(In reply to Blake Wu [:bwu][:blakewu] from comment #65)
> Hi Diego, 
> May I have your opinion there? Thanks!

How would this flag be set and by whom? Would it be on be default? Would the vendor have to set it?

Comment 67

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

Good questions! I don't have good answers right yet :). 
I think it would be better to create another bug to discuss further how to handle this case well. 
Before that, to avoid build break, could we check in this patch to CAF first and then check in the corresponding codes at gecko side? Would it be ok for you?

Comment 68

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

Ni Diego for comment 67.

Comment 69

[Sotaro Ikeda [:sotaro]]

(In reply to Diego Wilson [:diego] from comment #66)
> (In reply to Blake Wu [:bwu][:blakewu] from comment #65)
> > Hi Diego, 
> > May I have your opinion there? Thanks!
> 
> How would this flag be set and by whom? Would it be on be default? Would the
> vendor have to set it?

I also think b) is reasonable.

If we want to prevent build break on some builds, "off by default" seems better. But without it, we could not get good decoding performance...

Comment 70

[Diego Wilson [:diego]]

So if we add the patch to CAF without adding the compile flags first, wouldn't that break CAF builds?

Comment 71

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

Since this patch, attachment 8462264 [details] [diff] [review], only adds new member function and new member variable, it should not cause CAF build break, right?

Comment 72

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

Hi All,
FYI. I have created a bug (Bug 1049332) to further discuss how to handle this case.

Comment 73

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

Since this patch adds *new* function member and data member, it should not cause build break if it is checked in to CAF first, and then the corresponding codes at gecko side is checked in later. 
May I have your feedback? 
Thanks!

Comment 74

[Diego Wilson [:diego]]

OK, I'll let you know when it's in CAF.

Comment 75

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

Thank you very much!

Comment 76

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

Hi Diego, 
Would you mind letting us know when you will finish checking in codes to CAF? 
Since this bug is flagged as 2.1 feature, in order to meet our schedule it would be helpful to know your plan and we can prepare our gecko's codes and have a further integration. 
Thanks!

Comment 77

[Kai-Chih Hu [:khu]]

Set this target milestone as sprint 3. Diego, if it's not what you're expecting to see as the target milestone, please tell us as soon as possible. Thanks.

Comment 78

[Diego Wilson [:diego]]

(In reply to Kevin Hu [:khu] from comment #77)
> Set this target milestone as sprint 3. Diego, if it's not what you're
> expecting to see as the target milestone, please tell us as soon as
> possible. Thanks.

This should make it to CAF before August 29th. I'll let you know when it's there.

Comment 79

[Kai-Chih Hu [:khu]]

Thanks, Diego. It would be great to have this done as soon as possible, because we still have other bugs that are depended on this one, and we're targeting to land them all by this week.

Comment 80

[Diego Wilson [:diego]]

Sotaro,

Can you please land this patch to Mozilla's frameworks/av [1] branch first?

After that I can pull it into the CAF branch.

Also, please hold off on landing gecko patches that depend on this bug until we can verify it has been pulled to CAF.

[1] http://git.mozilla.org/b2g/platform_frameworks_av.git [master]

Comment 81

[Milan Sreckovic [:milan] (needinfo for best results)]

Anthony, this ended up on Sotaro, but it's an A/V bug - can you take care of what Diego is asking for?

Comment 82

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

Attachment: Expose Graphic Buffer to MediaCodec

Merge the attachment 8462264 [details] [diff] [review] to Mozilla's framework code base.

Comment 83

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

Attachment: Expose Graphic Buffer to MediaCodec (merge to mozilla git hub b2g-4.3_r2.1)

Merge the attachment 8462264 [details] [diff] [review] to branch b2g-4.3_r2.1 @ https://github.com/mozilla-b2g/platform_frameworks_av.

Comment 84

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

Attachment: Expose Graphic Buffer to MediaCodec (merge to mozilla git hub b2g-4.4.2_r1)

Merge the attachment 8462264 [details] [diff] [review] [diff] [review] to branch b2g-4.4.2_r1 @ https://github.com/mozilla-b2g/platform_frameworks_av.

Comment 85

[Sotaro Ikeda [:sotaro]]

https://github.com/mozilla-b2g/platform_frameworks_av/commit/1875af348ecfba0c583723e7b05d53e256874bca
https://github.com/mozilla-b2g/platform_frameworks_av/commit/cbe5719bb7158a5e51af6afa416e423d1486a5a8

Comment 86

[Ryan VanderMeulen [:RyanVM]]

(In reply to Sotaro Ikeda [:sotaro] from comment #85)
> https://github.com/mozilla-b2g/platform_frameworks_av/commit/
> 1875af348ecfba0c583723e7b05d53e256874bca
> https://github.com/mozilla-b2g/platform_frameworks_av/commit/
> cbe5719bb7158a5e51af6afa416e423d1486a5a8

Reverted from both branches for non-unified bustage:
https://tbpl.mozilla.org/php/getParsedLog.php?id=46876968&tree=B2g-Inbound

Comment 87

[Kai-Zhen Li [:seinlin][:kli]]

Blake, 

I created branches b2g_jb_3.2 and b2g_kk_3.5 on https://github.com/mozilla-b2g/platform_frameworks_av.

Can you send you PR to these branches? Then the patches can be pulled to CAF from these branches.

Comment 88

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

Kai-Zhen, 
Thanks for your help! 

We got build break in comment 86 since the code bases between Mozilla github and current Flame's framework/av is different which causes our patch (based on Flame's code base) cannot fit into. 
With your created branch, I can have a right code base to check.

Comment 89

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

Attachment: Expose Graphic Buffer to MediaCodec (merge to mozilla git hub b2g_jb_3.2)

Rebase to branch b2g_jb_3.2 and carry r+ from sotaro

Comment 90

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

Attachment: Expose Graphic Buffer to MediaCodec (merge to mozilla git hub b2g_kk_3.5)

Rebase to b2g_kk_3.5 and carry r+ from sotaro

Comment 91

[Kai-Zhen Li [:seinlin][:kli]]

Merged.

b2g_kk_3.5: https://github.com/mozilla-b2g/platform_frameworks_av/commit/92d004dc6e2b4b28d143a0f357881c6164babf90

b2g_jb_3.2: https://github.com/mozilla-b2g/platform_frameworks_av/commit/eb5f34986419f0350913852d97744a073285644f

Comment 92

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

Hi Diego,

That patches are merged as comment 91. 
Please help pull it into CAF.
Thanks!

Comment 93

[Michael Vines [:m1] [:evilmachines]]

Done

Comment 94

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

(In reply to Michael Vines [:m1] [:evilmachines] from comment #93)
> Done
Thanks! 
Which branch did you check in? 
I don't see the check-in in our currently-used branches, neither b2g_jb_3.2 or b2g_kk_3.5.

Comment 95

[Michael Vines [:m1] [:evilmachines]]

Both.  It's just not visible on CAF yet.

Comment 96

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

(In reply to Michael Vines [:m1] [:evilmachines] from comment #95)
> Both.  It's just not visible on CAF yet.
When will they be visible?

Comment 97

[Michael Vines [:m1] [:evilmachines]]

Probably mid to late next week

Comment 98

[Michael Vines [:m1] [:evilmachines]]

https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/commit/?h=b2g_kk_3.5&id=de3145b279dd897c1805b3c6ef994e68c9a6faf1

Comment 99

[Ken Chang[:kenkai]]

Blake, can we close this bug?

Comment 100

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

(In reply to Michael Vines [:m1] [:evilmachines] from comment #98)
> https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/commit/
> ?h=b2g_kk_3.5&id=de3145b279dd897c1805b3c6ef994e68c9a6faf1
Thanks! 
I don't see the patch in branch, b2g_jb_3.2. 
Will you check in it to b2g_jb_3.2?

Comment 101

[Michael Vines [:m1] [:evilmachines]]

b2g_jb_3.2 is only for FxOS v1.3.  You should be on KK now for Flame.

Comment 102

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

Close this bug since the patch is checked in to CAF. 
For those devices/platforms whose frameworks/av codes are not from CAF, they need to modify the codes according to attachment 8480447 [details] [review] (JB) or attachment 8480455 [details] [review] (KK) due to comment 35.