Closed Bug 877313 Opened 7 years ago Closed 7 years ago
[Music] Progress bar indicator for short music overlaps the remaining time space
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 → [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
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.