878057 - Custom Ringtone selection from file system

Status: RESOLVED FIXED

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

Description

[Sankha Narayan Guria [:sankha]]

Right now the only ringtones that can be selected are the ones bundled with the B2G builds. It would be great if there is an option to allow the user to select the ringtone from a music file in the external storage file system.

Comment 1

[Ralph Daub [:rdaub]]

For tracking purposes, a user in the Mozilla-Hispano forums is also requesting this feature:
https://www.mozilla-hispano.org/foro/viewtopic.php?f=47&t=16338&sid=35e645caae3e9102106c7fde4361a358

Please let me know if there are any questions. Thanks!! =)

- Ralph

Comment 2

[Ralph Daub [:rdaub]]

For tracking purposes, another user in the Mozilla-Hispano forums is requesting this feature:
https://www.mozilla-hispano.org/foro/viewtopic.php?f=47&t=16414#p63741

Thanks!! =)

- Ralph

Comment 3

[Ralph Daub [:rdaub]]

For tracking purposes, another user in the Mozilla-Hispano forums is requesting this feature:
https://www.mozilla-hispano.org/foro/viewtopic.php?f=47&t=16413&p=63776#p63776

Thanks!! =)

Comment 4

[Anthony Ricaud (:rik)]

I'll mark this as a duplicate of bug 905849 since there is already some work done there to implement this.

Comment 5

[motanualb]

C`mon people , it was possible to use custom (.mp3) ringtones   on phones over 7 years old. Small things like this ,makes FFOS not user friendly OS.

Comment 6

[David Flanagan [:djf]]

Technically, this is trivial to implement. My worry is that if the user sets a 5mb mp3 file as their ringtone, then when the phone rings there will be a spike in memory usage and background apps will be closed.  

I'm taking this bug to investigate the memory usage implications.  If there aren't memory performance issues then we'll see if we can get it landed ASAP!

A full-featured solution would allow the user to select a starting point in the music file and possibly also a duration so that you could, for example, use only the chorus of a song as the ringtone.  It would be awesome if we could use the Web Audio API to "clip" a song to just the part that the user wants as their ringtone.  WebAudio lets us get the audio content, but doesn't seem to have any way to re-encode it.  We'd be limited to creating WAV files, I think.  

For now, I'll assume that the scope of this bug is just to use entire songs as ringtones.

Comment 7

[David Flanagan [:djf]]

(In reply to motanualb from comment #5)
> C`mon people , it was possible to use custom (.mp3) ringtones   on phones
> over 7 years old. Small things like this ,makes FFOS not user friendly OS.

Actually, neither Android nor iOS allow casual users to create custom ringtones either.  You can do it, but it is complicated.  If we can make it just work in FFOS, we'll actually be way ahead of the smartphone competition.

Noming this for 1.2.  If we can do this without memory impact, I think we should seriously consider uplifting it because it is such a frequently requested feature and because it will set us apart from Android and iOS.

Comment 8

[David Flanagan [:djf]]

Oops. Meant to set myself as assignee, not QA contact.

Comment 9

[David Flanagan [:djf]]

Gregor: can you tell me whether there are bad memory implications if we store a 5mb blob (a mp3 file) into the settings database and read it back?  I think that indexedDB stores blobs as separate files, and if settings db is the same, then I think we can read the ringtone blob without causing a large memory allocation, right?

Ben: if we read a big blob from the settings db and create a blob url for it that doesn't allocate a big chunk of memory, does it?

Comment 10

[David Flanagan [:djf]]

Paul: if I have a 5mb audio file in a file-backed blob and I create a blob: URL for it and then play it, how much memory gets used, and when does it get used?  The dialer reads the ringtone blob, creates the url, and sets the audio.src property at startup, and then calls play() when there is an incoming call.  Will setting the src property cause memory to be used?  Will play() stream music from the blob and keep memory usage low, or does the entire blob get read into memory, do you know?

Comment 11

[Gregor Wagner [:gwagner]]

