All submissions to this site are governed by the Second Life Viewer Contribution Agreement. By submitting patches and other information using this site, you acknowledge that you have read, understood, and agreed to those terms.

Review Board 1.6.11

Welcome to the Second Life Viewer Code Review tool.
See the documentation on our wiki for how to use this site.

STORM-830 Volume slider isn't properly remembered if set to zero

Review Request #72 - Created Jan. 6, 2011 and submitted

Jonathan Yap Reviewers
2.5 viewer
STORM-830
None viewer-development 
There is an edge case in setMasterGain during startup which prevents setInternalGain from being called if the master volume setting and mInternalGain both equal 0.

Setting mInternalGain to a very low but non-zero value fixes this issue.



In Preferences / Sound & Media tested:
Buttons
Ambient
Sound Effects
Stream Music
Media
Voice Chat
Aleric Inglewood
Posted (Jan. 6, 2011, 5:37 p.m.)
This is really not how you want to deal with this bug :/.  It's a known fact that audio mixers are very bad with low volumes. Setting a volume to 0 (or something really small) can put a very high load on the CPU for the audio mixer, which causes severe problems. See https://jira.secondlife.com/browse/VWR-14914

So, on the contrary (as VWR-14914 fixed a horrible bug that made FPS drop drastically): when the volume is set to 0 (or even close to zero) the audio channel has to be muted and not mixed, ever. Assuming that VWR-14914 is in Viewer 2, and wasn't broken in the meantime, a volume of 0 would cause the channel to be muted, but setting it to 0.000001 will not cause it to be muted, but result in a high CPU load.

  1. After discussion on IRC Jonathan explained to me what this is all about. The patch is correct.
Nevertheless, I was confused about the fact that this value of 0.000001 is never going to be USED.
Perhaps a different value and comment can avoid that in the future when other coders look at it.
mInternalGain is only ever compared with, and the whole point of this patch is to make sure
that the first comparison fails (I now understand). A common value used to make sure that
a first comparison fails is a value outside the valid range of that variable. The valid
range being [0, 1], I'd suggest using -1 and adding a comment in the lines of:

    // Make sure that the first call to setMasterGain will cause setInternalGain to be called.
    mInternalGain = -1.f;

  2. Aleric is correct, and if the approach is technically possible it would be preferred to use an out-of-range value. It appears to me that his suggested fix will work, but I haven't tried it myself. Jonathan, can you try it, please?

  3. I changed the initial value of mInternalGain to -1, did a quick test with key clicks, and saw that that fixes the problem, too.
Ship it!
Kent Quirk
Posted (Jan. 10, 2011, 3:15 p.m.)
Looks good now.