Closed Bug 1098970 Opened 10 years ago Closed 10 years ago

[lollipop] Porting NativeWindow from LL to gonk

Categories

(Firefox OS Graveyard :: GonkIntegration, defect)

Product:
Firefox OS Graveyard
Component:
GonkIntegration
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(feature-b2g:2.2+)

Status:
RESOLVED FIXED
Milestone:
2.2 S3 (9jan)
Project Flags:
feature-b2g 2.2+

People

(Reporter: pchang, Assigned: boris)

References

Details

Attachments

(8 files, 38 obsolete files)

link_errors on gui/IProducerListener
82.96 KB, text/plain
Details
Part 1: Add new LL native files (v3)
174.14 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
Part 2: Rename files (v4)
5.85 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
Part 3: Add new files for Nativewindow (v1)
50.79 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
Part 6: Fix the usage of GonkNativeWindow in Omx (v4)
6.39 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
Part 5: Fix link errors casued by symbol visibility (v5)
3.31 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
Part 7: Turn on omx decoder (v3)
956 bytes, patch
sotaro
: review+
mwu
: review+
Details | Diff | Splinter Review
Part 4: Fix naming problems and buffer allocation (v7, carry sotaro's r+)
252.25 KB, patch
boris
: review+
Details | Diff | Splinter Review
Create this bug to upgrade the NativeWindow from LL to gonk
Boris, please evaluate the difference of NativeWindow between LL and KK
Assignee: nobody → bchiou
Flags: needinfo?(boris.chiou)
Blocks: gonk-L
Assignee: bchiou → nobody
Attachment #8523645 - Attachment is obsolete: true
Attached patch libhardware diff (b2g-l branch) (obsolete) — — Splinter Review
Attachment #8523644 - Attachment is obsolete: true
FYI, The following is Diagrams of ANativewWindow on Lollipop. https://github.com/sotaroikeda/android-diagrams/blob/master/graphics/ANativeWindow_5.0.pdf?raw=true Diagrams of ANativewWindow on JB and KK are in the following. https://github.com/sotaroikeda/android-diagrams/wiki/Android-Diagrams
(In reply to Sotaro Ikeda [:sotaro] from comment #8) > FYI, > > The following is Diagrams of ANativewWindow on Lollipop. > https://github.com/sotaroikeda/android-diagrams/blob/master/graphics/ > ANativeWindow_5.0.pdf?raw=true > > Diagrams of ANativewWindow on JB and KK are in the following. > https://github.com/sotaroikeda/android-diagrams/wiki/Android-Diagrams Thanks, Sotaro. It's a good reference to compare LL with KK.
Depends on: 959505
Depends on: 1105452
Assignee: nobody → boris.chiou
feature-b2g: --- → 2.2?
Blocks: gonk-L-Camera
(In reply to Boris Chiou [:boris] from comment #27) > Created attachment 8536408 [details] [diff] [review] > [WIP] Part 5: Fix link errors casued by symbol visibility. (v1) > > We got some link errors: > ex. > /prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.8/bin/../lib/gcc/arm-linux-androideabi/4.8/../../../../arm-linux-androideabi/bin/ld: error: /objdir-gecko/toolkit/library/../../widget/gonk/nativewindow/GonkBufferQueueProducer.o: requires unsupported dynamic reloc R_ARM_REL32; recompile with -fPIC > > /prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.8/bin/../lib/gcc/arm-linux-androideabi/4.8/../../../../arm-linux-androideabi/bin/ld: error: /objdir-gecko/toolkit/library/../../widget/gonk/nativewindow/GonkNativeWindowClientLL.o: requires unsupported dynamic reloc R_ARM_REL32; recompile with -fPIC > > /prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.8/bin/../lib/gcc/arm-linux-androideabi/4.8/../../../../arm-linux-androideabi/bin/ld: error: hidden symbol '_ZN7android5FenceD1Ev' is not defined locally > > Use the workaround to fix these problems. Hi, Michael After adding these files from aosp, we got some link errors which may be caused by the symbol visibility. So the part 5 (Attachment 8536408 [details] [diff]) use a workaround method to make it to be built successfully. Do you have any other possible good idea to fix this linking problem? Thank you.
Flags: needinfo?(mwu)
QA Whiteboard: [2.2-feature-qa+]
I need all of the errors to debug something like this.
Flags: needinfo?(mwu)
Attached file link_errors.txt (obsolete) —
(In reply to Michael Wu [:mwu] from comment #29) > I need all of the errors to debug something like this. OK, Please check attachment 8536919 [details]. I copy the linking part into the text file. Thanks.
Flags: needinfo?(mwu)
Attachment #8536976 - Flags: review?(mwu)
Attachment #8536978 - Flags: review?(mwu)
Attachment #8536979 - Flags: review?(mwu)
Attachment #8536980 - Flags: review?(mwu)
Attachment #8537030 - Flags: review?(mwu)
Attachment #8537032 - Flags: review?(mwu)
Attachment #8536980 - Flags: review?(mwu)
Attachment #8536980 - Flags: review?(mwu)
Comment on attachment 8536976 [details] [diff] [review] Part 1: Add new LL native files (v2) Sotaro owns this.
Attachment #8536976 - Flags: review?(mwu) → review?(sotaro.ikeda.g)
Attachment #8536980 - Flags: review?(mwu) → review?(sotaro.ikeda.g)
Attachment #8537030 - Flags: review?(mwu) → review?(sotaro.ikeda.g)
Attachment #8537032 - Flags: review?(mwu) → review?(sotaro.ikeda.g)
Boris Chiou, how can I try doing Lollipop build? Before doing review, I want to try the build.
Flags: needinfo?(boris.chiou)
(In reply to Sotaro Ikeda [:sotaro] from comment #40) > Boris Chiou, how can I try doing Lollipop build? Before doing review, I want > to try the build. Sure, just follow the steps: 1. git clone https://github.com/Seinlin/B2G -b b2g-5.0 2. cd B2G 3. BRANCH=b2g-l ./config.sh nexus-5-l 4. ./build.sh Note: After you run the build.sh, it will apply some temporary patches to avoid build break from "B2G/patches/all-hammerhead/gecko".
Flags: needinfo?(boris.chiou)
+ this one, based on the comment I got from Peter: "We should include this bug into 2.2 because it's part of work for L porting. And this patch is necessary to enable the camera/video for v 2.2 with L."
feature-b2g: 2.2? → 2.2+
Attachment #8536408 - Flags: feedback?(sotaro.ikeda.g)
Attachment #8536919 - Attachment is obsolete: true
(In reply to Boris Chiou [:boris] from comment #43) > Created attachment 8537692 [details] > link_errors on gui/IProducerListener It looks like we can not see the IProcuderListener in GonkNativeWindowClientLL.cpp.
Use the NO_VISIBILITY_FLAGS flag to fix the hidden symbol visibility and some warnings.
Attachment #8537743 - Attachment is obsolete: true
Attachment #8537748 - Flags: review?(sotaro.ikeda.g)
Attachment #8536982 - Flags: review?(sotaro.ikeda.g)
(In reply to Boris Chiou [:boris] from comment #47) > Created attachment 8537748 [details] [diff] [review] > Part 5: Fix link errors casued by symbol visibility. (v3) > > Use the NO_VISIBILITY_FLAGS flag to fix the hidden symbol visibility and > some warnings. Use NO_VISIBILITY_FLAGS to fix the link errors: Reference: Bug 1001320
Whiteboard: [POVB]
Whiteboard: [POVB]
Comment on attachment 8536976 [details] [diff] [review] Part 1: Add new LL native files (v2) Review of attachment 8536976 [details] [diff] [review]: ----------------------------------------------------------------- Files are added under widget/gonk/nativewindow/LL/ It seems not consistent with other patches like "/widget/gonk/nativewindow/GonkBufferQueueLL/". Can you update the patch as to be consistent? And this patch seems to add file that do not need moficiation for gonk. One example is "Surface.cpp". I want to prevent to add such files.
Attachment #8536976 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8537030 [details] [diff] [review] Part 2: Rename files (v3) Review of attachment 8537030 [details] [diff] [review]: ----------------------------------------------------------------- This patch remove current implementation and create new file. It lose git history info. Can you move/re-name a file as not to lose the git history? Bug 959505 Comment 22 is similar comment for KK.
Attachment #8537030 - Flags: review?(sotaro.ikeda.g)
Copied from aosp/framework/native/gui.
Attachment #8536976 - Attachment is obsolete: true
Add the "Gonk" prefix & "LL" suffix, and collect all BufferQueue related files on Lollipop in one directory.
Attachment #8537030 - Attachment is obsolete: true
1. Copied Surface.* and rename them as GonkNativeWindowClientLL.* 2. Copied BufferItemConsumer.* and rename them as GonkNativeWindowLL.* 3. Add new IGonkGraphicBufferConsumer.h to control the version 4. Fix the file mode for some JB files (remove execute permission)
1. Fix class naming by adding "Gonk" prefix, and rename Surface & BufferItemConsumer. 2. Revise the buffer allocation
Attachment #8537032 - Attachment is obsolete: true
Attachment #8537032 - Flags: review?(sotaro.ikeda.g)
Use the NO_VISIBILITY_FLAGS flag to fix the hidden symbol visibility and some warnings.
Attachment #8537748 - Attachment is obsolete: true
Attachment #8537748 - Flags: review?(sotaro.ikeda.g)
Use new APIs for GonkNativeWindowLL.
Attachment #8536980 - Attachment is obsolete: true
Attachment #8536980 - Flags: review?(sotaro.ikeda.g)
Attached patch Part 7: Turn on omx decoder (v2) (obsolete) — — Splinter Review
Attachment #8536982 - Attachment is obsolete: true
Attachment #8536982 - Flags: review?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro PTO Dec/24-Jan/6] from comment #49) > Comment on attachment 8536976 [details] [diff] [review] > Part 1: Add new LL native files (v2) > > Review of attachment 8536976 [details] [diff] [review]: > ----------------------------------------------------------------- > > Files are added under widget/gonk/nativewindow/LL/ It seems not consistent > with other patches like "/widget/gonk/nativewindow/GonkBufferQueueLL/". Can > you update the patch as to be consistent? > > And this patch seems to add file that do not need moficiation for gonk. One > example is "Surface.cpp". I want to prevent to add such files. OK, I copied into "/widget/gonk/nativewindow/GonkBufferQueueLL/" directly and didn't copy the Surface.* and BufferItemConsumer.* in this patch.
(In reply to Sotaro Ikeda [:sotaro PTO Dec/24-Jan/6] from comment #50) > Comment on attachment 8537030 [details] [diff] [review] > Part 2: Rename files (v3) > > Review of attachment 8537030 [details] [diff] [review]: > ----------------------------------------------------------------- > > This patch remove current implementation and create new file. It lose git > history info. Can you move/re-name a file as not to lose the git history? > Bug 959505 Comment 22 is similar comment for KK. How about the newer part 2? In part 2, I only "git mv" these files so you can see the git history. In conclusion: Part 1: only copy these files from aosp (except for Surface and BufferItemConsumer) Part 2: only rename these files in part 1 Part 3: copy Surface and BufferItemConsumer and rename their filenames as GonkNativeWindowClientLL and GonkNativeWindowLL Part 4: Fix class names and the allocator Part 5: Fix hidden symbol link errors Part 6: Use new APIs in the omx decoder Part 7: Turn on the omx decoder
Attachment #8538224 - Flags: review?(sotaro.ikeda.g)
Attachment #8538299 - Flags: review?(sotaro.ikeda.g)
Attachment #8538301 - Flags: review?(sotaro.ikeda.g)
Attachment #8538302 - Flags: review?(sotaro.ikeda.g)
Attachment #8538304 - Flags: review?(sotaro.ikeda.g)
Attachment #8538305 - Flags: review?(sotaro.ikeda.g)
Attachment #8538306 - Flags: review?(sotaro.ikeda.g)
Attachment #8538306 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8538304 [details] [diff] [review] Part 5: Fix link errors casued by symbol visibility (v4) Review of attachment 8538304 [details] [diff] [review]: ----------------------------------------------------------------- From Bug 961264 Comment 15, isn't "NO_VISIBILITY_FLAGS = True' enough to fix the link problem?
Attachment #8538304 - Flags: review?(sotaro.ikeda.g)
(In reply to Boris Chiou [:boris] from comment #59) > (In reply to Sotaro Ikeda [:sotaro PTO Dec/24-Jan/6] from comment #50) > > Comment on attachment 8537030 [details] [diff] [review] > > Part 2: Rename files (v3) > > > > Review of attachment 8537030 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > This patch remove current implementation and create new file. It lose git > > history info. Can you move/re-name a file as not to lose the git history? > > Bug 959505 Comment 22 is similar comment for KK. > > How about the newer part 2? In part 2, I only "git mv" these files so you > can see the git history. Thanks, it is good!
Boris, GonkNativeWindow is also used for Camera and WebRTC. Is there a bug for them?
Flags: needinfo?(boris.chiou)
(In reply to Sotaro Ikeda [:sotaro PTO Dec/24-Jan/6] from comment #62) > Boris, GonkNativeWindow is also used for Camera and WebRTC. Is there a bug > for them? Yes, for camera: Bug 1107300. They have to cherry-pick my patches to work. (We already verified these patches on camera.) Unfortunately, I don't know which bug is about WebRTC right now. Maybe we should create a new one.
Flags: needinfo?(boris.chiou)
1. Fix class naming by adding "Gonk" prefix, and rename Surface & BufferItemConsumer. 2. Revise the buffer allocation
Attachment #8538302 - Attachment is obsolete: true
Attachment #8538302 - Flags: review?(sotaro.ikeda.g)
Use the NO_VISIBILITY_FLAGS flag to fix the hidden symbol visibility and some warnings.
Attachment #8538304 - Attachment is obsolete: true
(In reply to Sotaro Ikeda [:sotaro PTO Dec/24-Jan/6] from comment #60) > Comment on attachment 8538304 [details] [diff] [review] > Part 5: Fix link errors casued by symbol visibility (v4) > > Review of attachment 8538304 [details] [diff] [review]: > ----------------------------------------------------------------- > > From Bug 961264 Comment 15, isn't "NO_VISIBILITY_FLAGS = True' enough to fix > the link problem? Yes, you are right. I uploaded a new patch to remove the redundant code. Thanks.
Attachment #8538926 - Flags: review?(sotaro.ikeda.g)
Attachment #8538927 - Flags: review?(sotaro.ikeda.g)
Status: NEW → ASSIGNED
Attachment #8538224 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8538299 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8538301 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8538927 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8538926 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8538305 - Flags: review?(sotaro.ikeda.g) → review+
rebased.
Attachment #8538306 - Attachment is obsolete: true
Attachment #8539913 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8539913 [details] [diff] [review] Part 7: Turn on omx decoder (v3) Review of attachment 8539913 [details] [diff] [review]: ----------------------------------------------------------------- Change to "configure.in" needs build peers review+. mwu seems correct person for it.
Attachment #8539913 - Flags: review?(sotaro.ikeda.g) → review+
And before check-in, please check if the change does not break other b2g(ICS,JB,KK) builds by using tryserver.
Attachment #8539913 - Flags: review?(mwu)
(In reply to Sotaro Ikeda [:sotaro PTO Dec/24-Jan/6] from comment #69) > And before check-in, please check if the change does not break other > b2g(ICS,JB,KK) builds by using tryserver. Sure, I will upload to try server to check this. Thanks.
(In reply to Sotaro Ikeda [:sotaro PTO Dec/24-Jan/6] from comment #68) > Comment on attachment 8539913 [details] [diff] [review] > Part 7: Turn on omx decoder (v3) > > Review of attachment 8539913 [details] [diff] [review]: > ----------------------------------------------------------------- > > Change to "configure.in" needs build peers review+. mwu seems correct person > for it. OK, I also send a review request to mwu. Thanks for your review.
1. Fix class naming by adding "Gonk" prefix, and rename Surface & BufferItemConsumer. 2. Revise the buffer allocation Fix JB build error
Attachment #8538926 - Attachment is obsolete: true
Attachment #8539938 - Flags: review+
(In reply to Boris Chiou [:boris] from comment #72) > (In reply to Boris Chiou [:boris] from comment #70) > > (In reply to Sotaro Ikeda [:sotaro PTO Dec/24-Jan/6] from comment #69) > > > And before check-in, please check if the change does not break other > > > b2g(ICS,JB,KK) builds by using tryserver. > > > > Sure, I will upload to try server to check this. Thanks. > > Try server > https://treeherder.mozilla.org/#/jobs?repo=try&revision=98191551229f try server https://treeherder.mozilla.org/#/jobs?repo=try&revision=22842fffcb40
Attachment #8539913 - Flags: review?(mwu) → review+
Keywords: checkin-needed
Depends on: 1141311
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: