Closed Bug 1017421 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(blocking-b2g:2.0+, b2g-v2.0 verified, b2g-v2.1 verified)

VERIFIED FIXED
2.0 S5 (4july)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: tif, Assigned: squib)

Details

(Whiteboard: interaction-design)

Attachments

(3 files)

Attached file 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.
Whiteboard: interaction-design
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?
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.
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?
blocking-b2g: --- → 2.0?
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.
QA Wanted to test this across different devices to check if this is truly device specific or not.
Keywords: qawanted
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.
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.
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
(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.
(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
Keywords: qawanted
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)
blocking-b2g: 2.0? → ---
Target Milestone: --- → 2.0 S4 (20june)
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.
blocking-b2g: --- → 2.0?
Thanks for the nom, Jim. This is a must-have and cannot be broken.
accidentally moved it out of 2.0?...meant to block it in comment 11 based on discussion in media triage last week
blocking-b2g: 2.0? → 2.0+
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
Flags: needinfo?(wchang)
Flags: needinfo?(frlee)
Flags: needinfo?(echou)
Summary: [ringtones] previewing ringtones doesn't stop when tapped again → [ringtones][Flame only] previewing ringtones doesn't stop when tapped again
QA Wanted to check Open C on 2.0. If it happens there, then it's a JB-specific issue.
Keywords: qawanted
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?
Flags: needinfo?(frlee) → needinfo?(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
Flags: needinfo?(mlien)
(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.
Also, this shouldn't be happening in 1.3 at all because the relevant code is only present in 2.0.
(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.
Flags: needinfo?(echou)
(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
Flags: needinfo?(brsun)
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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Keywords: qawanted
Component: Gaia::Ringtones → Video/Audio
Product: Firefox OS → Core
Version: unspecified → 32 Branch
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+][lead-review+]
Flags: needinfo?(wchang)
Flags: needinfo?(jmitchell)
It seems we have some odd behavior while reusing the same HTMLMediaElement to play different contents. I am going to dig into it.
Flags: needinfo?(brsun)
Assignee: nobody → 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.
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
Flags: needinfo?(squibblyflabbetydoo)
If there's a better event that guarantees an audio file is valid, then I'm all ears.
Flags: needinfo?(squibblyflabbetydoo)
(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?
You can directly call |this._player.pause()| when |this._player.paused| attribute is false.
(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.
(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.
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.
Un-assign myself for ringtone player to do corresponding changes.
Assignee: brsun → nobody
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).
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
Attachment #8446318 - Flags: review?(dflanagan)
Component: Video/Audio → Gaia::Ringtones
Product: Core → Firefox OS
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
Version: 32 Branch → unspecified
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.)
Attachment #8446318 - Flags: review?(dflanagan) → review+
(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.
Landed: https://github.com/mozilla-b2g/gaia/commit/122325e6bf9ddc0a0ac07f5148880f159b9ad023
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
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
Attachment #8446318 - Flags: approval-gaia-v2.0?
Comment on attachment 8446318 [details] [review]
Switch to loadedmetadata

2.0+ have auto-approval
Attachment #8446318 - Flags: approval-gaia-v2.0?
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.