922510 - Extend GonkNativeWindow to support android JB 4.2.2

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Peter Chang[:pchang]]

As title, extend GonkNativeWindow to support android JB 4.2.2 under widget/gonk/nativewindow.

Comment 1

[Peter Chang[:pchang]]

For Android 4.2.2, it still uses surfacetexture/surfacetextureclient for buffer management. Therefore, we have to create two kinds of GonkNativeWindowJB for android 4.3 and android 4.2.2 under widget/gonk/nativewindow. And we also have two version of consumerbase, bufferqueue classes.

Comment 2

[Sotaro Ikeda [:sotaro]]

Code duplication needs to be minimum, I think. If we can share the code, we should share them as much as possible.

Comment 3

[Peter Chang[:pchang]]

I think I could share the code for consumerbase and bufferqeue classes for JB 4.2 and JB 4.3. But GonkNativeWindowJB has lots of difference between these two versions.

Comment 4

[Sotaro Ikeda [:sotaro]]

Difference of internal implementations is not important, difference of interfaces is important.

Comment 5

[Peter Chang[:pchang]]

(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> Difference of internal implementations is not important, difference of
> interfaces is important.

Yes, I mean the difference of interface and the usage of interface.

Comment 6

[Peter Chang[:pchang]]

Attachment: WIP

The patch tries to reuse GonkBufferQueue and GonkConsumerBase calssess, and create JB42 folder to put GonkNativeWindowJB and GonkNativeWindowClientJB.

Right now it was built pass but still had the following problem to launch camera.


a. hit undequeuebuffer count exceed problem
   E BufferQueue: [unnamed-765-0] dequeueBuffer: min undequeued buffer count (1) exceeded (dequeued=6 undequeudCount=0)
b. more than two buffers are under acquired state after workaround problem a.

   E GonkBufferQueue: acquireBuffer: max acquired buffer count reached: 2 (max=1)

Comment 7

[Peter Chang[:pchang]]

(In reply to peter chang[:pchang] from comment #6)
> Created attachment 813030 [details] [diff] [review]
> WIP
> 
> The patch tries to reuse GonkBufferQueue and GonkConsumerBase calssess, and
> create JB42 folder to put GonkNativeWindowJB and GonkNativeWindowClientJB.
> 
> Right now it was built pass but still had the following problem to launch
> camera.
> 
> 
> a. hit undequeuebuffer count exceed problem
>    E BufferQueue: [unnamed-765-0] dequeueBuffer: min undequeued buffer count
> (1) exceeded (dequeued=6 undequeudCount=0)

I saw JB 4.3 version called cancelbuffer twice during camera init, but JB 4.2 didn't.
But I just increased the buffer count in GonkBufferQueue::setBufferCount to workaround this issue first.

> b. more than two buffers are under acquired state after workaround problem a.
> 
>    E GonkBufferQueue: acquireBuffer: max acquired buffer count reached: 2
> (max=1)
I found the acquired buffer didn't call returnbuffer. It mean those buffer were still being used.

Comment 8

[Peter Chang[:pchang]]

> > a. hit undequeuebuffer count exceed problem
> >    E BufferQueue: [unnamed-765-0] dequeueBuffer: min undequeued buffer count
> > (1) exceeded (dequeued=6 undequeudCount=0)
> 
> I saw JB 4.3 version called cancelbuffer twice during camera init, but JB
> 4.2 didn't.
> But I just increased the buffer count in GonkBufferQueue::setBufferCount to
> workaround this issue first.
> 
> > b. more than two buffers are under acquired state after workaround problem a.
> > 
> >    E GonkBufferQueue: acquireBuffer: max acquired buffer count reached: 2
> > (max=1)
> I found the acquired buffer didn't call returnbuffer. It mean those buffer
> were still being used.

This problem could be solved by setting acquiredbuffer count as 2 inside GonkNativeWindowsJB.cpp.

I'm still debugging the problem a.

Comment 9

[Peter Chang[:pchang]]

Attachment: WIP v2

Comment 10

[Peter Chang[:pchang]]

Attachment: Camera HAL patch

Comment 11

[Peter Chang[:pchang]]

Attached two patches that make camera working on my Nexus 4.2.2 device.

attachment 814049 [details] [diff] [review] is related to gecko modification that shares GonkBufferQueue/GonkConsumerBase/GonkNativeWindowClientJB classes with JB 4.3. It only creates new GonkNativeWindowClientJB for JB 4.2.
I will continue working to reduce the changes scope.

attachment 814050 [details] [diff] [review] is the android camera HAL changes which tries to cancel two buffers back to NativeWindow server. And it could fix "undequeuebuffer count exceed" problem I met.

Comment 12

[Peter Chang[:pchang]]

Attachment: fix build error

Comment 13

[Peter Chang[:pchang]]

Attachment: GonkNativeWindow for JB 4.2 v3

Updated patch v3 which minimize the change scope.
- Share GonkConsumerBase/GonkBufferQueue/GonkNativeWindowClientJB
- Separate GonkNativeWindowJB for 4.2

I just checked the difference of GonkNativeWindowJB between 4.2 and 4.3.
JB 4.3 moved some common functions like isExternalFormat/getCurrentCrop functions to GLConsumer class which we didn't import to Gecko.

Therefore, we can keep the GonkNativeWindowJB 4.2 version or use the same GonkNativeWindowJB for 4.2 and 4.3 but remove isExternalFormat/getCurrentCrop...etc functions if no use.

Sotaro, which one you prefer or any suggestion?

Comment 14

[Peter Chang[:pchang]]

To try GonkNativeWindow on JB 4.2, 
a. Apply attachment 814746 [details] [diff] [review] and attachment 814750 [details] [diff] [review] on gecko.
b. Apply attachment 814050 [details] [diff] [review] on hardware/qcom/camera/

Comment 15

[Sotaro Ikeda [:sotaro]]

(In reply to peter chang[:pchang] from comment #13)
> 
> I just checked the difference of GonkNativeWindowJB between 4.2 and 4.3.
> JB 4.3 moved some common functions like isExternalFormat/getCurrentCrop
> functions to GLConsumer class which we didn't import to Gecko.
> 
> Therefore, we can keep the GonkNativeWindowJB 4.2 version or use the same
> GonkNativeWindowJB for 4.2 and 4.3 but remove
> isExternalFormat/getCurrentCrop...etc functions if no use.
> 
> Sotaro, which one you prefer or any suggestion?

isExternalFormat/getCurrentCrop are not necessary. Unnecessary functions can be removed at all. I feel that current patch has a lot of differences that can be removed. 4.3 is the newest version. So, 4.3's code has preference than 4.2. And to work 4.2 correctly, only necessary interfaces to gonk are ANativeWindow and ISurfaceTexture. Other function and code differences should be merged to 4.3.

Comment 16

[Sotaro Ikeda [:sotaro]]

For example, I feel that the following parts do not have enough reasons to keep version difference.

>+#if ANDROID_VERSION == 17
>+status_t GonkBufferQueue::dequeueBuffer(int *outBuf, sp<Fence>& outFence,
>+#else
> status_t GonkBufferQueue::dequeueBuffer(int *outBuf, sp<Fence>* outFence,
>+#endif

>+#if ANDROID_VERSION == 17
>+            mSlots[buf].mFence.clear();
>+#else
>             mSlots[buf].mFence = Fence::NO_FENCE;
>+#endif


>+#if ANDROID_VERSION == 17
>+        outFence = mSlots[buf].mFence;
>+        mSlots[buf].mFence.clear();
>+#else
>         *outFence = mSlots[buf].mFence;
>         mSlots[buf].mFence = Fence::NO_FENCE;
>+#endif


>+#if ANDROID_VERSION >= 18
>     if (fence == NULL) {
>         ST_LOGE("queueBuffer: fence is NULL");
>         return BAD_VALUE;
>     }
>+#endif


>+#if ANDROID_VERSION == 17
>+void GonkBufferQueue::cancelBuffer(int buf, sp<Fence> fence) {
>+#else
> void GonkBufferQueue::cancelBuffer(int buf, const sp<Fence>& fence) {
>+#endif

Comment 17

[Peter Chang[:pchang]]

(In reply to Sotaro Ikeda [:sotaro] from comment #16)
> For example, I feel that the following parts do not have enough reasons to
> keep version difference.

The following modification are still needed because the base class of GonkBufferQueue is BnSurfaceTexture which has different interfaces, compared to BnGraphicBufferProducer.

But I modified a little in GonkBufferQueue to support Android JB 4.2 and 4.3.

> >+#if ANDROID_VERSION == 17
> >+status_t GonkBufferQueue::dequeueBuffer(int *outBuf, sp<Fence>& outFence,
> >+#else
> > status_t GonkBufferQueue::dequeueBuffer(int *outBuf, sp<Fence>* outFence,
> >+#endif
> 
> >+#if ANDROID_VERSION == 17
> >+void GonkBufferQueue::cancelBuffer(int buf, sp<Fence> fence) {
> >+#else
> > void GonkBufferQueue::cancelBuffer(int buf, const sp<Fence>& fence) {
> >+#endif




Thanks for suggestion. The following modification are necessary.
> >+#if ANDROID_VERSION == 17
> >+            mSlots[buf].mFence.clear();
> >+#else
> >             mSlots[buf].mFence = Fence::NO_FENCE;
> >+#endif
> 
> 
> >+#if ANDROID_VERSION == 17
> >+        outFence = mSlots[buf].mFence;
> >+        mSlots[buf].mFence.clear();
> >+#else
> >         *outFence = mSlots[buf].mFence;
> >         mSlots[buf].mFence = Fence::NO_FENCE;
> >+#endif

Comment 18

[Peter Chang[:pchang]]

Attachment: GonkNativeWindow for JB 4.2 v4

Create patch v4 to use current GonkNativeWindow(JB 4.3) implementation to support GonkNativeWindow on JB 4.2.

Comment 19

[Peter Chang[:pchang]]

Attachment: GonkNativeWindow for JB 4.2 v5

Remove some redundant space

Comment 20

[Sotaro Ikeda [:sotaro]]

Comment on attachment 817719 [details] [diff] [review]
GonkNativeWindow for JB 4.2 v5

Looks good! Makefile.in part needs to be reviewed by a build peer.

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 817719 [details] [diff] [review]
GonkNativeWindow for JB 4.2 v5

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

::: widget/gonk/nativewindow/moz.build
@@ +20,5 @@
>      'GonkNativeWindow.h',
>      'GonkNativeWindowClient.h',
>  ]
>  
> +if CONFIG['ANDROID_VERSION'] in ('17', '18'):

In the future, you may want to set a flag in configure and just key off that here, as hardcoding versions across the tree is fragile.

Comment 22

[Peter Chang[:pchang]]

(In reply to Gregory Szorc [:gps] from comment #21)
> Comment on attachment 817719 [details] [diff] [review]
> GonkNativeWindow for JB 4.2 v5
> 
> Review of attachment 817719 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/nativewindow/moz.build
> @@ +20,5 @@
> >      'GonkNativeWindow.h',
> >      'GonkNativeWindowClient.h',
> >  ]
> >  
> > +if CONFIG['ANDROID_VERSION'] in ('17', '18'):
> 
> In the future, you may want to set a flag in configure and just key off that
> here, as hardcoding versions across the tree is fragile.

Thanks for comment. The nativewindow feature had some dependency with Android interface.
For example, android 15 is totally different with android 17 or 18.

But here we could think about how to support new Android (no interface changed) without Makefile modification.

Comment 23

[Peter Chang[:pchang]]

Attachment: GonkNativeWindow for JB 4.2 v5

Update reviewer name

Comment 24

[Peter Chang[:pchang]]

https://tbpl.mozilla.org/?tree=Try&rev=6dd0986159a0

Positive try result.

Comment 25

[Peter Chang[:pchang]]

Push to inbound.

https://hg.mozilla.org/integration/mozilla-inbound/rev/62c49e2095d3

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/62c49e2095d3

Comment 27

[Vincent Liu[:vliu]]

This bug should uplift to v1.2 or it still blocks bugs. Hi Francis, can you help mark this bug as kio+? Thanks.

Comment 28

[Michael Wu [:mwu]]

Comment on attachment 818855 [details] [diff] [review]
GonkNativeWindow for JB 4.2 v5

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

::: configure.in
@@ +226,5 @@
>          MOZ_OMX_DECODER=1
>          AC_SUBST(MOZ_OMX_DECODER)
>          MOZ_RTSP=1
>          ;;
> +    17|18)

Please request review from me when adding support for additional base versions in the future.

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/89196e7b35e7