1036539 - Add async mode support to GonkNativeWindow

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect)

Description

[Sotaro Ikeda [:sotaro]]

This bug is made based on Bug 1036451 Comment 5.

-------------------------------------------

GonkNativeWindow(GonkBufferQueue) always work in sync mode. To handle this situation, it seems better to use async mode. In async mode, GonkBufferQueue keeps one latest buffer. In sync mode, GonkBufferQueue keeps all enqueued buffers.

There is a historical reason why GonkNativeWindow(GonkBufferQueue) works only on sync mode. On ICS, if GonkNativeWindow is async mode, some b2g devices' camera hal did not works correctly. It caused a crash at camera hal.

And between JB and KK, how to determine sync/async is different. Until JB, it is GonkBufferQueue's setting. Since JB, ANativeWindow's user can also set it to buffer's meta-data.

Comment 1

[Sotaro Ikeda [:sotaro]]

The meaning of async mode is different on each android version.

- On ICS: sync/async mode is used both dequeueBuffer() and queueBuffer()
  + dequeueBuffer() code made problem in async mode in the past.
  + In async mode, queueBuffer() store at most one latest video frame in a queue.
- On JB: sync/async mode is used only at queueBuffer().
  + In async mode, queueBuffer() store at most one latest video frame in a queue.
- On KK: there is no sync/async mode that was present in the past. But similar thing exists.
     It is "DequeueBufferCannotBlock". It is similar to ICS's async mode.

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add async mode to GonkNativeWindow

Implement only for JB. Need to add ICS and KK.

Comment 3

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

Do you think your patch also helps undoing the workaround in bug 1031500?
If I understand [1] correctly, when operating in async mode, buffer queue can "swap buffers" as long as there is at least one pending in the queue. Since queueBuffer() is called [2] before dequeueBuffer() [3], it will get the swapped (free) buffer without blocking, right?

[1] http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/GonkBufferQueueJB.cpp#534
[2] https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/tree/media/libstagefright/ACodec.cpp?h=kitkat#n3586
[3] https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/tree/media/libstagefright/ACodec.cpp?h=kitkat#n3609

Comment 4

[Sotaro Ikeda [:sotaro]]

