[lollipop] Porting NativeWindow from LL to gonk

RESOLVED FIXED in 2.2 S3 (9jan)

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: pchang, Assigned: boris)

Tracking

unspecified
2.2 S3 (9jan)
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.2+)

Details

Attachments

(8 attachments, 38 obsolete attachments)

82.96 KB, text/plain
Details
174.14 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
5.85 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
50.79 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
6.39 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
3.31 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
956 bytes, patch
sotaro
: review+
mwu
: review+
Details | Diff | Splinter Review
252.25 KB, patch
boris
: review+
Details | Diff | Splinter Review
Reporter

Description

5 years ago
Create this bug to upgrade the NativeWindow from LL to gonk
Reporter

Comment 1

5 years ago
Boris, please evaluate the difference of NativeWindow between LL and KK
Assignee: nobody → bchiou
Flags: needinfo?(boris.chiou)
Blocks: gonk-L
Comment hidden (obsolete)
Comment hidden (obsolete)

Updated

5 years ago
Assignee: bchiou → nobody
Assignee

Comment 4

5 years ago
Attachment #8523645 - Attachment is obsolete: true
Assignee

Comment 5

5 years ago
Attachment #8523644 - Attachment is obsolete: true
Comment hidden (obsolete)
Comment hidden (obsolete)
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
Assignee

Comment 9

5 years ago
(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.
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Assignee

Updated

5 years ago
Depends on: 959505
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Assignee

Updated

5 years ago
Depends on: 1105452
Reporter

Updated

5 years ago
Assignee: nobody → boris.chiou
Reporter

Updated

5 years ago
feature-b2g: --- → 2.2?
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Assignee

Comment 28

5 years ago
(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)
Assignee

Comment 30

5 years ago
Posted file link_errors.txt (obsolete) —
Assignee

Comment 31

5 years ago
(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)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Assignee

Updated

5 years ago
Attachment #8536976 - Flags: review?(mwu)
Assignee

Updated

5 years ago
Attachment #8536978 - Flags: review?(mwu)
Assignee

Updated

5 years ago
Attachment #8536979 - Flags: review?(mwu)
Assignee

Updated

5 years ago
Attachment #8536980 - Flags: review?(mwu)
Comment hidden (obsolete)
Assignee

Updated

5 years ago
Attachment #8537030 - Flags: review?(mwu)
Comment hidden (obsolete)
Assignee

Updated

5 years ago
Attachment #8537032 - Flags: review?(mwu)
Assignee

Updated

5 years ago
Attachment #8536980 - Flags: review?(mwu)
Assignee

Updated

5 years ago
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)

Updated

5 years ago
Attachment #8536980 - Flags: review?(mwu) → review?(sotaro.ikeda.g)

Updated

5 years ago
Attachment #8537030 - Flags: review?(mwu) → review?(sotaro.ikeda.g)

Updated

5 years ago
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)
Assignee

Comment 41

5 years ago
(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+
Assignee

Updated

5 years ago
Attachment #8536408 - Flags: feedback?(sotaro.ikeda.g)
Assignee

Comment 43

5 years ago
Attachment #8536919 - Attachment is obsolete: true
Assignee

Comment 44

5 years ago
(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.
Comment hidden (obsolete)
Comment hidden (obsolete)
Assignee

Comment 47

5 years ago
Use the NO_VISIBILITY_FLAGS flag to fix the hidden symbol visibility and
some warnings.
Attachment #8537743 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8537748 - Flags: review?(sotaro.ikeda.g)
Assignee

Updated

5 years ago
Attachment #8536982 - Flags: review?(sotaro.ikeda.g)
Assignee

Comment 48

5 years ago
(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)
Assignee

Comment 51

5 years ago
Copied from aosp/framework/native/gui.
Attachment #8536976 - Attachment is obsolete: true
Assignee

Comment 52

5 years ago
Add the "Gonk" prefix & "LL" suffix, and collect all BufferQueue
related files on Lollipop in one directory.
Attachment #8537030 - Attachment is obsolete: true
Assignee

Comment 53

5 years ago
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)
Assignee

Comment 54

5 years ago
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)
Assignee

Comment 55

5 years ago
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)
Assignee

Comment 56

5 years ago
Use new APIs for GonkNativeWindowLL.
Attachment #8536980 - Attachment is obsolete: true
Attachment #8536980 - Flags: review?(sotaro.ikeda.g)
Assignee

Comment 57

5 years ago
Attachment #8536982 - Attachment is obsolete: true
Attachment #8536982 - Flags: review?(sotaro.ikeda.g)
Assignee

Comment 58

5 years ago
(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.
Assignee

Comment 59

5 years ago
(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
Assignee

Updated

5 years ago
Attachment #8538224 - Flags: review?(sotaro.ikeda.g)
Assignee

Updated

5 years ago
Attachment #8538299 - Flags: review?(sotaro.ikeda.g)
Assignee

Updated

5 years ago
Attachment #8538301 - Flags: review?(sotaro.ikeda.g)
Assignee

Updated

5 years ago
Attachment #8538302 - Flags: review?(sotaro.ikeda.g)
Assignee

Updated

5 years ago
Attachment #8538304 - Flags: review?(sotaro.ikeda.g)
Assignee

Updated

5 years ago
Attachment #8538305 - Flags: review?(sotaro.ikeda.g)
Assignee

Updated

5 years ago
Attachment #8538306 - Flags: review?(sotaro.ikeda.g)
Assignee

Updated

5 years ago
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)
Assignee

Comment 63

5 years ago
(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)
Assignee

Comment 64

5 years ago
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)
Assignee

Comment 65

5 years ago
Use the NO_VISIBILITY_FLAGS flag to fix the hidden symbol visibility and
some warnings.
Attachment #8538304 - Attachment is obsolete: true
Assignee

Comment 66

5 years ago
(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.
Assignee

Updated

5 years ago
Attachment #8538926 - Flags: review?(sotaro.ikeda.g)
Assignee

Updated

5 years ago
Attachment #8538927 - Flags: review?(sotaro.ikeda.g)
Assignee

Updated

5 years ago
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+
Assignee

Comment 67

5 years ago
rebased.
Attachment #8538306 - Attachment is obsolete: true
Assignee

Updated

5 years ago
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.
Assignee

Updated

5 years ago
Attachment #8539913 - Flags: review?(mwu)
Assignee

Comment 70

5 years ago
(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.
Assignee

Comment 71

5 years ago
(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.
Comment hidden (obsolete)
Assignee

Comment 73

5 years ago
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+
Assignee

Comment 74

5 years ago
(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

Updated

5 years ago
Attachment #8539913 - Flags: review?(mwu) → review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
Assignee

Updated

4 years ago
Depends on: 1141311
You need to log in before you can comment on or make changes to this bug.