853742 - [Buri][FM]Pause the FM, and then press play key to continue paly the FM, but the FM response very slowly( about 5 seconds reaction)

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::FMRadio, defect, P2)

Description

[sync-1]

+++ This bug was initially created as a clone of Bug #407350 +++
 
 CONTACT INFO (Name,Phone number):
 [WuYing][6915]
 DEFECT DESCRIPTION:
 Pause the FM, and then press play key to continue paly the FM, but  the FM  response very slowly( about 5 seconds reaction)
 
 
 REPRODUCING PROCEDURES:
 1.When listening the FM Radio
 2.Pause the FM
 3.then press play key to continue paly the FM
 4.but  the FM  response very slowly( about 5 seconds reaction)---KO
 
 
 
 EXPECTED BEHAVIOUR:
 When you tap the "play" button, the FM should be playing immediately.
 
 ASSOCIATE SPECIFICATION:
 
 TEST PLAN REFERENCE:
 
 TOOLS AND PLATFORMS USED:
 V109
 USER IMPACT:
 Medium
 REPRODUCING RATE:
 5/5
 For FT PR, Please list reference mobile's behavior:
 
 AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.19.037
 Firefox os  v1.0.1
 Mozilla build ID: 20130310070203.
 
 ++++++++++ end of initial bug #407350 description ++++++++++
 
 
 
 CONTACT INFO (Name,Phone number):
 
 DEFECT DESCRIPTION:
 
 REPRODUCING PROCEDURES:
 
 EXPECTED BEHAVIOUR:
 
 ASSOCIATE SPECIFICATION:
 
 TEST PLAN REFERENCE:
 
 TOOLS AND PLATFORMS USED:
 
 USER IMPACT:
 
 REPRODUCING RATE:
 
 For FT PR, Please list reference mobile's behavior:

Comment 1

[Marco Chen [:mchen]]

Hi,

Is this no the same result on Android build?
Every time we started FMRadio, we need some time to download FW.
And I think this would be the same on Android.

Comment 2

[Peter Dolanjski [:pdol]]

Josh, what's your opinion here, from a UX perspective?  The delay is about 4 seconds with no visual affordance to the user to indicate what is happening.

Comment 3

[Peter Dolanjski [:pdol]]

Sorry Al, I accidentally overwrote your change.

Comment 4

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

pzhang, minding taking a look at this? thanks

Comment 5

[Pin Zhang [:pzhang] (inactive)]

(In reply to Joe Cheng [:jcheng] from comment #4)
> pzhang, minding taking a look at this? thanks

I checked FM-related logs with `adb logcat` command, obviously, loading FM module took much time, we encountered the same problem before on the Unagi/Otoro, you can check the comment 83 of the bug 749053:
  https://bugzilla.mozilla.org/show_bug.cgi?id=749053#c83

We did a lot of optimization for Unagi, nevertheless, there is still more than 1 second delay when re-enabling the FM on Unagi.

I don't know if we can do same optimizations for Buri, Steven might be more familiar with this, cc him here.

Anyway, I don't think we can turn on the FM radio without any delay, so I do think we should add an indicator to make sure we do not confuse the end user, just like Peter said.

Comment 6

[sync-1]

Can we dont

Comment 7

[sync-1]

Can we do not disable FM module when press pause button, just do a pause? Then
 when press continue button,  will do not need to reload FM module.

Comment 8

[Pin Zhang [:pzhang] (inactive)]

(In reply to sync-1 from comment #7)
> Can we do not disable FM module when press pause button, just do a pause?
> Then
>  when press continue button,  will do not need to reload FM module.

I don't think I am the right person to answer the question. 
@mwu, what do you think?

Comment 9

[Michael Wu [:mwu]]

We can probably cut down the fm initialization time significantly. The fm radio initialization time was slowed down by http://hg.mozilla.org/mozilla-central/rev/d4f04762dc07 , but if we're sure we don't need that, we can make things go faster.

Comment 10

[Marco Chen [:mchen]]

Hi Michael,

1. Could you help to reply the comment 9 from Michael about how to reducing time of enabling FM?

2. And I didn't join the FM development before, so may I know that whether FM chip supports low power mode? 

Thanks.

Comment 11

[Michael Wu [:mwu]]

Marco, doing fast fm initialization is actually the officially supported way. We're only using the slow init we have now because some phones were freezing up during initialization.

Comment 12

[Michael Vines [:m1] [:evilmachines]]

What phones were freezing?  Unagi only?

Comment 13

[Michael Wu [:mwu]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #12)
> What phones were freezing?  Unagi only?

I think it was unagi - bug 800263 .

Comment 14

[Michael Vines [:m1] [:evilmachines]]

Ah, ok.  So maybe we should back this out for v1.0.1?   Unagi isn't a shipping device.

Comment 15

[Michael Wu [:mwu]]

Attachment: Directly invoke fm_qsoc_patches

This is a bit faster, though I haven't measured it yet.

Comment 16

[Michael Wu [:mwu]]

Just measured the amount of time initTavaruaRadio takes - This patch brings that down from 4s to 2.7s on my device. Not sure if we have any more easy wins though, so we should probably have the UI give some sort of indication that the radio is initializing.

Comment 17

[Marco Chen [:mchen]]

By the way,

does FM chip support any addtional commands to pause the FM so we don't need to shutdown/powerup FM chip again when stop/start from UI?
Ex: Entering low power mode.

Thanks.

Comment 18

[Michael Vines [:m1] [:evilmachines]]

Comment on attachment 730274 [details] [diff] [review]
Directly invoke fm_qsoc_patches

I don't think we should bypass the mechanism that Android uses here.  This looks like it'll work but it's still not fast enough, so we need a UI patch regardless and we've deviated from a well-tested Android path.  I wonder if we'd get a similar 4s -> 2.7s win simply by changing that sleep(1) to a ~100ms sleep time in that loop.

I also checked on Android w/ QRD and FM radio init time is also roughly the same however the UI makes it clear that the radio is still initializing, and this makes  a huge difference in perception.  On Android I "know" that the radio is still coming up and in FFOS I'm just "confused", even though the elapsed wall clock is virtually identical.

Comment 19

[Daniel Coloma:dcoloma]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #18)

> 
> I also checked on Android w/ QRD and FM radio init time is also roughly the
> same however the UI makes it clear that the radio is still initializing, and
> this makes  a huge difference in perception.  On Android I "know" that the
> radio is still coming up and in FFOS I'm just "confused", even though the
> elapsed wall clock is virtually identical.

When we triaged this bug that was exactly the point I was making. The problem is not that initializing radio was slow, the problem is users are not aware of what is happening and might believe radio is not working. I think showing a spinner or something like that would just mitigate the blocking issue here without doing any risky change at lower layers.

Comment 20

[Josh Carpenter [:jcarpenter]]

(In reply to Daniel Coloma:dcoloma from comment #19)
> (In reply to Michael Vines [:m1] [:evilmachines] from comment #18)
> 
> > 
> > I also checked on Android w/ QRD and FM radio init time is also roughly the
> > same however the UI makes it clear that the radio is still initializing, and
> > this makes  a huge difference in perception.  On Android I "know" that the
> > radio is still coming up and in FFOS I'm just "confused", even though the
> > elapsed wall clock is virtually identical.
> 
> When we triaged this bug that was exactly the point I was making. The
> problem is not that initializing radio was slow, the problem is users are
> not aware of what is happening and might believe radio is not working. I
> think showing a spinner or something like that would just mitigate the
> blocking issue here without doing any risky change at lower layers.

Absolutely correct. The cognitive science guidelines here are pretty clear. From our FFOS UX Performance & Responsiveness deck:

> 1 Second: If an operation will take more than a 1-2 seconds, a progress indicator is needed. Progress indicators are the system's way of keeping its side of the conversation. eg: “I’m working on the problem. Here’s how much progress I’ve made and here's how much work is left.”

In our case we should display an asynchronous spinner if there is no playback within 1 second of the user pressing Play.

Patryk / Peter, can you guys make a recommendation for placement of said spinner? Something nondescript would be best. Definitely one of the circular options, as opposed to one of the horizontals.

Comment 21

[Daniel Coloma:dcoloma]

Seems we all agree we should fix it only at a user interface layer, partner request, awful experience, marking it as tef+

Comment 22

[Peter La]

Attachment: Mockup - Spinner Location

Attached is a mockup of where the spinner can appear during the delay before the FM Radio resumes play, and after the play button is pressed.

I'm also attaching an idea I have for better visual feedback between play and pause.

Comment 23

[Peter La]

Attachment: Mockup - Dimming to give feedback that the player is paused

Can we do a dimming effect like in the attached mockup when the player is paused?  The transition can be a 1 second fade to 15% opacity on the current channel displayed, as well as darkening the background in the top region.  Upon resuming play, everything can fade back to normal.

Comment 24

[Josh Carpenter [:jcarpenter]]

Comment on attachment 734004 [details]
Mockup - Dimming to give feedback that the player is paused

I think that's a neat idea, and am generally willing to be a bit more playful in apps like FM Radio and Clock. Let's give it a shot if the dev LOE isn't too onerous for this release.

Comment 25

[Josh Carpenter [:jcarpenter]]

(In reply to Peter La from comment #22)
> Created attachment 734003 [details]
> Mockup - Spinner Location
> 
> Attached is a mockup of where the spinner can appear during the delay before
> the FM Radio resumes play, and after the play button is pressed.
> 
> I'm also attaching an idea I have for better visual feedback between play
> and pause.

Dumb question, Peter: I thought the speaker phone button went there? My Unagi is dead right now so I can't just check it myself. Will follow up with you later today in person.

Comment 26

[Josh Carpenter [:jcarpenter]]

Comment on attachment 734003 [details]
Mockup - Spinner Location

Minusing until we clarify question of competing placement of speaker button.

Comment 27

[Daniel Coloma:dcoloma]

(In reply to Josh Carpenter [:jcarpenter] from comment #25)
> (In reply to Peter La from comment #22)
> > Created attachment 734003 [details]
> > Mockup - Spinner Location
> > 
> > Attached is a mockup of where the spinner can appear during the delay before
> > the FM Radio resumes play, and after the play button is pressed.
> > 
> > I'm also attaching an idea I have for better visual feedback between play
> > and pause.
> 
> Dumb question, Peter: I thought the speaker phone button went there? My
> Unagi is dead right now so I can't just check it myself. Will follow up with
> you later today in person.

As far as I know FM Radio can be only played through wired headsets so I don't think we have a speaker button.

Comment 28

[Andrew Overholt [:overholt]]

(In reply to Daniel Coloma:dcoloma from comment #27)
> As far as I know FM Radio can be only played through wired headsets so I
> don't think we have a speaker button.

Correct, the headphones are the FM Radio's antenna.

Comment 29

[Patryk Adamczyk [:patryk] UX]

Comment on attachment 734004 [details]
Mockup - Dimming to give feedback that the player is paused

When paused, it would be nice to dim part or the entire app. And then focus on the pause button, perhaps add a looping colour animation.

Comment 30

[Patryk Adamczyk [:patryk] UX]

Comment on attachment 734003 [details]
Mockup - Spinner Location

Can we integrate the loading spinner with the play button. The loading is specifically linked to the play, so it would be nice to group the 2.

Comment 31

[Patryk Adamczyk [:patryk] UX]

Peter can you provide new mock ups for the above ^

Comment 32

[Mark Shiao]

Currently the WebFM api does not have a way of notifying us when the radio actually resumes play.  Will have to talk to Steven lee to see if we can set something up so that we can implement the suggested UI changes.

Comment 33

[Mark Shiao]

(In reply to Mark from comment #32)
> Currently the WebFM api does not have a way of notifying us when the radio
> actually resumes play.  Will have to talk to Steven lee to see if we can set
> something up so that we can implement the suggested UI changes.

Issue above a non-factor.  Ready to implement after the mocks are done.

Comment 34

[Mark Shiao]

Attachment: Quick link to git pull request

This bug includes the darkening of the fequency display along with replacing the play icon with the spinner when FM is initalizing

Comment 35

[Pin Zhang [:pzhang] (inactive)]

(In reply to Mark Shiao from comment #34)
> Created attachment 736630 [details]
> Quick link to git pull request
> 
> This bug includes the darkening of the fequency display along with replacing
> the play icon with the spinner when FM is initalizing
You replaced the play icon with the spinner, but the mockup (attachment 734004 [details]) shows that the spinner should be in the left-top corner, is the mockup final version? or is there anything that I missed?

Comment 36

[Pin Zhang [:pzhang] (inactive)]

(In reply to Pin Zhang [:pzhang] from comment #35)
> (In reply to Mark Shiao from comment #34)
> > Created attachment 736630 [details]
> > Quick link to git pull request
> > 
> > This bug includes the darkening of the fequency display along with replacing
> > the play icon with the spinner when FM is initalizing
> You replaced the play icon with the spinner, but the mockup (attachment
> 734004 [details]) shows that the spinner should be in the left-top corner,
> is the mockup final version? or is there anything that I missed?
Wrong attachment id here, I meant attachment 734003 [details] which is feedback-, so there should be a final mockup version that I missed, Mark, could you post the link refer to the right mockup before I can review your code?

Comment 37

[Mark Shiao]

Attachment: FM Radio in intialization mode

@Pin I got ahead of myself my and applied Patryk's suggestion in comment 30.  

Guys can you review the screenshot and let me know if the spinner on the play button is a better solution than having it at the upper-left-hand corner.

Thanks

Comment 38

[Pin Zhang [:pzhang] (inactive)]

(In reply to Mark Shiao from comment #34)
> Created attachment 736630 [details]
> Quick link to git pull request
> 
> This bug includes the darkening of the fequency display along with replacing
> the play icon with the spinner when FM is initalizing

@mark, there are lots of coding style nits, you need run gjslint to check your code, please follow the instruction:
  https://wiki.mozilla.org/Gaia/Hacking#Before_submitting_a_patch

Comment 39

[Mark Shiao]

(In reply to Pin Zhang [:pzhang] from comment #38)
> (In reply to Mark Shiao from comment #34)
> > Created attachment 736630 [details]
> > Quick link to git pull request
> > 
> > This bug includes the darkening of the fequency display along with replacing
> > the play icon with the spinner when FM is initalizing
> 
> @mark, there are lots of coding style nits, you need run gjslint to check
> your code, please follow the instruction:
>   https://wiki.mozilla.org/Gaia/Hacking#Before_submitting_a_patch

@Pin
Thanks for the heads up.  The latest has been gjslinted.

Comment 40

[Peter La]

Attachment: New spinner mockup

Hi Mark,

I've attached a mockup of the design we want implemented.  We feel it's better to keep the button intact for continuity, and put the spinner inside the button border.  The spinner should be the same width, shape, colour and speed as the smaller version.

Let me know if you have any questions/concerns or if you need further assets from me.

Thanks!
Peter

Comment 41

[Mark Shiao]

(In reply to Peter La from comment #40)
> Created attachment 736762 [details]
> New spinner mockup
> 
> Hi Mark,
> 
> I've attached a mockup of the design we want implemented.  We feel it's
> better to keep the button intact for continuity, and put the spinner inside
> the button border.  The spinner should be the same width, shape, colour and
> speed as the smaller version.
> 
> Let me know if you have any questions/concerns or if you need further assets
> from me.
> 
> Thanks!
> Peter

Hi Peter,
Thanks for the mock.  Want to confirm that we are adding the spinner inside the button border and removing the dimming feature in previous mocks.  In addition, can u attach the new spinner creative since I won't be able to use the existing one.  

Thanks.

Comment 42

[Josh Carpenter [:jcarpenter]]

Comment on attachment 736649 [details]
FM Radio in intialization mode

Based on the screenshot, the implementation does not match the mockup. I have not had time to apply the patch and test the actual implementation, however.

Comment 43

[Pin Zhang [:pzhang] (inactive)]

(In reply to Peter La from comment #40)
> Created attachment 736762 [details]
> New spinner mockup
> 
> Hi Mark,
> 
> I've attached a mockup of the design we want implemented.  We feel it's
> better to keep the button intact for continuity, and put the spinner inside
> the button border.  The spinner should be the same width, shape, colour and
> speed as the smaller version.
> 
> Let me know if you have any questions/concerns or if you need further assets
> from me.
> 
> Thanks!
> Peter

@Mark, I have commented your patch on Github, please check it, thanks.

Comment 44

[Peter La]

Attachment: Spinner Graphical Asset

Hi Mark,

1)
We are keeping the dimming.  Apologies if the last mockup was misleading - it was just meant to show the new spinner.

2)
Attached is the asset for the spinner, which is overlayed ontop of the play/pause button.  I've included a 2x version as well, in case you need it.  If you would like the asset in a different format (eg. higher res or whatever), let me know.

Comment 45

[Mark Shiao]

Hi Pin, 

I've updated the code with your suggestions along with some style changes to reflect the desired UX.  Please review when you get the chance.  

Thanks,
Mark

Comment 46

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 736649 [details]
FM Radio in intialization mode

I believe the latest patch comes with the 2x spinner.
Mark could you attach the new screenshot for UX/Visual review?

Comment 47

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 736630 [details]
Quick link to git pull request

Let's not ask UX/Visual to look the source code :P

Comment 48

[Pin Zhang [:pzhang] (inactive)]

(In reply to Mark Shiao from comment #45)
> Hi Pin, 
> 
> I've updated the code with your suggestions along with some style changes to
> reflect the desired UX.  Please review when you get the chance.  
> 
> Thanks,
> Mark

Hi Mark, I have commented your patch on Github, if those nits are fixed, then r=me.

BTW, I found that the duration of the spinner animation lasts 680ms, the same for dim fade in/out, it's fine to me, but I think it's better to let UX guys double check this[1], Peter & Josh, any comments?

[1] http://pinzhang.github.io/gaia/apps/fm/

Comment 49

[Mark Shiao]

Attachment: UI screenshot of updated radio initialization state

UX review of screenshot with updated radio initialization state

Comment 50

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

The ui here seems good enough to move it out of the tef+ list bug if we land it. Is there a patch somewhere that can be reviewed?

Comment 51

[Peter La]

Comment on attachment 738293 [details]
UI screenshot of updated radio initialization state

Hi Mark,

A couple of comments.

1) Are you scaling the 2x spinner to 50% exactly?  There should be no gap between the outer edge of the spinner and the outline of the button.  This isn't a big deal, but it is a departure from the mockup.

2) I'm thinking now that the dimmed channel number at the top is a little too dim.  Originally, I set it for 15% opacity white, but can we bump it up to 20%?  I'll attach a screenshot after this comment to show you.

Thanks,
Peter

Comment 52

[Peter La]

Attachment: New dimmed opacity (20%)

Comment 53

[Mark Shiao]

Attachment: UI screenshot display dimmed @ 20%

Comment 54

[Mark Shiao]

(In reply to Peter La from comment #51)
> Comment on attachment 738293 [details]
> UI screenshot of updated radio initialization state
> 
> Hi Mark,
> 
> A couple of comments.
> 
> 1) Are you scaling the 2x spinner to 50% exactly?  There should be no gap
> between the outer edge of the spinner and the outline of the button.  This
> isn't a big deal, but it is a departure from the mockup.
> 
> 2) I'm thinking now that the dimmed channel number at the top is a little
> too dim.  Originally, I set it for 15% opacity white, but can we bump it up
> to 20%?  I'll attach a screenshot after this comment to show you.
> 
> Thanks,
> Peter

Hi Peter,

In regards to the spinner, the gap is due to the variation in the size of the button and spinner.  The css currently has the button at 72px to match the reflection button image, however the spinner is smaller at 66px thus causing the 3px gap on the edge.  We can stretch the image but that will cause fidelity issues.  Please let me know what you would like to do.

I've also attached a screenshot of the display dimmed @ 20%

Thanks,
Mark

Comment 55

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 736630 [details]
Quick link to git pull request

r=me

Comment 56

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Pin Zhang [:pzhang] from comment #55)
> Comment on attachment 736630 [details]
> Quick link to git pull request
> 
> r=me

Let's make sure if the patch does do what UX ppl wants before merging :)

Comment 57

[Pin Zhang [:pzhang] (inactive)]

Attachment: Patch II: for bug 862672

Hi Mark, because of bug 862672, enabling FM may fail, i.e. mozFMRadio.disable() is called when the FM Radio is enabling, then the enabling spinner will keep showing, the patch is for this scene.

@timdream, I am not sure if it's appropriate to cross review with Mark in the same bug, so add you to the reviewer list, please feel free to re-assign it if there is a better candidate, thx.

Comment 58

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 739416 [details] [diff] [review]
Patch II: for bug 862672

Looks alright.

Comment 59

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 738864 [details]
UI screenshot display dimmed @ 20%

Patryk verified the design with me offline yesterday.

Comment 60

[Tim Guan-tin Chien [:timdream] (please needinfo)]

master: https://github.com/mozilla-b2g/gaia/commit/213d1527bf44afa97b61b0bfe0c4a1b879e1e64d

Comment 61

[James Lal [:lightsofapollo]]

I was not able to uplift this bug to v1-train and v1.0.1.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train and v1.0.1, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 213d1527bf44afa97b61b0bfe0c4a1b879e1e64d
  <RESOLVE MERGE CONFLICTS>
  git commit
  git checkout v1.0.1
  git cherry-pick -x $(git log -n1 v1-train --pretty=%H)

Comment 62

[James Lal [:lightsofapollo]]

Can you supply a patch to uplift / uplift yourself on effected branches?

Comment 63

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

v1-train:
f9853be3ec811ebd1a3d09932c698e4b508df860

Will uplift to v1.0.1 later.

Thanks.

Comment 64

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Sorry, correct the commit hash
v1-train:
340a89ded4d40b7a17bb340e3a9685e5a3149297

Comment 65

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Gaia v1.0.1:
ab5c2e1f6346cb9ffddde7f41e160614ef634e1e

Comment 66

[bov]

Tested with Build:
unagi-ICS.user.manifest.V1.0.1.Rel0.4.Sprint8.B-114.Gecko-ea6a890.Gaia-b750757
REPRODUCING PROCEDURES:
 1.When listening the FM Radio
 2.Pause the FM
 3.Then press play key to continue paly the FM
 4.The FM  response in about 2 seconds -> OK

Comment 67

[Pin Zhang [:pzhang] (inactive)]

@Mark, I found that the changes for index.html are indented by `\t` instead of spaces, I didn't realize it when I was reviewing the patch on Github, it's really my fault, could you correct them on your next bug fixing? 

And I think you should also make your editor well configured to prevent the auto-indention with '\t'.