Closed Bug 1447273 Opened 6 years ago Closed 6 years ago

WebAudio changing volume not working

Categories

(Core :: Audio/Video: MediaStreamGraph, defect, P2)

Product:
Core
Component:
Audio/Video: MediaStreamGraph
Version:
58 Branch
Platform:
x86_64
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- wontfix
firefox60 + verified
firefox61 + verified

People

(Reporter: zhanelj, Assigned: pehrsons)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files, 5 obsolete files)

Bug 1447273 - Reduce AudioChunk::CanCombineWithFollowing indentation.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1447273 - Consider all AudioChunk members in AudioChunk::CanCombineWithFollowing.
59 bytes, text/x-review-board-request
padenot
: review+
jcristau
: approval-mozilla-beta+
Details
bug1447273_test.patch
8.58 KB, patch
padenot
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.186 Safari/537.36 OPR/51.0.2830.55

Steps to reproduce:

I need to fadein webaudio volume while playng sound.
Bug is desribed here: https://github.com/photonstorm/phaser-ce/issues/459

I think same problem is here: https://www.html5rocks.com/en/tutorials/webaudio/intro/#toc-volume
try to change volume in FF, nothing happens.


Actual results:

Volume stays constat, after next loop (if looping) volume changes to desired value.


Expected results:

Volume should change (in time)
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180322220118

Hello,

I have managed to reproduce this issue on latest Firefox release 59.0.1, latest Nightly build 61.0a1, using Windows 10x64.
I have accessed the "https://www.html5rocks.com/en/tutorials/webaudio/intro/#toc-volume" page and changed the volume, but it didn't change considering the custom volume position.

This issue is not reproducible on older Nightly versions, such as Nightly 58.0a1 (2017-09-21). Considering this, I've managed to find a regression-window using the Mozregression tool. Here are the results:
Last good revision: 5e311cd7074e6b649187f5a79371f0681c2d7504
First bad revision: 8448eee20c9afa97a9679cae293ad1f21e7f6668
Pushlog: https://goo.gl/b1GUSC
From the pushlog it seems that bug 1296531 might have caused this. Andreas, can you please take a look at this issue?

This issue is also reproducible Mac 10.13.3 and Ubuntu 14.04 x64, but a little bit different: there is a brief delay if you change the volume, but not as noticeable as on Windows OS.
This issue is not reproducible on Chrome.
Status: UNCONFIRMED → NEW
status-firefox59: --- → affected
status-firefox60: --- → affected
status-firefox61: --- → affected
Component: Untriaged → Audio/Video: Recording
Ever confirmed: true
Flags: needinfo?(apehrson)
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → x86_64
Assignee: nobody → apehrson
Status: NEW → ASSIGNED
Rank: 11
Flags: needinfo?(apehrson)
Priority: -- → P2
I think I now understand what goes wrong and how this occurred.

We combine two audio chunks (first with volume 1, second with our custom volume) into one, the stack is essentially [1]->[2]->[3] with [1] being the lowest. The combined chunk ends up having a volume of 1.

Bug 1296531 made changes to AdvanceOutputSegment() [2] to now use AppendFrom() [1] to append a segment, doing the combination check for each of the segment's chunks. Previously we appended a chunk directly without attempting to combine.

Combining and losing the volume is allowed because AudioChunk::CanCombineWithFollowing() doesn't consider volume in its check. After the first loop it works as expected because the two chunks we're trying to combine have different mBuffer members and combining is therefor disallowed. I didn't look into why these mBuffer members now differ though. Paul, do you know? Does it matter?

To summarize, bug 1296531 regressed this by accident, as the core of the problem is a latent bug in the core bits of the AudioChunk code.


[1] https://searchfox.org/mozilla-central/rev/003262ae12ce937950ffb8d3b0fa520d1cc38bff/dom/media/MediaSegment.h#457-461
[2] https://searchfox.org/mozilla-central/rev/003262ae12ce937950ffb8d3b0fa520d1cc38bff/dom/media/webaudio/AudioNodeStream.cpp#657
[3] https://searchfox.org/mozilla-central/rev/003262ae12ce937950ffb8d3b0fa520d1cc38bff/dom/media/webaudio/AudioNodeStream.cpp#592
Component: Audio/Video: Recording → Audio/Video: MediaStreamGraph
Flags: needinfo?(padenot)
Comment on attachment 8962401 [details]
Bug 1447273 - Reduce AudioChunk::CanCombineWithFollowing indentation.

