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

RESOLVED FIXED in Firefox 30

Status

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: schiu, Assigned: schiu)

Tracking

unspecified
1.4 S2 (28feb)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.4+, firefox28 wontfix, firefox29 wontfix, firefox30 fixed, b2g-v1.4 fixed)

Details

(Whiteboard: [caf priority: p3][CR 608289])

Attachments

(8 attachments, 22 obsolete attachments)

125.07 KB, text/plain
Details
5.76 KB, text/plain
Details
162.66 KB, patch
schiu
: review+
Details | Diff | Splinter Review
2.46 KB, patch
schiu
: review+
Details | Diff | Splinter Review
155.10 KB, patch
schiu
: review+
Details | Diff | Splinter Review
1.15 KB, patch
schiu
: review+
Details | Diff | Splinter Review
33.75 KB, patch
schiu
: review+
Details | Diff | Splinter Review
1.18 KB, patch
mwu
: review+
Details | Diff | Splinter Review
Fix the interface change relative to KitKat in GonkNativeWindowClient, GonkBufferQueue, and FakeSurfaceComposer, which block the camera functions.
Posted patch GonkNativeWindow-KK.diff (obsolete) — Splinter Review
Attachment #8359644 - Flags: review?(sotaro.ikeda.g)
Attachment #8359644 - Flags: review?(pchang)
Attachment #8359644 - Flags: review?(mwu)
Is this enough to turn on omx decoding?
Please assign bugs to yourself when appropriate.
Assignee: nobody → schiu
Blocks: gonk-kk
Blocks: 946241
No longer blocks: gonk-kk
Blocks: gonk-kk
(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.
Shouldn't we have a KK version instead of conditionally patching the JB version?
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.
(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.
Note that the patch here can enable Camera and Video playing with OMXCodec on Nexus-4.
But can't enable them on Nexus-5.
Attachment #8359644 - Attachment is obsolete: true
Attachment #8359644 - Flags: review?(sotaro.ikeda.g)
Attachment #8359644 - Flags: review?(pchang)
Attachment #8359644 - Flags: review?(mwu)
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.
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.
(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;
 }
--
Flags: needinfo?(schiu)
Attachment #8365684 - Attachment is obsolete: true
Flags: needinfo?(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.
(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?
Flags: needinfo?(schiu)
Attachment #8371621 - Attachment is obsolete: true
Attachment #8372609 - Flags: review?(sotaro.ikeda.g)
Flags: needinfo?(schiu)
Attachment #8372610 - Flags: review?(sotaro.ikeda.g)
(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 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?
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.
Attachment #8372609 - Attachment is obsolete: true
Attachment #8372610 - Attachment is obsolete: true
Attachment #8372609 - Flags: review?(sotaro.ikeda.g)
Attachment #8372610 - Flags: review?(sotaro.ikeda.g)
Attachment #8373260 - Flags: review?(sotaro.ikeda.g)
Attachment #8373260 - Flags: review?(pchang)
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.
Posted patch k01.patch (obsolete) — Splinter Review
Attachment #8373260 - Attachment is obsolete: true
Attachment #8373260 - Flags: review?(sotaro.ikeda.g)
Attachment #8373260 - Flags: review?(pchang)
Attachment #8374242 - Flags: review?(sotaro.ikeda.g)
Posted patch k02.patch (obsolete) — Splinter Review
Attachment #8374243 - Flags: review?(sotaro.ikeda.g)
Posted patch k03.patch (obsolete) — Splinter Review
Attachment #8374246 - Flags: review?(sotaro.ikeda.g)
Attachment #8374242 - Attachment is obsolete: true
Attachment #8374243 - Attachment is obsolete: true
Attachment #8374246 - Attachment is obsolete: true
Attachment #8374242 - Flags: review?(sotaro.ikeda.g)
Attachment #8374243 - Flags: review?(sotaro.ikeda.g)
Attachment #8374246 - Flags: review?(sotaro.ikeda.g)
Attachment #8374251 - Flags: review?(sotaro.ikeda.g)
Attachment #8374253 - Flags: review?(sotaro.ikeda.g)
Attachment #8374255 - Flags: review?(sotaro.ikeda.g)
Posted patch k05.patch (obsolete) — Splinter Review
Attachment #8374259 - Flags: review?(sotaro.ikeda.g)
Attachment #8374259 - Attachment is obsolete: true
Attachment #8374259 - Flags: review?(sotaro.ikeda.g)
Attachment #8374264 - Flags: review?(sotaro.ikeda.g)
(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.
Attachment #8374251 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8374255 - Flags: review?(sotaro.ikeda.g) → review+
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.
Attachment #8374890 - Flags: review?(sotaro.ikeda.g)
Attachment #8374264 - Attachment is obsolete: true
Attachment #8374264 - Flags: review?(sotaro.ikeda.g)
(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.
Attachment #8374253 - Flags: review?(sotaro.ikeda.g) → review+
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 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?
> ::: 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... :(
(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.
(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
Attachment #8374254 - Attachment is obsolete: true
Attachment #8374254 - Flags: review?(sotaro.ikeda.g)
Attachment #8376498 - Flags: review?(sotaro.ikeda.g)
Attachment #8374251 - Attachment description: Start with adding KK's original files → (1/5) Start with adding KK's original files
Attachment #8374253 - Attachment description: Rename KK's file to corrensponding Gonk version → (2/5) Rename KK's file to corrensponding Gonk version
Attachment #8374255 - Attachment description: Rename GonkBufferQueue/GonkConsumerBase to JB version → (4/5) Rename GonkBufferQueue/GonkConsumerBase to JB version
Attachment #8374890 - Attachment is obsolete: true
Attachment #8374890 - Flags: review?(sotaro.ikeda.g)
Attachment #8376501 - Flags: review?(sotaro.ikeda.g)
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.
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.
Attachment #8376501 - Attachment is obsolete: true
Attachment #8376501 - Flags: review?(sotaro.ikeda.g)
Attachment #8376566 - Flags: review?(sotaro.ikeda.g)
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?
Attachment #8376498 - Flags: review?(sotaro.ikeda.g) → review+
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?
Attachment #8376566 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8376566 - Attachment is obsolete: true
Attachment #8377179 - Flags: review?(sotaro.ikeda.g)
(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.
(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.
Attachment #8377177 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8377179 - Flags: review?(sotaro.ikeda.g) → review+
As in Comment 47, "configure.in" part needs build peer's review.
blocking-b2g: --- → 1.4+
Attachment #8377179 - Flags: review?(fabrice)
Fabrice, would you please help to review the file configure.in in patch 5/5?
Thanks.
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.
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?
Attachment #8377179 - Flags: review?(mwu)
Michael, would you please help to review the file "configure.in" in patch 5/5?
Thanks.
I also can reproduce the problem that Michael reported. Just filed another bug to track the front-facing camera crash issue in bug#974919.
Attachment #8374251 - Attachment is obsolete: true
Attachment #8377179 - Attachment is obsolete: true
Attachment #8377179 - Flags: review?(mwu)
Attachment #8377179 - Flags: review?(fabrice)
Attachment #8379450 - Flags: review?(mwu)
Attachment #8379450 - Flags: review?(fabrice)
I regenerated the patches again with hg-export to have a better patch header.
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.
Attachment #8379450 - Flags: review?(fabrice)
I think front camera crash is not related to this bug. Please see Bug 970557 for more information.
Whiteboard: [CR 608289]
vliu,
I think the kErrorSetThumbnailSizeFailed can be fixed by bug 975472.
(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.
Target Milestone: --- → 1.4 S2 (28feb)
Blocks: 976427
Attachment #8379450 - Flags: review?(mh+mozilla)
Mike, would you please help to review the file "configure.in" in patch 5/5?
Thanks.
Solomon, are the 4 first parts ready to be reviewed?
Flags: needinfo?(schiu)
(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.
(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+.
Attachment #8379444 - Flags: review+
Flags: needinfo?(schiu)
Attachment #8379445 - Flags: review+
Attachment #8379446 - Flags: review+
Attachment #8379448 - Flags: review+
Attachment #8379450 - Flags: review+
Carry r+ for 5 patches.
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
Attachment #8379450 - Flags: review?(mh+mozilla)
split changes of configure.in to patch-6, rebase to m+c, and carry r+.
Attachment #8379450 - Attachment is obsolete: true
Attachment #8379450 - Flags: review?(mwu)
Attachment #8381978 - Flags: review+
Attachment #8381980 - Attachment description: k06.patch → (6/6) Add camera and OMX support in gecko's configure.in
Attachment #8379448 - Attachment description: (4/5) Rename GonkBufferQueue/GonkConsumerBase to JB version → (4/6) Rename GonkBufferQueue/GonkConsumerBase to JB version
Attachment #8379446 - Attachment description: (3/5)Modify BufferQueue/ConsumerBase/NativeWindow/NativeWindowClient/GraphicBufferConsumer for Gonk → (3/6)Modify BufferQueue/ConsumerBase/NativeWindow/NativeWindowClient/GraphicBufferConsumer for Gonk
Attachment #8379445 - Attachment description: (2/5) Rename KK's file to corrensponding Gonk version → (2/6) Rename KK's file to corrensponding Gonk version
Attachment #8379444 - Attachment description: (1/5) Start with adding KK's original files → (1/6) Start with adding KK's original files
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.
Attachment #8381980 - Flags: review?(mwu) → review+
Duplicate of this bug: 946241
Keywords: checkin-needed
Kai Zhen,
As we discussed earlier, would you please help to provide your opinion?
Thanks.
Flags: needinfo?(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
Flags: needinfo?(kli)
Keywords: checkin-needed
I think it could be better to submit them in a seperate bug, restore the keyword.
Keywords: checkin-needed
Whiteboard: [CR 608289] → [caf priority: p3][CR 608289]
Blocks: 1098970
You need to log in before you can comment on or make changes to this bug.