Closed
Bug 1009410
Opened 11 years ago
Closed 10 years ago
Expose graphic Buffer to MediaCodec
Categories
(Core :: Audio/Video, defect)
Tracking
()
People
(Reporter: bwu, Unassigned)
References
Details
(Whiteboard: [POVB])
Attachments
(3 files, 5 obsolete files)
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•11 years ago
|
Reporter | ||
Comment 1•11 years ago
|
||
MediaCodec friends with ACodec to expose graphicBuffer as previosuly discussed in bug 941302.
Attachment #8421562 -
Flags: review?(sotaro.ikeda.g)
Comment 2•11 years ago
|
||
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•11 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)
Reporter | ||
Comment 5•11 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•11 years ago
|
||
More info on bug 1009420.
Comment 7•11 years ago
|
||
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)
Comment 8•11 years ago
|
||
(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)
Comment 9•11 years ago
|
||
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•11 years ago
|
||
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)
Comment 11•11 years ago
|
||
(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)
Comment 12•11 years ago
|
||
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•11 years ago
|
||
In Comment 12's case, change to MediaCodec/ACodec becomes just change to gecko.
Comment 14•11 years ago
|
||
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.
Updated•11 years ago
|
Flags: needinfo?(mvines) → needinfo?(dwilson)
Reporter | ||
Comment 15•11 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].
Comment 16•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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•11 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)
Comment 21•11 years ago
|
||
(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)
Comment 22•11 years ago
|
||
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)
Comment 23•11 years ago
|
||
In other words, for media playback I suggest using ACodec with at least 10 output native window buffers.
Reporter | ||
Comment 24•11 years ago
|
||
Hi Diego,
Do you have further concerns for comment 20 and attachment 8421562 [details] [diff] [review]?
Flags: needinfo?(dwilson)
Updated•11 years ago
|
feature-b2g: --- → 2.0
Comment 25•11 years ago
|
||
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•11 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)
Comment 27•11 years ago
|
||
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•11 years ago
|
See Also: → mediacodec
Reporter | ||
Comment 28•11 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•11 years ago
|
Flags: needinfo?(dwilson)
Reporter | ||
Comment 29•11 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.
Comment 30•11 years ago
|
||
(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)
Comment 31•11 years ago
|
||
So technically you are doing the right thing here: you need to allocate 10 buffers as a minumum for the hardware decoder.
Comment 32•11 years ago
|
||
(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•11 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)
Comment 34•11 years ago
|
||
(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•11 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•11 years ago
|
||
>
> Diego,
> From comment 43 as John mentioned, current WebRTC solution needs to add 8
I mean comment 34 :)
Comment 37•11 years ago
|
||
(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)
Comment 38•11 years ago
|
||
(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)
Comment 39•11 years ago
|
||
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)
Comment 40•11 years ago
|
||
Randell,
Are you aware of the blocking issue mentioned on comment 20?
Flags: needinfo?(rjesup)
Comment 41•11 years ago
|
||
(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•11 years ago
|
Flags: needinfo?(dwilson)
Reporter | ||
Comment 42•11 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•11 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.
Comment 44•11 years ago
|
||
Not sure what's the ni? for. If you need me input, can you please clarify and reset ni?
Flags: needinfo?(dwilson)
Comment 45•10 years ago
|
||
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•10 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)
Comment 47•10 years ago
|
||
(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.
Reporter | ||
Comment 48•10 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•10 years ago
|
Updated•10 years ago
|
Comment 49•10 years ago
|
||
Hi Diego,
not sure what is blocking here, need your help to feedback on this.
BR,
Marvin
Comment 50•10 years ago
|
||
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•10 years ago
|
Attachment #8421562 -
Flags: review?(dwilson)
Comment 51•10 years ago
|
||
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)
Comment 52•10 years ago
|
||
OK. Lets pull in attachment 8446689 [details] [diff] [review] into CAF gonk
Updated•10 years ago
|
Attachment #8421562 -
Attachment is obsolete: true
Updated•10 years ago
|
Assignee: bwu → dwilson
Flags: needinfo?(rjesup)
Whiteboard: [POVB]
Comment 53•10 years ago
|
||
(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•10 years ago
|
||
I am going to create a patch that implement Comment 53.
Comment 55•10 years ago
|
||
I did not tested the patch, though it seems to work.
Comment 56•10 years ago
|
||
attachment 8461906 [details] [diff] [review] seems to have a problem. I am going to update it.
Comment 58•10 years ago
|
||
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•10 years ago
|
||
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•10 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+
Comment 61•10 years ago
|
||
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•10 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+
Comment 63•10 years ago
|
||
(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•10 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 | ||
Comment 65•10 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)
Comment 66•10 years ago
|
||
(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•10 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)
Updated•10 years ago
|
Attachment #8462264 -
Flags: feedback?(jolin) → feedback+
Comment 69•10 years ago
|
||
(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•10 years ago
|
||
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•10 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•10 years ago
|
||
Hi All,
FYI. I have created a bug (Bug 1049332) to further discuss how to handle this case.
Reporter | ||
Comment 73•10 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!
Reporter | ||
Comment 75•10 years ago
|
||
Thank you very much!
Reporter | ||
Comment 76•10 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•10 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•10 years ago
|
feature-b2g: 2.1 → ---
Updated•10 years ago
|
feature-b2g: --- → 2.1
Comment 78•10 years ago
|
||
(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•10 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.
Comment 80•10 years ago
|
||
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)
Comment 81•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
Attachment #8479702 -
Flags: review?(sotaro.ikeda.g) → review+
Updated•10 years ago
|
Attachment #8479767 -
Flags: review?(sotaro.ikeda.g) → review+
Comment 85•10 years ago
|
||
Comment 86•10 years ago
|
||
(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•10 years ago
|
||
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•10 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•10 years ago
|
||
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•10 years ago
|
||
Rebase to b2g_kk_3.5 and carry r+ from sotaro
Attachment #8479767 -
Attachment is obsolete: true
Attachment #8480455 -
Flags: review+
Comment 91•10 years ago
|
||
Reporter | ||
Comment 92•10 years ago
|
||
Hi Diego,
That patches are merged as comment 91.
Please help pull it into CAF.
Thanks!
Flags: needinfo?(dwilson)
Reporter | ||
Comment 94•10 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)
Reporter | ||
Comment 96•10 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?
Comment 97•10 years ago
|
||
Probably mid to late next week
Comment 98•10 years ago
|
||
Assignee: dwilson → nobody
Reporter | ||
Comment 100•10 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)
Comment 101•10 years ago
|
||
b2g_jb_3.2 is only for FxOS v1.3. You should be on KK now for Flame.
Flags: needinfo?(mvines)
Reporter | ||
Comment 102•10 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•