814326 - Group audio channel if possible and make sound setting alignment with platform

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect, P2)

Description

[Joe Cheng [:jcheng] (please needinfo)]

Currently gaia UI has 10 steps as default for adjusting volume (under all scenarios)
However, under different scenarios, the audio volume steps are actually different base on the underlying system's capability 
Below are different volume steps capability, the UI should sync to the system capability

       5 steps,  // STREAM_VOICE_CALL   <-- can be adjusted during voice call
       7 steps,  // STREAM_SYSTEM       <-- I think it can sync with ringer
       7 steps,  // STREAM_RING         <-- ringer volume, can be adjusted by vol-key  on home screen
       15 steps, // STREAM_MUSIC        <-- music playback volume, can be adjusted by vol-key when music is playing 
       7 steps,  // STREAM_ALARM           <-- alarm sound volume, can be changed on setting
       7 steps,  // STREAM_NOTIFICATION    <-- sync with ringer
       15 steps, // STREAM_BLUETOOTH_SCO   <-- voice call on and can be adjusted by volume key or bt handset-free.
       7 steps,  // STREAM_SYSTEM_ENFORCED  <-- force to output camera shutter sound
       15 steps, // STREAM_DTMF             <-- this tone volume can be adjust with ring
       15 steps,  // STREAM_TTS             X no use
       15 steps,  // STREAM FM          <-- I think this can sync with music.

Comment 1

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

setting can get max volume step by audioManager.getMaxStreamVolumeIndex API and those values are defined in AudioManager.cpp

Comment 2

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

gaia can't access the audioManager object, maybe we need new api for sound_manager to query the default maximum value.

Comment 3

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Randy Lin [:rlin] from comment #2)
> gaia can't access the audioManager object, maybe we need new api for
> sound_manager to query the default maximum value.

Can you file a bug for that and link to this bug and bug 810780? Thanks.
Audio scope is getting bigger and bigger. :/

Comment 4

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

From mail discussed before, settings app can group those volumes into.
- content volume (music/fm streams)
- Ringer volume (ringer and notification and system streams) 
- Alarm volume (alarm stream)

The voice /bt sco volume can be adjusted by hw volume key during the voice call.
The maximal volume should be queried by this Bug 815024

Comment 5

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Dear Alive, 
After discuss with Jonas and we think it's better to hard-code the maximal value in the setting/system apps first at this stage.

Comment 6

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: suggest volume control interface for gaia

suggest group into 5 types
gaia setting strings (maximal volume step) ->control those stream volume
========================================================================
audio.volume.content (15) ->control the SYSTEM, MUSIC, FM stream volume
audio.volume.notification (7) ->control the RING, NOTIFICATION
audio.volume.alarm (7)-> ALARM
audio.volume.telephony (5)->VOICE_CALL
audio.volume.bt_sco-(15)>BT_SCO

Comment 7

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: test code for gaia

enlarge the volume step to 15 and change the naming of volume setting

Comment 8

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

suggestion change:

Comment 9

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Miss press the save changes button, please ignore upper comment.

Comment 10

[(inactive after 6/18) Alive Kuo [:alive]]

Randy:

Please test your gecko with https://github.com/mozilla-b2g/gaia/pull/6682
Tell me if any problem, thanks!

Comment 11

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: gecko part patch 1

Comment 12

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

