Expose graphic Buffer to MediaCodec

RESOLVED FIXED in 2.1 S3 (29aug)

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: bwu, Unassigned)

Tracking

unspecified
2.1 S3 (29aug)
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.1)

Details

(Whiteboard: [POVB])

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

3 years ago
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.

Updated

3 years ago
Blocks: 941302, 904177
Hardware: x86 → ARM
Version: 30 Branch → unspecified
(Reporter)

Comment 1

3 years ago
Created attachment 8421562 [details] [diff] [review]
MediaCodec_Friend_With_ACodec.patch

MediaCodec friends with ACodec to expose graphicBuffer as previosuly discussed in bug 941302.
Attachment #8421562 - Flags: review?(sotaro.ikeda.g)
Blocks: 1009420
Comment on attachment 8421562 [details] [diff] [review]
MediaCodec_Friend_With_ACodec.patch

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

LGTM.
Attachment #8421562 - Flags: review?(sotaro.ikeda.g) → review+
(Reporter)

Comment 3

3 years ago
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?
Flags: needinfo?(mvines)
Why is this necessary?
Flags: needinfo?(mvines)
(Reporter)

Comment 5

3 years ago
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?
Flags: needinfo?(mvines)
(Reporter)

Comment 6

3 years ago
More info on bug 1009420.
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?
Flags: needinfo?(mvines)
(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?
Flags: needinfo?(mvines)
After MediaCodec/ACodec usage is enabled, we are going to stop using android::OMXCodec for video/audio decoding. It could reduce the porting cost.
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?
Flags: needinfo?(mvines)
(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?
Flags: needinfo?(mvines)
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.
In Comment 12's case, change to MediaCodec/ACodec becomes just change to gecko.
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.
Flags: needinfo?(mvines) → needinfo?(dwilson)
(Reporter)

Comment 15

3 years ago
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].
(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
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
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.
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
Flags: needinfo?(dwilson)
(Reporter)

Comment 20

3 years ago
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.
Flags: needinfo?(jolin)
Flags: needinfo?(dwilson)
(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 ===
Flags: needinfo?(jolin)
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.
Flags: needinfo?(dwilson)
In other words, for media playback I suggest using ACodec with at least 10 output native window buffers.
(Reporter)

Comment 24

3 years ago
Hi Diego,

Do you have further concerns for comment 20 and attachment 8421562 [details] [diff] [review]?
Flags: needinfo?(dwilson)

Updated

3 years ago
feature-b2g: --- → 2.0
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.
Flags: needinfo?(dwilson)
(Reporter)

Comment 26

3 years ago
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?
Flags: needinfo?(dwilson)
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.
Flags: needinfo?(dwilson)

Updated

3 years ago
See Also: → bug 1014614
(Reporter)

Comment 28

3 years ago
 (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?
(Reporter)

Updated

3 years ago
Flags: needinfo?(dwilson)
(Reporter)

Comment 29

3 years ago
(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.
(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
Flags: needinfo?(dwilson)
So technically you are doing the right thing here: you need to allocate 10 buffers as a minumum for the hardware decoder.
(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
(Reporter)

Comment 33

3 years ago
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?
Flags: needinfo?(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).
Flags: needinfo?(jolin)
(Reporter)

Comment 35

3 years ago
Recap
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?
Flags: needinfo?(dwilson)
(Reporter)

Comment 36

3 years ago
> 
> Diego,  
> From comment 43 as John mentioned, current WebRTC solution needs to add 8
I mean comment 34 :)
(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?
Flags: needinfo?(dwilson) → needinfo?(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 ===
Flags: needinfo?(jolin)
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?
Flags: needinfo?(jolin)
Randell,

Are you aware of the blocking issue mentioned on comment 20?
Flags: needinfo?(rjesup)
(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.
Flags: needinfo?(jolin)
(Reporter)

Updated

3 years ago
Flags: needinfo?(dwilson)
(Reporter)

Comment 42

3 years ago
(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.
(Reporter)

Comment 43

3 years ago
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.
Not sure what's the ni? for. If you need me input, can you please clarify and reset ni?
Flags: needinfo?(dwilson)
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.
Flags: needinfo?(itsay)

Comment 46

3 years ago
(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.
feature-b2g: 2.0 → 2.1
Flags: needinfo?(itsay)
(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.

Updated

3 years ago
Blocks: 1031500

Updated

3 years ago
Blocks: 1033903
(Reporter)

Comment 48

3 years ago
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.
Attachment #8421562 - Flags: review?(dwilson)
(Reporter)

Updated

3 years ago
Depends on: 1037282
(Reporter)

Updated

3 years ago
Blocks: 1037282
No longer depends on: 1037282
(Reporter)

Updated

3 years ago
Depends on: 1039182

Updated

3 years ago
Blocks: 1039182
No longer depends on: 1039182

Updated

3 years ago
No longer blocks: 904177
Hi Diego,
not sure what is blocking here, need your help to feedback on this.

BR,
Marvin
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?
Flags: needinfo?(sotaro.ikeda.g)

Updated

3 years ago
Attachment #8421562 - Flags: review?(dwilson)
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.
Flags: needinfo?(sotaro.ikeda.g)
OK. Lets pull in attachment 8446689 [details] [diff] [review] into CAF gonk

Updated

3 years ago
Attachment #8421562 - Attachment is obsolete: true

Updated

3 years ago
Assignee: bwu → dwilson
Flags: needinfo?(rjesup)
Whiteboard: [POVB]
(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.
I am going to create a patch that implement Comment 53.
Created attachment 8461906 [details] [diff] [review]
patch - modification to MediaCodec

I did not tested the patch, though it seems to work.
attachment 8461906 [details] [diff] [review] seems to have a problem. I am going to update it.
Created attachment 8462264 [details] [diff] [review]
patch - modification to MediaCodec

Fix the problem.
Attachment #8461906 - Attachment is obsolete: true
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 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.
Attachment #8462264 - Flags: feedback?(jolin)
Attachment #8462264 - Flags: feedback?(bwu)
Attachment #8462264 - Flags: feedback?(brsun)
(Reporter)

Comment 60

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

Looks good to me and Thanks!
Attachment #8462264 - Flags: feedback?(bwu) → feedback+
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

3 years ago
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|?
Attachment #8462264 - Flags: feedback?(brsun) → feedback+
(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?
Flags: needinfo?(brsun)

Comment 64

3 years ago
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.
Flags: needinfo?(brsun)
(Reporter)

Updated

3 years ago
Blocks: 1043274
(Reporter)

Comment 65

3 years ago

(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!
Flags: needinfo?(dwilson)
(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?
Flags: needinfo?(dwilson) → needinfo?(bwu)
(Reporter)

Comment 67

3 years ago
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?
Flags: needinfo?(bwu)
(Reporter)

Comment 68

3 years ago
Ni Diego for comment 67.
Flags: needinfo?(dwilson)
Attachment #8462264 - Flags: feedback?(jolin) → feedback+
(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...
So if we add the patch to CAF without adding the compile flags first, wouldn't that break CAF builds?
Flags: needinfo?(dwilson) → needinfo?(bwu)
(Reporter)

Comment 71

3 years ago
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?
Flags: needinfo?(bwu) → needinfo?(dwilson)
(Reporter)

Comment 72

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

Comment 73

3 years ago
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!
OK, I'll let you know when it's in CAF.
Flags: needinfo?(dwilson)
(Reporter)

Comment 75

3 years ago
Thank you very much!
(Reporter)

Comment 76

3 years ago
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!
Flags: needinfo?(dwilson)

Comment 77

3 years ago
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.
Target Milestone: --- → 2.1 S3 (29aug)

Updated

3 years ago
feature-b2g: 2.1 → ---

Updated

3 years ago
feature-b2g: --- → 2.1
(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.
Flags: needinfo?(dwilson)

Comment 79

3 years ago
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.
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]
Flags: needinfo?(sotaro.ikeda.g)
Anthony, this ended up on Sotaro, but it's an A/V bug - can you take care of what Diego is asking for?
Flags: needinfo?(sotaro.ikeda.g) → needinfo?(ajones)
(Reporter)

Comment 82

3 years ago
Created attachment 8479569 [details] [review]
Expose Graphic Buffer to MediaCodec

Merge the attachment 8462264 [details] [diff] [review] to Mozilla's framework code base.
Attachment #8479569 - Flags: review?(sotaro.ikeda.g)
Flags: needinfo?(ajones)
(Reporter)

Comment 83

3 years ago
Created attachment 8479702 [details] [review]
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.
Attachment #8479569 - Attachment is obsolete: true
Attachment #8479569 - Flags: review?(sotaro.ikeda.g)
Attachment #8479702 - Flags: review?(sotaro.ikeda.g)
(Reporter)

Comment 84

3 years ago
Created attachment 8479767 [details] [review]
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.
Attachment #8479767 - Flags: review?(sotaro.ikeda.g)

Updated

3 years ago
Attachment #8479702 - Flags: review?(sotaro.ikeda.g) → review+

Updated

3 years ago
Attachment #8479767 - Flags: review?(sotaro.ikeda.g) → review+
https://github.com/mozilla-b2g/platform_frameworks_av/commit/1875af348ecfba0c583723e7b05d53e256874bca
https://github.com/mozilla-b2g/platform_frameworks_av/commit/cbe5719bb7158a5e51af6afa416e423d1486a5a8
(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
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.
(Reporter)

Comment 88

3 years ago
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.
(Reporter)

Comment 89

3 years ago
Created attachment 8480447 [details] [review]
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
Attachment #8479702 - Attachment is obsolete: true
Attachment #8480447 - Flags: review+
(Reporter)

Comment 90

3 years ago
Created attachment 8480455 [details] [review]
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
Attachment #8479767 - Attachment is obsolete: true
Attachment #8480455 - Flags: review+
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
(Reporter)

Comment 92

3 years ago
Hi Diego,

That patches are merged as comment 91. 
Please help pull it into CAF.
Thanks!
Flags: needinfo?(dwilson)
Done
Flags: needinfo?(dwilson)
(Reporter)

Comment 94

3 years ago
(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.
Flags: needinfo?(mvines)
Both.  It's just not visible on CAF yet.
Flags: needinfo?(mvines)
(Reporter)

Comment 96

3 years ago
(In reply to Michael Vines [:m1] [:evilmachines] from comment #95)
> Both.  It's just not visible on CAF yet.
When will they be visible?
Probably mid to late next week
https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/commit/?h=b2g_kk_3.5&id=de3145b279dd897c1805b3c6ef994e68c9a6faf1
Assignee: dwilson → nobody

Comment 99

3 years ago
Blake, can we close this bug?
Flags: needinfo?(bwu)
(Reporter)

Comment 100

3 years ago
(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?
Flags: needinfo?(bwu) → needinfo?(mvines)
b2g_jb_3.2 is only for FxOS v1.3.  You should be on KK now for Flame.
Flags: needinfo?(mvines)
(Reporter)

Comment 102

3 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
See Also: → bug 1039182

Updated

2 years ago
See Also: → bug 1198664
See Also: → bug 1226109
You need to log in before you can comment on or make changes to this bug.