Test suites should not make noise

RESOLVED WONTFIX

Status

defect
RESOLVED WONTFIX
8 years ago
8 days ago

People

(Reporter: zwol, Assigned: cpearce)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

8 years ago
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.
Reporter

Comment 2

8 years ago
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
Assignee: nobody → chris
I think I'd prefer a lower-level control than this. What if we started optimizing zero-volume output somehow?
Comment 4 is private: false
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?

Comment 8

8 years ago
(In reply to comment #4)
> Created attachment 532571 [details] [diff] [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.
Reporter

Comment 10

8 years ago
Printing works around that by calling PR_strtod on string prefs.
Posted patch Patch v2 (obsolete) — Splinter Review
* Use a string pref instead.
* Special case volume=0 in nsAudioStream::Write().
Attachment #532765 - Flags: review?(roc)
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.

Comment 13

8 years ago
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.
Posted patch Patch: master volume/scale (obsolete) — Splinter Review
Attachment #532765 - Attachment is obsolete: true
Attachment #532765 - Flags: review?(roc)
Attachment #533078 - Flags: review?(roc)
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.
Posted patch Patch: master volume/scale v2 (obsolete) — Splinter Review
Attachment #533078 - Attachment is obsolete: true
Attachment #533078 - Flags: review?(roc)
Attachment #533093 - Flags: review?(roc)
Comment on attachment 533093 [details] [diff] [review]
Patch: master volume/scale v2

Review of attachment 533093 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #533093 - Flags: review?(roc) → review+
Oh, one thing I forgot: add the new pref to all.js.
With pref added to all.js.
Attachment #533093 - Attachment is obsolete: true
Attachment #533112 - Flags: review+
Fixed.

Example sets <video> volume to 12% during mochitests:
$ EXTRA_TEST_ARGS='--setpref=media.volume_scale=\".12\"' make -C $objdir mochitest-plain
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: mozilla6 → ---
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.
Reporter

Comment 27

8 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

8 years ago
Blocks: 514067

Comment 28

7 years ago
Hrm, what's left to do here given bug 812281?
Reporter

Comment 29

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

Comment 30

7 years ago
(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.

Updated

7 years ago
Depends on: 812281
Status: REOPENED → RESOLVED
Closed: 8 years ago8 days ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.