1145758 - [Music] Dragging the timeline selector / scrubber to the end of a paused track will load the next track and automatically unpause it.

Status: VERIFIED FIXED

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

Description

[Joshua Mitchell (Inactive)]

Attachment: logcat_20150320_1237.txt

Description:
Pausing a music track and dragging the selector to the end of the track not only advances the track but it also over-rides the user input / intent and unpauses the music app as well. Only tapping the unpause and perhaps the track forward button should remove the current pause state.

Repro Steps:
1) Update a Flame to 20150320010204
2) Launch Music App
3) Select and begin playing a song
4) Pause the song and drag the timeline selector to the end of the track


Actual:
next track is loaded and the app unpauses

Expected:
app should remain in the pause state

Environmental Variables:
Device: Flame Master
Build ID: 20150320010204
Gaia: 8837f94418d69a0b06c1f4843b0779e2bb72165a
Gecko: 4d2d97b3ba34
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 39.0a1 (Master)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0


Repro frequency: 7/7
See attached: logcat

Comment 1

[Joshua Mitchell (Inactive)]

This issue also occurs on Flame 2.2, 2.1 and 2.0

Device: Flame 2.2 (KK - Nightly - Full Flash - 319mem)
Build ID: 20150318055750
Gaia: b8051d370ddf4e5bd8e7d8a19fb9eeb5fd6ffb39
Gecko: 41a61514461e
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 39.0a1 (Master)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Device: Flame 2.1 (KK - Nightly - Full Flash - 319mem)
Build ID: 20150318001207
Gaia: 13c85d57f49b4bfd657ff674f2b530c141c94803
Gecko: 2fbd284621e2
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 34.0 (2.1)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.0 (KK - Nightly - Full Flash - 319mem)
Build ID: 20150316000201
Gaia: 896803174633fc6acd3fd105f81c349b8e9b9633
Gecko: 2fc0c40fd074
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 32.0 (2.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 2

[KTucker [:KTucker][Inactive 3/4/2016]]

Android, iOS and Windows phones do not jump to the next song when scrubbing to the end of a song while in a paused state.

Not nominating to block on this since this appears to be a legacy issue.

Comment 3

[No-Jun Park [:njpark]]

ni?ing tif again to confirm whether it matches our current ux guideline

Comment 4

[Jacqueline Savory [:jsavory] UX]

I'll answer for Tif here as I am covering music UX.
The expected behaviour is correct, if the song is paused it should not skip to the next song even when the user drags the scrubber to the end.

Comment 5

[No-Jun Park [:njpark]]

In light of Comment 4, although it's not a regression, since it is something that can be hit rather easily, (and when it happens, the user might be frustrated to know that the song has changed) nominating for 2.2 to gauge consensus.

Comment 6

[Hema Koka [:hema]]

I am wondering if this deliberate act of pushing the slider to end of a paused song is a common user scenario. I don't think we should block a release on this. But lets discuss this in the triage and decide. 

Thanks
Hema

Comment 7

[Hema Koka [:hema]]

Not blocking for 2.2, but setting as priority bug to fix in the next release. Hub, please take this on. 

Thanks
Hema

Comment 8

[GitHub Autolander Bot]

Attachment: [gaia] hfiguiere:bug1145758-scrubber-paused > mozilla-b2g:master

Comment 9

[Hubert Figuiere [:hub]]

I propose we look at removing the workaround in bug 1154048. (see gh comments)

Comment 10

[Jim Porter (:squib)]

Comment on attachment 8590059 [details] [review]
[gaia] hfiguiere:bug1145758-scrubber-paused > mozilla-b2g:master

Sure. With that in mind, let's land this as a minimal fix for the bug here.

Comment 11

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/00130552f1913dab822762ba85d47ba2825bf80e

Comment 12

[Hubert Figuiere [:hub]]

Comment on attachment 8590059 [details] [review]
[gaia] hfiguiere:bug1145758-scrubber-paused > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Broken feature.
[User impact] if declined: Feature will stay broken. Not a regression.
[Testing completed]: Manually tested.
[Risk to taking this patch] (and alternatives if risky): none I can see
[String changes made]: none

Comment 13

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/14fb051e4e4bc47cd5355915a79122b56f291e76

Comment 14

[Shally Li（leave from Mozilla）]

According to the STR of Comment 0,this bug has been successfully verified on latest Nightly Flame v3.0.
See attachment: verified_v3.0.mp4.
Reproduce rate: 0/5

Device: Flame 3.0 build(Pass)
Build ID               20150414160204
Gaia Revision          8e28588496f82f8f069c171c65842d622b9d8d7d
Gaia Date              2015-04-14 18:43:50
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/de27ac2ab94f
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150414.194002
Firmware Date          Tue Apr 14 19:40:12 EDT 2015
Bootloader             L1TC000118D0

Comment 15

[Shally Li（leave from Mozilla）]

Attachment: verified_v3.0.mp4

Comment 16

[Shally Li（leave from Mozilla）]

According to the STR of Comment 0,this bug has been successfully verified on latest Nightly Flame v2.2.

Actual results:Pause the song and drag the timeline selector to the end of the track,music remains in the pause state.
Reproduce rate: 0/5


Leaving "verifyme" for Flame v2.1/2.0 uplift.

Comment 17

[Shally Li（leave from Mozilla）]

Device: Flame 2.2 build(Pass)
Build ID               20150415162504
Gaia Revision          89a94a8b9c9087da916751697a61ba15bc0688c3
Gaia Date              2015-04-15 20:05:38
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/20e92b626530
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150415.201243
Firmware Date          Wed Apr 15 20:12:52 EDT 2015
Bootloader             L1TC000118D0

Comment 18

[David Flanagan [:djf]]

This bug caused a serious regression. See duplicate bugs 1155118 and 1156519. Reopening it so we can back it out of master and out of 2.2

In Jim's review of the patch he suggested that maybe we could just remove the really old endedTimer workaround. We didn't have solid evidence that the workaround was no longer needed though.  While investigating bug 1156519, however, I discovered that the workaround was no longer being used any time the app was in the background. And since we haven't had trouble with the app working in the background, I think that is evidence that we don't need the workaround anymore.

So once we back this patch out, I'd guess we can resolve this bug by just removing all the endedTimer code.

Hub is busy with bug 1150322, so I'm going to take this one.

Comment 19

[GitHub Autolander Bot]

Attachment: [gaia] davidflanagan:bug1156519 > mozilla-b2g:master

Comment 20

[David Flanagan [:djf]]

Attachment: back out the previous patch

Comment 21

[Jim Porter (:squib)]

I'm not entirely convinced that the workaround can just be removed. I think we might need it for when we fast forward (by holding the >> button) through the end of a track and into the next. It's possible we could do something else for that, though.

Comment 22

[David Flanagan [:djf]]

Comment on attachment 8595446 [details] [review]
back out the previous patch

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): the previous patch in this bug
[User impact] if declined: music app won't work
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): not risky
[String changes made]: none

