Closed Bug 853742 Opened 12 years ago Closed 11 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:tef+, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

VERIFIED FIXED
1.0.1 Madrid (19apr)
blocking-b2g tef+
Tracking Status
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: sync-1, Assigned: mshiao)

References

Details

(Whiteboard: [status: uplift-needed][madrid], ux-tracking, ux-priority1.2)

Attachments

(6 files, 5 obsolete files)

+++ 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:
blocking-b2g: --- → tef?
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.
QA Contact: fyen
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.
QA Contact: fyen
Flags: needinfo?(jcarpenter)
Sorry Al, I accidentally overwrote your change.
QA Contact: fyen
pzhang, minding taking a look at this? thanks
Assignee: nobody → pzhang
(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.
Can we dont
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.
(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?
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.
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.
Flags: needinfo?(mvines)
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.
What phones were freezing?  Unagi only?
Flags: needinfo?(mvines)
(In reply to Michael Vines [:m1] [:evilmachines] from comment #12)
> What phones were freezing?  Unagi only?

I think it was unagi - bug 800263 .
Ah, ok.  So maybe we should back this out for v1.0.1?   Unagi isn't a shipping device.
Attached patch Directly invoke fm_qsoc_patches (obsolete) — Splinter Review
This is a bit faster, though I haven't measured it yet.
Attachment #730274 - Flags: review?(mvines)
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.
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 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.
Attachment #730274 - Flags: review?(mvines) → review-
(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.
(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.
Flags: needinfo?(jcarpenter) → needinfo?(pla)
Seems we all agree we should fix it only at a user interface layer, partner request, awful experience, marking it as tef+
blocking-b2g: tef? → tef+
Attached image Mockup - Spinner Location (obsolete) —
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.
Flags: needinfo?(pla) → needinfo?(jcarpenter)
Attachment #734003 - Flags: feedback?(padamczyk)
Attachment #734003 - Flags: feedback?(jcarpenter)
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.
Attachment #734004 - Flags: feedback?(padamczyk)
Attachment #734004 - Flags: feedback?(jcarpenter)
Flags: needinfo?(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.
Attachment #734004 - Flags: feedback?(jcarpenter) → feedback+
(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.
Whiteboard: u=user c=fmradio s=ux-most-wanted
Comment on attachment 734003 [details]
Mockup - Spinner Location

Minusing until we clarify question of competing placement of speaker button.
Attachment #734003 - Flags: feedback?(jcarpenter) → feedback-
(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.
(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.
Assignee: pzhang → mshiao
Attachment #734004 - Flags: feedback?(padamczyk) → feedback-
Attachment #734003 - Flags: feedback?(padamczyk) → feedback-
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 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.
Peter can you provide new mock ups for the above ^
Flags: needinfo?(pla)
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.
(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.
This bug includes the darkening of the fequency display along with replacing the play icon with the spinner when FM is initalizing
Attachment #736630 - Flags: review?(pzhang)
Attachment #736630 - Flags: feedback?(pla)
Attachment #736630 - Flags: feedback?(padamczyk)
Attachment #736630 - Flags: feedback?(jcarpenter)
(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?
(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?
Attached image FM Radio in intialization mode (obsolete) —
@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
Attachment #736649 - Flags: review?(pzhang)
Attachment #736649 - Flags: review?(pla)
Attachment #736649 - Flags: review?(padamczyk)
Attachment #736649 - Flags: review?(jcarpenter)
(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
(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.
Attached image 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
Attachment #734003 - Attachment is obsolete: true
Flags: needinfo?(pla)
(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 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.
Attachment #736649 - Flags: review?(jcarpenter) → review-
(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.
Component: Gaia → Gaia::FMRadio
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.
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
Flags: needinfo?(pzhang)
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?
Attachment #736649 - Attachment is obsolete: true
Attachment #736649 - Flags: review?(pzhang)
Attachment #736649 - Flags: review?(pla)
Attachment #736649 - Flags: review?(padamczyk)
Comment on attachment 736630 [details]
Quick link to git pull request

Let's not ask UX/Visual to look the source code :P
Attachment #736630 - Flags: feedback?(pla)
Attachment #736630 - Flags: feedback?(padamczyk)
Attachment #736630 - Flags: feedback?(jcarpenter)
Whiteboard: u=user c=fmradio s=ux-most-wanted → u=user c=fmradio s=ux-most-wanted [status: needs review pin]
(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/
Flags: needinfo?(pzhang)
UX review of screenshot with updated radio initialization state
Attachment #738293 - Flags: review?(pla)
Attachment #738293 - Flags: review?(padamczyk)
Whiteboard: u=user c=fmradio s=ux-most-wanted [status: needs review pin] → u=user c=fmradio s=ux-most-wanted [status: needs review pla, patryk]
Whiteboard: u=user c=fmradio s=ux-most-wanted [status: needs review pla, patryk] → u=user c=fmradio s=ux-most-wanted [status: needs review pla, patryk][madrid]
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 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
Attachment #738293 - Flags: review?(pla) → review-
Attachment #734004 - Attachment is obsolete: true
Attachment #738293 - Attachment is obsolete: true
Attachment #738293 - Flags: review?(padamczyk)
Attachment #738864 - Flags: review?(pla)
(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 on attachment 736630 [details]
Quick link to git pull request

r=me
Attachment #736630 - Flags: review?(pzhang) → review+
Attachment #730274 - Attachment is obsolete: true
(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 :)
Target Milestone: --- → Madrid (19apr)
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.
Attachment #739416 - Flags: review?(timdream)
Attachment #739416 - Flags: review?(mshiao)
Comment on attachment 739416 [details] [diff] [review]
Patch II: for bug 862672

Looks alright.
Attachment #739416 - Flags: review?(timdream)
Attachment #739416 - Flags: review?(mshiao)
Attachment #739416 - Flags: review+
Comment on attachment 738864 [details]
UI screenshot display dimmed @ 20%

Patryk verified the design with me offline yesterday.
Attachment #738864 - Flags: review?(pla) → review+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: u=user c=fmradio s=ux-most-wanted [status: needs review pla, patryk][madrid] → u=user c=fmradio s=ux-most-wanted [status: uplift-needed][madrid]
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)
Can you supply a patch to uplift / uplift yourself on effected branches?
Flags: needinfo?(mshiao)
v1-train:
f9853be3ec811ebd1a3d09932c698e4b508df860

Will uplift to v1.0.1 later.

Thanks.
Sorry, correct the commit hash
v1-train:
340a89ded4d40b7a17bb340e3a9685e5a3149297
Gaia v1.0.1:
ab5c2e1f6346cb9ffddde7f41e160614ef634e1e
Flags: needinfo?(mshiao)
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
Status: RESOLVED → VERIFIED
Resolution: FIXED → WORKSFORME
Resolution: WORKSFORME → FIXED
@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'.
Whiteboard: u=user c=fmradio s=ux-most-wanted [status: uplift-needed][madrid] → [status: uplift-needed][madrid], ux-tracking
Whiteboard: [status: uplift-needed][madrid], ux-tracking → [status: uplift-needed][madrid], ux-tracking, ux-priority1.2
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: