Closed
Bug 1032702
Opened 10 years ago
Closed 10 years ago
[v1.4] Cannot select preloaded MP3 as ringtone
Categories
(Firefox OS Graveyard :: Gaia::Ringtones, defect)
Firefox OS Graveyard
Gaia::Ringtones
Tracking
(blocking-b2g:1.4+, 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)
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.
Reporter | ||
Comment 1•10 years ago
|
||
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)
Comment 2•10 years ago
|
||
Jim Porter, Can you check this mp3 ringtone selection behavior on 1.4.
Flags: needinfo?(squibblyflabbetydoo)
Assignee | ||
Comment 3•10 years ago
|
||
Yeah, turns out this is a little broken on master too. But there's an easy fix.
Flags: needinfo?(squibblyflabbetydoo)
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
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.
Assignee | ||
Comment 8•10 years ago
|
||
Turns out null is also explicitly specced to not do anything, so it looks like the slice trick is the way to go here.
Updated•10 years ago
|
Flags: needinfo?(dkuo)
Comment 9•10 years ago
|
||
QA wanted to see if this is the regression or if there is anything to do with the tested file.
Keywords: qawanted
Assignee | ||
Comment 10•10 years ago
|
||
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)
Assignee | ||
Comment 11•10 years ago
|
||
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 12•10 years ago
|
||
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 13•10 years ago
|
||
Comment on attachment 8450506 [details] https://github.com/mozilla-b2g/gaia/pull/21366 Same thing here
Attachment #8450506 -
Flags: review?(dhylands) → review+
Assignee | ||
Comment 14•10 years ago
|
||
Oops, I filled the fields out wrong!
Attachment #8450506 -
Attachment is obsolete: true
Attachment #8450569 -
Flags: review+
Assignee | ||
Comment 15•10 years ago
|
||
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?
Assignee | ||
Comment 16•10 years ago
|
||
Comment on attachment 8450569 [details] [review] Fix it in 1.4 [Approval Request Comment] See comment 15
Attachment #8450569 -
Flags: approval-gaia-v1.4?
Assignee | ||
Comment 17•10 years ago
|
||
Landed on master.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 18•10 years ago
|
||
(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 19•10 years ago
|
||
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 20•10 years ago
|
||
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-
Comment 21•10 years ago
|
||
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 22•10 years ago
|
||
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?
Updated•10 years ago
|
blocking-b2g: --- → 1.4?
Comment 23•10 years ago
|
||
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?
Comment 24•10 years ago
|
||
(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
Comment 26•10 years ago
|
||
v1.4: https://github.com/mozilla-b2g/gaia/commit/229864ff4ad90899f017341b9e81ed0b53498c66
status-b2g-v1.4:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•