1017421 - [ringtones][Flame only] previewing ringtones doesn't stop when tapped again

Status: VERIFIED FIXED

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

Description

[Tiffanie Shakespeare [:tif] - slack @tif preferred]

Attachment: logs.rtf

STR
1. Go to Settings -> Sounds -> Manage Ringtones
2. Tap on a ringtone to play

Expected:
Tapping on the ringtone again will stop playing

Actual:
Nothing happens.

I'm attaching my logs in case that is helpful. It seems like this feature might be intermittently working as when I first asked Jim about it he said it worked and my subsequent trial did see that it worked. But now in trying it again it doesn't seem to be working anymore.

Comment 1

[Jim Porter (:squib)]

It's possible that the touch event just isn't firing (e.g. because the screen is dirty or something). Maybe we should add some visual feedback when you tap a ringtone to preview it?

Comment 2

[Tiffanie Shakespeare [:tif] - slack @tif preferred]

Updating with more info - I've discovered that it's only the custom ringtones. Tapping again on any of the Firefox one will stop the playback. For some reason the custom ones don't do that.

Comment 3

[Jim Porter (:squib)]

I think this is a Flame bug, since it works for me on an Inari with the latest gecko/gaia, but fails with the very same builds on the Flame.

The issue here is that the "canplay" event is never fired for the Audio object, so we end up erroring out. This also breaks setting a custom ringtone from the picker, so this is pretty bad. I'm not sure who's the best person to annoy about Flame-only issues. Ideas?

Comment 4

[Jim Porter (:squib)]

Oh, for anyone looking at this, the way things are set up in the ringtones app is that we have an Audio object fed through an AudioContext, and then we're playing a blob: URL. In this case, the "canplay" event is never fired. It works fine for app: URLs from within our app, though.

Comment 5

[Jason Smith [:jsmith]]

QA Wanted to test this across different devices to check if this is truly device specific or not.

Comment 6

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

I've checked the Flame and Buri 2.0 engineering builds and I'm unable to find the area described by the reporter.  When pressing change ringtone, no custom ringtones appear even though I had previously set a custom ringtone using the Music app.  Any more information you could provide would be useful.

Comment 7

[Jim Porter (:squib)]

What's the commit ID for the gaia revision you're on? It's possible that you're not on a current-enough version. Also note that you'll probably need to reset your profile on the device for everything to work.

Comment 8

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

I thought I had included these variables in my previous comment but apparently not.  Sorry for any inconvenience.

Environmental Variables:
Device: Flame
BuildID: 20140530013001
Gaia: 26d8fcab9b61f46451600f39c51e0387ef3c4f88
Gecko: e6f113c83095
Version: 32.0a1

Comment 9

[Jim Porter (:squib)]

