Every time I run the crashtests I have to remember to turn off my speakers so I don't get startled out of my chair by the "No! No! No!" that it generates at some point. There's probably just one or two offending tests here, and I could file bugs specifically against them, but I think there's an important general principle here: the test suites should not make noise. Now, of course, we gotta test <audio> and <video>, and that includes testing that they do in fact make noise, but allowing the sound to be routed to the computer's speakers is not an effective way of testing that! We should be instructing the OS to route sound output to a capture device and then checking the results against reference files (sort of like how reftests already do for images). ... However, I suspect that that's a big Twinkie and that the crashtests that are currently making noise don't *really* care whether noise is being produced, so maybe *those* should be told to play silence, as a stopgap till we have the big Twinkie.

The actual code mechanics for fake sound devices differ from those for real sound devices, as I understand it, so playing real sound is a requirement here. That said, such sounds could be much more muted, I suspect, without sacrificing test coverage.

Under Linux, it should be possible to have the test harness wrap the browser in its own personalized PulseAudio instance that's configured to send all output to a wav file, without the browser's knowledge. Similarly, I'm not about to dig through MSDN to find out exactly how you do it, but in Windows Vista/7 it seems like it ought to be possible to instantiate a capturing output "endpoint device" and redirect sound to it from the test harness, again, without the knowledge of the code that does the actual audio production. No clue what is possible on OSX; I've long since given up on trying to find anything in Apple's docs.

We could have a pref or environment variable which overrode the default volume on media elements. We'd still go through all the same code paths, we'd just end up writing silence to the hardware rather than the actual waveform.

Add a pref "media.volume.default" to set the initial volume of media elements. Accepts values between [0,100]. Example sets <video> volume to 12% during mochitests: $ EXTRA_TEST_ARGS='--setpref=media.volume.default=12' make -C $objdir mochitest-plain

I think I'd prefer a lower-level control than this. What if we started optimizing zero-volume output somehow?

You mean not writing audio when the volume is 0? Two reasons why not: 1. It's not worth us spending too much time on this. 2. When we're running tests, we want to write silence rather than nothing. We will still run and test all the same code paths as we would if we weren't muted, which is what we want.

How about memsetting the buffer to zero in nsAudioStreamLocal::Write --- after filling it?

(In reply to comment #4) > Created attachment 532571 [details] [review] > Add a pref "media.volume.default" to set the initial volume of media > elements. Accepts values between [0,100]. I think either the pref name is wrong or the acceptable values are wrong. Media volume ranges from 0 to 1 so a 'volume default' should be in the same range.

You can't have float prefs, only string, bool, and int.

Printing works around that by calling PR_strtod on string prefs.

* Use a string pref instead. * Special case volume=0 in nsAudioStream::Write().

Why do we need this full-fledged pref? Is there any use-case for it other than just disabling audio for tests? If not, it should just be a boolean. Tests should take the same code paths as real usage, as far as possible. So in comment #5 I said I don't think we should change the volume that we pass into the decoder. And for the same reason in comment #7 I suggested that we should let nsAudioStreamLocal::Write compute all the new sample values and then after that, if we're in test mode, we should just memset the buffer to zero.

This pref could also be used to fix the long standing issue of having no 'master volume' control in Firefox. Attach some UI to change the pref and the user can set a volume.

Because *I* want to turn the volume *down* on tests when I run them so that I can hear them running, so I know they're running and not finished or hung without having to watch the mochitest window, but I want the volume *down* so that they don't blast over top of the music I'm listening to at the same time. I don't want to mute!

Zack wants audio on tests to be completely disabled by default. Does everyone agree with that goal?

I have no problem with audio being disabled by default in tests provided we follow the same code paths as we would if we were playing audio. But not at the expense of having a default volume pref. We could easily change runtests.py to set the default volume pref to 0 by default.

So the current patch just sets the initial volume to the preffed value. This isn't what I thought of as the "master volume", and it doesn't satisfy Zack's goal of tests not playing sound (assuming we have tests that explicitly set the volume; if we don't, we should!). I can think of three kinds of prefs we could have here: 1) A pref setting a default volume which can be overriden by script (Chris P's patch, minus the nsAudioStream changes which I don't think are necessary for this patch). I'm not sure what the use-cases for this pref are, and it might be a spec violation for the volume to not initially be 1.0. 2) A pref setting a master volume which all other volume settings are scaled by. 3) A boolean which disables audio output at the nsAudioStream level or below. It seems to me that #2, implemented at the nsAudioStream level, would satisfy everyone here.

Comment on attachment 533078 [details] [diff] [review] Patch: master volume/scale Review of attachment 533078 [details] [diff] [review]: ----------------------------------------------------------------- Otherwise looks good. ::: content/media/nsAudioStream.cpp @@ +355,5 @@ > { > + nsContentUtils::UnregisterPrefCallback("media.volume_scale", > + VolumeScaleChanged, > + nsnull); > + delete gVolumeScaleLock; Set gVolumeScaleLock to null as well. @@ +471,4 @@ > if (s_data) { > + if (volume_scale == 0) { > + mBufferOverflow.Clear(); > + memset(s_data, 0, sizeof(short) * count); We don't want this optimization; if someone has set it to zero for testing, we want the code below to run so that at least the test executes it! @@ +501,5 @@ > } > + case FORMAT_FLOAT32: { > + const float* buf = static_cast<const float*>(aBuf); > + for (PRUint32 i = 0; i < aCount; ++i) { > + float scaled_value = floorf(0.5 + 32768 * buf[i] * mVolume * volume_scale); Probably worth hoisting out mVolume * volume_scale. In fact you should hoist it right out of the switch.

Comment on attachment 533093 [details] [diff] [review] Patch: master volume/scale v2 Review of attachment 533093 [details] [diff] [review]: -----------------------------------------------------------------

Oh, one thing I forgot: add the new pref to all.js.

With pref added to all.js.

Fixed. Example sets <video> volume to 12% during mochitests: $ EXTRA_TEST_ARGS='--setpref=media.volume_scale=\".12\"' make -C $objdir mochitest-plain

Comment on attachment 533112 [details] [diff] [review] Patch: master volume/scale v3 >--- a/content/media/nsAudioStream.cpp >+++ b/content/media/nsAudioStream.cpp >+static int VolumeScaleChanged(const char* aPref, void *aClosure) { >+ NS_ConvertUTF16toUTF8 utf8(value); >+ gVolumeScale = PR_MAX(0, PR_strtod(utf8.get(), nsnull)); This should be NS_MAX. See bug 512106. Please fix.

The bug report was "test suites should not make noise", not "there should be a knob so that test suites can be told not to make noise". A complete fix would include patches to the harnesses so that the default behavior is not to make noise.

Hrm, what's left to do here given bug 812281?

I think comment 17 is still relevant. Also, does bug 812281 cover *all* the test suites?

(In reply to comment #29) > I think comment 17 is still relevant. Also, does bug 812281 cover *all* the > test suites? It should cover mochitest-* and reftest/crashtest.