878662 - [System][Sound Manager] CEWarningDialog is displayed at CEWarningVol + 1.

Status: RESOLVED FIXED

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

Description

[Leo]

1. Title : [System][Sound Manager] CEWarningDialog is displayed at CEWarningVol + 1.
2. Precondition : Start playing video with earphone plugged in with low volume.
3. Tester's Action : Increase volume up to CEWarningVol.
4. Detailed Symptom (ENG.) : Notice the warning dialog is not displayed at CEWarningVol, but is displayed at CEWarningVol + 1.
5. Expected : The warning dialog should be displayed at CEWarningVol.
6.Reproducibility: Y
           1)Frequency Rate : 100%
7.
Mozilla build ID: 20130526070207
QCT Gaia Revision : 4d10e1297b859cacc174c0a54af61a7678d7c32d
8.Personal email id:  [e-mail]


This is because in headsetVolumeup function of sound_manager.js, the current volume is compared with CEWarningVol, when currentVolume + 1 should be compared.

  function headsetVolumeup() {
    if (currentVolume[getChannel()] >= CEWarningVol &&
        getChannel() == 'content') {

Comment 1

[Leo]

Similarly, in resetToCEMaxVolume function the Max Safe volume should be the one below the CEWarningVol.
Therefore, the code should be the following.

    var req = SettingsListener.getSettingsLock().set({
      'audio.volume.content': CEWarningVol - 1
    });

Comment 2

[Leo]

Attachment: Pull request url

Comment 3

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

Leo, this looks like not a bug to me.

Let's change the problem to:
Do we want to show the dialog when the volume is just above? or equal to the warning level?

And please keep in mind you could change the default settings of warning level.

IMO what we should do here is change the default setting if you think it's too high to be exceeded.

How do you think?

Comment 4

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

Comment on attachment 757212 [details]
Pull request url

See my comment

Comment 5

[hanj.kim25]

Attachment: Repro video

(In reply to Alive Kuo [:alive] from comment #3)

Yeah, I agree it's not a problem, if it's consistent with either the CEWarningVol or the CEWarningVol + 1.
However, the problem is that the actual warning levels are different in the following two cases.

1. When earphone is plugged at "CEWarningVol" during video playing.
2. When Volume is changed to "CEWarningVol + 1" during video playing.

Please refer to the attachment video to show the #2 issue and notice that the CEWarningDialog is prompted when it shouldn't.

00:21 - Press Volume up button to until the CEWarningDialog is prompted. 
          * The current volume must be already CEWarningVol for the CEWarningDialog to display, 
            because headsetVolumeup() compares with the current volume before increasing.
            Code: if (currentVolume[getChannel()] >= CEWarningVol &&
            
        Cancel is selected and the volume is set to CEWarningVol.
          * resetToCEMaxVolume(callback) sets the volume to CEWarningVol, 
            instead of the max under the warning level, which is CEWarningVol - 1.
          
00:28 - Fast-foward the video player. And CEWarningDialog is prompted because the current volume is >= CEWarningVol. 
          * headsetVolumeup() compares with >= CEWarningVol.
        Cancel is selected and the volume is set to CEWarningVol.
        
00:31 - Fast-foward the video player. And CEWarningDialog is prompted because the current volume is >= CEWarningVol. 
        Cancel is selected and the volume is set to CEWarningVol.
... repeats and CEWarningDialog keeps prompting without changing volume..

Comment 6

[hanj.kim25]

(In reply to Alive Kuo [:alive] from comment #4)
> Comment on attachment 757212 [details]
> Pull request url
> 
> See my comment

Pull request is updated now. Empty spaces are added around the + operator.

Thanks!

Comment 7

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

(In reply to hanj.kim25 from comment #6)
> (In reply to Alive Kuo [:alive] from comment #4)
> > Comment on attachment 757212 [details]
> > Pull request url
> > 
> > See my comment
> 
> Pull request is updated now. Empty spaces are added around the + operator.
> 
> Thanks!

Yes, the video convinces me this is a bug...lemme think again.

Comment 8

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

Comment on attachment 762538 [details]
Repro video

you are right!

Comment 9

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

Comment on attachment 757212 [details]
Pull request url

r=me

Comment 10

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

https://github.com/mozilla-b2g/gaia/commit/c0f91ba5ffc7fd9e696b3b577fbd8971aaaf1180

Comment 11

[Leo]

Uplift to v1-train? 

Thanks!

Comment 12

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

Transfer ni to uplift angel.

Comment 13

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted c0f91ba5ffc7fd9e696b3b577fbd8971aaaf1180 to:
v1-train: 5519797452732b398815188e80fc56123cbfbda0

Comment 14

[John Ford [:jhford] CET/CEST Berlin Time]

1.1hd: 5519797452732b398815188e80fc56123cbfbda0