(In reply to Jayme Mercado from comment #8)
> Gaia: 26d8fcab9b61f46451600f39c51e0387ef3c4f88

This commit predates the ringtones changes by 5 commits. You'll need commit 1477f488ddc8746b8efe8ba32a3a35a545a01422 or newer.

Comment 10

[Josh Schmitt [Joshs]]

(In reply to Jason Smith [:jsmith] from comment #5)
> QA Wanted to test this across different devices to check if this is truly
> device specific or not.

Issue does NOT repro on 2.0 Buri.

Issue DOES repro on 2.0 Flame.

Environmental Variables:
Device: Buri
Build ID: 20140604040204
Gaia: 1d4f6f7312882e78b57971152de75d1281a26187
Gecko: 668f29cd71b3
Version: 32.0a1 (2.0) MOZ
Firmware Version: v1.2device.cfg

Environmental Variables:
Device: Flame
Build ID: 20140604040204
Gaia: 1d4f6f7312882e78b57971152de75d1281a26187
Gecko: 668f29cd71b3
Version: 32.0a1 (2.0) 
Firmware Version: v10G-2

Comment 11

[Hema Koka [:hema]]

Jim, please keep an eye on this and get it fixed for sprint 4 if it is still an issue with latest (bug associated with feature going into 2.0)

Comment 12

[Jim Porter (:squib)]

This is still happening on the latest base build, gecko, and gaia. Who would be a good person to annoy about this? It should *definitely* block[1], since it means that you can't even set a custom ringtone from the ringtone picker! (We try to validate the ringtone first, and validation is stalling.)

[1] Unless we're ok with this being totally broken on flame.

Comment 13

[Stephany Wilkes]

Thanks for the nom, Jim. This is a must-have and cannot be broken.

Comment 14

[Hema Koka [:hema]]

accidentally moved it out of 2.0?...meant to block it in comment 11 based on discussion in media triage last week

Comment 15

[Hema Koka [:hema]]

Francis/Wayne,

We are unable to reproduce this issue on other devices (buri, inari) that we have. It looks like this issue is only happening on flame. However, we are not certain if this is a vendor issue or something on our side that is only surfacing on flame. We would like to request some help from you to see if the vendor can confirm that it is not an issue on their side. 

Also NI Eric to see if he has any insights on what the problem could be (see comment 3). 

Thanks
Hema

Comment 16

[Jason Smith [:jsmith]]

QA Wanted to check Open C on 2.0. If it happens there, then it's a JB-specific issue.

Comment 17

[Francis Lee [:frlee]]

hi Mike,

would you please help to test this issue on V122 (latest Flame v1.3 release) and see if this issue can be reproduced?

Comment 18

[Mike Lien[:mlien]]

verify with Flame v122 base image and Buri v1.3
both of them can reproduce this issue

Buri:
Gaia      7f361e67aa65e40adb2233ab247ebd0f142cfbf1
Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/4afb49342d25
BuildID   20140617024001
Version   28.0

Comment 19

[Jason Smith [:jsmith]]

(In reply to Mike Lien[:mlien] from comment #18)
> verify with Flame v122 base image and Buri v1.3
> both of them can reproduce this issue
> 
> Buri:
> Gaia      7f361e67aa65e40adb2233ab247ebd0f142cfbf1
> Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/4afb49342d25
> BuildID   20140617024001
> Version   28.0

This can't be right. Jim & QAnalyst's analysis concluded this was present on Flame, but not Buri. So I'm confused by the testing results here.

I'll ask QAnalysts to check again though.

Comment 20

[Jim Porter (:squib)]

Also, this shouldn't be happening in 1.3 at all because the relevant code is only present in 2.0.

Comment 21

[Eric Chou [:ericchou] [:echou]]

(In reply to Hema Koka [:hema] from comment #15)
> Francis/Wayne,
> 
> We are unable to reproduce this issue on other devices (buri, inari) that we
> have. It looks like this issue is only happening on flame. However, we are
> not certain if this is a vendor issue or something on our side that is only
> surfacing on flame. We would like to request some help from you to see if
> the vendor can confirm that it is not an issue on their side. 
> 
> Also NI Eric to see if he has any insights on what the problem could be (see
> comment 3). 

I can reproduce on my Flame with the latest gecko/gaia master(2.1). Bruce will take a first look since he is much more familiar than me in this topic.

Comment 22

[Hema Koka [:hema]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #21)
> (In reply to Hema Koka [:hema] from comment #15)
> > Francis/Wayne,
> > 
> > We are unable to reproduce this issue on other devices (buri, inari) that we
> > have. It looks like this issue is only happening on flame. However, we are
> > not certain if this is a vendor issue or something on our side that is only
> > surfacing on flame. We would like to request some help from you to see if
> > the vendor can confirm that it is not an issue on their side. 
> > 
> > Also NI Eric to see if he has any insights on what the problem could be (see
> > comment 3). 
> 
> I can reproduce on my Flame with the latest gecko/gaia master(2.1). Bruce
> will take a first look since he is much more familiar than me in this topic.

Thanks Eric! This is only happening on flame. NI Bruce so he can update based on his investigation.

Thanks
Hema

Comment 23

[Brogan Zumwalt [Inactive]]

Issue DOES occur on 2.0 Open C

Environmental Variables:
Device: Open_C 2.0
Build ID: 20140618000202
Gaia: 83844c7679b3b9f6e7f1116c1eeec2d1e7a64eec
Gecko: 55679dc2e72b
Version: 32.0a2 (2.0) 
Firmware Version: P821A10V1.0.0B06_LOG_DL

Actual result: When tapping on custom ringtone, tapping again does not stop ringtone preview

Comment 24

[Bruce Sun [:brsun]]

It seems we have some odd behavior while reusing the same HTMLMediaElement to play different contents. I am going to dig into it.

Comment 25

[Bruce Sun [:brsun]]

(In reply to Bruce Sun [:brsun] from comment #24)
> It seems we have some odd behavior while reusing the same HTMLMediaElement
> to play different contents. I am going to dig into it.
The reusing mechanism seems good.

One odd behavior found:
 - |AudioDecodedUsecs()| grows too slow in this case, so that |AudioDecodedUsecs() > LOW_AUDIO_USECS * mPlaybackRate| keeps false for a long time. As a result, |MediaDecoderStateMachine::GetNextFrameStatus()| keeps returning |MediaDecoderOwner::NEXT_FRAME_UNAVAILABLE|, and |HTMLMediaElement::mReadyState| keeps |nsIDOMHTMLMediaElement::HAVE_ENOUGH_DATA|, and "canplay" is not fired.
   - http://dxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLMediaElement.cpp#3181
 - Comparing to the playback of the same file in Music app, we don't have such odd behavior. Not found the root cause of this kind of difference yet.
 - Just for the experimental purpose, set |LOW_AUDIO_USECS| to a lower value (ex. 100000) can ease this issue.

Comment 26

[Bruce Sun [:brsun]]

Although the "canplay" event fired too late or event not fired in this case, I believe that this event is not used for validation purpose. This event is more related to the buffer status regarding to the current playback position:
 - http://www.w3.org/TR/html5/embedded-content-0.html#ready-states
 - http://www.w3.org/TR/html5/embedded-content-0.html#event-media-canplay

Hi Jim,
Would you mind thinking the possibility to not use the "canplay" event just for validation?
 - https://github.com/mozilla-b2g/gaia/blob/master/apps/ringtones/js/tone_player.js#L67

Comment 27

[Jim Porter (:squib)]

If there's a better event that guarantees an audio file is valid, then I'm all ears.

Comment 28

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Jim Porter (:squib) from comment #27)
> If there's a better event that guarantees an audio file is valid, then I'm
> all ears.

How about http://dev.w3.org/html5/spec-preview/media-elements.html#event-media-loadedmetadata?

Comment 29

[Bruce Sun [:brsun]]

You can directly call |this._player.pause()| when |this._player.paused| attribute is false.

Comment 30

[Bruce Sun [:brsun]]

(In reply to Bruce Sun [:brsun] from comment #29)
> You can directly call |this._player.pause()| when |this._player.paused|
> attribute is false.
to stop the playback.

Comment 31

[Jim Porter (:squib)]

(In reply to Bruce Sun [:brsun] from comment #30)
> (In reply to Bruce Sun [:brsun] from comment #29)
> > You can directly call |this._player.pause()| when |this._player.paused|
> > attribute is false.
> to stop the playback.

That doesn't actually help though (see comment 12). We *need* these audio files to be validated before we save them so that we don't end up with an invalid file set as the default ringtone. Doing what you suggest would just fix one small part of the problem.

Comment 32

[Chris Pearce [:cpearce (Not reading bugmail)]]

As JW Wang suggests, "loadedmetadata" could be used here instead of "canplay". That won't fire until the first chunk of audio data has decoded. So if "loadedetadata" fires, you can assume the file will play.

Comment 33

[Bruce Sun [:brsun]]

Un-assign myself for ringtone player to do corresponding changes.

Comment 34

[Jim Porter (:squib)]

Attachment: Switch to loadedmetadata

Ok, this seems to work now. Not sure how to test this, since it only happens on a real device (and only a handful of devices at that).

Comment 35

[David Flanagan [:djf]]

Comment on attachment 8446318 [details] [review]
Switch to loadedmetadata

This change is simple and the code is correct by inspection. cpearce assures us that loadedmetadata is a suitable event.  So looks good to me.

Given that you just recently fixed the bug where this code was incorrectly using 'self' instead of 'this', you might make sure that that change alone is not sufficient to also fix this bug.  (Though given the device specific nature of this bug it doesn't seem like it.)

Comment 36

[Jim Porter (:squib)]

(In reply to David Flanagan [:djf] from comment #35)
> Given that you just recently fixed the bug where this code was incorrectly
> using 'self' instead of 'this', you might make sure that that change alone
> is not sufficient to also fix this bug.  (Though given the device specific
> nature of this bug it doesn't seem like it.)

Yeah, I'd checked that before landing the other bug. No dice.

Comment 37

[Jim Porter (:squib)]

Landed: https://github.com/mozilla-b2g/gaia/commit/122325e6bf9ddc0a0ac07f5148880f159b9ad023

Comment 38

[Jim Porter (:squib)]

Comment on attachment 8446318 [details] [review]
Switch to loadedmetadata

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 960329
[User impact] if declined: On some devices (Flame and Open C), users won't be able to stop ringtone previews or even set them as their default ringer(!)
[Testing completed]: Manually tested
[Risk to taking this patch] (and alternatives if risky): Low risk
[String changes made]: None

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8446318 [details] [review]
Switch to loadedmetadata

2.0+ have auto-approval

Comment 40

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/fa91efd7ac5674a512531cc22eeab416dc709d66

Comment 41

[Alissa（leave from Mozilla）]

This issue has been successfully verified on Flame 2.1, 2.0
See attachment:verify20.3gp
Reproducing rate: 0/5

Flame 2.1 build:
Gaia-Rev        38e17b0219cbc50a4ad6f51101898f89e513a552
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/8b92c4b8f59a
Build-ID        20141205001201
Version         34.0

 Flame 2.0 new build:
Gaia-Rev        856863962362030174bae4e03d59c3ebbc182473
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e40fe21e37f1
Build-ID        20141208000206
Version         32.0

Comment 42

[Alissa（leave from Mozilla）]

Attachment: verify20.3gp