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

RESOLVED FIXED in 2.0 S1 (9may)

Status

defect
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: wei.gao, Assigned: etienne)

Tracking

({perf})

unspecified
2.0 S1 (9may)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:1.3T+, b2g-v1.3T verified, b2g-v1.4 affected)

Details

(Whiteboard: [c=progress p= s= u=tarako] [sprd307322][partner-blocker][tarako_only][dolphin land])

Attachments

(3 attachments, 2 obsolete attachments)

【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
Posted patch Remove hangup effects (obsolete) — Splinter Review
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 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]:
Attachment #8417793 - Flags: review?(tzhuang)
Attachment #8417793 - Flags: review?(ehung)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: [tarako]When hang up the phone, it takes time 3.5s, which costs 2s more than android → [tarako][perf] When hang up the phone, it takes time 3.5s, which costs 2s more than android
1.3t+ please.
Flags: needinfo?(ttsai)
Flags: needinfo?(styang)
Flags: needinfo?(kkuo)
Whiteboard: [sprd307322][partner-blocker][tarako_only]
Flags: needinfo?(fabrice)
Attachment #8417793 - Flags: review?(etienne)
Hi James, you can pick the reviewer from the suggested list when you're finding the proper owner to review it. thanks.
Flags: needinfo?(ttsai)
Flags: needinfo?(styang)
Flags: needinfo?(kkuo)
Flags: needinfo?(fabrice)
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.
Attachment #8417793 - Flags: review?(tzhuang)
Comment on attachment 8417793 [details] [diff] [review]
Remove hangup effects

Sorry, I can't review dialer patch.
Attachment #8417793 - Flags: review?(ehung)
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
Flags: needinfo?(mtsai)
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?
Flags: needinfo?(wei.gao)
We'll remove all gaia animation when enter and exit apps for tarako only.
(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.
Flags: needinfo?(wei.gao)
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.
Attachment #8417793 - Flags: review?(etienne) → review-
(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.
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.
Flags: needinfo?(mtsai)
(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.
Hi Etienne : ) Thanks for the comment. So the "CAll ended" message is 2 seconds black screen ?
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.
(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.
Flags: needinfo?(fabrice)
Flags: needinfo?(styang)
Flags: needinfo?(kkuo)
Posted video 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.
can you just shortern the animation for "call ended" screen? instead of taking the "call ended" screen out completely?
Flags: needinfo?(wei.gao)
(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.
Flags: needinfo?(wei.gao)
(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.
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.
Flags: needinfo?(fabrice)
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?
Assignee: nobody → wei.gao
Status: NEW → ASSIGNED
blocking-b2g: 1.3T? → 1.3T+
Keywords: perf
Priority: -- → P1
Whiteboard: [sprd307322][partner-blocker][tarako_only] → [c=progress p= s= u=tarako] [sprd307322][partner-blocker][tarako_only]
Flags: needinfo?(kkuo)
Flags: needinfo?(styang)
Posted patch Remove_hangup_effects_2.patch (obsolete) — Splinter Review
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.
Flags: needinfo?(fabrice)
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.
Attachment #8419254 - Flags: review?(etienne)
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.
Assignee: wei.gao → etienne
Attachment #8417793 - Attachment is obsolete: true
Attachment #8419254 - Attachment is obsolete: true
Attachment #8419254 - Flags: review?(etienne)
Attachment #8419508 - Flags: feedback?(wei.gao)
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!
Attachment #8419508 - Flags: feedback?(wei.gao) → review+
Flags: needinfo?(fabrice)
(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.
Landed on 1.3t
https://github.com/mozilla-b2g/gaia/commit/d07f2dff31e71aed9711d988677fbc5b2c6e83e4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
(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.
Target Milestone: --- → 2.0 S1 (9may)
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.
This has 1.4 affected status, do we want this for 1.4 spreadtrum devices as well?
Flags: needinfo?(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.
Flags: needinfo?(mlee) → needinfo?(praghunath)
Lets add this on 1.4 dolphin branch
Flags: needinfo?(praghunath)
Whiteboard: [c=progress p= s= u=tarako] [sprd307322][partner-blocker][tarako_only] → [c=progress p= s= u=tarako] [sprd307322][partner-blocker][tarako_only][dolphin land]
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!
Flags: needinfo?(pcheng)
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.
Flags: needinfo?(pcheng)
Let's not uplift this patch for now since it performs the same as Buri.
Flags: needinfo?(etienne)
Wei, land this WIP patch in sprd_patch.
Flags: needinfo?(wei.gao)
(In reply to James Zhang (Spreadtrum) from comment #38)
> Wei, land this WIP patch in sprd_patch.

OK, I will do that right now.
Flags: needinfo?(wei.gao)
Flags: needinfo?(etienne)
You need to log in before you can comment on or make changes to this bug.