899487 - [ZFFOS1.1] [Settings] when scrolls the scrollbar at Settings->Sound->Volume, there is no warning tone

Status: RESOLVED DUPLICATE of bug 961980

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

Description

[滕飞]

User Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0; .NET4.0C; .NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729; InfoPath.2; TCO_20130730153133)

Steps to reproduce:

version :OPEN_LATAM_FFOS1.1_V1.0.0B02;
when scrolls the scrollbar at Settings->Sound->Volume;


Actual results:

There is no warning tone,we will not know whether the sound level is suitable; 
But there has the warning tone on the verion  FFos1.0 .


Expected results:

We think it could be the same between the version FFOS1.1 and FFOS1.0.

Comment 1

[Scoobidiver (away)]

leo+ requested per bug 899451.

Comment 2

[Ivan Tsay (:ITsay)]

Comments from partner that it is important for the users

Comment 3

[:gerard-majax]

Looks like I'm reproducing this.

Comment 4

[:gerard-majax]

Attachment: Link to Github https://github.com/mozilla-b2g/gaia/pull/11429

Please find attached a link to the github pull request https://github.com/mozilla-b2g/gaia/pull/11429 that addresses this issue.

While testing this on master, I noticed that the actual sound volume was not changed when a notification came. I'll file another bug for this.

Comment 5

[:gerard-majax]

Comment on attachment 787522 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/11429

Actually, turns out this code is the reason why we play at the wrong volume.

Comment 6

[:gerard-majax]

Comment on attachment 787522 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/11429

Even adding a setTimeout() of two seconds, volume change does not seems to be taken into account, I'll investigate but I don't think this should block us for this.

Comment 7

[:gerard-majax]

I'm wondering whether bug 869793 does not somehow relates to this. At least, comment #6 https://bugzilla.mozilla.org/show_bug.cgi?id=869793#c6 shows similar behavior (changing hardware buttons with the notification/alarm slider).

Comment 8

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

Comment on attachment 787522 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/11429

The notification channel obeys the settings of notification volume(audio.volume.notification),
alarm settings(audio.volume.alarm) doesn't affect notification channel.
You need audio-channel-alarm permission and another audio tag to preview alarm channel volume.

Comment 9

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

IMO, I suggest that the fake audio should generate sound-able PCM data for testing. Just like fake video.

Comment 10

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

Replay to wrong bug, please ignore my comment.

Comment 11

[:gerard-majax]

(In reply to Alive Kuo [:alive] from comment #8)
> Comment on attachment 787522 [details]
> Link to Github https://github.com/mozilla-b2g/gaia/pull/11429
> 
> The notification channel obeys the settings of notification
> volume(audio.volume.notification),
> alarm settings(audio.volume.alarm) doesn't affect notification channel.
> You need audio-channel-alarm permission and another audio tag to preview
> alarm channel volume.

Thanks for this feedback, I'll update my patch !

Comment 12

[:gerard-majax]

After testing on gecko 18 and gaia v1-train, it works. I've got interesting errors in logcat, I'll investigate after updating my pull request and file a bug.

Comment 13

[:gerard-majax]

Comment on attachment 787522 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/11429

I've updated the pull request thanks to your previous feedback. This pull request is against master, on which I suspect a gecko-side bug (I'll investigate and file a bug accordingly), but backporting it (not very hard) to gaia v1-train, it works quite well, and both |dumpsys media.audio_policy| and my hearing confirms tht the behavior is correct.

Comment 14

[Preeti Raghunath(:Preeti)]

Has the investigation completed? Can this bug be closed?

Comment 15

[:gerard-majax]

