Closed Bug 878662 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:leo+, b2g18 fixed, b2g-v1.1hd fixed)

RESOLVED FIXED
blocking-b2g leo+
Tracking Status
b2g18 --- fixed
b2g-v1.1hd --- fixed

People

(Reporter: leo.bugzilla.gaia, Assigned: leo.bugzilla.gaia)

References

Details

Attachments

(2 files)

198 bytes, text/html
alive
: review+
Details
2.40 MB, application/octet-stream
alive
: feedback+
Details
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:  hanj.kim25@gmail.com


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') {
See Also: → 855792
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
    });
Attached file Pull request url
Attachment #757212 - Flags: review?(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?
Assignee: nobody → leo.bugzilla.gaia
Flags: needinfo?(leo.bugzilla.gaia)
Comment on attachment 757212 [details]
Pull request url

See my comment
Attachment #757212 - Flags: review?(alive)
Attached file 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..
Attachment #762538 - Flags: feedback?(alive)
Flags: needinfo?(leo.bugzilla.gaia)
(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!
(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 on attachment 762538 [details]
Repro video

you are right!
Attachment #762538 - Flags: feedback?(alive) → feedback+
Comment on attachment 757212 [details]
Pull request url

r=me
Attachment #757212 - Flags: review+
Uplift to v1-train? 

Thanks!
blocking-b2g: --- → leo+
Flags: needinfo?(alive)
Transfer ni to uplift angel.
Flags: needinfo?(alive)
Uplifted c0f91ba5ffc7fd9e696b3b577fbd8971aaaf1180 to:
v1-train: 5519797452732b398815188e80fc56123cbfbda0
1.1hd: 5519797452732b398815188e80fc56123cbfbda0
You need to log in before you can comment on or make changes to this bug.