1006274 - [tarako][perf] When hang up the phone, it takes time 3.5s, which costs 2s more than android

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Dialer, defect, P1)

Description

[Wei Gao (Spreadtrum)]

【Repro Steps*】
1. Dial a phone number, such as 10086
2. Click to hang up the call
3. After that, it takes time 3.5s to show dialpad, which costs 2s more than android

Spreadtrum bug 307322
http://bugzilla.spreadtrum.com/bugzilla/show_bug.cgi?id=307322

Comment 1

[Wei Gao (Spreadtrum)]

Attachment: Remove hangup effects

When hang up the phone, the interface transform effects takes 2s which is too long for hand up. After removing this effects, the time to hang up the phone is similar to android.

I propose to remove the effects of hang up, taking into account the performance of 6821.

The patch file Remove_hangup_effects.patch is to remove the effects of hang up.

Please help to review, thanks.

Comment 2

[Wei Gao (Spreadtrum)]

Comment on attachment 8417793 [details] [diff] [review]
Remove hangup effects

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

Comment 3

[James Zhang (Spreadtrum)]

1.3t+ please.

Comment 4

[Steven Yang [:styang]]

Hi James, you can pick the reviewer from the suggested list when you're finding the proper owner to review it. thanks.

Comment 5

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Comment on attachment 8417793 [details] [diff] [review]
Remove hangup effects

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

I can't review it but only give comments since Etienne is the module owner.

I'll suggest you directly remove the two transition lines instead of mark them off.

Also, this changes UX/UI behavior. We should ask for UX input.

Comment 6

[Evelyn Hung]

Comment on attachment 8417793 [details] [diff] [review]
Remove hangup effects

Sorry, I can't review dialer patch.

Comment 7

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Attachment: Video with transition effect removed

Hi Mike,

We need your opinion. (Or reassign to others to have a look at it) 

I record a video here with transition effect removed in case you'd like to see it right away.

Thanks

Comment 8

[Joe Cheng [:jcheng] (please needinfo)]

triage: i don't think this is acceptable to remove the "call ended" screen
the "call ended" screen was requested to be added as a feature by one of our carrier partner for their IOT and it is a bit confusing that the call ends immediately like the video

i think the problem is the black screen after ending the call, not the "call ended" indication screen

ni? Wei.gao, is it really your intention to remove the "call ended" indication screen?

Comment 9

[James Zhang (Spreadtrum)]

We'll remove all gaia animation when enter and exit apps for tarako only.

Comment 10

[Wei Gao (Spreadtrum)]