(In reply to Preeti Raghunath(:Preeti) from comment #14)
> Has the investigation completed? Can this bug be closed?

No, I'm off this week and I found nothing so far. The pull request should be okay for 1.1 as far as I could test, it's only master that seems broken.

Comment 16

[:gerard-majax]

FYI, dumping the audio policy. The first dump is without touching anything, the second one is after reducing the "Ring & notifications" volume to 1/3. As you can see, no channel volume gets changed in the end.

I'm reproducing this on Inari and Peak, with master. I'll open a follow up bug for this.

alex@portable-alex:~/codaz/hycs-hardware_xdandroid-ril$ adb shell dumpsys media.audio_policy
PolicyManager Interface: 0x158df20
Command Thread: 0x158dd40
Tones Thread: 0x158dbf0
AudioCommandThread 0x158dd40 Dump
- Commands:
   Command Time        Wait pParam
  Last Command
   02      000003.263  1    0x1590390
AudioCommandThread 0x158dbf0 Dump
- Commands:
   Command Time        Wait pParam
  Last Command
   -1      000000.000  0    0x0

AudioPolicyManager Dump: 0x158dfb0
 Hardware Output: 1
 A2DP Output: 0
 Duplicated Output: 0
 A2DP device address: 
 SCO device address: 
 Output devices: 00000003
 Input devices: 00400000
 Phone state: 0
 Ringer mode: 0
 Force use for communications 0
 Force use for media 0
 Force use for record 0
 Force use for dock 0

Outputs dump:
- Output 1 dump:
 Sampling rate: 44100
 Format: 1
 Channels: 00000003
 Latency: 54
 Flags 00000000
 Devices 00000002
 Stream volume refCount muteCount
 00     1.000     00       00
 01     1.000     00       00
 02     1.000     00       00
 03     1.000     00       00
 04     1.000     00       00
 05     1.000     00       00
 06     -1.000     00       00
 07     1.000     00       00
 08     1.000     00       00
 09     1.000     00       00
 10     1.000     00       00
 11     1.000     00       00

Inputs dump:

Streams dump:
 Stream  Index Min  Index Max  Index Cur  Can be muted
 00      00         05         01         1
 01      00         15         01         1
 02      00         15         01         1
 03      00         15         01         1
 04      00         15         01         1
 05      00         15         01         1
 06      00         15         00         1
 07      00         15         15         0
 08      00         15         01         1
 09      00         15         01         1
 10      00         15         01         1
 11      00         01         01         1

Total Effects CPU: 0.000000 MIPS, Total Effects memory: 0 KB
Registered effects:
alex@portable-alex:~/codaz/hycs-hardware_xdandroid-ril$ adb shell dumpsys media.audio_policy
PolicyManager Interface: 0x158df20
Command Thread: 0x158dd40
Tones Thread: 0x158dbf0
AudioCommandThread 0x158dd40 Dump
- Commands:
   Command Time        Wait pParam
  Last Command
   02      000003.263  1    0x1590390
AudioCommandThread 0x158dbf0 Dump
- Commands:
   Command Time        Wait pParam
  Last Command
   -1      000000.000  0    0x0

AudioPolicyManager Dump: 0x158dfb0
 Hardware Output: 1
 A2DP Output: 0
 Duplicated Output: 0
 A2DP device address: 
 SCO device address: 
 Output devices: 00000003
 Input devices: 00400000
 Phone state: 0
 Ringer mode: 0
 Force use for communications 0
 Force use for media 0
 Force use for record 0
 Force use for dock 0

Outputs dump:
- Output 1 dump:
 Sampling rate: 44100
 Format: 1
 Channels: 00000003
 Latency: 54
 Flags 00000000
 Devices 00000002
 Stream volume refCount muteCount
 00     1.000     00       00
 01     1.000     00       00
 02     1.000     00       00
 03     1.000     00       00
 04     1.000     00       00
 05     1.000     00       00
 06     -1.000     00       00
 07     1.000     00       00
 08     1.000     00       00
 09     1.000     00       00
 10     1.000     00       00
 11     1.000     00       00

Inputs dump:

Streams dump:
 Stream  Index Min  Index Max  Index Cur  Can be muted
 00      00         05         01         1
 01      00         15         01         1
 02      00         15         01         1
 03      00         15         01         1
 04      00         15         01         1
 05      00         15         01         1
 06      00         15         00         1
 07      00         15         15         0
 08      00         15         01         1
 09      00         15         01         1
 10      00         15         01         1
 11      00         01         01         1

Total Effects CPU: 0.000000 MIPS, Total Effects memory: 0 KB
Registered effects:

Comment 17

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

(In reply to 滕飞 from comment #0)
> User Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0;
> .NET4.0C; .NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.4506.2152;
> .NET CLR 3.5.30729; InfoPath.2; TCO_20130730153133)
> 
> Steps to reproduce:
> 
> version :OPEN_LATAM_FFOS1.1_V1.0.0B02;
> when scrolls the scrollbar at Settings->Sound->Volume;
> 
> 
> Actual results:
> 
> There is no warning tone,we will not know whether the sound level is
> suitable; 
> But there has the warning tone on the verion  FFos1.0 .
> 
> 
> Expected results:
> 
> We think it could be the same between the version FFOS1.1 and FFOS1.0.

What's FFOS1.0? I don't remember I implement warning tone anyway.
What makes you think there's warning tone in previous version?

IMO this is an enhancement, we should consider v1.2 for this feature but not blocking leo release.

Comment 18

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

Comment on attachment 787522 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/11429

Code is alright. Let's get some feedback from gecko side.

Comment 19

[滕飞]

(In reply to Alive Kuo [:alive] from comment #17)
> (In reply to 滕飞 from comment #0)
> User Agent: Mozilla/4.0 (compatible; MSIE
> 8.0; Windows NT 5.1; Trident/4.0;
> .NET4.0C; .NET CLR 1.1.4322; .NET CLR
> 2.0.50727; .NET CLR 3.0.4506.2152;
> .NET CLR 3.5.30729; InfoPath.2;
> TCO_20130730153133)
> 
> Steps to reproduce:
> 
> version
> :OPEN_LATAM_FFOS1.1_V1.0.0B02;
> when scrolls the scrollbar at
> Settings->Sound->Volume;
> 
> 
> Actual results:
> 
> There is no warning
> tone,we will not know whether the sound level is
> suitable; 
> But there
> has the warning tone on the verion  FFos1.0 .
> 
> 
> Expected results:
> 
>
> We think it could be the same between the version FFOS1.1 and FFOS1.0.
> What's FFOS1.0? I don't remember I implement warning tone anyway.
What makes
> you think there's warning tone in previous version?

IMO this is an
> enhancement, we should consider v1.2 for this feature but not blocking leo
> release.

Dear kuo,

I just  test on the old version OPEN_US_DEV_FFOS_V1.0.0B01 and the phone indeed made warning tone when scrolling the settings->sound->scroll bar.

I used the tool unyaffs to decompilation the system.img and got the sound.js
from b2g/webapps/settings.gaiamobile.org/js/.

I will attach the file.

InitializeSoundsPanel_Init() -> listenVolumeChanged() -> observe('audio.volume.notification') -> playAudio();

Comment 20

[滕飞]

Attachment: sound.js

Please see the js file.
Tks very much.

Comment 21

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

(In reply to 滕飞 from comment #20)
> Created attachment 792048 [details]
> sound.js
> 
> Please see the js file.
> Tks very much.

Hi,
I don't know what's this..our v1.0.1 code base doesn't have those functions:
https://github.com/mozilla-b2g/gaia/blob/v1.0.1/apps/settings/js/sound.js

:|

Comment 22

[滕飞]

It's so strange. I have update my code and I can't see any git log about the change/delete of those functions. But they really existed in some old version。Are there any different branches to get different codes?

-.-!!

Comment 23

[:gerard-majax]

(In reply to Alive Kuo [:alive] from comment #21)
> (In reply to 滕飞 from comment #20)
> > Created attachment 792048 [details]
> > sound.js
> > 
> > Please see the js file.
> > Tks very much.
> 
> Hi,
> I don't know what's this..our v1.0.1 code base doesn't have those functions:
> https://github.com/mozilla-b2g/gaia/blob/v1.0.1/apps/settings/js/sound.js
> 
> :|

Worst, it seems that no code has ever been committed relating to "audio.volume.*".

Teng Fei, could you search where this code comes from ? This is clearly NOT from out codebase :(

Comment 24

[滕飞]

Tks a lot , I will verify the code again.

Comment 25

[滕飞]

Dear sirs,

We have found the reason.
On version v1.0,we have a requirement for scrolling with warning tone, so we commit a patch to fix it.

I'm so so sorry for making a fool of ourselves.

Tks a lot .

Comment 26

[Ivan Tsay (:ITsay)]

I change it to resolved invalid since this is what we've never implemented. Things like this should go for the feature request.

Comment 27

[:gerard-majax]

(In reply to Ivan Tsay (:ITsay) from comment #26)
> I change it to resolved invalid since this is what we've never implemented.
> Things like this should go for the feature request.

What about my pull request ?

Comment 28

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

(In reply to Alexandre LISSY :gerard-majax from comment #27)
> (In reply to Ivan Tsay (:ITsay) from comment #26)
> > I change it to resolved invalid since this is what we've never implemented.
> > Things like this should go for the feature request.
> 
> What about my pull request ?

How about firing a new clean bug and ask for review?

Comment 29

[:gerard-majax]

(In reply to Alive Kuo [:alive] from comment #28)
> (In reply to Alexandre LISSY :gerard-majax from comment #27)
> > (In reply to Ivan Tsay (:ITsay) from comment #26)
> > > I change it to resolved invalid since this is what we've never implemented.
> > > Things like this should go for the feature request.
> > 
> > What about my pull request ?
> 
> How about firing a new clean bug and ask for review?

What's the problem with this one ? Why not just renominate for koi? and change title ?

Comment 30

[:gerard-majax]

FYI this patch is not applicable anymore on current master.

Comment 31

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

Hi, could you submit your patch in a readable format? We will be happy to review it and include it in the upstream open source codebase.

https://developer.mozilla.org/en-US/Firefox_OS/Platform/Gaia/Hacking#Contributing_to_Gaia

Comment 32

[:gerard-majax]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #31)
> Hi, could you submit your patch in a readable format? We will be happy to
> review it and include it in the upstream open source codebase.
> 
> https://developer.mozilla.org/en-US/Firefox_OS/Platform/Gaia/
> Hacking#Contributing_to_Gaia

Looks like they are not willing to ... I'll update my patch if you like.

Comment 33

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

(In reply to Alexandre LISSY :gerard-majax from comment #32)
> Looks like they are not willing to ... I'll update my patch if you like.

Don't ask for permission for stealing bugs :)

BTW how is this a regression? We never have this feature right?

Comment 34

[:gerard-majax]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #33)
> (In reply to Alexandre LISSY :gerard-majax from comment #32)
> > Looks like they are not willing to ... I'll update my patch if you like.
> 
> Don't ask for permission for stealing bugs :)
> 
> BTW how is this a regression? We never have this feature right?

That is the fun part I lost a couple of days on: while we never had this feature in 1.0.1, partners did hack it. And hence considered a regression with 1.1 not having it :)

Comment 35

[Bruce Huang [:bhuang] (inactive)]

Hi Alexandre, 
Are you still working on this one or do you just need a review?  It would be great to include this feature if you feel it's ready.
Does it also address bug 961980?

Comment 36

[:gerard-majax]

(In reply to Bruce Huang [:bhuang] <[e-mail]> from comment #35)
> Hi Alexandre, 
> Are you still working on this one or do you just need a review?  It would be
> great to include this feature if you feel it's ready.
> Does it also address bug 961980?

No. As far as I know, this has not been triaged and it is not blocking any release, so considering the current context, there is absolutely no reason I would have worked on this.

Old patch is obviously obsolete now.

Comment 37

[:gerard-majax]

Maybe we can do it in 1.4 ?

Comment 38

[Gregor Wagner [:gwagner]]

(In reply to Alexandre LISSY :gerard-majax from comment #37)
> Maybe we can do it in 1.4 ?

You can land on master if you have proper tests. No need for a blocking flag.

Comment 39

[:gerard-majax]

Some body landed this feature, I have it working on my current master.

Comment 41

[:gerard-majax]

Comment on attachment 787522 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/11429

Cancelling review.