1165200 - Fix GonkDisplay::QueueBuffer() for bootAnim

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 #1163681 +++

After bug 1163681, we can't see the bootAnim anymore.

Comment 1

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

Attachment: fix GonkDisplay::QueueBuffer() for bootAnim. v1

I don't see the error message as [1].
BootAnim uses separated gralloc buffer. It's not from bufferQueue. So I update the QueueBuffer() for bootAnim case.

[1]
https://bugzilla.mozilla.org/show_bug.cgi?id=1162715#c0

Comment 2

[Sotaro Ikeda [:sotaro]]

I set review+, but I do not fun of using mBootAnimBuffer. HWC architecture requests, at least 2 buffers for animation update.

Comment 3

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8606126 [details] [diff] [review]
fix GonkDisplay::QueueBuffer() for bootAnim. v1

Sorry, I clear the review+, until the necessity of mBootAnimBuffer becomes clear. I rechecked GonkDisplayJB::QueueBuffer()'s implementation. since it's first implementation, it is not implemented correctly. It seems that just fixing it seems to address the problem.

Comment 4

[Sotaro Ikeda [:sotaro]]

On Bug 1163681, I focused only on b2g v2.2. I did not checked master's code enough. Bug 1163681 should basically fixed the problem of GonkDisplayJB::QueueBuffer(). Do same thing as in b2g v2.2 should fix the problem also on master, I think.

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove mBootAnimBuffer

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove mBootAnimBuffer

Comment 7

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> On Bug 1163681, I focused only on b2g v2.2. I did not checked master's code
> enough. Bug 1163681 should basically fixed the problem of
> GonkDisplayJB::QueueBuffer().

Therefore, mBootAnimBuffer seems not necessary.

Comment 8

[Michael Wu [:mwu]]

mBootAnimBuffer is intentional - we want to be able to pass the animation buffer to gecko during startup and allow gecko to decide when to stop the animation. That's not done at the moment, but that's the goal.

Comment 9

[Sotaro Ikeda [:sotaro]]

(In reply to Michael Wu [:mwu] from comment #8)
> mBootAnimBuffer is intentional - we want to be able to pass the animation
> buffer to gecko during startup and allow gecko to decide when to stop the
> animation. That's not done at the moment, but that's the goal.

mwu, can you explain more about why FrameBufferSurface's buffer does not work for it. In current gecko's code, FrameBufferSurface's buffer is enough.

Comment 10

[Sotaro Ikeda [:sotaro]]

My concern is that use single buffer for animation could cause flickering. At least, 2 buffers are necessary for the bootanimation.

Comment 11

[Michael Wu [:mwu]]

Yeah we probably need two.

FrameBufferSurface doesn't work because it gets taken by EGL during gecko's gfx initialization.

Comment 12

[Sotaro Ikeda [:sotaro]]

Android create a surface for bootanimation. Similar way of implementation seems more architecturally clean than handling raw buffer.
 http://androidxref.com/5.1.0_r1/xref/frameworks/base/cmds/bootanimation/BootAnimation.cpp#241

Comment 13

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #12)
> Android create a surface for bootanimation. Similar way of implementation
> seems more architecturally clean than handling raw buffer.
>  http://androidxref.com/5.1.0_r1/xref/frameworks/base/cmds/bootanimation/
> BootAnimation.cpp#241

If the surface is destroyed when bootanimation stop. GraphicBuffer usage increase does not affect to actual Firefox OS's normal usage.

Comment 14

[Michael Wu [:mwu]]

Yeah agreed - creating a proper surface to manage these buffers is the right way to go.

Comment 15

[Sotaro Ikeda [:sotaro]]

Yhea, it is a way to go:-)

Comment 16

[Sotaro Ikeda [:sotaro]]

Jerry, can you update a patch as in comment 12? Single buffer could cause flickering or lower fps.

Comment 17

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #16)
> Jerry, can you update a patch as in comment 12? Single buffer could cause
> flickering or lower fps.

And please do not forget to fix fence handling correctly.

Comment 18

[Michael Wu [:mwu]]

Comment on attachment 8606126 [details] [diff] [review]
fix GonkDisplay::QueueBuffer() for bootAnim. v1

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

To summarize, I think we want to do these things:

1. Switch the boot animation to drawing to a double buffered surface rather than just juggling raw gralloc buffers.
2. Have the hardware compositor take the boot animation surface from GonkDisplay when it initializes, and start displaying buffers from there.
3. Drop the surface and stop displaying the animation when we receive the browser-ui-startup-complete notification. (fired from b2g/chrome/content/shell.js)

Comment 19

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

I will ask :chiajung the reason for the original single buffer usage and check the items in comment 18 later. thx.

Comment 20

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

Attachment: fix GonkDisplay::QueueBuffer() for bootAnim. v2

Create a new surface for bootAnim.

Comment 21

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

(In reply to Michael Wu [:mwu] from comment #18)
> To summarize, I think we want to do these things:
> 
> 1. Switch the boot animation to drawing to a double buffered surface rather
> than just juggling raw gralloc buffers.

attachment 8606752 [details] [diff] [review] will do that.

> 2. Have the hardware compositor take the boot animation surface from
> GonkDisplay when it initializes, and start displaying buffers from there.

I'm not sure why HwcComposer2D needs to touch the boot animation surface. During booting, b2g will not call HwcComposer2D::TryHwComposition() and HwcComposer2D::Render(). In bootAnim case, GonkDisplay can handle everything.

> 3. Drop the surface and stop displaying the animation when we receive the
> browser-ui-startup-complete notification. (fired from
> b2g/chrome/content/shell.js)

Bug 1165161 will do that.

Comment 22

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8606752 [details] [diff] [review]
fix GonkDisplay::QueueBuffer() for bootAnim. v2

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

Looks good.

Comment 23

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

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

Comment 24

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

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

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/90bc8b02e164

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/90bc8b02e164

Comment 27

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

Attachment: fix GonkDisplay::QueueBuffer() for bootAnim. (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 28

[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 29

[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 30

[Josh Cheng [:josh]]

Comment on attachment 8647270 [details] [diff] [review]
fix GonkDisplay::QueueBuffer() for bootAnim. (b2g v2.2)

Thanks Jerry,
Make this blocking-b2g: 2.2r+.
Currently no 2.2r approval required.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/033e5cbcd522