Comment 23

[David Flanagan [:djf]]

Backed out on master: https://github.com/mozilla-b2g/gaia/commit/5b4660ba64e7792bb7bc8369056c6d7419dc99fb

Comment 24

[David Flanagan [:djf]]

Backed out of v2.2: https://github.com/mozilla-b2g/gaia/commit/41a85c5f9db291d4f7c0e94c8416b5115b4ee407

Comment 25

[David Flanagan [:djf]]

Next step is to create a patch that just removes the really old endedTimer workaround and see whether that is enough to fix this bug. I should be able to do that later today.

Comment 26

[Hubert Figuiere [:hub]]

Bug 1154048? There is already a PR.

Comment 27

[David Flanagan [:djf]]

(In reply to Hubert Figuiere [:hub] from comment #26)
> Bug 1154048? There is already a PR.

That PR probably won't apply now that we've backed this out.  And that PR removes the code that sets endedTimer, but doesn't remove the other code that uses it or initializes it.

Comment 28

[David Flanagan [:djf]]

If I just remove the endedTimer workaround, then dragging the scrubber to the end of a song while paused does not unpause it.  So that resolves this bug.

But now we've got another problem: when we're in this state pressing the play button does not start the next song for something like 10 seconds!  And that seems like an equally bad bug, so I think I need to investigate that as well.

Comment 29

[Hubert Figuiere [:hub]]

(In reply to David Flanagan [:djf] from comment #27)
> (In reply to Hubert Figuiere [:hub] from comment #26)
> > Bug 1154048? There is already a PR.
> 
> That PR probably won't apply now that we've backed this out.  And that PR
> removes the code that sets endedTimer, but doesn't remove the other code
> that uses it or initializes it.

Will close PR and dupe bug then.

Comment 31

[David Flanagan [:djf]]

I've tested my patch that removes endedTimer with this MP3 file https://bug783512.bugzilla.mozilla.org/attachment.cgi?id=659976 which was the one that was originaly used to reproduce bug 738512 which is what the endedTimer was working around. Bug 738512 seems to have gone away, no one is sure why, but I can confirm that that mp3 plays correctly to the end and we get an ended event and move on to the next song just as we are supposed to even without the workaround code.

Comment 32

[David Flanagan [:djf]]

More observations:

If I pause, seek to the end, and then press play, it takes quite a while before the next song starts. The app is seeking to within a fraction of a second of the end of the song (because of the Math.floor() call in seekAudio()). Then, when I press play, I get a playing event right away, but there is a long pause before I get the ended event. This only happens when I'm near the end of the song.  If I seek to the middle then press play, playback is fast. And, the length of the delay before I get the seek event seems to be proportional to the length of the song. For a 5 minute song, it is around 12 seconds. For a 2 minute song, it is more like 6 seconds.  For the 7second MP3 mentioned above, there is not a noticeable delay. (I assume because of the length, but maybe because of the audio type.  My other songs are all .m4a files.)

So my hypothesis here is that when we get really close to the end of the song like that, gecko discards its decoded audio buffer, and when we restart it, gecko has to decode and seek to the end again. 

The solution is going to be to notice that we are at or very near the end of the song when the user pressses play and instead of playing the same song, to skip to the next one instead.

This would be easier if there wasn't that weird Math.floor() call in seekAudio(). But if I comment that out, I find that I'm back to the original bug here, but for mp3s only. Without the floor() call, seeking to the end while paused generates an ended event and we skip to the next song.  I think this is mp3 only, though.

Comment 33

[David Flanagan [:djf]]

I've tried again with some longer mp3 files and .ogg files. The long pause between songs does not happen for those files types; it only occurs for my .m4a files.  Also, without the floor() call, seeking all the way to the end of an ogg file while paused just starts the next song instead of staying paused. And this seems to happen for some MP3s but not for others.

Comment 34

[GitHub Autolander Bot]

Attachment: [gaia] davidflanagan:bug1145758 > mozilla-b2g:master

Comment 35

[David Flanagan [:djf]]

Comment on attachment 8595535 [details] [review]
[gaia] davidflanagan:bug1145758 > mozilla-b2g:master

Jim,

I thought I could fix this bug by just backing out the old workaround.  But when I did that, I found that this bug was masking another issue where resuming a .m4a audio file from the very end would take ~10s to get and ended event and skip to the next song. 

So this PR has two commits currently. The first just backs out the endedTimer thing. And the second commit handles the case where we press the play button when we are less than one second from the end of a song.

This second commit means that during a pick activity, if the user pauses and then seeks to the end and then presses play, we will just skip to the start of the song and pause again. So the user has to tap play twice to restart the song. But if I'm understanding that code correctly that is what would have happened before this patch, too. The user just doesn't have to wait as long now.

Actually now that I'm writing this out, I realize that I should only apply the "near the end" test for songs that are > 10s or something. So I'm going to push one more update to the PR.

Comment 36

[Jim Porter (:squib)]

Comment on attachment 8595535 [details] [review]
[gaia] davidflanagan:bug1145758 > mozilla-b2g:master

This looks mostly good, but there's at least one actual bug (and another bit I'm not sure about); see GitHub for comments.

Comment 37

[David Flanagan [:djf]]

Comment on attachment 8595535 [details] [review]
[gaia] davidflanagan:bug1145758 > mozilla-b2g:master

Thanks for catching the this.next(true) issue, Jim. I've updated the patch based on your comments and am ready for re-review.

Comment 38

[Jim Porter (:squib)]

Comment on attachment 8595535 [details] [review]
[gaia] davidflanagan:bug1145758 > mozilla-b2g:master

r=me if you're sure about the 20s length in the patch (although a comment about it would be nice). Bonus if you file a bug about the Gecko issue.

Comment 39

[David Flanagan [:djf]]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/a7dcc5fb595030dab140d5ff0e7eb5ef04017d51

(I forgot to use autolander)

Comment 40

[David Flanagan [:djf]]

Comment on attachment 8595535 [details] [review]
[gaia] davidflanagan:bug1145758 > mozilla-b2g:master

This bug was never made a 2.2 blocker, but the previous fix that I backed out had been uplifted, so I'm nominating this for uplift as well.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): not a regression
[User impact] if declined: seeking to the end while paused will start playing the next song, which is kind of strange, but not completely terrible. I noticed that my Android phone does that, too.
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): it removes a very old workaround that we're pretty certain is no longer needed, and in fixing this I had to workaround a new gecko issue, so this is not a no-risk patch, but it is not high risk.
[String changes made]: none.

I could really go either way here. It would be nice to uplift but not necessary.  What do you think, Jim? Would you recommend uplifting this?

Comment 41

[David Flanagan [:djf]]

I filed bug 1157118 for the underlying gecko issue described in comment #32.

Comment 42

[Jim Porter (:squib)]

Personally, I'd rather not uplift (but then, I'll almost invariably say this about anything that's not a regression or dataloss bug). Especially in this case, I don't know if we can be 100% sure that removing the track-end workaround will work in all cases. I think it's safe to do this on 3.0, but only because we'll have plenty of time to catch any problems.

Comment 43

[Joshua Mitchell (Inactive)]

Issue has been verified fixed on the latest Nightly build

Actual Results: Pausing the track and dragging the selector to the end of the timeline does not advance the track to the next one. 

Device: Flame 3.0 (KK - Nightly - Full Flash - 319mem)
Build ID: 20150423010203
Gaia: 9d4f756aa35cb7f030a92f3c1f65fb55254ddd1d
Gecko: 0b202671c9e2
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Comment 44

[bhavana bajaj [:bajaj]]

(In reply to David Flanagan [:djf] from comment #40)
> Comment on attachment 8595535 [details] [review]
> [gaia] davidflanagan:bug1145758 > mozilla-b2g:master
> 
> This bug was never made a 2.2 blocker, but the previous fix that I backed
> out had been uplifted, so I'm nominating this for uplift as well.
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #): not a regression
> [User impact] if declined: seeking to the end while paused will start
> playing the next song, which is kind of strange, but not completely
> terrible. I noticed that my Android phone does that, too.
> [Testing completed]: yes
> [Risk to taking this patch] (and alternatives if risky): it removes a very
> old workaround that we're pretty certain is no longer needed, and in fixing
> this I had to workaround a new gecko issue, so this is not a no-risk patch,
> but it is not high risk.
> [String changes made]: none.
> 
> I could really go either way here. It would be nice to uplift but not
> necessary.  What do you think, Jim? Would you recommend uplifting this?

I think we should be fine to live without this in 2.2 given where we are in the release cycle. So unless its helping any existing blockers or a blocker in itself and this has been already backed out once, I am hesitant. Also note this isn't a regression.