(In reply to John Lin[:jolin][:jhlin] from comment #3)
> Do you think your patch also helps undoing the workaround in bug 1031500?
> If I understand [1] correctly, when operating in async mode, buffer queue
> can "swap buffers" as long as there is at least one pending in the queue.

I am not sure I understand you correctly. In async mode, GonkBufferQueue store at most one buffer for buffer consumer. If there is already one pending buffer is is replaced by new one. 

GonkBufferQueue::acquireBuffer() could be failed by the following code, if gecko side(buffer consumer) holds too much buffers. In this case, the pending buffer is not acquired by gecko. When next new buffers comes, the pending buffers becomes two in sync GonkNativeWindow. async GonkNativeWindow prevents this case.

http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/GonkBufferQueueKK.cpp?from=GonkBufferQueueKK.cpp&case=true#885

Comment 5

[Sotaro Ikeda [:sotaro]]

Therefore async GonkNativeWindow does not undo workaround in bug 1031500. It is actually a workaround of Bug 1036451.

Comment 6

[Sotaro Ikeda [:sotaro]]

Ah, I understand what you want to way. I saw your comment from consumer point of view. But your comment was from MediaCodec point of view.

Yes, it makes MediaCodec unblocked for calling mCodec->dequeueBufferFromNativeWindow(). async GonkNativeWindow always prepare at least one buffer for MediaCodec.

But it seems not totally undo bug 1031500, because webrtc seems to need buffers more than 2. But I do not understand WebRTC code clearly. I am not clear about it.

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch ver 2 - Add async mode to GonkNativeWindow

Add ICS and KK support.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch ver 3 - Add async mode to GonkNativeWindow

Fix nits.

Comment 9

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8468639 [details] [diff] [review]
patch ver 3 - Add async mode to GonkNativeWindow

jolin, can I have a feedback about the patch from you?

Comment 10

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8468639 [details] [diff] [review]
patch ver 3 - Add async mode to GonkNativeWindow

Sorry, I am going to update the patch.

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch ver 4 - Add async mode to GonkNativeWindow

Fix GonkBufferQueue::getMinUndequeuedBufferCount() in KK.

Comment 12

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8468656 [details] [diff] [review]
patch ver 4 - Add async mode to GonkNativeWindow

jolin, can I have a feedback?

Comment 13

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

Comment on attachment 8468656 [details] [diff] [review]
patch ver 4 - Add async mode to GonkNativeWindow

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

LGTM except for some comments that contradict the code.

::: widget/gonk/nativewindow/GonkBufferQueueJB.h
@@ +228,5 @@
>      // a buffer is available, the currently bound buffer can be dequeued and
>      // queued buffers will be acquired in order.  In asynchronous mode,
>      // a queued buffer may be replaced by a subsequently queued buffer.
>      //
>      // The default mode is asynchronous.

This contradicts constructor code, which sets mSynchronousMode to true.

::: widget/gonk/nativewindow/GonkBufferQueueKK.h
@@ +192,5 @@
> +    // a buffer is available, the currently bound buffer can be dequeued and
> +    // queued buffers will be acquired in order.  In asynchronous mode,
> +    // a queued buffer may be replaced by a subsequently queued buffer.
> +    //
> +    // The default mode is asynchronous.

This contradicts constructor code, which sets mSynchronousMode to true.

::: widget/gonk/nativewindow/GonkNativeWindowICS.h
@@ +108,5 @@
> +    // a buffer is available, the currently bound buffer can be dequeued and
> +    // queued buffers will be acquired in order.  In asynchronous mode,
> +    // a queued buffer may be replaced by a subsequently queued buffer.
> +    //
> +    // The default mode is asynchronous.

This contradicts constructor code, which sets mSynchronousMode to true.

Comment 14

[Sotaro Ikeda [:sotaro]]

(In reply to John Lin[:jolin][:jhlin] from comment #13)
> 
> ::: widget/gonk/nativewindow/GonkNativeWindowICS.h
> @@ +108,5 @@
> > +    // a buffer is available, the currently bound buffer can be dequeued and
> > +    // queued buffers will be acquired in order.  In asynchronous mode,
> > +    // a queued buffer may be replaced by a subsequently queued buffer.
> > +    //
> > +    // The default mode is asynchronous.
> 
> This contradicts constructor code, which sets mSynchronousMode to true.

Thanks for the feedback! I am going to update the above comments.

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: patch ver 5 - Add async mode to GonkNativeWindow

Apply the comment. Remove a change of async patch on ICS. On ICS, async mode is not necessary, because ICS does not have a failure like GonkBufferQueue::acquireBuffer().

http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/GonkBufferQueueJB.cpp#805

Comment 16

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8470874 [details] [diff] [review]
patch ver 5 - Add async mode to GonkNativeWindow

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

I can't really comment on the GonkBufferQueue changes, but the camera changes look fine.

Sotaro, what effect (if any) will this have on the camera operation/performance?

Comment 17

[Sotaro Ikeda [:sotaro]]

(In reply to Mike Habicher [:mikeh] from comment #16)
> Comment on attachment 8470874 [details] [diff] [review]
> patch ver 5 - Add async mode to GonkNativeWindow
> 
> Review of attachment 8470874 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I can't really comment on the GonkBufferQueue changes, but the camera
> changes look fine.
> 
> Sotaro, what effect (if any) will this have on the camera
> operation/performance?

The patch prevent camera preview performance drop that could be caused by "max acquired buffer count reached" case.

Comment 18

[Peter Chang[:pchang]]

Comment on attachment 8470874 [details] [diff] [review]
patch ver 5 - Add async mode to GonkNativeWindow

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

::: widget/gonk/nativewindow/GonkBufferQueueKK.cpp
@@ +614,5 @@
>                      // line to be dequeued again.
>                      mSlots[front->mBuf].mFrameNumber = 0;
>                  }
>                  // and we record the new buffer in the queued list
>                  *front = item;

Why do we need to set listener when we replace the front but the mQueue number is no changed? It means async mode will notify consumerlistener one more time in this case.

Comment 19

[Sotaro Ikeda [:sotaro]]

(In reply to peter chang[:pchang][:peter] from comment #18)
> 
> Why do we need to set listener when we replace the front but the mQueue
> number is no changed? It means async mode will notify consumerlistener one
> more time in this case.

It is necessary to handle GonkBufferQueue::acquireBuffer() failure. The failure could happen when gecko side owns too much buffers.
http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/GonkBufferQueueJB.cpp#805

Basically GonkNativeWindow's owers are triggered a new frame arrival event by GonkNativeWindowNewFrameCallback::OnNewFrame(). Within the OnNewFrame(), they try to get a new frame by calling GonkNativeWindow::getCurrentBuffer(). The getCurrentBuffer() could fail at GonkBufferQueue::acquireBuffer(). In this failure case, on next GonkBufferQueue::queueBuffer(), GonkBufferQueue::mQueue has front buffer. If front buffer exist, current implementation does not always calls callback function. If the callback function is not called, GonkNativeWindow's owers  does not handle a new frame anymore. We need to avoid this case.

Comment 20

[Peter Chang[:pchang]]

(In reply to Sotaro Ikeda [:sotaro] from comment #19)
> (In reply to peter chang[:pchang][:peter] from comment #18)
> > 
> > Why do we need to set listener when we replace the front but the mQueue
> > number is no changed? It means async mode will notify consumerlistener one
> > more time in this case.
> 
> It is necessary to handle GonkBufferQueue::acquireBuffer() failure. The
> failure could happen when gecko side owns too much buffers.
> http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/
> GonkBufferQueueJB.cpp#805
> 
> Basically GonkNativeWindow's owers are triggered a new frame arrival event
> by GonkNativeWindowNewFrameCallback::OnNewFrame(). Within the OnNewFrame(),
> they try to get a new frame by calling GonkNativeWindow::getCurrentBuffer().
> The getCurrentBuffer() could fail at GonkBufferQueue::acquireBuffer(). In
> this failure case, on next GonkBufferQueue::queueBuffer(),
> GonkBufferQueue::mQueue has front buffer. If front buffer exist, current
> implementation does not always calls callback function. If the callback
> function is not called, GonkNativeWindow's owers  does not handle a new
> frame anymore. We need to avoid this case.

The following line will try to reset the buffer state as FREE state. Does it fix above problem?

http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/GonkBufferQueueKK.cpp#597

Comment 21

[Sotaro Ikeda [:sotaro]]

(In reply to peter chang[:pchang][:peter] from comment #20)
> The following line will try to reset the buffer state as FREE state. Does it
> fix above problem?
> 
> http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/
> GonkBufferQueueKK.cpp#597

What is "above problem"? Sorry, I can not understand your question. The above code just clear a buffer slot that is exist in front of the queue. and then the following replace the font item by new one.
http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/GonkBufferQueueKK.cpp#603

The above code is same to the following JB's one.
http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/GonkBufferQueueJB.cpp#534

Comment 22

[Peter Chang[:pchang]]

(In reply to Sotaro Ikeda [:sotaro] from comment #21)
> (In reply to peter chang[:pchang][:peter] from comment #20)
> > The following line will try to reset the buffer state as FREE state. Does it
> > fix above problem?
> > 
> > http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/
> > GonkBufferQueueKK.cpp#597
> 
> What is "above problem"? Sorry, I can not understand your question. The
> above code just clear a buffer slot that is exist in front of the queue. and
> then the following replace the font item by new one.
> http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/
> GonkBufferQueueKK.cpp#603

What is the buffer state of front if case [1] happenes?
If it is acquired state, then [1] tries to set buffer state as FREE which could reduce numAcquiredBuffers in [2]. Therefore, it won't fail to acquire buffer.

But it looks like the front is BufferSlot::QUEUED state in [1]. Am I right?

[1]http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/GonkBufferQueueKK.cpp#597

[2]http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/GonkBufferQueueKK.cpp#880


http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/GonkBufferQueueJB.cpp#801
> 
> The above code is same to the following JB's one.
> http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/
> GonkBufferQueueJB.cpp#534

Comment 23

[Peter Chang[:pchang]]

Comment on attachment 8470874 [details] [diff] [review]
patch ver 5 - Add async mode to GonkNativeWindow

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

I mentioned a minor question in comment 22.
But I'm ok with this changes.

Comment 24

[Sotaro Ikeda [:sotaro]]

(In reply to peter chang[:pchang][:peter] from comment #22)
> 
> What is the buffer state of front if case [1] happenes?

BufferSlot::QUEUED state.

> If it is acquired state, then [1] tries to set buffer state as FREE which
> could reduce numAcquiredBuffers in [2]. Therefore, it won't fail to acquire
> buffer.
> 
> But it looks like the front is BufferSlot::QUEUED state in [1]. Am I right?

Yes. It is front buffer in the queue. Therefore, it should be in BufferSlot::QUEUED.
A number of acquire buffer is decreased only when gecko returns the buffer to GonkNativeWindow.

> 
> [1]http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/
> GonkBufferQueueKK.cpp#597
> 
> [2]http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/
> GonkBufferQueueKK.cpp#880
> 
>

Comment 25

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=358e78910a42

Comment 26

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b35bd37348e

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9b35bd37348e