Closed Bug 1032702 Opened 5 years ago Closed 5 years ago

[v1.4] Cannot select preloaded MP3 as ringtone

Categories

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

defect
Not set

Tracking

(blocking-b2g:1.4+, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.0 S5 (4july)
blocking-b2g 1.4+
Tracking Status
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: casper.vandonderen, Assigned: squib)

References

Details

Attachments

(2 files, 1 obsolete file)

46 bytes, text/x-github-pull-request
dhylands
: review+
Details | Review
46 bytes, text/x-github-pull-request
squib
: review+
Details | Review
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.153 Safari/537.36

Steps to reproduce:

- Create a gaia customization folder with an mp3 ringtone and list.json
- build gaia with GAIA_DISTRIBUTION_DIR set.
- Go to Settings -> Sound -> Ringer


Actual results:

When selecting the ringtone the preview for it plays correctly and I can hear the ringtone. When clicking 'Done' I get a message stating 'the ringtone was not changed because the selected file cannot be played on this phone.'

When running the debugger I get a media decode error for 'oncanplay' in http://mxr.mozilla.org/gaia/source/apps/settings/js/sound.js#298, this seems to come from the mimetype being hardcoded to 'audio/ogg' in the ringtones app for v1.4


Expected results:

Be able to set an mp3 file as a ringtone.
I am aware the whole ringtones app is rewritten in master, and looking at the code it seems to be possible to select mp3 files there. Is there any way we can uplift or fix this in v1.4, since we are targeting that version?
blocking-b2g: --- → 1.4?
Flags: needinfo?(dkuo)
Flags: needinfo?(dflanagan)
Jim Porter, Can you check this mp3 ringtone selection behavior on 1.4.
Flags: needinfo?(squibblyflabbetydoo)
Yeah, turns out this is a little broken on master too. But there's an easy fix.
Flags: needinfo?(squibblyflabbetydoo)
Attached file Fix it on master
David: I verified that this works, but it's maybe a bit evil. Thoughts?

And sorry to keep asking you to do stuff right before your vacation! I can steal a review from you if you like. :)
Assignee: nobody → squibblyflabbetydoo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8448851 - Flags: feedback?(dflanagan)
To elaborate: the other option is to do as the old comment says and map extensions to mimetypes, but that's kind of weird too (and not always accurate either).
Comment on attachment 8448851 [details] [review]
Fix it on master

If the audio element works correctly without any type information then that would be better than us trying to guess the information based on the file extension, since we could easily be fooled by a poorly named file. 

So this seems good to me. Instead of using slice it would be even better if setting overrideMimeType to null or the empty string would achieve the same thing.  Calling slice() on a blob may change the code path it goes through in the IPC blob transfer stuff and I've seen enough blob-related bugs to be wary about slicing() if there is another way.

But if overrideMimeType = '' doesn't work then I'm totally cool with this fix.
Attachment #8448851 - Flags: feedback?(dflanagan) → feedback+
Flags: needinfo?(dflanagan)
I tried overrideMimeType('') and it didn't work, but I'll try a couple other ways to be sure. Otherwise, I'll just leave it as-is.
Turns out null is also explicitly specced to not do anything, so it looks like the slice trick is the way to go here.
Flags: needinfo?(dkuo)
QA wanted to see if this is the regression or if there is anything to do with the tested file.
Keywords: qawanted
Comment on attachment 8448851 [details] [review]
Fix it on master

Dave: Could you do a quick review of this? I'll be posting a 1.4 patch soon too.
Attachment #8448851 - Flags: review?(dhylands)
Same deal here. I've manually verified that this works in both cases. I don't think there's a good automated test for this, since the test involves changing the build...
Attachment #8450506 - Flags: review?(dhylands)
Comment on attachment 8448851 [details] [review]
Fix it on master

Looks good to me - but I'm really just rubber stamping this casue djf gave it f+
Attachment #8448851 - Flags: review?(dhylands) → review+
Comment on attachment 8450506 [details]
https://github.com/mozilla-b2g/gaia/pull/21366

Same thing here
Attachment #8450506 - Flags: review?(dhylands) → review+
Attached file Fix it in 1.4
Oops, I filled the fields out wrong!
Attachment #8450506 - Attachment is obsolete: true
Attachment #8450569 - Flags: review+
Comment on attachment 8448851 [details] [review]
Fix it on master

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 #): N/A
[User impact] if declined: People distributing FxOS can't ship mp3 ringtones
[Testing completed]: Manually tested
[Risk to taking this patch] (and alternatives if risky): Very low
[String changes made]: None
Attachment #8448851 - Flags: approval-gaia-v2.0?
Comment on attachment 8450569 [details] [review]
Fix it in 1.4

[Approval Request Comment]
See comment 15
Attachment #8450569 - Flags: approval-gaia-v1.4?
Landed on master.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
blocking-b2g: 1.4? → ---
Keywords: qawanted
(In reply to Ivan Tsay (:ITsay) from comment #9)
> QA wanted to see if this is the regression or if there is anything to do
> with the tested file.

It's because in a corner of the ringtones app, we assumed all built-in tones were Ogg. As far as I know, this has been the case forever.

Also, sorry to anyone to whom I said that mp3s should work as built-in tones! It turns out they didn't (but we certainly intended for them to work)!
Comment on attachment 8448851 [details] [review]
Fix it on master

Approved for 2.0
Attachment #8448851 - Flags: approval-gaia-v2.0? → approval-gaia-v2.0+
Comment on attachment 8450569 [details] [review]
Fix it in 1.4

Small fix but we're only accepting partner driven changes on 1.4 at this point. If you think this needs to be taken on 1.4, please nom 1.4?.
Attachment #8450569 - Flags: approval-gaia-v1.4? → approval-gaia-v1.4-
How do we "nom 1.4?" ? We really want this to land for our 1.4 release since our OEM partner is supplying ringtones in MP3 only.
Comment on attachment 8450569 [details] [review]
Fix it in 1.4

Re-nom because of partner requirement from Telenor, see https://bugzilla.mozilla.org/show_bug.cgi?id=1032702#c21.
Attachment #8450569 - Flags: approval-gaia-v1.4- → approval-gaia-v1.4?
blocking-b2g: --- → 1.4?
Comment on attachment 8450569 [details] [review]
Fix it in 1.4

I'm clearing the 1.4 approval request. With the 1.4 nom, this bug will be reviewed in triage. If approved, you can land without further approval.
Attachment #8450569 - Flags: approval-gaia-v1.4?
(In reply to Jim Porter (:squib) from comment #17)
> Landed on master.

In the future, please include a changeset link too.

Master: https://github.com/mozilla-b2g/gaia/commit/b597b86274ab109d7ef530bc0c4b6adccddae4e4
v2.0:   https://github.com/mozilla-b2g/gaia/commit/74e0588c1ddd2be4eea121d435628ecd4ed4c869
Target Milestone: --- → 2.0 S5 (4july)
Taking for 1.4+ as feature was broken.
blocking-b2g: 1.4? → 1.4+
Duplicate of this bug: 1035732
Depends on: 1045186
You need to log in before you can comment on or make changes to this bug.