Closed Bug 878057 Opened 9 years ago Closed 8 years ago

Custom Ringtone selection from file system

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.3 Sprint 3 - 10/25

People

(Reporter: sankha, Assigned: djf)

References

Details

(Whiteboard: [fxos:media])

Attachments

(5 files)

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.
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
Summary: Ringtone selection from file system → Custom Ringtone selection from file system
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
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!! =)
OS: Windows 7 → Gonk (Firefox OS)
Hardware: x86_64 → ARM
I'll mark this as a duplicate of bug 905849 since there is already some work done there to implement this.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 905849
Status: RESOLVED → REOPENED
Component: Gaia::Settings → Gaia::Music
Resolution: DUPLICATE → ---
Depends on: 905849
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.
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.
QA Contact: dflanagan
(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.
blocking-b2g: --- → koi?
Whiteboard: [fxos:media]
Oops. Meant to set myself as assignee, not QA contact.
Assignee: nobody → dflanagan
QA Contact: dflanagan
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?
Flags: needinfo?(bent.mozilla)
Flags: needinfo?(anygregor)
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?
Flags: needinfo?(paul)
(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.
Flags: needinfo?(anygregor)
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.
Attachment #809564 - Flags: review?(kaze)
Attachment #809564 - Flags: review?(etienne)
Attachment #809564 - Flags: review?(dkuo)
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?
(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.)
Flags: needinfo?(anygregor)
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 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 :)
Attachment #809564 - Flags: review?(etienne) → feedback+
(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.
(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!
Flags: needinfo?(paul)
(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
(In reply to Robert Kaiser (:kairo@mozilla.com) 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.
(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.
blocking-b2g: koi? → 1.3?
Blocks: 909619
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.
Attachment #809564 - Flags: review?(dkuo) → review+
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 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.
Attachment #809564 - Flags: ui-review?(rmacdonald)
Attachment #809564 - Flags: review?(dkuo)
(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 :)
Flags: needinfo?(anygregor)
Keywords: perf
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.
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 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.
Attachment #809564 - Flags: review?(dkuo)
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.
Depends on: 830040
Putting it back into the triage queue given we have more info from David's email
blocking-b2g: 1.3? → koi?
Flags: needinfo?(skasetti)
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!
Attachment #809564 - Flags: ui-review?(rmacdonald) → ui-review+
Attachment #809564 - Flags: review?(kaze)
I've rebased the PR on top of bug 830040 and addressed Dominic's github comments.
Flags: needinfo?(bent.mozilla)
Landed to master: https://github.com/mozilla-b2g/gaia/commit/e0f8bd90ab820de7aa9b436f19de7c7e225b24ed
Status: REOPENED → RESOLVED
Closed: 9 years ago8 years ago
Resolution: --- → FIXED
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.
Attachment #809564 - Flags: approval-gaia-v1.2?
Flags: in-moztrap?(mozillamarcia.knous)
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.
Attachment #809564 - Flags: approval-gaia-v1.2?
Moving to 1.3
blocking-b2g: koi? → 1.3?
Keywords: perf
As discussed in a meeting with Jaime, Sri, Jonas, and David - this feature is targeted for 1.3 now.

Thanks
Hema
Target Milestone: --- → 1.3 Sprint 3 - 10/25
Flags: needinfo?(skasetti)
This is currently targeted for 1.3
(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.
Flags: needinfo?(dflanagan)
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.
Flags: needinfo?(dflanagan)
Not a 1.3 blocker, removing 1.3? flag
blocking-b2g: 1.3? → ---
#10649 added to Moztrap for testing purposes.
Flags: in-moztrap?(mozillamarcia.knous) → in-moztrap+
This feature is "resolved fixed" but I got FirefoxOS 1.4 and can't find it ?
Is it implemented already ?
Any news about this feature?
(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.
See Also: → 951932
blocking-b2g: --- → 1.5?
https://marketplace.firefox.com/app/ringtone-picker
I have do it an app for this, scan the ogg file in the sdcard.
blocking-b2g: 2.0? → ---
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.
You need to log in before you can comment on or make changes to this bug.