Closed Bug 1165161 Opened 5 years ago Closed 5 years ago

Modify bootAnim stop timing

Categories

(Core Graveyard :: Widget: Gonk, defect)

ARM
Gonk (Firefox OS)
defect
Not set
blocker

Tracking

(blocking-b2g:2.2r+, firefox41 fixed, b2g-v2.1 wontfix, b2g-v2.1S wontfix, b2g-v2.2 wontfix, b2g-v2.2r fixed, b2g-master fixed)

RESOLVED FIXED
mozilla41
blocking-b2g 2.2r+
Tracking Status
firefox41 --- fixed
b2g-v2.1 --- wontfix
b2g-v2.1S --- wontfix
b2g-v2.2 --- wontfix
b2g-v2.2r --- fixed
b2g-master --- fixed

People

(Reporter: jerry, Assigned: jerry)

References

Details

Attachments

(2 files, 6 obsolete files)

+++ This bug was initially created as a clone of Bug #1160877 +++

Bug 1160877 fix the flickering problem but it breaks the Bug 1156140 patch which want to extend the boot animation time. With Bug 1160877, bootAnimation will stop earlier. Bug 1152135 changes hwc flow that make Bug 1156140 not work.

This bug would like to have a clear interface to control bootAnimation stop timing for both fbdev and hwc usage.
Attachment #8606125 - Flags: review?(mwu)
Attachment #8606125 - Flags: review?(mwu)
Status: NEW → ASSIGNED
Depends on: 1165200
Attachment #8606125 - Attachment is obsolete: true
Comment on attachment 8607084 [details] [diff] [review]
modify boot animation stop timing. v2

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

::: gfx/gl/GLContextProviderEGL.cpp
@@ +191,5 @@
>  CreateSurfaceForWindow(nsIWidget* widget, const EGLConfig& config) {
>      EGLSurface newSurface = EGL_NO_SURFACE;
>  
> +    #ifdef MOZ_WIDGET_GONK
> +        GetGonkDisplay()->RequestGLContext();

We are going to create glContext. Stop the bootAnimation which uses the same bufferQueue as glContext wants.

::: widget/gonk/HwcComposer2D.cpp
@@ +679,5 @@
>  #if ANDROID_VERSION >= 17
>  bool
>  HwcComposer2D::TryHwComposition()
>  {
> +    GetGonkDisplay()->RequestHwcComposition();

Stop the bootAnimation since HwcComposer2D starts to use hwc::prepare() and hwc::set().

@@ +774,5 @@
>  
>  bool
>  HwcComposer2D::Render()
>  {
> +    GetGonkDisplay()->RequestHwcComposition();

Stop the bootAnimation since HwcComposer2D starts to use hwc::prepare() and hwc::set().
Attachment #8607084 - Flags: review?(mwu)
Comment on attachment 8607084 [details] [diff] [review]
modify boot animation stop timing. v2

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

Hmm this isn't quite ideal. What I had in mind was to hand the boot animation surface to HwcComposer2D which will take over the rendering of the boot animation until it receives a browser-ui-startup-complete notification. Doing it this way lets us run the boot animation past HWC/GL initialization. We shouldn't need to touch code in gfx/gl/GLContextProviderEGL.cpp to achieve this. The boot animation code needs to be adjusted to draw to the surface directly instead of pulling buffers out of GonkDisplay. After that, it should be possible to pass the surface to HwcComposer2D.

Does this make sense now?
(In reply to Michael Wu [:mwu] from comment #4)
> Hmm this isn't quite ideal. What I had in mind was to hand the boot
> animation surface to HwcComposer2D which will take over the rendering of the
> boot animation until it receives a browser-ui-startup-complete notification.
> Doing it this way lets us run the boot animation past HWC/GL initialization.

Here is the time stamp from boot for each step:
AnimationThread stat        5.010451
hwc ctor                    11.688057
browser-ui-startup-complete 19.499878
RequestGLContext            19.970886
RequestHwcComposition       20.396237

I'm not sure how to pass the buffer to hwc for bootAnim. We show anim before hwc ctor. BootAnim could own its bufferQueue, but it still need GonkDisplayJB::Post() to show on screen.

> We shouldn't need to touch code in gfx/gl/GLContextProviderEGL.cpp to

In some device(v2.0m just uses fbdev), we only can create two buffer with "GRALLOC_USAGE_HW_FB" flag. So we need to release the bootAnim buffer before gl context creation. If we stop the anim at browser-ui-startup-complete event, we will still have 0.5s without bootanim. We can play the anim until context creation in GLContextProviderEGL.cpp for fbdev device.

browser-ui-startup-complete 19.499878
RequestGLContext            19.970886

> achieve this. The boot animation code needs to be adjusted to draw to the
> surface directly instead of pulling buffers out of GonkDisplay. After that,
> it should be possible to pass the surface to HwcComposer2D.
> 
> Does this make sense now?

I can do for next patch are:

a) bootAnim has its bufferQueue and handle dequeue and queue itself.
b) export post() function into GonkDisplay.h(only GonkDisplayJB has post now). Then bootAnim could call post() to show the anim.
c) stop bootanim in nsAppShell::Observe with browser-ui-startup-complete event. We will not see the anim in some duration, since browser-ui-startup-complete is before context creation and the first composition call to hwc.

browser-ui-startup-complete 19.499878
RequestGLContext            19.970886  <== the device with fbdev only
RequestHwcComposition       20.396237  <== the device with hwc

Is browser-ui-startup-complete event still a good position for stopping anim?

-----
Currently, we call stopAnim() in GonkDisplayJB::GetNativeWindow(). It's far before browser-ui-startup-complete. User can see there is no update of bootanim on screen for a long time.
Flags: needinfo?(mwu)
Attachment #8607084 - Flags: review?(mwu)
Thanks for doing the measurements.

The use of browser-ui-startup-complete isn't the important thing. We can use any notification that suits our goal - we just can't sprinkle hints all over the gfx code. That's too adhoc and makes the gfx guys mad. So I suggest moving the point at which browser-ui-startup-complete fires, possibly with a hint from the gaia side. That can be done separately though. For testing purposes, you can set a timer which fires X seconds after window load to send browser-ui-startup-complete. Passing the boot animation to HwcComposer2D is probably necessary in order to end the animation past HwcComposer2D initialization.
Flags: needinfo?(mwu)
This is the simplest fix to extend the bootAnim until we create GL context.
I don't have a good idea to pass the anim to HwcComposer2D yet. Maybe bootAnim can go through compositorOGL just like the normal layer. Then we don't need to write a special case for it.

AnimationThread stat        5.010451
hwc ctor                    11.688057
browser-ui-startup-complete 19.499878
Create gl context           19.970886  <=== we will stop bootAnim here
The first time we use hwc   20.396237
Attachment #8611287 - Flags: feedback?(mwu)
Comment on attachment 8611287 [details] [diff] [review]
Create GonkSurface to control bootAnim stopping timing. v1

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

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +155,5 @@
> +  }
> +
> +  virtual int connect(int api) override
> +  {
> +    StopBootAnimation();

When we call connect() during context creation, stop the bootAnim.
Comment on attachment 8611287 [details] [diff] [review]
Create GonkSurface to control bootAnim stopping timing. v1

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

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +186,5 @@
>      aNativeWindow = new SurfaceTextureClient(
>          static_cast<sp<ISurfaceTexture>>(aDisplaySurface->getBufferQueue()));
>  #else
> +    //aNativeWindow = new Surface(producer);
> +    aNativeWindow = new GonkSurface(producer);

If this idea is good, I will also create another class for SurfaceTextureClient.
Attachment #8611287 - Attachment is obsolete: true
Attachment #8611287 - Flags: feedback?(mwu)
Attachment #8612118 - Flags: review?(mwu)
fix build break at jb.
Attachment #8612118 - Attachment is obsolete: true
Attachment #8612118 - Flags: review?(mwu)
Attachment #8612156 - Flags: review?(mwu)
This is a step backwards. No doubt it works, but it takes us away from the architecture we want here.
Comment on attachment 8612156 [details] [diff] [review]
Create GonkSurface to control bootAnim stopping timing. v3

Let me have a discussion about our goal later.
Attachment #8612156 - Flags: review?(mwu)
How about this?
Attachment #8614550 - Flags: review?(mwu)
Sotaro will simplify the bootanimation API a bit in bug 1171671. I think you can build on top of that with the same approach you have here.
Depends on: 1171671
Attachment #8607084 - Attachment is obsolete: true
Attachment #8612156 - Attachment is obsolete: true
Attachment #8614550 - Attachment is obsolete: true
Attachment #8614550 - Flags: review?(mwu)
Attachment #8615852 - Flags: review?(mwu)
Comment on attachment 8615852 [details] [diff] [review]
stop bootAnim with "browser-ui-startup-complete" event. v2

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

Not 100% sure about the safety of this, but we can try it.
Attachment #8615852 - Flags: review?(mwu) → review+
https://hg.mozilla.org/mozilla-central/rev/0580333b6546
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1160877
Blocks: 1191674
[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Bug 1191674
User impact if declined:
User will see the bootAnim pause for a long times.
Testing completed:
The are already at m-c.
Risk to taking this patch (and alternatives if risky):
low
String or UUID changes made by this patch:
no
Attachment #8647274 - Flags: approval-mozilla-b2g37?
TAM's comment for this issue: 

This is one of the three dependent issues of bug 1191674, which is needed by partner TCL for their product based on 2.2. Thought this is not functional critical however indeed impact first time user experience (power on animation is the first thing user will see), plus the risk is low so I think it's worth fixing. In comment 38 of bug 1191674 we can already see those patches work for their problem.

Hi Josh: 

how is your view about landing it on 2.2
Flags: needinfo?(jocheng)
Hi Wesly,
We currently do not have plan for shipping device on 2.2 but 2.2r.
I like to make this 2.2r+. Thanks!
blocking-b2g: --- → 2.2r+
Flags: needinfo?(jocheng)
Comment on attachment 8647274 [details] [diff] [review]
stop bootAnim with "browser-ui-startup-complete" event. (b2g v2.2)

Thanks Jerry,
Make this blocking-b2g: 2.2r+.
Currently no 2.2r approval required.
Attachment #8647274 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37-
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.