(In reply to Randy Lin [:rlin] from comment #11)
> Created attachment 686402 [details] [diff] [review]
> gecko part patch 1
try server
https://tbpl.mozilla.org/?tree=Try&rev=5aca8af1beaa

Comment 13

[[:fabrice] Fabrice Desré]

Comment on attachment 686402 [details] [diff] [review]
gecko part patch 1

Review of attachment 686402 [details] [diff] [review]:
-----------------------------------------------------------------

The code looks good, but I want to know why we don't use the same values as android.

::: b2g/chrome/content/settings.js
@@ +83,5 @@
>        let audioManager = Services.audioManager;
>        if (!audioManager)
>          return;
>  
> +      for each(let st in t) {

I know this is not your fault, but can you use more descriptive variable names?

::: dom/system/gonk/AudioManager.cpp
@@ +36,5 @@
>  
>  // Refer AudioService.java from Android
>  static int sMaxStreamVolumeTbl[AUDIO_STREAM_CNT] = {
> +  5,   // voice call
> +  15,  // system

this is different from http://androidxref.com/4.0.4/xref/frameworks/base/media/java/android/media/AudioService.java#178

why?

Comment 14

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

(In reply to Fabrice Desré [:fabrice] from comment #13)
> Comment on attachment 686402 [details] [diff] [review]
> gecko part patch 1
> 
> Review of attachment 686402 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The code looks good, but I want to know why we don't use the same values as
> android.
> 
> ::: b2g/chrome/content/settings.js
> @@ +83,5 @@
> >        let audioManager = Services.audioManager;
> >        if (!audioManager)
> >          return;
> >  
> > +      for each(let st in t) {
> 
> I know this is not your fault, but can you use more descriptive variable
> names?
OK, I will change it.
> 
> ::: dom/system/gonk/AudioManager.cpp
> @@ +36,5 @@
> >  
> >  // Refer AudioService.java from Android
> >  static int sMaxStreamVolumeTbl[AUDIO_STREAM_CNT] = {
> > +  5,   // voice call
> > +  15,  // system
> 
> this is different from
> http://androidxref.com/4.0.4/xref/frameworks/base/media/java/android/media/
> AudioService.java#178
> 
> why?
We want to group the system sound into content, so we should align those maximal value to the same.

Comment 15

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: gecko part patch 2

*Change the naming of variable and voice maximal volume index,
*Another change has increased the maximal index of System /FM to 15.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2a0cc53acdc
The reason of system volume sync with content volume is avoid sound shack issue.
(low content volume mix with high volume of system sound)

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't understand comment 0 here.

All channels except for telephony and fmradio are ones that are played by gecko and could be mixed by gecko. So I don't understand why we couldn't have an arbitrary number of steps in the volume of those channels.

Randy mention that we use audioflinger to mix those channels. I'm not sure I understand why we're not mixing them in gecko, but that's outside my area of expertise. But I was told that audioflinger has 15 volume settings. Is that not true?

Comment 17

[Joe Cheng [:jcheng] (please needinfo)]

Randy will be a better person to explain comment 0. posted the bug for Randy for tracking purpose.

Comment 18

[Joe Cheng [:jcheng] (please needinfo)]

this is related to the latest updated sound spec from lco. 
adding lco

Comment 19

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

The magic number 15 comes from AudioService.java. android phone used it as music/fm volume step.
We can also modify it in gonk:AudioManager.cpp:sMaxStreamVolumeTbl.

Comment 20

[[:fabrice] Fabrice Desré]

(In reply to Randy Lin [:rlin] from comment #19)
> The magic number 15 comes from AudioService.java. android phone used it as
> music/fm volume step.
> We can also modify it in gonk:AudioManager.cpp:sMaxStreamVolumeTbl.

Don't we need to make sure that this table is kept in sync with the one in settings.js ?

Comment 21

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

(In reply to Fabrice Desré [:fabrice] from comment #20)
> (In reply to Randy Lin [:rlin] from comment #19)
> > The magic number 15 comes from AudioService.java. android phone used it as
> > music/fm volume step.
> > We can also modify it in gonk:AudioManager.cpp:sMaxStreamVolumeTbl.
> 
> Don't we need to make sure that this table is kept in sync with the one in
> settings.js ?
refer to 
https://hg.mozilla.org/mozilla-central/file/1b7103181091/dom/system/gonk/AudioManager.cpp
    38 // Refer AudioService.java from Android
    39 static int sMaxStreamVolumeTbl[AUDIO_STREAM_CNT] = {
    40   10,  // voice call
    41   15,  // system
    42   7,   // ring
    43   15,  // music
    44   7,   // alarm
    45   7,   // notification
    46   15,  // BT SCO
    47   7,   // enforced audible
    48   15,  // DTMF
    49   15,  // TTS
    50   15,  // FM
    51 };
This patch would change voice call to 5, others are sync to settings.js

Comment 22

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: gecko checkin-patch

Fix conflict.

Comment 23

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

gaia part would land after  attachment 687091 [details] [diff] [review] land.

Comment 24

[(inactive after 6/18) Alive Kuo [:alive]]

lco 's proposed settings app change is here: http://people.mozilla.com/~lco/FFOS/Sound/R1_Sound%20Spec_v1%20DRAFT.pdf

But for v1 c2,
1. I won't add ringtone selection in this bug because bug 807431 is covering that.
2. The content volume adjust will fail until 811222 and 810780 lands

Comment 25

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: correct volume step

follow by sicking suggestion.

Comment 26

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: correct volume step

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 688192 [details] [diff] [review]
correct volume step

Review of attachment 688192 [details] [diff] [review]:
-----------------------------------------------------------------

Sweet! Thanks!

Comment 28

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: check-in patch

final check-in patch

Comment 29

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: PR 6682

This patch does:
1. Try to align the spec as much as possible.
2. Resolve a C2 bug 815705
3. Set the segments of all channels except voice to 15.

This patch does not:
1. Remove the content slider. Because this is based on bug 810780 and bug 811222's work and if I removed it, regression will come.
2. Add final ringtones. Leave it to bug 807431

The remaining volume work will be done in bug 810780, another c2 bug if 811222 landed.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3589b294fa16

Comment 31

[(inactive after 6/18) Alive Kuo [:alive]]

Set late-l10n and reviewer to stas for l10n touch.
Reset r? to timdream coz seems kaze doesn't have time to review :)

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3589b294fa16

Comment 33

[(inactive after 6/18) Alive Kuo [:alive]]

Sorry we still have relevant gaia patch reviewing :(

Comment 34

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 688206 [details]
PR 6682

See Github, please re-request for review after explain to me the desired behavior.

Also, please file separate bugs for Gaia and Gecko fixes next time. :)

Good job; thanks for cleaning up style sheets.

Comment 35

[Joe Cheng [:jcheng] (please needinfo)]

since it blocks C2 bugs, change to P2 & C2 to be consistent

Comment 36

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 688206 [details]
PR 6682

r=me, thanks. Great work!

Comment 37

[(inactive after 6/18) Alive Kuo [:alive]]

I propose to land this ASAP because of gecko has landed, so I am going to remove the l10n change in this patch, open another bug to track it.

Comment 38

[Axel Hecht [:Pike]]

Comment on attachment 688206 [details]
PR 6682

There seems to be something for me to review, I guess, but not this?

Comment 39

[(inactive after 6/18) Alive Kuo [:alive]]

Already landed https://github.com/mozilla-b2g/gaia/commit/bffc1fa13724d9cd37f9fcc44f90839d0e944eb9

Comment 40

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0978db919b27
https://hg.mozilla.org/releases/mozilla-beta/rev/b675946f1fd3