1164148 - [Music] Pick activity initially displays -00:01 for the duration instead of ---:--

Status: VERIFIED FIXED

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

Description

[Oliver Nelson [:oliverthor]]

Attachment: timelinediff.png

Description:
When a user attempts to import a new track to their ringtones from within the Settings app, the user may observe that the timeline will display "0:00 <--> -0:01", and the preview track does not play initially. The user would expect the track to play upon selecting from their shared list from Music.

PreReq:
* phone has mp3 songs in Music app
Repro Steps:
1) Update a Flame to 20150512010209
2) Open the Settings app
3) Navigate to 'Manage Tones'
4) Tap '+' to observe songs from your Music app
5) Tap a song to open preview
6) Observe UI

Actual:
Music Timeline displays "0:00 <--> -0:01", does not play on open

Expected:
Music Timeline displays full length of song, plays on initial load


Environmental Variables:
--------------------------------------------------

Device: Flame Master
Build ID: 20150512010209
Gaia: 6089234ace8b294a8feef064387604bae16254e3
Gecko: 502e1a5e722f
Gonk: 040bb1e9ac8a5b6dd756fdd696aa37a8868b5c67
Version: 41.0a1 (Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0
*******************************
Issue has different UI on 2.2 and 2.1 for flame devices:
Results: UI displays "00:00 <--> ---:--", song does not play on load

Device: Flame 2.2
BuildID: 20150512002502
Gaia: c4c1bf443f2b01c2ba918780510fd4c639a3c360
Gecko: 70782f19acbf
Gonk: ab265fb203390c70b8f2a054f38cf4b2f2dad70a
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Device: Flame 2.1
BuildID: 20150512001201
Gaia: c80865cb0bf73f1b97defbc646083b404feb3ac4
Gecko: 2fd3ef3fc14a
Gonk: ab265fb203390c70b8f2a054f38cf4b2f2dad70a
Version: 34.0 (2.1) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
--------------------------------------------------

Repro frequency: 5/5
See attached: 
video- https://youtu.be/3hUvmsl4Tgc
screenshot
logcat

Comment 1

[Oliver Nelson [:oliverthor]]

Attachment: logcat_20150512_1151.txt

Comment 2

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

Flame 2.2 and 2.1 seems to have the same issue as 3.0 but just exhibit slightly different behavior. NI component owner to see if this should block release

Comment 3

[No-Jun Park [:njpark]]

The reason that 2.2 and 2.1 has different issue could be because of the fix for bug 1150322, but it's strange since that bug is already uplifted as well.  Perhaps :djf might be able to see whether it is related.

The problem with this bug is that it does not auto start the music. The song does play when the user taps the play button. I'm sort of surprised to see that this is not a regression, but it does not stop the use to check on the ringtone.  ni?ing hema for assessment.

Comment 4

[David Flanagan [:djf]]

First off, this issue is with the Music app's pick activity, and is not specific to ringtones, and it is certainly not a settings app bug, so I've changed the component to music and have edited the bug description.

No-Jun says in comment 3 that the bug is that the music does not auto-start. I suspect that was an intentional ux decision. If you're listening to a song and then decide you want it as your ringtone, or decide that you want to email or text it to a friend, you'd like to be able to pick it without interrupting the music that is currently playing.  So I think that is correct as it stands. Let's ignore that part. (No-Jun you can consult with UX on that if you want to double-check)

From the screenshot that Oliver attached, it seems like the major issue he was reporting is that the display shows a duration of one second when, in fact, we have no idea what the duration of the song is. That part of the bug is 3.0 only and is a (minor) regression.

In bug 1150322, I landed a gaia patch to help work around the issue before we had a gecko patch. Once we got the gecko patch we never uplifted the gaia patch to 2.2, so I'm guessing that this one second duration thing on master is a regression that I caused with 1150322.

Hub: is this something you have time to take?  If not, please assign it to me and I'll get to it once my current batch of 2.2 gallery bugs are under control.

Clearing the needinfo on Hema.

Comment 5

[No-Jun Park [:njpark]]

Thanks for the clarification. :djf is right, the issues can be summarized as:
- The selected song does not auto-start (this could be per design)
- Counter displays --:-- or -0:01

So I'm guessing the initial display of --:-- is correct, because this is about the ringtone and not about playing a song?

Comment 6

[Hubert Figuiere [:hub]]

I can reproduce....

--:-- is when we have no duration.
-0:01 we got a duration of one second.

Comment 7

[Hubert Figuiere [:hub]]

David,

If I revert your patch for bug 1150322 I get -:-- until I play.

Shall we revert it? (I can do it)


Thanks

Comment 8

[David Flanagan [:djf]]

Reverting that patch would re-introduce code that we know to be wrong (the startTime thing), so I'd rather figure out a better fix here. 

Where is the 00:01 coming from? This happens for songs of any length, so it seems to me that something weird is going on. Is the <audio> element actually reporting a one second duration?

Comment 9

[Hubert Figuiere [:hub]]

The -0:01 is the '1' that is the "max" value of the seek bar that is set player_view.js:771 when we pass a duration that is NaN with a currentTime of 0 (this change come from bug 1150322). I don't know where that '1' value come from though.

If I put a breakpoint at player_view.js:781 (it is first called to "reset" the seek bar) then I end up getting the correct song time displayed - like because this time we do get the currentTime properly.

So it looks like that: 

1. the changes in bug 1150322 force having a finite endTime that we use to set seekBar.max - but then seekBar.max is already set at 1.
2. we are getting the event "timeupdate"/"durationchange" but the audio.duration isn't set properly yet... (the breakpoint temporise this)

I'll dig it more.

Comment 10

[Hubert Figuiere [:hub]]

As for seekBar.max it is set to 1 because it can't be zero:

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/progress

"The max attribute, if present, must have a value greater than zero and be a valid floating point number."
So addressing item 1. to not set the endTime if it is not finite ought to be enough to keep the previous behaviour.

Comment 11

[GitHub Autolander Bot]

Attachment: [gaia] hfiguiere:bug1164148 > mozilla-b2g:master

Comment 12

[Hubert Figuiere [:hub]]

Comment on attachment 8608327 [details] [review]
[gaia] hfiguiere:bug1164148 > mozilla-b2g:master

cancelling review for now. After looking more closely, it looks like the fix has to be more thorough in the way we deal with the <progress> element.

Comment 13

[David Flanagan [:djf]]

Comment on attachment 8608327 [details] [review]
[gaia] hfiguiere:bug1164148 > mozilla-b2g:master

I left a couple of comments on github: you probably shouldn't set the accessiblity properties when we have an unknown duration.

Otherwise, the patch looks okay. (I didn't test it though).

Note that if gecko ever returns an infinite duration then this patch will take us back to where we were in 1150322 with the thumb displayed to the far left instead of the far right.

Another approach would be to leave in the Math.max() for endTime but add a new flag that specifies whether we've ever gotten a valid duration from gecko. If that flag is not set (no valid duration received) then the code that actually displays the remaining time in the song can just always display "---:--".  Actually, you probably don't even need a flag to do it that way: maybe in the code that displays the remaining time, you could just use audio.duration instead of slider.maxValue, and if duration is not finite, then always display "---:--".

I'd slightly prefer that approach, but I'm okay with landing this patch as it stands if you'd prefer to just be done with this for now. Also, I like the simplification in this patch of getting rid of the startTime argument.

Comment 14

[Hubert Figuiere [:hub]]

Thanks.

Dealt with the aria-maxvalue case and rebased.

When we support stream we'll have to revisit the slider anyway.

Comment 15

[Hubert Figuiere [:hub]]

Merged
https://github.com/mozilla-b2g/gaia/commit/47ebdd5d942e980bfac9fe097d28ffdd7645a107

Comment 16

[Shally Li（leave from Mozilla）]

This bug has been verified as "pass" on latest Nightly build of Flame v3.0 by the STR in Comment 0.

Actual results: Music Timeline displays full length of song and plays on initial load, and it does not shows the "-00:01" before music plays.
See attachment: verified_v3.0.mp4
Reproduce rate: 0/6

Device: Flame v3.0 build(Pass)
Build ID               20150614010203
Gaia Revision          1bf2da102560481748ff3f6202fbed5c4daa5832
Gaia Date              2015-06-13 00:22:05
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/c223b8844264
Gecko Version          41.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150614.044513
Firmware Date          Sun Jun 14 04:45:25 EDT 2015
Bootloader             L1TC000118D0

Comment 17

[Shally Li（leave from Mozilla）]

Attachment: verified_v3.0.3gp