Gonk EGL rendering should use hardware composer API's instead of eglSwapBuffers

RESOLVED FIXED in mozilla18

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: romaxa, Assigned: mwu)

Tracking

Trunk
mozilla18
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 639784 [details] [diff] [review]
hwc compoer APIs usage

Some android vendors providing hw compositor module which include specific extra stuff which is need to be done for specific device.

http://androidxref.com/source/xref/hardware/libhardware/include/hardware/hwcomposer.h
http://androidxref.com/source/xref/hardware/libhardware/modules/hwcomposer/hwcomposer.cpp

for example some time default eglSwapBuffers call is not enough, and additional actions need to be performed for proper display refresh, like OMAPFB_WINDOW_UPDATE.
all that specific calls hidden by vendor behind hwcomposer.h apis
for example here is hwcomposer for N9 
http://gitorious.org/android-n900/vendor_nitdroid_n9/blobs/ics/hwcomposer/hwcomposer.cpp

we don't use surface flinger, and replacing surfaceFlinger, but surface flinger use hwcomposer.h APIS, so vendors rely on that...
probably we should apply patch below for gonk port
Attachment #639784 - Flags: feedback?(mwu)
Attachment #639784 - Flags: feedback?(jones.chris.g)
(Assignee)

Comment 1

6 years ago
Comment on attachment 639784 [details] [diff] [review]
hwc compoer APIs usage

The idea is right. We'll need something like this to support the pandaboard.

Two things -
1. As discussed on IRC, we need to also call prepare. I don't know if this needs to be called before every swap, or just once.
2. Gingerbread doesn't have hwc afaik, so we'd need to fallback to normal swaps in that case. The try server will tell you if your patch builds on GB.
Attachment #639784 - Flags: feedback?(mwu) → feedback+
(Assignee)

Updated

6 years ago
Blocks: 769167
(Assignee)

Comment 2

6 years ago
Created attachment 640360 [details] [diff] [review]
romaxa's patch with prepare call

Just a version of the hwc patch that adds a prepare call so it works on the pandaboard.
Comment on attachment 639784 [details] [diff] [review]
hwc compoer APIs usage

The bug here only affects the display framebuffer's GLContext, right?  Using this for swapping all GLContexts seems wrong, and in fact likely wouldn't work in content processes.

Along those lines, I don't like this living in gfx/gl ... is there a way to localize it to widget/gonk?
Attachment #639784 - Flags: feedback?(jones.chris.g)
(Reporter)

Comment 4

6 years ago
> Along those lines, I don't like this living in gfx/gl ... is there a way to
> localize it to widget/gonk?

then we need to expose eglSurface and eglDisplay to widget gonk...
also we don't need it for content process... it is only needed for process which does actually composition and holding top level eglSurface.
(Assignee)

Comment 5

6 years ago
This bug blocks bug 769167 which is blocking basecamp.
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
(Assignee)

Comment 6

6 years ago
Created attachment 649737 [details] [diff] [review]
romaxa's patch with prepare call and unbitrotted
Attachment #639784 - Attachment is obsolete: true
Attachment #640360 - Attachment is obsolete: true
Michael, does this still need to block Basecamp?  And would you be a good assignee?  Thanks.
Assignee: nobody → mwu
(Assignee)

Comment 8

6 years ago
(In reply to Andrew Overholt [:overholt] from comment #7)
> Michael, does this still need to block Basecamp?  And would you be a good
> assignee?  Thanks.

Depends on whether we still consider bug 769167 a blocker, which it appears to be.

I'm planning on cleaning this up and landing it next week.
(Assignee)

Comment 9

6 years ago
Looks like bug 769167 isn't a blocker any more.
blocking-basecamp: + → ?
(In reply to Michael Wu [:mwu] from comment #8)
> (In reply to Andrew Overholt [:overholt] from comment #7)
> > Michael, does this still need to block Basecamp?  And would you be a good
> > assignee?  Thanks.
> 
> Depends on whether we still consider bug 769167 a blocker, which it appears
> to be.

While we've removed the blocker flag, that's only since it's not a development blocker.  If that makes it unclear, feel free to re-nom it :)

> I'm planning on cleaning this up and landing it next week.

Great, thanks!
blocking-basecamp: ? → +
I wonder if Andrew meant to blocking+ this. I don't think it needs to block, though I will be happy to take this patch of course.
blocking-basecamp: + → ?
Correct: this doesn't technically block release, but is highest-not-blocking-priority because we need this to run tests on real HW.
blocking-basecamp: ? → -
Sorry, you're right.
(Assignee)

Comment 14

6 years ago
Created attachment 653518 [details] [diff] [review]
Use HWComposer instead of swapBuffer where appropriate

The ifdef for the swap buffers call is a little weird. I'm open to suggestions there.

Another issue is that hwcomposer is a ICS specific thing. This patch just assumes ICS, but I'll try to get it building on GB before landing.
Attachment #649737 - Attachment is obsolete: true
Attachment #653518 - Flags: review?(jones.chris.g)

Comment 15

6 years ago
You can ignore GB at this point.
(Reporter)

Comment 16

6 years ago
it is hard to ignore something that has public tinderbox builds...
can we just check for some ICS/GB specific define in headers and use that for compile time detection?

Comment 17

6 years ago
We will only ship ICS-kernel based builds, but we can stick in a define if you like for testing purposes.
Comment on attachment 653518 [details] [diff] [review]
Use HWComposer instead of swapBuffer where appropriate

Need to fall back on SwapBuffers() when we can't initialize the HAL.

Is HWC available on all our ICS builds?  I assume so, but want to confirm.
Attachment #653518 - Flags: review?(jones.chris.g)
(Assignee)

Comment 19

6 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #18)
> Comment on attachment 653518 [details] [diff] [review]
> Use HWComposer instead of swapBuffer where appropriate
> 
> Need to fall back on SwapBuffers() when we can't initialize the HAL.
> 
> Is HWC available on all our ICS builds?  I assume so, but want to confirm.

Yup. See http://androidxref.com/4.0.4/xref/hardware/libhardware/modules/hwcomposer/hwcomposer.cpp which is the default implementation for most devices that aren't omap.
(Assignee)

Updated

6 years ago
Attachment #653518 - Flags: review?(jones.chris.g)
Comment on attachment 653518 [details] [diff] [review]
Use HWComposer instead of swapBuffer where appropriate

>commit 981d5b41a9a0d1c3cba3544b2bd279ad7c108c89
>Author: Michael Wu <mwu@mozilla.com>
>Date:   Mon Aug 20 17:02:07 2012 -0400
>
>    Use HWComposer to swap buffers
>
>diff --git a/gfx/gl/GLContextProviderEGL.cpp b/gfx/gl/GLContextProviderEGL.cpp
>index f214192..e6cb1b8 100644
>--- a/gfx/gl/GLContextProviderEGL.cpp
>+++ b/gfx/gl/GLContextProviderEGL.cpp

>+        if (!aIsOffscreen)
>+            mHwc = new HWComposer();

Sorry, I think my first comment wasn't clear enough.  If initializing hwc fails, we'll just crash trying to use it below.
Attachment #653518 - Flags: review?(jones.chris.g)
(Assignee)

Comment 21

6 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #20)
> Sorry, I think my first comment wasn't clear enough.  If initializing hwc
> fails, we'll just crash trying to use it below.

Ah. Why not crash, though? If hwc init fails, something has gone horribly wrong anyway and a crash report would help us see it instead of letting things silently fail.
(Assignee)

Comment 22

6 years ago
Created attachment 655103 [details] [diff] [review]
Use HWComposer instead of swapBuffer where appropriate, v2
Attachment #653518 - Attachment is obsolete: true
Attachment #655103 - Flags: review?(jones.chris.g)
Attachment #655103 - Flags: review?(jones.chris.g) → review+
We'll need to land this before we get pandaboard automation going.
blocking-basecamp: - → ?
(Assignee)

Comment 24

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6075cc5dbe31
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/6075cc5dbe31
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
blocking-basecamp: ? → +
This broke B2G GB builds:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=B2G%20gb_armv7a_gecko%20mozilla-inbound%20build&rev=6075cc5dbe31

The push before was also busted, but that was caused by the now backed out bug 749518.

B2G on GB was hidden on try, inbound and m-c - when I came to this bug I had no idea why, but I'm now guessing because of:

(In reply to Andreas Gal :gal from comment #15)
> You can ignore GB at this point.

I've just spent a while tracking this down, after fx-team experienced B2G bustage (due to them not having GB B2G builds hidden). Please can we communicate the deliberate hiding of builds like this to sheriffs a bit more, just so we know what's going on & can make sure they get hidden across all trunk trees, and not just the main three :-)

Also, presuming we can stop running the GB builds now? Want me to file the bug for that?
(In reply to Ed Morley [:edmorley] from comment #26)
> more, just so we know what's going on & can make sure they get hidden across
> all trunk trees, and not just the main three :-)

Have also hidden on:
Fx-team, Ionmonkey, Services-Central, Build-System, Alder, Maple, Pine,
(Assignee)

Comment 28

6 years ago
Bug 780915 was filed to remove the builds
You need to log in before you can comment on or make changes to this bug.