(In reply to David Flanagan [:djf] from comment #9)
> Gregor: can you tell me whether there are bad memory implications if we
> store a 5mb blob (a mp3 file) into the settings database and read it back? 
> I think that indexedDB stores blobs as separate files, and if settings db is
> the same, then I think we can read the ringtone blob without causing a large
> memory allocation, right?
> 
> Ben: if we read a big blob from the settings db and create a blob url for it
> that doesn't allocate a big chunk of memory, does it?

afaik it's just a file handle and we try to avoid big allocations. So it shouldn't be a big problem.
But I would really like to see a profile when we want to play a 5mb ringtone for an incoming call.

Comment 12

[David Flanagan [:djf]]

Attachment: link to patch on github

Setting r? for Dominic, Kaze and Etienne to get this bug on your radar. It is just a 5-line patch right now, and doesn't modify the Dialer at all.  I'm thinking about adding more, like another setting that specifies the time the song should start at.  

But I wanted you all to know about it because if this doesn't have adverse memory or performance impact, I'd like to land this in 1.2.

Comment 13

[David Flanagan [:djf]]

Paul: another question for you... If I create an audio element, set its src to a blob url, and then set its currentTime to something non-zero, does that make it allocate memory?  Can we start playing a song from somewhere in the middle as quickly (or almost) as we can from the beginning?  That is, can I extend this feature so that it starts playing the ringtone at some user-specified offset?

Comment 14

[David Flanagan [:djf]]

(In reply to Gregor Wagner [:gwagner] from comment #11)

> afaik it's just a file handle and we try to avoid big allocations. So it
> shouldn't be a big problem.
> But I would really like to see a profile when we want to play a 5mb ringtone
> for an incoming call.

The attached patch is ready for profiling, if you want to try it out. Or, if not, could you tell me how to do that?  (I don't think I know how to do a memory profile.)

Comment 15

[David Flanagan [:djf]]

I think I want to drop the idea I mentioned in comment 12 about storing a user-specified starting point for ringtone playback. That seems like too much complexity for 1.2.

But I should extend the patch so that the ringtone app can accept share activities and set the ringtones that way.

And, both the ringtone app and the settings app should change to verify that the audio blob is playable before setting it in the settings db. Silencing the ringer with a bad audio file would be nasty so we need to be sure we can prevent that.

Comment 16

[Etienne Segonzac (:etienne)]

Comment on attachment 809564 [details]
link to patch on github

Did some simple (procrank) memory profiling and didn't see any significant difference in memory consumption between a built-in ringtone and a 25Mo .m4a.

I guess the Audio element _is_ magic :)

Comment 17

[Robert Kaiser]

(In reply to David Flanagan [:djf] from comment #15)
> And, both the ringtone app and the settings app should change to verify that
> the audio blob is playable before setting it in the settings db. Silencing
> the ringer with a bad audio file would be nasty so we need to be sure we can
> prevent that.

Doesn't the settings app preview-play a ringtone anyhow? Doing the above check could be coupled with that.

Comment 18

[Paul Adenot (:padenot)]

(In reply to David Flanagan [:djf] from comment #10)
> Paul: if I have a 5mb audio file in a file-backed blob and I create a blob:
> URL for it and then play it, how much memory gets used, and when does it get
> used?

iirc, we would only load in memory what is needed to play. But this is largely the responsibility of the Blob implementation, the HTMLMediaElement itself is only keeping in memory a few buffers decoded in advance, during playback. 

> The dialer reads the ringtone blob, creates the url, and sets the
> audio.src property at startup, and then calls play() when there is an
> incoming call.  Will setting the src property cause memory to be used?

It depends on the 'preload' attribute, on the <audio> element. It defaults to none on mobile (b2g/fennec), meaning setting the `src` attribute does not trigger a load (the url being merely referenced from the <audio> element). Of course, when you call play(), we load the src. You can set it to 'metadata' (get the duration, artist, album, number of channels, etc.), or 'auto' (start the load). I think you want 'none', here, to be more conservative about memory, which is conveniently the default.

> Will play() stream music from the blob and keep memory usage low, or does the
> entire blob get read into memory, do you know?

That would be my expectations, if you observe the opposite behavior, it's a bug and has to be fixed: imagine what would happen if one tries to open a 200MB video on a modest device!

Comment 19

[Paul Adenot (:padenot)]

(In reply to David Flanagan [:djf] from comment #13)
> Paul: another question for you... If I create an audio element, set its src
> to a blob url, and then set its currentTime to something non-zero, does that
> make it allocate memory?

You would have to set preload=metadata on the <audio> element: seeking requires the duration.

Maybe you could use media fragments [1] to achieve that, effectively encoding in the URL what portion of the file you want to play:

Play from 0:50 until the end: 

> <audio controls src="http://example.com/audio.ogg#t=50" preload=metadata></audio>

Play from 0:50 to 0:60:

> <audio controls src="http://example.com/audio.ogg#t=50,60" preload=metadata></audio>

Otherwise, setting the currentTime works.

>  Can we start playing a song from somewhere in the
> middle as quickly (or almost) as we can from the beginning?  That is, can I
> extend this feature so that it starts playing the ringtone at some
> user-specified offset?

I would not expect this to be be a problem, for a blob-backed source: the blob is randomly seekable.

[1]: http://www.w3.org/2008/WebVideo/Fragments/WD-media-fragments-spec/#naming-time

Comment 20

[David Flanagan [:djf]]

(In reply to Robert Kaiser (:[e-mail]) from comment #17)
> (In reply to David Flanagan [:djf] from comment #15)
> > And, both the ringtone app and the settings app should change to verify that
> > the audio blob is playable before setting it in the settings db. Silencing
> > the ringer with a bad audio file would be nasty so we need to be sure we can
> > prevent that.
> 
> Doesn't the settings app preview-play a ringtone anyhow? Doing the above
> check could be coupled with that.

Not anymore. Now it uses a pick activity to select a ringtone. The new Ringtones app does an audio preview. But any 3rd party app could handle the pick activity and return an invalid blob, so I should defend against that.

Comment 21

[David Flanagan [:djf]]

(In reply to Paul Adenot (:padenot) from comment #19)

> Maybe you could use media fragments [1] to achieve that, effectively
> encoding in the URL what portion of the file you want to play:
> 
> Play from 0:50 until the end: 
> 
> > <audio controls src="http://example.com/audio.ogg#t=50" preload=metadata></audio>
> 
> Play from 0:50 to 0:60:
> 
> > <audio controls src="http://example.com/audio.ogg#t=50,60" preload=metadata></audio>
> 

I wonder if that works with blob: urls...  Would be pretty cool, if so.

Comment 22

[Dominic Kuo [:dkuo]]

Comment on attachment 809564 [details]
link to patch on github

David,

This looks great, and I also believe this should be a strongly requested feature from users or our partners, if we want this in 1.2, probably we can just filter out the songs that are too long or large than 5mb to avoid the memory issue, and for 1.3 we can have some new ui for users to trim their songs if they like it to be a ringtone, just like you said in comment 6. It can be done in the picker, or another app that specially handles it, and that will need some spec from UX for sure.

Comment 23

[David Flanagan [:djf]]

I've updated the pull request so that ringtones can be set via share activity.

There is now another system app "setringtone" that exists only to handle the share activity for music files and set them as the current ringtone.

The patch also adds a share button to the titlebar of the player view in the music app.

It updates the settings app so that when it gets a ringtone by pick activity, it tests that hte ringtone is actually playable before setting it.  (That would be a bad denial of service attack to respond to a pick ringtone request by returning an unplayable file!)

And it updates the setting app to listen for changes in the dialer.ringtone setting so that if the setting is changed by the new setringtone app it gets updated immediately in the settings app.

Comment 24

[David Flanagan [:djf]]

Comment on attachment 809564 [details]
link to patch on github

Dominic,

Would you take another look at this, since the patch now modifies the music app more substantially.

Rob,

I'm asking for your UI review here, too. Of the share button in the music title bar and of the new setringtone app.

Comment 25

[David Flanagan [:djf]]

Attachment: share button in the music app

Comment 26

[David Flanagan [:djf]]

Attachment: list of sharing choices (note app name and cheesy icon)

Comment 27

[David Flanagan [:djf]]

Attachment: The new setringtone app handling the share activity. Note one-button music control

Comment 28

[David Flanagan [:djf]]

Attachment: Ringtone menu in the settings app showing song title

Comment 29

[Gregor Wagner [:gwagner]]

(In reply to David Flanagan [:djf] from comment #14)
> (In reply to Gregor Wagner [:gwagner] from comment #11)
> 
> > afaik it's just a file handle and we try to avoid big allocations. So it
> > shouldn't be a big problem.
> > But I would really like to see a profile when we want to play a 5mb ringtone
> > for an incoming call.
> 
> The attached patch is ready for profiling, if you want to try it out. Or, if
> not, could you tell me how to do that?  (I don't think I know how to do a
> memory profile.)

I don't have a profiler enabled build around. But we have a perf team :)

Comment 30

[David Flanagan [:djf]]

I've used B2G/tools/get_about_memory.py to profile memory usage while the phone was ringing with a small system ringtone and then again while the phone was playing a 19mb .m4a audio file as the ringtone. I didn't see any differences between the two memory reports that were more than a fraction of a megabyte.  So it doesn't look like using a song as a ringtone uses more memory.

Comment 31

[Ralph Daub [:rdaub]]

Hey everyone, 

I just want to drop by and say how awesome it is to see this patch come to fruition! This has been one of the biggest feedback pieces we receive from end-users at SUMO, and it is very exciting to see this feature come to life. Firefox OS users will be very happy!

Thank you *so much* for all your work on this! =)

- Ralph

Comment 32

[Dominic Kuo [:dkuo]]

Comment on attachment 809564 [details]
link to patch on github

David,

Before I finish this review, I found the part of sharing songs you implemented is the option 1 in:
https://bugzilla.mozilla.org/show_bug.cgi?id=830040#c10
which Mahsa has already went for option 2 and got r+ from me, I am not sure we are back to option 1 or you just pick one suitable solution. Also, with your approach, I found there is a noticeable style issue that caused by the rules of building blocks(please see github), so I think let's figure out which option we want and if it's option 2, maybe we can land bug 830040 first.

Comment 33

[David Flanagan [:djf]]

Dominic,

I didn't know that work was being done on bug 830040.  I just put something in there as a proof of concept.  I'm happy to rebase this after 830040 lands.

Comment 34

[Hema Koka [:hema]]

Putting it back into the triage queue given we have more info from David's email

Comment 35

[Rob MacDonald [:robmac]]

Comment on attachment 809564 [details]
link to patch on github

+ based on screenshots of new playback control. Also confirmed that the new ringtone icon (provided by Peter) will be inserted prior to uplift. 

Thanks!

Comment 36

[David Flanagan [:djf]]

I've rebased the PR on top of bug 830040 and addressed Dominic's github comments.

Comment 37

[David Flanagan [:djf]]

Landed to master: https://github.com/mozilla-b2g/gaia/commit/e0f8bd90ab820de7aa9b436f19de7c7e225b24ed

Comment 38

[David Flanagan [:djf]]

Comment on attachment 809564 [details]
link to patch on github

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 #): its a new feature, not a bug, but it is one of the most often requested features by our users.  

[User impact] if declined: many sad users saying that smart phones are dumber than feature phones

[Testing completed]: I've tested it locally

[Risk to taking this patch] (and alternatives if risky): Its a pretty small patch, for the amount of feature we get, so not too much risk. I haven't seen any memory or time performance penalty for using large music files instead of small ringtone files, and that was the risk I was worried about. Getting people dogfooding this will be important.


[String changes made]: There is a new "setringtone" app (for handling the share activity) that has a few new strings. The settings app (which initiates a pick activity for a ringtone) has two new strings, including an error string that handles the case of a 3rd party app returning an invalid audio file.

Comment 39

[David Flanagan [:djf]]

Comment on attachment 809564 [details]
link to patch on github

Unsetting the approval-gaia-v1.2 flag because a bug has been reported against this patch.

Comment 40

[Preeti Raghunath(:Preeti)]

Moving to 1.3

Comment 41

[Hema Koka [:hema]]

As discussed in a meeting with Jaime, Sri, Jonas, and David - this feature is targeted for 1.3 now.

Thanks
Hema

Comment 42

[Sri Kasetti]

This is currently targeted for 1.3

Comment 43

[Beatriz Rodríguez [:brg]]

(In reply to David Flanagan [:djf] from comment #39)
> Comment on attachment 809564 [details]
> link to patch on github
> 
> Unsetting the approval-gaia-v1.2 flag because a bug has been reported
> against this patch.

Hi David, Can you please provide bug number? Thanks in advance.

Comment 44

[David Flanagan [:djf]]

https://bugzilla.mozilla.org/show_bug.cgi?id=929029  Looks like it is a gecko bug exposed by this patch. Not actually a bug in this patch.

Comment 45

[Hema Koka [:hema]]

Not a 1.3 blocker, removing 1.3? flag

Comment 46

[Marcia Knous [:marcia]]

#10649 added to Moztrap for testing purposes.

Comment 47

[mozilla]

This feature is "resolved fixed" but I got FirefoxOS 1.4 and can't find it ?
Is it implemented already ?

Comment 48

[Daniele "Mte90" Scasciafratte]

Any news about this feature?

Comment 49

[Dominic Kuo [:dkuo]]

(In reply to mozilla from comment #47)
> This feature is "resolved fixed" but I got FirefoxOS 1.4 and can't find it ?
> Is it implemented already ?

To set custom ringtones from music app was fixed, but to pick custom ringtones in settings app from music app was disabled in bug 951932.

Comment 50

[Daniele "Mte90" Scasciafratte]

https://marketplace.firefox.com/app/ringtone-picker
I have do it an app for this, scan the ogg file in the sdcard.

Comment 51

[sai.krn2]

we can use any mp3 as our ringtone just by selecting the mp3 file by using edit option then use share feature, where you find set as ringtone option.