959505 - [Display][gonk-kk] Porting GonkNativeWindowClient, GonkBufferQueue, and FakeSurfaceComposer

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Solomon Chiu [:schiu]]

Fix the interface change relative to KitKat in GonkNativeWindowClient, GonkBufferQueue, and FakeSurfaceComposer, which block the camera functions.

Comment 1

[Solomon Chiu [:schiu]]

Attachment: GonkNativeWindow-KK.diff

Comment 2

[Michael Wu [:mwu]]

Is this enough to turn on omx decoding?

Comment 3

[Michael Wu [:mwu]]

Please assign bugs to yourself when appropriate.

Comment 4

[Solomon Chiu [:schiu]]

(In reply to Michael Wu [:mwu] from comment #2)
> Is this enough to turn on omx decoding?

Video playback is ok(H.264). Camera previewing function depends on 946241.

Comment 5

[Andreas Gal :gal]

Shouldn't we have a KK version instead of conditionally patching the JB version?

Comment 6

[Sotaro Ikeda [:sotaro]]

I also think it is better to create KK version from KK source. BufferQueue have a enough difference between JB and KK. The code need to be created as same as possible to original KK. It is a good way to prevent a difficult to reproduce bug in the product.

Comment 7

[Solomon Chiu [:schiu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> I also think it is better to create KK version from KK source. BufferQueue
> have a enough difference between JB and KK. The code need to be created as
> same as possible to original KK. It is a good way to prevent a difficult to
> reproduce bug in the product.

Thanks for your comment. I will write a KK version BufferQueue and GonkNativeWindowClient.

Comment 8

[Marco Chen [:mchen]]

Note that the patch here can enable Camera and Video playing with OMXCodec on Nexus-4.
But can't enable them on Nexus-5.

Comment 9

[Solomon Chiu [:schiu]]

Attachment: 0001-Changes-to-GonkNativeWindow-relative-files-correspon.patch

Comment 10

[Solomon Chiu [:schiu]]

Just uploaded a patch as Sotaro's recommendation, and here I brief my changes:

1. GonkBufferQueue
The original source files - GonkBufferQueue.* are replaced with GonkBufferQueueJB.* and GonkBufferQueueKK.*. Alonging with KK's refactoring work, the major difference is BufferItem, which is moved to IGraphicsBufferConsumer. And one of the base class - IGraphiBufferConsumer is substituted with IGonkGraphiBufferConsumer, please see item-5 for reason.  

2. GonkConsumerBase
The original source files - GonkConsumerBase.* are replaced with GonkConsumerBase.* and GonkBufferQueueKK.*.

3. GonkNativeWindow
Add GonkNativeWindowKK.*, sync KK's change and removed JB relative conditional flags. 

4. GonkNativeWindowClient
Add GonkNativeWindoClientwKK.*, sync KK's change and removed JB relative conditional flags. 

5. GonkIGraphicBufferConsumer
The reason why I decided to create this class is: the pure virtual functions of IGraphicsBufferConsumer require BufferItem, but we previously customized BufferItem with an additional Gonk-dedicated data member mSurfaceDescriptor.

Comment 11

[vijayanand.hongal]

About the changes to FakeSurfaceComposer.h for the 2 methods:
+    virtual void destroyDisplay(const sp<android::IBinder>& display);
+    virtual status_t captureScreen(const sp<IBinder>& display, const sp<IGraphicBufferProducer>& producer,
+            uint32_t reqWidth, uint32_t reqHeight, uint32_t minLayerZ, uint32_t maxLayerZ);


If the methods are virtual and do not need any definitions, this header file changes would be fine. But the cpp source file has dummy/empty definitions (when i pulled the latest code)

I think the #if def block for FakeSurfaceComposer.cpp should be added for the above 2 methods definitions even if its empty/dummy.

Comment 12

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

(In reply to vijayanand.hongal from comment #11)
> I think the #if def block for FakeSurfaceComposer.cpp should be added for
> the above 2 methods definitions even if its empty/dummy.

Yep, I had to make that change in my tree to get attachment 8365684 [details] [diff] [review] to compile.   Solomon, can you please add this to your patch?

diff --git a/widget/gonk/nativewindow/FakeSurfaceComposer.cpp b/widget/gonk/nativewindow/FakeSurfaceComposer.cpp
index fddd6b9..508886d 100644
--- a/widget/gonk/nativewindow/FakeSurfaceComposer.cpp
+++ b/widget/gonk/nativewindow/FakeSurfaceComposer.cpp
@@ -59,6 +59,19 @@ sp<IBinder> FakeSurfaceComposer::createDisplay(const String8& displayName,
     return nullptr;
 }

+#if ANDROID_VERSION >= 19
+void FakeSurfaceComposer::destroyDisplay(const sp<IBinder>& display)
+{
+}
+
+status_t FakeSurfaceComposer::captureScreen(const sp<IBinder>& display,
+        const sp<IGraphicBufferProducer>& producer,
+        uint32_t reqWidth, uint32_t reqHeight,
+        uint32_t minLayerZ, uint32_t maxLayerZ) {
+    return INVALID_OPERATION;
+}
+#endif
+
 sp<IBinder> FakeSurfaceComposer::getBuiltInDisplay(int32_t id) {
     return nullptr;
 }
--

Comment 13

[Solomon Chiu [:schiu]]

Attachment: 0001-Changes-to-GonkBufferQueue-relative-files-correspond.patch

Comment 14

[Solomon Chiu [:schiu]]

Just rebased my code base and split my previous patch into 2 patches to make the porting/syncing works easier. The first one is mostly about changes of GonkBufferQueue, and is enough for camera functions.
Another one is relative minor changes(comparing to BufferQueue) for GonkNativeWindowClient, GonkNativeWindow, and GonkConsumerBase. Please kindly let me know if you still encounter any problem.

Comment 15

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

(In reply to Solomon Chiu [:schiu] from comment #14)
> Just rebased my code base and split my previous patch into 2 patches 

I see only one non-obsolete patch in this bug?  Any hints on where to find the other patch?

Comment 16

[Solomon Chiu [:schiu]]

Attachment: 0001-Changes-to-GonkBufferQueue-relative-files-correspond.patch

Comment 17

[Solomon Chiu [:schiu]]

Attachment: 0002-Changes-to-GonkNativeWindow-GonkNativeWindowClient-G.patch

Comment 18

[Solomon Chiu [:schiu]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #15)
> (In reply to Solomon Chiu [:schiu] from comment #14)
> > Just rebased my code base and split my previous patch into 2 patches 
> 
> I see only one non-obsolete patch in this bug?  Any hints on where to find
> the other patch?

Hi Michael,

Sorry for missed second patch, I just upload my patches again.

Comment 19

[Peter Chang[:pchang]]

Comment on attachment 8372609 [details] [diff] [review]
0001-Changes-to-GonkBufferQueue-relative-files-correspond.patch

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

::: widget/gonk/nativewindow/GonkConsumerBase.cpp
@@ +217,5 @@
>      }
>  }
>  
> +#if ANDROID_VERSION >= 19
> +typedef IGonkGraphicBufferConsumer::BufferItem BufferItem;

KK is using BufferQueue::BufferItem, why need to add above typedef

::: widget/gonk/nativewindow/GonkConsumerBase.h
@@ +39,3 @@
>          protected GonkBufferQueue::ConsumerListener {
> +#else
> +        protected ConsumerListener {

you can define the following to reduce the modification.

#if ANDROID_VERSION < 19
#typedef GonkBufferQueue::ConsumerListener ConsumerListener
#endif

@@ +233,3 @@
>      sp<GonkBufferQueue> mBufferQueue;
> +#else
> +    sp<GonkBufferQueue> mConsumer;

mConsumer comes from KK, I think it is good to sync mBufferQueue to mConsumer.

::: widget/gonk/nativewindow/moz.build
@@ +30,5 @@
> +    ]
> +elif CONFIG['ANDROID_VERSION'] in ('17', '18'):
> +    EXPORTS += [
> +        'GonkBufferQueueJB.h',
> +#        'GonkConsumerBaseJB.h',

redundant modification?

@@ +55,3 @@
>          SOURCES += [
> +            'GonkBufferQueueJB.cpp',
> +#            'GonkConsumerBaseJB.cpp',

redundant modification?

Comment 20

[Peter Chang[:pchang]]

Looks like patch 1 and patch 2 have similar modification scope, could you merge your patch to one?
I didn't see why you need to separate into two patches.

Comment 21

[Solomon Chiu [:schiu]]

Attachment: 0001-Bug-959505-Display-gonk-kk-Porting-GonkNativeWindowC.patch

Comment 22

[Sotaro Ikeda [:sotaro]]

schiu, can you create a patch as to rename the file like "GonkBufferQueue.cpp -> GonkBufferQueueJB.cpp"? Your patch remove the GonkBufferQueue.cpp and create GonkBufferQueueJB.cpp. This will lose hg commit information to the file. I did same mistake for ICS in the past :-( And can you create KK part patch based on android base soruce? It makes clear which part is changed for gonk. And this information will become very helpful when the problem happens.

It might be easier to split the patch to some parts.

Comment 23

[Solomon Chiu [:schiu]]

Attachment: k01.patch

Comment 24

[Solomon Chiu [:schiu]]

Attachment: k02.patch

Comment 25

[Solomon Chiu [:schiu]]

Attachment: k03.patch

Comment 26

[Solomon Chiu [:schiu]]

Attachment: (1/5) Start with adding KK's original files

Comment 27

[Solomon Chiu [:schiu]]

Attachment: (2/5) Rename KK's file to corrensponding Gonk version

Comment 28

[Solomon Chiu [:schiu]]

Attachment: Modify BufferQueue/ConsumerBase/NativeWindow/NativeWindowClient/GraphicBufferConsumer for Gonkk03.patch

Comment 29

[Solomon Chiu [:schiu]]

Attachment: (4/5) Rename GonkBufferQueue/GonkConsumerBase to JB version

Comment 30

[Solomon Chiu [:schiu]]

Attachment: k05.patch

Comment 31

[Solomon Chiu [:schiu]]

Attachment: Modify nativewindow relative files which affected by KK's change

Comment 32

[Solomon Chiu [:schiu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #22)
> schiu, can you create a patch as to rename the file like
> "GonkBufferQueue.cpp -> GonkBufferQueueJB.cpp"? Your patch remove the
> GonkBufferQueue.cpp and create GonkBufferQueueJB.cpp. This will lose hg
> commit information to the file. I did same mistake for ICS in the past :-(
> And can you create KK part patch based on android base soruce? It makes
> clear which part is changed for gonk. And this information will become very
> helpful when the problem happens.
> 
> It might be easier to split the patch to some parts.

Sotora,

Many thanks for your precious advice. I just split the previous patch into 5 patches.

Comment 33

[vijayanand.hongal]

In the last patch : Attachment #8374264 [details] [diff] "Modify nativewindow relative files which affected by KK's change"

Comment #11 and #12 needs to be addressed.

Issue is in FakeSurfaceComposer.h file, the two virtual methods are declared in #ifdef block using android_version 19.

+#if ANDROID_VERSION >= 19
+    virtual void destroyDisplay(const sp<android::IBinder>& display);
+    virtual status_t captureScreen(const sp<IBinder>& display, const sp<IGraphicBufferProducer>& producer,
+            uint32_t reqWidth, uint32_t reqHeight, uint32_t minLayerZ, uint32_t maxLayerZ);
+#endif


Whereas in FakeSurfaceComposer.cpp, the two methods are defined without using #ifdef block. This might cause build failures.

Comment 34

[Solomon Chiu [:schiu]]

Attachment: Modify nativewindow relative files which affected by KK's change

Comment 35

[Solomon Chiu [:schiu]]

(In reply to vijayanand.hongal from comment #33)
> In the last patch : Attachment #8374264 [details] [diff] [diff] "Modify
> nativewindow relative files which affected by KK's change"
> 
> Comment #11 and #12 needs to be addressed.
> 
> Issue is in FakeSurfaceComposer.h file, the two virtual methods are declared
> in #ifdef block using android_version 19.
> 
> +#if ANDROID_VERSION >= 19
> +    virtual void destroyDisplay(const sp<android::IBinder>& display);
> +    virtual status_t captureScreen(const sp<IBinder>& display, const
> sp<IGraphicBufferProducer>& producer,
> +            uint32_t reqWidth, uint32_t reqHeight, uint32_t minLayerZ,
> uint32_t maxLayerZ);
> +#endif
> 
> 
> Whereas in FakeSurfaceComposer.cpp, the two methods are defined without
> using #ifdef block. This might cause build failures.

Thanks, Vijayanand. Just upload a new patch and it includes your correction. I did missed a patch when I cherry-picked patches from git with hg.

Comment 36

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8374890 [details] [diff] [review]
Modify nativewindow relative files which affected by KK's change

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

The patch seems basically good. But I am not fan of adding '../../../frameworks/native/opengl/include' a lot of places. Can you remove the dependency to it? In JB, I removed the dependency to EGL and GL from BufferQueue to prevent it.

::: content/media/omx/Makefile.in
@@ +11,5 @@
>  		-I$(ANDROID_SOURCE)/frameworks/base/include/utils/ \
>  		-I$(ANDROID_SOURCE)/frameworks/base/include/media/ \
>  		-I$(ANDROID_SOURCE)/frameworks/base/include/media/stagefright/openmax \
>  		-I$(ANDROID_SOURCE)/frameworks/base/media/libstagefright/include/ \
> +                -I$(ANDROID_SOURCE)/frameworks/native/opengl/include \

We need to try to prevent to include OpenGL in top directly. Why is it necessary here? Other 'moz.build' files already add the include.

::: content/media/webrtc/moz.build
@@ +23,5 @@
>      SOURCES += [
>          'MediaEngineWebRTC.cpp',
>      ]
>      LOCAL_INCLUDES += [
> +        '../../../../frameworks/native/opengl/include',

It might be better to add a comment about why this include is necessary.

::: dom/camera/GonkCameraHwMgr.cpp
@@ +196,5 @@
>  #else
> +  mCamera->setPreviewTarget(mNativeWindow->getBufferQueue());
> +#endif
> +
> +#else

It seems better to flatten "#if " nesting with "#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 17"

::: dom/camera/moz.build
@@ +47,5 @@
>  
>  FAIL_ON_WARNINGS = True
>  
>  LOCAL_INCLUDES += [
> +    '../../../frameworks/native/opengl/include',

It might be better to add a comment about why this include is necessary.

::: dom/media/moz.build
@@ +7,5 @@
>  if CONFIG['MOZ_WEBRTC']:
>      DIRS += ['bridge']
>  
>      LOCAL_INCLUDES += [
> +        '../../../frameworks/native/opengl/include',

It might be better to add a comment about why this include is necessary.

::: widget/gonk/nativewindow/GonkBufferQueueJB.cpp
@@ +271,3 @@
>  status_t GonkBufferQueue::dequeueBuffer(int *outBuf, sp<Fence>* outFence,
>          uint32_t w, uint32_t h, uint32_t format, uint32_t usage) {
> +#endif

Is it necessary for JB? JB is until 18.

@@ +496,1 @@
>  

Is it necessary for JB? JB is until 18.

@@ +638,4 @@
>  status_t GonkBufferQueue::connect(int api, QueueBufferOutput* output) {
> +#else
> +status_t GonkBufferQueue::connect(const sp<IBinder>& token, int api, bool producerControlledByApp, QueueBufferOutput* output) {
> +#endif

Is it necessary for JB? JB is until 18.

::: widget/gonk/nativewindow/GonkBufferQueueJB.h
@@ +197,4 @@
>      virtual status_t dequeueBuffer(int *buf, sp<Fence>* fence,
>              uint32_t width, uint32_t height, uint32_t format, uint32_t usage);
> +#endif
> +

Is it necessary for JB? JB is until 18.

@@ +250,5 @@
>      virtual status_t connect(int api, QueueBufferOutput* output);
> +#else
> +    virtual status_t connect(const sp<IBinder>& token,
> +            int api, bool producerControlledByApp, QueueBufferOutput* output);
> +#endif

Is it necessary for JB? JB is until 18.

::: widget/gonk/nativewindow/Makefile.in
@@ +16,5 @@
>  include $(topsrcdir)/config/rules.mk
> +
> +CXXFLAGS += \
> +        -I$(ANDROID_SOURCE)/frameworks/native/opengl/include \
> +        $(NULL)

Is it possible to move the above to moz.build?

Comment 37

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8374254 [details] [diff] [review]
Modify BufferQueue/ConsumerBase/NativeWindow/NativeWindowClient/GraphicBufferConsumer for Gonkk03.patch

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

The patch is basically good! I have a question. In b2g, we do not need BnGonkGraphicBufferConsumer  and BpGonkGraphicBufferConsumer. Only IGonkGraphicBufferConsumer is necessary. Is there a reason to implement them?

::: widget/gonk/nativewindow/GonkBufferQueueKK.cpp
@@ +252,5 @@
>          return BAD_VALUE;
>      } else if (mSlots[slot].mBufferState != BufferSlot::DEQUEUED) {
> +        // XXX: I vaguely recall there was some reason this can be valid, but
> +        // for the life of me I can't recall under what circumstances that's
> +        // the case.

Is there a reason to add the above comment? Can you explain more about it?

@@ -997,4 @@
>          int buf, uint64_t frameNumber, EGLDisplay display,
>          EGLSyncKHR eglFence, const sp<Fence>& fence) {
> -    ATRACE_CALL();
> -    ATRACE_BUFFER_INDEX(buf);

Is there a reason to remove ATRACE_CALL()? Some ATRACE is removed but some other ATRACE is still present.

@@ -1128,5 @@
>      return NO_ERROR;
>  }
>  
> -status_t BufferQueue::setDefaultMaxBufferCount(int bufferCount) {
> -    ATRACE_CALL();

Is there a reason to remove ATRACE_CALL()?  Some ATRACE is removed but some other ATRACE is still present.

@@ -1145,5 @@
>      return NO_ERROR;
>  }
>  
> -status_t BufferQueue::setMaxAcquiredBufferCount(int maxAcquiredBuffers) {
> -    ATRACE_CALL();

Is there a reason to remove ATRACE_CALL()?

::: widget/gonk/nativewindow/GonkConsumerBaseKK.cpp
@@ -93,4 @@
>      CB_LOGV("freeBufferLocked: slotIndex=%d", slotIndex);
>      mSlots[slotIndex].mGraphicBuffer = 0;
>      mSlots[slotIndex].mFence = Fence::NO_FENCE;
> -    mSlots[slotIndex].mFrameNumber = 0;

Is there a reason to remove it?

@@ -184,5 @@
>      if (item->mGraphicBuffer != NULL) {
>          mSlots[item->mBuf].mGraphicBuffer = item->mGraphicBuffer;
>      }
>  
> -    mSlots[item->mBuf].mFrameNumber = item->mFrameNumber;

Is there a reason to remove it?

::: widget/gonk/nativewindow/GonkNativeWindowKK.h
@@ +169,5 @@
> +                // If the window doesn't exist any more, release the buffer
> +                // directly.
> +                ImageBridgeChild *ibc = ImageBridgeChild::GetSingleton();
> +                ibc->DeallocSurfaceDescriptorGralloc(mSurfaceDescriptor);
> +            } 

nit: white space.

::: widget/gonk/nativewindow/IGonkGraphicBufferConsumer.h
@@ +176,5 @@
>      // flag) and has dequeueBuffer() return WOULD_BLOCK instead.
>      //
>      // This can only be called before consumerConnect().
>      virtual status_t disableAsyncBuffer() = 0;
> +*/

Is there a reason to comment out?

Comment 38

[Solomon Chiu [:schiu]]

> ::: widget/gonk/nativewindow/GonkBufferQueueKK.cpp
> @@ +252,5 @@
> >          return BAD_VALUE;
> >      } else if (mSlots[slot].mBufferState != BufferSlot::DEQUEUED) {
> > +        // XXX: I vaguely recall there was some reason this can be valid, but
> > +        // for the life of me I can't recall under what circumstances that's
> > +        // the case.
> 
> Is there a reason to add the above comment? Can you explain more about it?
> 

Sotaro,

I will upload another patch according your advices later. For above comment, it was sync from previous version GonkBufferQueue.cpp, and I found the comment was introduced in the git commit: ee8da0c0af92c6e4cdd9a01c64a138ff7f3d743f, which is submit by you. I am not pretty sure should it be keep for historical reason or not... :(

Comment 39

[Sotaro Ikeda [:sotaro]]

(In reply to Solomon Chiu [:schiu] from comment #38)
> Sotaro,
> 
> I will upload another patch according your advices later. For above comment,
> it was sync from previous version GonkBufferQueue.cpp, and I found the
> comment was introduced in the git commit:
> ee8da0c0af92c6e4cdd9a01c64a138ff7f3d743f, which is submit by you. I am not
> pretty sure should it be keep for historical reason or not... :(

I recall the comment. It was already present in the original code I used in JB4.3.

Comment 40

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #39)
> 
> I recall the comment. It was already present in the original code I used in
> JB4.3.

http://androidxref.com/4.3_r2.1/xref/frameworks/native/libs/gui/BufferQueue.cpp#246

Comment 41

[Solomon Chiu [:schiu]]

Attachment: (3/5)Modify BufferQueue/ConsumerBase/NativeWindow/NativeWindowClient/GraphicBufferConsumer for Gonk

Comment 42

[Solomon Chiu [:schiu]]

Attachment: (5/5) Modify nativewindow relative files which affected by KK's change

Comment 43

[Solomon Chiu [:schiu]]

I just upload the revised patch(3/5 and 5/5). Please see my comment below:

(In reply to Sotaro Ikeda [:sotaro] from comment #36)
> Comment on attachment 8374890 [details] [diff] [review]
> Modify nativewindow relative files which affected by KK's change
> 
> Review of attachment 8374890 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch seems basically good. But I am not fan of adding
> '../../../frameworks/native/opengl/include' a lot of places. Can you remove
> the dependency to it? In JB, I removed the dependency to EGL and GL from
> BufferQueue to prevent it.
> 

I removed the GL/EGL relative dependency as the way in Gonk JB. So all the including path in moz.build and Makefile.in I added before can be removed now.


> ::: content/media/omx/Makefile.in
> @@ +11,5 @@
> >  		-I$(ANDROID_SOURCE)/frameworks/base/include/utils/ \
> >  		-I$(ANDROID_SOURCE)/frameworks/base/include/media/ \
> >  		-I$(ANDROID_SOURCE)/frameworks/base/include/media/stagefright/openmax \
> >  		-I$(ANDROID_SOURCE)/frameworks/base/media/libstagefright/include/ \
> > +                -I$(ANDROID_SOURCE)/frameworks/native/opengl/include \
> 
> We need to try to prevent to include OpenGL in top directly. Why is it
> necessary here? Other 'moz.build' files already add the include.
> 
removed.

> ::: content/media/webrtc/moz.build
> @@ +23,5 @@
> >      SOURCES += [
> >          'MediaEngineWebRTC.cpp',
> >      ]
> >      LOCAL_INCLUDES += [
> > +        '../../../../frameworks/native/opengl/include',
> 
> It might be better to add a comment about why this include is necessary.
> 
removed.

> ::: dom/camera/GonkCameraHwMgr.cpp
> @@ +196,5 @@
> >  #else
> > +  mCamera->setPreviewTarget(mNativeWindow->getBufferQueue());
> > +#endif
> > +
> > +#else
> 
> It seems better to flatten "#if " nesting with "#if defined(MOZ_WIDGET_GONK)
> && ANDROID_VERSION >= 17"
> 
Flattened, please see patch 5/5.

> ::: dom/camera/moz.build
> @@ +47,5 @@
> >  
> >  FAIL_ON_WARNINGS = True
> >  
> >  LOCAL_INCLUDES += [
> > +    '../../../frameworks/native/opengl/include',
> 
> It might be better to add a comment about why this include is necessary.
> 

removed.

> ::: dom/media/moz.build
> @@ +7,5 @@
> >  if CONFIG['MOZ_WEBRTC']:
> >      DIRS += ['bridge']
> >  
> >      LOCAL_INCLUDES += [
> > +        '../../../frameworks/native/opengl/include',
> 
> It might be better to add a comment about why this include is necessary.
> 
removed.

> ::: widget/gonk/nativewindow/GonkBufferQueueJB.cpp
> @@ +271,3 @@
> >  status_t GonkBufferQueue::dequeueBuffer(int *outBuf, sp<Fence>* outFence,
> >          uint32_t w, uint32_t h, uint32_t format, uint32_t usage) {
> > +#endif
> 
> Is it necessary for JB? JB is until 18.
> 
Revised, please check patch 5/5.

> @@ +496,1 @@
> >  
> 
> Is it necessary for JB? JB is until 18.
> 
Revised.

> @@ +638,4 @@
> >  status_t GonkBufferQueue::connect(int api, QueueBufferOutput* output) {
> > +#else
> > +status_t GonkBufferQueue::connect(const sp<IBinder>& token, int api, bool producerControlledByApp, QueueBufferOutput* output) {
> > +#endif
> 
> Is it necessary for JB? JB is until 18.
> 
Revised.

> ::: widget/gonk/nativewindow/GonkBufferQueueJB.h
> @@ +197,4 @@
> >      virtual status_t dequeueBuffer(int *buf, sp<Fence>* fence,
> >              uint32_t width, uint32_t height, uint32_t format, uint32_t usage);
> > +#endif
> > +
> 
> Is it necessary for JB? JB is until 18.
> 
Revised.


> @@ +250,5 @@
> >      virtual status_t connect(int api, QueueBufferOutput* output);
> > +#else
> > +    virtual status_t connect(const sp<IBinder>& token,
> > +            int api, bool producerControlledByApp, QueueBufferOutput* output);
> > +#endif
> 
> Is it necessary for JB? JB is until 18.
> 
Revised.

> ::: widget/gonk/nativewindow/Makefile.in
> @@ +16,5 @@
> >  include $(topsrcdir)/config/rules.mk
> > +
> > +CXXFLAGS += \
> > +        -I$(ANDROID_SOURCE)/frameworks/native/opengl/include \
> > +        $(NULL)
> 
> Is it possible to move the above to moz.build?

Removed.

Comment 44

[Solomon Chiu [:schiu]]

Sotaro, the patch 5/5 was also revised according your advice. Please see my comment below:

(In reply to Sotaro Ikeda [:sotaro] from comment #37)
> Comment on attachment 8374254 [details] [diff] [review]
> Modify
> BufferQueue/ConsumerBase/NativeWindow/NativeWindowClient/
> GraphicBufferConsumer for Gonkk03.patch
> 
> Review of attachment 8374254 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch is basically good! I have a question. In b2g, we do not need
> BnGonkGraphicBufferConsumer  and BpGonkGraphicBufferConsumer. Only
> IGonkGraphicBufferConsumer is necessary. Is there a reason to implement them?
> 

GonkBufferQueue is derived from BnGonkGraphicBufferConsumer according to KK's refactoring changes, so we have to keep it. And I tried to remove BpGonkGraphicBufferConsumer, and unfortunately found "IMPLEMENT_META_INTERFACE(GonkGraphicBufferConsumer, "android.gui.IGonkGraphicBufferConsumer");" also need BpGonkGraphicBufferConsumer (http://androidxref.com/4.4_r1/xref/frameworks/native/include/binder/IInterface.h#98)...

> ::: widget/gonk/nativewindow/GonkBufferQueueKK.cpp
> @@ +252,5 @@
> >          return BAD_VALUE;
> >      } else if (mSlots[slot].mBufferState != BufferSlot::DEQUEUED) {
> > +        // XXX: I vaguely recall there was some reason this can be valid, but
> > +        // for the life of me I can't recall under what circumstances that's
> > +        // the case.
> 
> Is there a reason to add the above comment? Can you explain more about it?
> 
discussed in comment #38 ~ #40

> @@ -997,4 @@
> >          int buf, uint64_t frameNumber, EGLDisplay display,
> >          EGLSyncKHR eglFence, const sp<Fence>& fence) {
> > -    ATRACE_CALL();
> > -    ATRACE_BUFFER_INDEX(buf);
> 
> Is there a reason to remove ATRACE_CALL()? Some ATRACE is removed but some
> other ATRACE is still present.
> 
I thought we don't need it before. However we can use systrace now, so I put ATRACE_CALL() back.

> @@ -1128,5 @@
> >      return NO_ERROR;
> >  }
> >  
> > -status_t BufferQueue::setDefaultMaxBufferCount(int bufferCount) {
> > -    ATRACE_CALL();
> 
cancled.

> Is there a reason to remove ATRACE_CALL()?  Some ATRACE is removed but some
> other ATRACE is still present.
> 
> @@ -1145,5 @@
> >      return NO_ERROR;
> >  }
> >  
> > -status_t BufferQueue::setMaxAcquiredBufferCount(int maxAcquiredBuffers) {
> > -    ATRACE_CALL();
> 
> Is there a reason to remove ATRACE_CALL()?
> 
canceled.

> ::: widget/gonk/nativewindow/GonkConsumerBaseKK.cpp
> @@ -93,4 @@
> >      CB_LOGV("freeBufferLocked: slotIndex=%d", slotIndex);
> >      mSlots[slotIndex].mGraphicBuffer = 0;
> >      mSlots[slotIndex].mFence = Fence::NO_FENCE;
> > -    mSlots[slotIndex].mFrameNumber = 0;
> 
> Is there a reason to remove it?
> 
I put it back. It was removed during my debugging.

> @@ -184,5 @@
> >      if (item->mGraphicBuffer != NULL) {
> >          mSlots[item->mBuf].mGraphicBuffer = item->mGraphicBuffer;
> >      }
> >  
> > -    mSlots[item->mBuf].mFrameNumber = item->mFrameNumber;
> 
> Is there a reason to remove it?
> 
canceled.

> ::: widget/gonk/nativewindow/GonkNativeWindowKK.h
> @@ +169,5 @@
> > +                // If the window doesn't exist any more, release the buffer
> > +                // directly.
> > +                ImageBridgeChild *ibc = ImageBridgeChild::GetSingleton();
> > +                ibc->DeallocSurfaceDescriptorGralloc(mSurfaceDescriptor);
> > +            } 
> 
> nit: white space.
> 
revised.

> ::: widget/gonk/nativewindow/IGonkGraphicBufferConsumer.h
> @@ +176,5 @@
> >      // flag) and has dequeueBuffer() return WOULD_BLOCK instead.
> >      //
> >      // This can only be called before consumerConnect().
> >      virtual status_t disableAsyncBuffer() = 0;
> > +*/
> 
> Is there a reason to comment out?

I put it back. It was removed during my debugging.

Comment 45

[Solomon Chiu [:schiu]]

Attachment: (5/5) Modify nativewindow relative files which affected by KK's change

Comment 46

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8376498 [details] [diff] [review]
(3/5)Modify BufferQueue/ConsumerBase/NativeWindow/NativeWindowClient/GraphicBufferConsumer for Gonk

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

Looks good. review+ by answering the questions.

::: widget/gonk/nativewindow/GonkBufferQueueKK.cpp
@@ -983,5 @@
> -    // buffer on the consumer side.
> -    if (buffer->mAcquireCalled) {
> -        buffer->mGraphicBuffer = NULL;
> -    }
> -

It might be better to keep this code as commented out to make clear the change.

@@ -1009,5 @@
> -    // we can ignore this releaseBuffer for the old buffer.
> -    if (frameNumber != mSlots[buf].mFrameNumber) {
> -        return STALE_BUFFER_SLOT;
> -    }
> -

It might be better to keep this code as commented out to make clear the change.

::: widget/gonk/nativewindow/GonkNativeWindowClientKK.cpp
@@ -540,2 @@
>  {
> -    ATRACE_CALL();

Is there a reason to remove ATRACE?

::: widget/gonk/nativewindow/IGonkGraphicBufferConsumer.cpp
@@ +438,5 @@
>              status_t result = disableAsyncBuffer();
>              reply->writeInt32(result);
>              return NO_ERROR;
>          } break;
> +*/

Is there a reason to comment out?

@@ +482,4 @@
>              reply->writeString8(result);
>              return NO_ERROR;
>          }
> +*/

Is there a reason to comment out?

Comment 47

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8376566 [details] [diff] [review]
(5/5) Modify nativewindow relative files which affected by KK's change

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

LGTM. review+ by applying the comment. "configure.in" part needs build peer's review.

::: dom/camera/GonkCameraHwMgr.cpp
@@ +200,5 @@
> +
> +#else
> +  mCamera->setPreviewTexture(mNativeWindow);
> +#endif
> +*/

Is it forget to remove?

::: dom/camera/moz.build
@@ +47,5 @@
>  
>  FAIL_ON_WARNINGS = True
>  
>  LOCAL_INCLUDES += [
> +    '../../../frameworks/native/opengl/include',

Can't we remove this include?

::: dom/media/moz.build
@@ +7,5 @@
>  if CONFIG['MOZ_WEBRTC']:
>      DIRS += ['bridge']
>  
>      LOCAL_INCLUDES += [
> +        '../../../frameworks/native/opengl/include',

Can't we remove this include?

Comment 48

[Solomon Chiu [:schiu]]

Attachment: (3/5)Modify BufferQueue/ConsumerBase/NativeWindow/NativeWindowClient/GraphicBufferConsumer for Gonk

Comment 49

[Solomon Chiu [:schiu]]

Attachment: (5/5) Modify nativewindow relative files which affected by KK's change

Comment 50

[Solomon Chiu [:schiu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #46)
> Comment on attachment 8376498 [details] [diff] [review]
> (3/5)Modify
> BufferQueue/ConsumerBase/NativeWindow/NativeWindowClient/
> GraphicBufferConsumer for Gonk
> 
> Review of attachment 8376498 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. review+ by answering the questions.
> 
> ::: widget/gonk/nativewindow/GonkBufferQueueKK.cpp
> @@ -983,5 @@
> > -    // buffer on the consumer side.
> > -    if (buffer->mAcquireCalled) {
> > -        buffer->mGraphicBuffer = NULL;
> > -    }
> > -
> 
> It might be better to keep this code as commented out to make clear the
> change.
> 
done in patch 3/5

> @@ -1009,5 @@
> > -    // we can ignore this releaseBuffer for the old buffer.
> > -    if (frameNumber != mSlots[buf].mFrameNumber) {
> > -        return STALE_BUFFER_SLOT;
> > -    }
> > -
> 
> It might be better to keep this code as commented out to make clear the
> change.
> 
done in patch 3/5

> ::: widget/gonk/nativewindow/GonkNativeWindowClientKK.cpp
> @@ -540,2 @@
> >  {
> > -    ATRACE_CALL();
> 
> Is there a reason to remove ATRACE?
> 
I missed it in last patch, put it back in latest patch.

> ::: widget/gonk/nativewindow/IGonkGraphicBufferConsumer.cpp
> @@ +438,5 @@
> >              status_t result = disableAsyncBuffer();
> >              reply->writeInt32(result);
> >              return NO_ERROR;
> >          } break;
> > +*/
> 
uncommented in patch 3/5.

> Is there a reason to comment out?
> 
> @@ +482,4 @@
> >              reply->writeString8(result);
> >              return NO_ERROR;
> >          }
> > +*/
> 
> Is there a reason to comment out?
uncommented in patch 3/5.

Comment 51

[Solomon Chiu [:schiu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #47)
> Comment on attachment 8376566 [details] [diff] [review]
> (5/5) Modify nativewindow relative files which affected by KK's change
> 
> Review of attachment 8376566 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM. review+ by applying the comment. "configure.in" part needs build
> peer's review.
> 
> ::: dom/camera/GonkCameraHwMgr.cpp
> @@ +200,5 @@
> > +
> > +#else
> > +  mCamera->setPreviewTexture(mNativeWindow);
> > +#endif
> > +*/
> 
> Is it forget to remove?
> 
removed in latest patch 5/5

> ::: dom/camera/moz.build
> @@ +47,5 @@
> >  
> >  FAIL_ON_WARNINGS = True
> >  
> >  LOCAL_INCLUDES += [
> > +    '../../../frameworks/native/opengl/include',
> 
> Can't we remove this include?
> 
removed.

> ::: dom/media/moz.build
> @@ +7,5 @@
> >  if CONFIG['MOZ_WEBRTC']:
> >      DIRS += ['bridge']
> >  
> >      LOCAL_INCLUDES += [
> > +        '../../../frameworks/native/opengl/include',
> 
> Can't we remove this include?

removed.

Many thanks for your comment, Sotaro.

Comment 52

[Sotaro Ikeda [:sotaro]]

As in Comment 47, "configure.in" part needs build peer's review.

Comment 53

[Solomon Chiu [:schiu]]

Fabrice, would you please help to review the file configure.in in patch 5/5?
Thanks.

Comment 54

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

Attachment: Front-facing camera app crash dump

With the current patches here the back-facing camera works great for preview and still capture, but I get the attached crash in the camera app when switching to the front-facing camera.

Comment 55

[Vincent Liu[:vliu]]

Attachment: msg-aDOMCameraControl.txt

I can reproduce the issue. From the gdb tracking, it fails on a callback function.

Breakpoint 1, mozilla::DOMCameraControlListener::Callback::RunCallback (this=0xb2b5f880, aDOMCameraControl=0xb33a1460) at ../../../gecko/dom/camera/DOMCameraControlListener.cpp:306
306	      switch (mError) {

The message in mError shows 

(gdb) p mError
$1 = mozilla::CameraControlListener::kErrorSetThumbnailSizeFailed

I also attach a file for the message of aDOMCameraControl.

Hi Michael,

Does the kErrorSetThumbnailSizeFailed can be a clue to find out the problem from the driver side?

Comment 56

[Solomon Chiu [:schiu]]

Try server result:
https://tbpl.mozilla.org/?tree=Try&rev=aa1a22b58460

Comment 57

[Solomon Chiu [:schiu]]

Michael, would you please help to review the file "configure.in" in patch 5/5?
Thanks.

Comment 58

[Solomon Chiu [:schiu]]

I also can reproduce the problem that Michael reported. Just filed another bug to track the front-facing camera crash issue in bug#974919.

Comment 59

[Solomon Chiu [:schiu]]

Attachment: (1/6) Start with adding KK's original files

Comment 60

[Solomon Chiu [:schiu]]

Attachment: (2/6) Rename KK's file to corrensponding Gonk version

Comment 61

[Solomon Chiu [:schiu]]

Attachment: (3/6)Modify BufferQueue/ConsumerBase/NativeWindow/NativeWindowClient/GraphicBufferConsumer for Gonk

Comment 62

[Solomon Chiu [:schiu]]

Attachment: (4/6) Rename GonkBufferQueue/GonkConsumerBase to JB version

Comment 63

[Solomon Chiu [:schiu]]

Attachment: (5/5) Modify nativewindow relative files which affected by KK's change

Comment 64

[Solomon Chiu [:schiu]]

I regenerated the patches again with hg-export to have a better patch header.

Comment 65

[[:fabrice] Fabrice Desré]

Comment on attachment 8379450 [details] [diff] [review]
(5/5) Modify nativewindow relative files which affected by KK's change

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

I don't know much about this code, handing off to mwu.

Comment 66

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

I think front camera crash is not related to this bug. Please see Bug 970557 for more information.

Comment 67

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

vliu,
I think the kErrorSetThumbnailSizeFailed can be fixed by bug 975472.

Comment 68

[Vincent Liu[:vliu]]

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #67)
> vliu,
> I think the kErrorSetThumbnailSizeFailed can be fixed by bug 975472.

Yes, I confirmed that the patch of bug 975472 solves the crash issue. Thanks for your great info.

Comment 69

[Solomon Chiu [:schiu]]

Mike, would you please help to review the file "configure.in" in patch 5/5?
Thanks.

Comment 70

[[:fabrice] Fabrice Desré]

Solomon, are the 4 first parts ready to be reviewed?

Comment 71

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

(In reply to Fabrice Desré [:fabrice] from comment #70)
> Solomon, are the 4 first parts ready to be reviewed?

Fabrice, all parts are reviewed only part of patch 5/5 need to be reviewed by module peer per comment 47.

Comment 72

[[:fabrice] Fabrice Desré]

(In reply to Kai-Zhen Li from comment #71)
> (In reply to Fabrice Desré [:fabrice] from comment #70)
> > Solomon, are the 4 first parts ready to be reviewed?
> 
> Fabrice, all parts are reviewed only part of patch 5/5 need to be reviewed
> by module peer per comment 47.

Then someone needs to set the appropriate flags on the first 4 patches, because they are not marked as r+.

Comment 73

[Solomon Chiu [:schiu]]

Carry r+ for 5 patches.

Comment 74

[Mike Hommey [:glandium]]

Comment on attachment 8379450 [details] [diff] [review]
(5/5) Modify nativewindow relative files which affected by KK's change

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

I defer to mwu

Comment 75

[Solomon Chiu [:schiu]]

Attachment: (5/6) Modify nativewindow relative files which affected by KK's change

split changes of configure.in to patch-6, rebase to m+c, and carry r+.

Comment 76

[Solomon Chiu [:schiu]]

Attachment: (6/6) Add camera and OMX support in gecko's configure.in

Comment 77

[Solomon Chiu [:schiu]]

Try server result:
https://tbpl.mozilla.org/?tree=Try&rev=446c9179d5f1

Comment 78

[Michael Wu [:mwu]]

Comment on attachment 8381980 [details] [diff] [review]
(6/6) Add camera and OMX support in gecko's configure.in

Hmm not a fan of sticking all the includes in GONK_INCLUDES again. Oh well.

Do we need AC_SUBST(MOZ_OMX_DECODER)? If not, we should probably remove it from the version 18 section too.

Comment 80

[Solomon Chiu [:schiu]]

Kai Zhen,
As we discussed earlier, would you please help to provide your opinion?
Thanks.

Comment 81

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

Solomon, 

The follow two flags were disabled due to compile error. Now they can get compiled properly. Can you help to include them in you patch 5/5?
Thanks!

MOZ_OMX_ENCODER=1
MOZ_RTSP=1

Comment 82

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

I think it could be better to submit them in a seperate bug, restore the keyword.

Comment 83

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c762455e8f3d
https://hg.mozilla.org/integration/b2g-inbound/rev/fb68ede267b9
https://hg.mozilla.org/integration/b2g-inbound/rev/9756667a9645
https://hg.mozilla.org/integration/b2g-inbound/rev/6009d477682c
https://hg.mozilla.org/integration/b2g-inbound/rev/53b27959f2b4
https://hg.mozilla.org/integration/b2g-inbound/rev/d4d4303a616e

For future patches, please add a space between your name and email address in your commit information. :)

Comment 84

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c762455e8f3d
https://hg.mozilla.org/mozilla-central/rev/fb68ede267b9
https://hg.mozilla.org/mozilla-central/rev/9756667a9645
https://hg.mozilla.org/mozilla-central/rev/6009d477682c
https://hg.mozilla.org/mozilla-central/rev/53b27959f2b4
https://hg.mozilla.org/mozilla-central/rev/d4d4303a616e