https://reviewboard.mozilla.org/r/231248/#review237250
Attachment #8962401 - Flags: review?(padenot) → review+
Comment on attachment 8962402 [details]
Bug 1447273 - Consider all AudioChunk members in AudioChunk::CanCombineWithFollowing.

https://reviewboard.mozilla.org/r/231250/#review237252

Please write a test for this. You can inspect the actual volume using a ScriptProcessorNode, right?
Attachment #8962402 - Flags: review?(padenot) → review+
Clearing NI.
Flags: needinfo?(padenot)
(In reply to Paul Adenot (:padenot) from comment #6)
> Comment on attachment 8962402 [details]
> Bug 1447273 - Consider all AudioChunk members in
> AudioChunk::CanCombineWithFollowing.
> 
> https://reviewboard.mozilla.org/r/231250/#review237252
> 
> Please write a test for this. You can inspect the actual volume using a
> ScriptProcessorNode, right?

Well yes, but I wasn't successful in reproducing this. I wrote this fiddle to make a 5 second buffer piped through a gain mode but it works fine, https://jsfiddle.net/pehrsons/v4s45Lvc/

If by any chance the bug is dependent on the type of source buffer or where it comes from I can pursue this further, otherwise I'm stumped.

Paul, the original question remains and could be key to a repro. When could the chunks' mBuffer be the same and when are they not?
Flags: needinfo?(padenot)
See comment 1: this happens because the source is a MediaElementAudioSourceNode (it would be the same for a MediaStreamAudioSourceNode of course). [0] might be useful here, it lets you get an opus file generated with Web Audio, to be used later in the test, so we don't have to check something in.

[0]: https://searchfox.org/mozilla-central/source/dom/media/tests/mochitest/test_getUserMedia_audioCapture.html#23
Flags: needinfo?(padenot)
(In reply to Paul Adenot (:padenot) from comment #9)
> See comment 1: this happens because the source is a
> MediaElementAudioSourceNode (it would be the same for a
> MediaStreamAudioSourceNode of course). [0] might be useful here, it lets you
> get an opus file generated with Web Audio, to be used later in the test, so
> we don't have to check something in.
> 
> [0]:
> https://searchfox.org/mozilla-central/source/dom/media/tests/mochitest/
> test_getUserMedia_audioCapture.html#23

Hmm, no, the source for the sample page [1] uses an AudioBufferSourceNode, [2].

[1] https://www.html5rocks.com/en/tutorials/webaudio/intro/#toc-volume
[2] https://www.html5rocks.com/en/tutorials/webaudio/intro/js/volume-sample.js
The plot thickens. We should have a look at what is happening in the `AudioBufferSourceNode`.
requesting tracking for: 58 regression; the WebAudio GainNode's gain attribute sometimes doesn't have any effect. We should get this fixed for 60.
Blocks: 1296531
status-firefox59: affected → wontfix
status-firefox-esr52: --- → unaffected
tracking-firefox60: --- → ?
tracking-firefox61: --- → ?
Keywords: regression
Version: 60 Branch → 58 Branch
(In reply to Andreas Pehrson [:pehrsons] from comment #8)
> (In reply to Paul Adenot (:padenot) from comment #6)
> > Comment on attachment 8962402 [details]
> > Bug 1447273 - Consider all AudioChunk members in
> > AudioChunk::CanCombineWithFollowing.
> > 
> > https://reviewboard.mozilla.org/r/231250/#review237252
> > 
> > Please write a test for this. You can inspect the actual volume using a
> > ScriptProcessorNode, right?
> 
> Well yes, but I wasn't successful in reproducing this. I wrote this fiddle
> to make a 5 second buffer piped through a gain mode but it works fine,
> https://jsfiddle.net/pehrsons/v4s45Lvc/
> 
> If by any chance the bug is dependent on the type of source buffer or where
> it comes from I can pursue this further, otherwise I'm stumped.
> 
> Paul, the original question remains and could be key to a repro. When could
> the chunks' mBuffer be the same and when are they not?

Unless missing something the volume changes at both Firefox Nightly 60 and Chromium 65 at linked jsfiddle
(In reply to guest271314 from comment #13)
> Unless missing something the volume changes at both Firefox Nightly 60 and
> Chromium 65 at linked jsfiddle

Yes, like I said
> I wrote this fiddle to make a 5 second buffer piped through a gain mode but it works fine

We're trying to figure out why it works fine so we can write one that doesn't (and a test), and perhaps optimize something else by making it not work fine without the fix. (it works because a small chunk of a buffer is copied instead of referenced - copying is bad for perf so we should try to avoid it)
(In reply to Andreas Pehrson [:pehrsons] from comment #14)
> (In reply to guest271314 from comment #13)
> We're trying to figure out why it works fine so we can write one that
> doesn't (and a test), and perhaps optimize something else by making it not
> work fine without the fix. (it works because a small chunk of a buffer is
> copied instead of referenced - copying is bad for perf so we should try to
> avoid it)

Not following the purpose of composing one that does not work fine? Is the bug relevant only as to https://github.com/photonstorm/phaser-ce ?
At Firefox Nightly 60 at *nix the volume at https://www.html5rocks.com/en/tutorials/webaudio/intro/#toc-volume changes when input range is changed.
(In reply to guest271314 from comment #15)
> (In reply to Andreas Pehrson [:pehrsons] from comment #14)
> > (In reply to guest271314 from comment #13)
> > We're trying to figure out why it works fine so we can write one that
> > doesn't (and a test), and perhaps optimize something else by making it not
> > work fine without the fix. (it works because a small chunk of a buffer is
> > copied instead of referenced - copying is bad for perf so we should try to
> > avoid it)
> 
> Not following the purpose of composing one that does not work fine? Is the
> bug relevant only as to https://github.com/photonstorm/phaser-ce ?

The patches on this bug fixes the issue, but we need to write a test case to catch future regressions.
Looks like this is something to do with stereo. Fiddle expanding on the above: https://jsfiddle.net/SingingTree/70sj8v4p/

When we're running with a mono source we're fine, but with a stereo things get weird. I see the same with the original techno.wav if I copy out a single channel to a buffer.
 t(In reply to Bryce Van Dyk (:bryce) from comment #18)
> Looks like this is something to do with stereo. Fiddle expanding on the
> above: https://jsfiddle.net/SingingTree/70sj8v4p/
> 
> When we're running with a mono source we're fine, but with a stereo things
> get weird. I see the same with the original techno.wav if I copy out a
> single channel to a buffer.

The effect appears to be related to the jsfiddle. When isolating the code to provide two entirely different <input type="range"> and an oscillator node, changing gain has same effect whether there are one or two channels https://jsfiddle.net/e0jzcakm/
var ab = context.createBuffer(1, context.sampleRate, context.sampleRate); can be commented at the linked jsfiddle. Perhaps still not gathering what the issue is, or cannot reproduce, here.
https://jsfiddle.net/e0jzcakm/1/ sets channelCount to 2, 1, respectively. Cannot determine an audible difference between changing gain.value where channelCount is 2 or 1 at an oscillator node.

How to reproduce the bug?
(In reply to guest271314 from comment #21)
> https://jsfiddle.net/e0jzcakm/1/ sets channelCount to 2, 1, respectively.
> Cannot determine an audible difference between changing gain.value where
> channelCount is 2 or 1 at an oscillator node.
> 
> How to reproduce the bug?

This is not about OscillatorNode. See comment 0 and comment 1 for repro.
At Nightly 60 at the "Equal power crossfading" example the generated audio does have a "fade in" effect. With "Playlist crossfading" example, am not sure what the expected result is.
It appears that beginning the procedure by initially setting gainNode.gain.value = 0, then using gainNodeStereo.gain.linearRampToValueAtTime(1.0, context.currentTime + secondsInFuture) does not ramp the value over the span of secondsInFuture https://jsfiddle.net/e0jzcakm/3/. Beginning with 
gainNodeStereo.gain.linearRampToValueAtTime(0.0, context.currentTime) then using gainNodeStereo.gain.linearRampToValueAtTime(1.0, context.currentTime + secondsInFuture) does ramp the value over secondsInFuture https://jsfiddle.net/e0jzcakm/4/. 

Have not tried the linked library, and am not sure what expected result of https://phaser.io/examples/v2/audio/fade-in demonstration is.
I've been trying to adapt the fiddle above into a test, but it's been ill fated thus far. Findings and various dead ends:

- I don't observe the issue when using gain.setValueAtTime(). It looks like the value has to be set via explicit assignment.
- Given the above I don't think I can do an offline audio context test. Since we don't impl context.suspend(time) I'm uncertain as to how to assign the gain without using setValueAtTime, which doesn't repro.
- Tried to do a live test with analyser nodes, however the analyses appear to report the correct values, even if we don't hear it in output. Here's a fiddle showing this: https://jsfiddle.net/SingingTree/tpz0zabo/
(In reply to Bryce Van Dyk (:bryce) from comment #25)
> - Tried to do a live test with analyser nodes, however the analyses appear
> to report the correct values, even if we don't hear it in output. Here's a
> fiddle showing this: https://jsfiddle.net/SingingTree/tpz0zabo/

Better luck piping it to a MediaStreamTrack and back to a webaudio analyser: https://jsfiddle.net/tpz0zabo/42/
Is the bug relevant to the noticeable time between toggling buffers before changing gain.value at input[type="range"] occurs; which does not occur at Chromium 65?
Does Firefox automatically convert the string input[type="range"].value (as used at the jsfiddle examples) to floating point number, internally, immediately? Or does that conversion take time?
Does Firefox require some form of user gesture to adjust audio output when a value relating to the audio output changes?
(In reply to Andreas Pehrson [:pehrsons] from comment #26)
> (In reply to Bryce Van Dyk (:bryce) from comment #25)
> > - Tried to do a live test with analyser nodes, however the analyses appear
> > to report the correct values, even if we don't hear it in output. Here's a
> > fiddle showing this: https://jsfiddle.net/SingingTree/tpz0zabo/
> 
> Better luck piping it to a MediaStreamTrack and back to a webaudio analyser:
> https://jsfiddle.net/tpz0zabo/42/

Able to reproduce an approximately 5 second duration between setting gainNode.gain.value and the audio output reflecting that change following toggling the source buffer between the two options at the linked jsfiddle. 

This occurs at Nightly 60. 

Chromium 65 reflects the changes to gainNode.gain.value immediately (perceptibly).
Using linearRampToValueAtTime() appears to resolve the issue https://jsfiddle.net/tpz0zabo/45/
setValueAtTime also resolves the issue https://jsfiddle.net/tpz0zabo/46/. Is there an internal clock associated with when gainNode.gain.value is checked and set?
First draft at a test. Interested on if there's any better way to do various things:
- Timer usage in order to get gain.value explicit assignment.
- Media stream round trip to get analyser to see bad data.
- Checking outputs by scraping analysers time data.
- requestAnimationFrame based polling of analysers.
(In reply to Bryce Van Dyk (:bryce) from comment #33)
> Created attachment 8965539 [details]
> test_gainNodeStereoInputVolumeChange.html
> 
> First draft at a test. Interested on if there's any better way to do various
> things:
> - Timer usage in order to get gain.value explicit assignment.
This is fine. I don't see another way to do it. Just ensure that the buffer is long enough to outlive any timing issues a slow machine might have. Well over 5 seconds would be fine.

> - Media stream round trip to get analyser to see bad data.
You could name the test case after this bug to make it clear that we're testing a corner case.

> - Checking outputs by scraping analysers time data.
It's either ScriptProcessorNodes or AnalyserNodes. But you could try to use our helper to make your code a bit more high level. [1]

> - requestAnimationFrame based polling of analysers.
The biggest risk here is probably starving out the setTimeout() that sets the gain, so make sure to start the rAF after the gain has been set or use setTimeout instead. Or if you use waitForAnalysisSuccess [2] it's taken care of for you (but indeed also uses rAF underneath).


[1] https://searchfox.org/mozilla-central/rev/d0413b711da4dac72b237d0895daba2537f1514c/dom/media/tests/mochitest/head.js#98
[2] https://searchfox.org/mozilla-central/rev/d0413b711da4dac72b237d0895daba2537f1514c/dom/media/tests/mochitest/head.js#195
Hrm, that didn't quite work how I wanted. Let's fix this up.
Attachment #8965758 - Attachment is obsolete: true
Attachment #8965758 - Flags: review?(padenot)
Attachment #8965759 - Attachment is obsolete: true
Attachment #8965759 - Flags: review?(padenot)
Attachment #8965760 - Attachment is obsolete: true
Attachment #8965760 - Flags: review?(apehrson)
Attachment #8965758 - Attachment is obsolete: false
Attachment #8965759 - Attachment is obsolete: false
Attachment #8965760 - Attachment is obsolete: false
Attached patch bug1447273_test.patch (obsolete) — — Splinter Review 
Let's try the olde review ways.

- Rename test.
- Shorten some var names.
- Only start requestAnimationFrame after gain setTimeout called.
- More comments.
- I didn't bring in the /dom/media/tests/mochitest/head.js for fear of polluting the webaudio test encapsulation. So the existing analyser checks remain.
Attachment #8965539 - Attachment is obsolete: true
Attachment #8965762 - Flags: review?(apehrson)
(In reply to Bryce Van Dyk (:bryce) from comment #39)
> Created attachment 8965762 [details] [diff] [review]
> bug1447273_test.patch
> 
> Let's try the olde review ways.
> 
> - Rename test.
> - Shorten some var names.
> - Only start requestAnimationFrame after gain setTimeout called.
> - More comments.
> - I didn't bring in the /dom/media/tests/mochitest/head.js for fear of
> polluting the webaudio test encapsulation. So the existing analyser checks
> remain.

Where are "/tests/SimpleTest/SimpleTest.js" and "webaudio.js" located? How to run the tests locally?
(In reply to guest271314 from comment #40) 
> Where are "/tests/SimpleTest/SimpleTest.js" and "webaudio.js" located? How
> to run the tests locally?

Those are part of the testing components in mozilla-central[0][1]. In order to run this particular test you'll need a build of firefox[2] from the changesets on this bug (or mozilla-central once these changes are in), and then to the mach command ot run the test: ` ./mach mochitest dom/media/webaudio/test/test_bug1447273.html`.

[0]: https://hg.mozilla.org/mozilla-central/file/tip/testing/mochitest/tests/SimpleTest/SimpleTest.js
[1]: https://hg.mozilla.org/mozilla-central/file/tip/dom/media/webaudio/test/webaudio.js
[2]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
(In reply to Bryce Van Dyk (:bryce) from comment #41)
> (In reply to guest271314 from comment #40) 
> > Where are "/tests/SimpleTest/SimpleTest.js" and "webaudio.js" located? How
> > to run the tests locally?
> 
> Those are part of the testing components in mozilla-central[0][1]. In order
> to run this particular test you'll need a build of firefox[2] from the
> changesets on this bug (or mozilla-central once these changes are in), and
> then to the mach command ot run the test: ` ./mach mochitest
> dom/media/webaudio/test/test_bug1447273.html`.
> 
> [0]:
> https://hg.mozilla.org/mozilla-central/file/tip/testing/mochitest/tests/
> SimpleTest/SimpleTest.js
> [1]:
> https://hg.mozilla.org/mozilla-central/file/tip/dom/media/webaudio/test/
> webaudio.js
> [2]:
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Build_Instructions/Simple_Firefox_build

Unfortunately, currently only have access to a device with <200MB of available disk space (not enough to build firefox from scratch). Running the test manually at nightly 60 should reproduce the bug, correct?
That's correct, and it is possible to download the test artifacts and run them against a Firefox release. However, since this test isn't in the repo yet you'd need to massage it into a test artifact. If you want to manually repro I'd suggest that the fiddles discussed here provide a more ergonomic way to do so. The test is essentially a reworked version of the last fiddle mentioned by Andreas above.
(In reply to Bryce Van Dyk (:bryce) from comment #43)
> That's correct, and it is possible to download the test artifacts and run
> them against a Firefox release. However, since this test isn't in the repo
> yet you'd need to massage it into a test artifact. If you want to manually
> repro I'd suggest that the fiddles discussed here provide a more ergonomic
> way to do so. The test is essentially a reworked version of the last fiddle
> mentioned by Andreas above.

Substituting connecting `src` to `dest`, `srcStream` to `gain` and `gain` to `ac.destination` 

  src.connect(dest);
  srcStream.connect(gain);
  gain.connect(ac.destination);

for 

  src.connect(gain);
  gain.connect(dest);
  srcStream.connect(analyser);
  analyser.connect(ac.destination);


appears to produce expected result https://jsfiddle.net/tpz0zabo/48/
Does AnalyserNode connected to AudioContext.destination affect the audio output?
(In reply to Bryce Van Dyk (:bryce) from comment #39)
> Created attachment 8965762 [details] [diff] [review]
> bug1447273_test.patch
> 
> Let's try the olde review ways.

You can push a single changeset to review with `hg push -c [rev] review` or multiple ones with `hg push -r [rev1]::[rev2] review`.
Comment on attachment 8965762 [details] [diff] [review]
bug1447273_test.patch

Review of attachment 8965762 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Aside from fixing the comments below I'd only like to see a try push where this fails pre-fix, before giving r+.

I'll also r? padenot as he's a webaudio peer.

::: dom/media/webaudio/test/mochitest.ini
@@ +106,5 @@
>  skip-if = toolkit == 'android' # bug 1056706
>  [test_bug1255618.html]
>  [test_bug1267579.html]
>  [test_bug1355798.html]
> +[test_bug1447273.html]

Have you tried this on Android? It's prone to intermittents, especially when you don't stick to just pure audio but involve something heavier, like the audio analysis and canvas drawing we do here.

::: dom/media/webaudio/test/test_bug1447273.html
@@ +25,5 @@
> +  let context = new AudioContext();
> +
> +  let numChannels = 2;
> +  let sampleRate = context.sampleRate;
> +  // 8 seconds to mitigate timing issues on slow test machines

How long does the test take on the slowest machines? I.e., roughly how much margin do we have?

@@ +121,5 @@
> +    info(`maxExplicitAssignment: ${maxExplicitAssignment}`);
> +    info(`maxSetValueAtTime: ${maxSetValueAtTime}`);
> +    ok(gainExplicitlyIncreased,
> +       "Gain should be explicitly assinged during test!");
> +    ok(maxExplicitAssignment > maxNoGainChange,

Let's put some margins here so we're sure we don't just hit some 1-off rounding issue.

@@ +125,5 @@
> +    ok(maxExplicitAssignment > maxNoGainChange,
> +       "Volume should increase due to explicit assignment to gain.value");
> +    ok(maxSetValueAtTime > maxNoGainChange,
> +       "Volume should increase due to setValueAtTime on gain.value");
> +    SimpleTest.finish();

Re my other comment, it would make sense to end the test and call finish() if all the criteria have been met before the source reaches the end. That way the test duration doesn't depend on the source length.

It also lets us use a longer source to further eliminate intermittents while not prolonging the test duration.

Just make sure to flag it so we don't end up double-finishing.
Attachment #8965762 - Flags: review?(apehrson)
Attachment #8965762 - Flags: review?(padenot)
(In reply to guest271314 from comment #44)
> Substituting connecting `src` to `dest`, `srcStream` to `gain` and `gain` to
> `ac.destination` 
> 
>   src.connect(dest);
>   srcStream.connect(gain);
>   gain.connect(ac.destination);
> 
> for 
> 
>   src.connect(gain);
>   gain.connect(dest);
>   srcStream.connect(analyser);
>   analyser.connect(ac.destination);
> 
> 
> appears to produce expected result https://jsfiddle.net/tpz0zabo/48/

This is about reproducing the original issue, not about finding workarounds.

Piping the source buffer to a MediaStream likely copies the buffer to smaller chunks, which would explain why it no longer repros.
Comment on attachment 8965762 [details] [diff] [review]
bug1447273_test.patch

Clearing r? while I work on comments above. I've decided to bring in some of the analysis code from dom/tests/mochitests after more discussion to simplify the analysis step and make is easier for us to end early.
Attachment #8965762 - Flags: review?(padenot)
Updated to address comments.
- Significantly longer timeout 8 -> 60 seconds.
- Polling for gain changes function now ends the test early if it detects the pass condition.
- Comparisons between control stream and other streams now add an epsilon to the control streams gain to make sure we don't get flaky passes.
- Try pushed and seems happy across all platforms, but am happy to have it disabled on android if this is the kind of thing likely to cause pain.

Didn't end up using the analysis code from other tests. Readding r?
Attachment #8965762 - Attachment is obsolete: true
Attachment #8966291 - Flags: review?(padenot)
Do you have a failing pre-fix try run too?
Pushed a fresh one above. Will require some more time to get through all platforms, but it's looking appropriately unhappy for the explicit gain assignment case thus far.
(In reply to Andreas Pehrson [:pehrsons] from comment #48)
> (In reply to guest271314 from comment #44)
> > Substituting connecting `src` to `dest`, `srcStream` to `gain` and `gain` to
> > `ac.destination` 
> > 
> >   src.connect(dest);
> >   srcStream.connect(gain);
> >   gain.connect(ac.destination);
> > 
> > for 
> > 
> >   src.connect(gain);
> >   gain.connect(dest);
> >   srcStream.connect(analyser);
> >   analyser.connect(ac.destination);
> > 
> > 
> > appears to produce expected result https://jsfiddle.net/tpz0zabo/48/
> 
> This is about reproducing the original issue, not about finding workarounds.
> 
> Piping the source buffer to a MediaStream likely copies the buffer to
> smaller chunks, which would explain why it no longer repros.

Got it. One item that am curious about is that when looking into trying to find workarounds are several descriptions of AnalyserNode not being reliable as to output, as attempted inquired about at https://bugzilla.mozilla.org/show_bug.cgi?id=1447273#c45



> You'll have to use a script processor, because you will need to process every sample in order not to miss any data. Analyzer nodes take snippets of data, but will miss some. Just examine the input data in an onaudioprocess.

https://stackoverflow.com/a/29184485

> Yeah, you can't really use an analyzer. There's too much uncertainty in when it will get run, and you can't guarantee precisely when it will run. You're better off using a ScriptProcessor for now (AudioWorklet eventually), and doing the FFT (or other recognition code) yourself

https://stackoverflow.com/a/43194475

Is AnalyserNode passing through the audio as expected? Does using AudioWorklet node produce same results?
Comment on attachment 8966291 [details] [diff] [review]
bug1447273_test.patch

Review of attachment 8966291 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!
Attachment #8966291 - Flags: review?(padenot) → review+
(In reply to guest271314 from comment #55)\
> Got it. One item that am curious about is that when looking into trying to
> find workarounds are several descriptions of AnalyserNode not being reliable
> as to output, as attempted inquired about at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1447273#c45]
> 
> (...)
> 
> Is AnalyserNode passing through the audio as expected? Does using
> AudioWorklet node produce same results?

Depends on your definition of "expected". You can't get data good enough to feed your speakers and expect it to sound glitch-free, but you can get good enough data to analyse applied volume like we're doing here.

AudioWorklet could likely be used instead of the analyser here, but would take more work to interface with since it runs in a worker. Since we don't need glitch-free data for our analysis the AudioAnalyserNode is a simple choice.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8321b37abde4af73e8000cda1688190c39060f54
Bug 1447273 - Reduce AudioChunk::CanCombineWithFollowing indentation. r=padenot

https://hg.mozilla.org/integration/mozilla-inbound/rev/594a01bb3d1220e70a7bb8843c08145baaacfd3a
Bug 1447273 - Consider all AudioChunk members in AudioChunk::CanCombineWithFollowing. r=padenot

https://hg.mozilla.org/integration/mozilla-inbound/rev/d11a0711723ee564b9e09e6714e0c13049c225ae
Bug 1447273 - Add test to check that different GainNode configuraitons produce correct output. r=padenot
Comment on attachment 8962402 [details]
Bug 1447273 - Consider all AudioChunk members in AudioChunk::CanCombineWithFollowing.

This approval request applies to all patches on this bug.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1296531
[User impact if declined]: A basic feature of WebAudio (changing volume with GainNode) sometimes (depending on the source is connected to it) doesn't have any effect.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: I checked just now and it looks good.
[Needs manual test from QE? If yes, steps to reproduce]: It would help, see comment 1
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: One patch is trivial, the other (this) is very simple too. I don't really see any side effects from it either.
[String changes made/needed]: None
Attachment #8962402 - Flags: approval-mozilla-beta?
Comment on attachment 8962402 [details]
Bug 1447273 - Consider all AudioChunk members in AudioChunk::CanCombineWithFollowing.

webaudio fix, approved for 60.0b12
Attachment #8962402 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
I managed to reproduce this issue on Firefox Release 59.0.2 and on an older Nightly build 61.0a1 (Build ID: 20180322220118): the volume has a delay until it's changing according to its custom position, using Windows 10 x64.
 
I retested the issue and it is no longer reproducible using latest Firefox Beta 60.0b12 (Build ID: 20180412172954) and Firefox Nightly 61.0a1 (Build ID: 20180415220108) on Windows 10 x64, Mac 10.12.6 and Ubuntu 14.04 x64. The volume is correctly changing according to its custom position, on https://www.html5rocks.com/en/tutorials/webaudio/intro/#toc-volume. It seems that the bug has been fixed.

Thanks.
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
status-firefox61: fixed → verified
Flags: qe-verify+
Depends on: 1454929
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: