Closed Bug 877313 Opened 7 years ago Closed 7 years ago

[Music] Progress bar indicator for short music overlaps the remaining time space.

Categories

(Firefox OS Graveyard :: Gaia::Music, defect)

x86
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:leo+, b2g18 fixed)

RESOLVED FIXED
1.1 QE2 (6jun)
blocking-b2g leo+
Tracking Status
b2g18 --- fixed

People

(Reporter: leo.bugzilla.gaia, Assigned: dkuo)

Details

(Whiteboard: [TD-24236] c=music , MiniWW)

Attachments

(2 files)

1. Title : Music Progress bar overlaps the remaining time space.
2. Precondition : Load a short music file which is less than 10 secs(play duration less than 10 secs)
3. Tester's Action : (1) Open Music App
                     (2) Play the song (song duration should be lesser than 10 secs)
                     (3) Check the progress bar.
4. Detailed Symptom (ENG.) : Music progress bar indicator overlaps the remaining time space (--:--).
5. Expected : Music progress bar indicator should not overlap remaining time space (--:--).
6.Reproducibility: Y
1)Frequency Rate : 100%
blocking-b2g: --- → leo+
OS: Windows XP → Gonk (Firefox OS)
Whiteboard: MiniWW
Assignee: nobody → dkuo
Whiteboard: MiniWW → [TD-24236] c=music , MiniWW
Target Milestone: --- → 1.1 QE2 (6jun)
David,

The root cause of this issue is the seekAudio() of Player.js, I had Math.floor() on the audio.duration(endTime) which I guess I was trying to get a rounded number from the bug that mp3 returns microseconds, this will cause a noticeable bug when the player is playing a short song, such as a 3~5 seconds song.

Becasue the endTime was floored, like 3.5 was changed to 3, but the currentTime could be any floating between 3.0~3.5, so the indicator will be out of the seek area when the currentTime is large than endTime, I didn't notice this cause in my test data I don't have short songs, and I am glad Leo has found this one.

This should be a simple fix with the mp3 workaround/comments removed.
Attachment #755617 - Flags: review?(dflanagan)
Comment on attachment 755617 [details]
Remove Math.floor() on the audio duration

The fix looks good. I haven't actually tested, though. r=djf for github commit  2092b1a.

As part of my review, I looked in utils.js at formatTime() to make sure it wasn't assuming that floor had been called.

formatTime() looks like it is using parseInt as a strange substitute for Math.floor(). That is inefficient (numbers repeatedly converted to strings and then back to numbers) and this function is called frequently.

You should only need a single call to Math.floor() at the start of formatTime.

Also, formatTime() will do the wrong thing with times > 24 hours: they shouldn't wrap around. If you have a very, very long song file, it should just display all the hours.

So r+, but I suggest you include a fix to formatTime() in this patch since it is time-related anyway.
Attachment #755617 - Flags: review?(dflanagan) → review+
Thanks for reviewing this, David, besides removing the workaround I have also addressed those issues in formatTime() which you suggested to fix, and I am landing it with r+.
Landed on master: 204025be379064233596eadb08f6766c029bcbbf
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Uplifted 204025be379064233596eadb08f6766c029bcbbf to:
v1-train: ec36e38d6e0ae4d5e3cd7d6d8b3cc731bc9db350
Flags: in-moztrap?
Added test case in MozTrap: 
https://moztrap.mozilla.org/manage/case/8519/
Flags: in-moztrap? → in-moztrap+
You need to log in before you can comment on or make changes to this bug.