1165161 - Modify bootAnim stop timing

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect)

Description

[Jerry Shih[:jerry] (UTC+8) (inactive)]

+++ 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.

Comment 1

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: modify boot animation stop timing. v1

Comment 2

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: modify boot animation stop timing. v2

Comment 3

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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().

Comment 4

[Michael Wu [: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?

Comment 5

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(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.

Comment 6

[Michael Wu [: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.

Comment 7

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Create GonkSurface to control bootAnim stopping timing. v1

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

Comment 8

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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 9

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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.

Comment 10

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Create GonkSurface to control bootAnim stopping timing. v2

Comment 11

[Jerry Shih[:jerry] (UTC+8) (inactive)]

try link:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=730069247bcf

Comment 12

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Create GonkSurface to control bootAnim stopping timing. v3

fix build break at jb.

Comment 13

[Michael Wu [:mwu]]

This is a step backwards. No doubt it works, but it takes us away from the architecture we want here.

Comment 14

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8612156 [details] [diff] [review]
Create GonkSurface to control bootAnim stopping timing. v3

Let me have a discussion about our goal later.

Comment 15

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: stop bootAnim with "browser-ui-startup-complete" event. v1

How about this?

Comment 16

[Michael Wu [: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.

Comment 17

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: stop bootAnim with "browser-ui-startup-complete" event. v2

Comment 18

[Michael Wu [: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.

Comment 19

[Jerry Shih[:jerry] (UTC+8) (inactive)]

try link:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7459fdbd14be

Comment 20

[Jerry Shih[:jerry] (UTC+8) (inactive)]

All dependent bugs were fixed. New try link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a57bea000deb

Comment 21

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Please land the attachment 8615852 [details] [diff] [review] to m-c.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0580333b6546

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0580333b6546

Comment 24

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: stop bootAnim with "browser-ui-startup-complete" event. (b2g v2.2)

[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

Comment 25

[Wesly Huang (EPM)]

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

Comment 26

[Josh Cheng [:josh]]

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!

Comment 27

[Josh Cheng [:josh]]

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.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/67eaa9cfee66