(In reply to Joe Cheng [:jcheng] from comment #8)
> triage: i don't think this is acceptable to remove the "call ended" screen
> the "call ended" screen was requested to be added as a feature by one of our
> carrier partner for their IOT and it is a bit confusing that the call ends
> immediately like the video
> 
> i think the problem is the black screen after ending the call, not the "call
> ended" indication screen
> 
> ni? Wei.gao, is it really your intention to remove the "call ended"
> indication screen?

hi Joe
The black screen is always exist no matter remove the transition effect or not and it is not the main reason that takes so much time when hang up. 
We hope the transition effect can bring better user experience, but if the performance can't display smoothly as we hope, I think removing the transition effect on 6821 is more sensible.
Thank you.

Comment 11

[Etienne Segonzac (:etienne)]

Comment on attachment 8417793 [details] [diff] [review]
Remove hangup effects

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

Like the comment just above the line modified mentions |This transition is needed|.
On the JS side we're waiting for the transitionend event.

Also like Joe said this is preventing us from displaying the the "Call ended" message which was a mandatory partner request.

I think we need a stronger rationale for this change, and if we move forward we need to make this a clean option with settings like for the app opening transitions.

Comment 12

[Wei Gao (Spreadtrum)]

(In reply to Etienne Segonzac (:etienne) from comment #11)
> Comment on attachment 8417793 [details] [diff] [review]
> Remove hangup effects
> 
> Review of attachment 8417793 [details] [diff] [review]:
> -----------------------------------------------------------------

After discussion within our group, our strategy is to remove those effects that are independent from logic functions, and reduce time of those effects which are related to internal logic. Such as changing 2s to 500ms, so as to make the performance better.

Comment 13

[GoodMike]

Thanks for the video, I counted it, I think there is a 2 seconds delay. If you want UX opinion, we would like it to hangout as soon as possible. But if the performance is not good, I think with that price band device, users may have the tolerance and patience to wait for 2 seconds.

Comment 14

[Etienne Segonzac (:etienne)]

(In reply to GoodMike from comment #13)
> Thanks for the video, I counted it, I think there is a 2 seconds delay. If
> you want UX opinion, we would like it to hangout as soon as possible. But if
> the performance is not good, I think with that price band device, users may
> have the tolerance and patience to wait for 2 seconds.

Just to clarify. The 2 second delay is a *feature*. The phone isn't too busy to do it quicker. It was added as part of a partner requested feature to display a "Call ended" message for 2 seconds before closing the callscreen.

Comment 15

[GoodMike]

Hi Etienne : ) Thanks for the comment. So the "CAll ended" message is 2 seconds black screen ?

Comment 16

[GoodMike]

If the black screen is the "feature" o inform user that the phone is hanged up. I think it doesn't make sense. Once user hangs up a call, it makes sense that it just go back to the dialer screen as the last UI in the video. Because users know it's hanged up when seeing that page.

Comment 17

[James Zhang (Spreadtrum)]

(In reply to wei.gao from comment #12)
> (In reply to Etienne Segonzac (:etienne) from comment #11)
> > Comment on attachment 8417793 [details] [diff] [review]
> > Remove hangup effects
> > 
> > Review of attachment 8417793 [details] [diff] [review]:
> > -----------------------------------------------------------------
> 
> After discussion within our group, our strategy is to remove those effects
> that are independent from logic functions, and reduce time of those effects
> which are related to internal logic. Such as changing 2s to 500ms, so as to
> make the performance better.

Fabrice, I need this patch for tarako only workaround.

Comment 18

[Wei Gao (Spreadtrum)]

Attachment: hang up effect.3gp

I uploaded a video with two different effects in it. In this video, the left mobilephone have removed hang up effect and the right one continue to keep it. We can see the effect that after hang up, the right phone continue too much time and when the transition effect is transforming, the bottom of screen is entire black.
 
We know the hang up effect is a "feature", and we are willing to keep the feather, but out purpose is to reduce the time of common use operation and improve the performance of the phone. I am sure our customer also would like to see that.

Do you think it right ? 

Thanks.

Comment 19

[Joe Cheng [:jcheng] (please needinfo)]

can you just shortern the animation for "call ended" screen? instead of taking the "call ended" screen out completely?

Comment 20

[Wei Gao (Spreadtrum)]

(In reply to Joe Cheng [:jcheng] from comment #19)
> can you just shortern the animation for "call ended" screen? instead of
> taking the "call ended" screen out completely?

hi Joe

After we tried to reduce animation duration of the call ended effect and to verified it, we found that even though we decreased the transition time to 100ms, the performance of the hang up effect was not smoothly as we hope. I'm afraid customer can't accept this result either.

Judging from our test data, only by reducing the transition duration to the less than 10ms, the effect of the hang up operation can be smoothly and perform well.

What do you think about it ?

Thanks.

Comment 21

[Wei Gao (Spreadtrum)]

(In reply to Joe Cheng [:jcheng] from comment #19)
> can you just shortern the animation for "call ended" screen? instead of
> taking the "call ended" screen out completely?

But the question is if we reduce the transition duration to less than 10ms, I think there is no difference between it and get rid of the effect directly. In my own opinion, if this is the case, I think the transition effect is more to be removed than to be reduced its duration to less than 10ms. 

How do you think about it ?

Thanks.

Comment 22

[[:fabrice] Fabrice Desré]

I'm fine with removing the transition totally. Please provide at updated patch that addresses Etienne's comment about being notified instead of relying on the transitionend event.

Comment 23

[Mike Lee [:mlee]]

Since both with transition and without transition lead to a blank screen but without is faster we (FxOS Perf Team) agree with removing the transition as it adds no value to the user experience and actually degrades it by making it slower.

(In reply to Etienne Segonzac (:etienne) from comment #11)
> 
> Like the comment just above the line modified mentions |This transition is
> needed|.
> On the JS side we're waiting for the transitionend event.
> 
> Also like Joe said this is preventing us from displaying the the "Call
> ended" message which was a mandatory partner request.

Can we use a different (custom) event to indicate being ready to show the "Call ended" message?

Comment 24

[Wei Gao (Spreadtrum)]

Attachment: Remove_hangup_effects_2.patch

hi Fabrice

I upload a file Remove_hangup_effects_2.patch that the code is changed. The original code's strategy is that there is a judgement to ensure what to execute during the transition. If the effect is not transitioning, it will add a eventlistener to listen the 'transitionend' event, and some code will be executed until the transition is over. But if the effect is transitioning, or a new call is incoming, those code will be executed directly, because it will loose the 'transitionend' event during the transition.

After the transition effect is removed, i modify the code to make sure those following code will be executed whatever, so as to ensure the current logic the same as before.

What's your opinion on this ?

Thank you.

Comment 25

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Comment on attachment 8419254 [details] [diff] [review]
Remove_hangup_effects_2.patch

I think we should ask Etienne if this patch is good or not.

Comment 26

[Etienne Segonzac (:etienne)]

Attachment: Patch proposal - remove the transition delay

Please take a look at this patch removing the 1.5sec transition delay.

It's much simpler and has the advantage of not breaking the test suite which will make landing easier.

Comment 27

[[:fabrice] Fabrice Desré]

Comment on attachment 8419508 [details] [diff] [review]
Patch proposal - remove the transition delay

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

works fine here. ship it!

Comment 28

[Wei Gao (Spreadtrum)]

(In reply to Etienne Segonzac (:etienne) from comment #26)
> Created attachment 8419508 [details] [diff] [review]
> Patch proposal - remove the transition delay
> 
> Please take a look at this patch removing the 1.5sec transition delay.
> 
> It's much simpler and has the advantage of not breaking the test suite which
> will make landing easier.

hi 
It is really more reliable to removing the 1.5sec transition delay only, thanks.
I want to know can it be meraged into 1.3t now ?

Thank you.

Comment 29

[Etienne Segonzac (:etienne)]

Landed on 1.3t
https://github.com/mozilla-b2g/gaia/commit/d07f2dff31e71aed9711d988677fbc5b2c6e83e4

Comment 30

[Wei Gao (Spreadtrum)]

(In reply to Etienne Segonzac (:etienne) from comment #29)
> Landed on 1.3t
> https://github.com/mozilla-b2g/gaia/commit/
> d07f2dff31e71aed9711d988677fbc5b2c6e83e4

OK.
Thanks for your kindly help.

Comment 31

[Rachel Pribble [:rpribble]]

Verified fixed on the Tarako v1.3T MOZ ril.

Environmental Variables:
Device: Tarako 1.3T
BuildID: 20140602014001
Gaia: 335486c42498fa7a93c21e4d6121199728602ab8
Gecko: 55e4d83019e5
Version: 28.1
Firmware Version: SP6821a-Gonk-4.0-4-29

Keypad displays at 1.74s.

Comment 32

[Jan Jongboom [:janjongboom] (inactive)]

This has 1.4 affected status, do we want this for 1.4 spreadtrum devices as well?

Comment 33

[Mike Lee [:mlee]]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #32)
> This has 1.4 affected status, do we want this for 1.4 spreadtrum devices as
> well?

I'm assuming you're asking about Dolphin. If so I believe that's Spreadtrum's decision to make. Preeti please confirm.

Comment 34

[Preeti Raghunath(:Preeti)]

Lets add this on 1.4 dolphin branch

Comment 35

[Ivan Tsay (:ITsay)]

Hi Pei-pei,

Please help to check if it performs the same on Dolphin and Buri before we can make better decision on the 1.4 uplift of this bug. Thanks!

Comment 36

[Peipei Cheng (needinfo if you need my action)]

Dolphin perform a little better than Tarako. The average time is about 2.5s.
Buri has same performance as dolphin. I tried 5 times, avg time is 2.5s.

Comment 37

[Ivan Tsay (:ITsay)]

Let's not uplift this patch for now since it performs the same as Buri.

Comment 38

[James Zhang (Spreadtrum)]

Wei, land this WIP patch in sprd_patch.

Comment 39

[Wei Gao (Spreadtrum)]

(In reply to James Zhang (Spreadtrum) from comment #38)
> Wei, land this WIP patch in sprd_patch.

OK, I will do that right now.