Noisy/distorted audio on Web Audio demo (Ring Modulator from BBC)

RESOLVED FIXED in Firefox 42

Status

()

defect
P1
normal
Rank:
15
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: epinal99-bugzilla2, Assigned: padenot)

Tracking

({regression})

41 Branch
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 unaffected, firefox41- wontfix, firefox42+ fixed, firefox43+ fixed, firefox44 fixed)

Details

(Whiteboard: [mozfr-community], )

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

4 years ago
Bug found when triaging bug 1203111.

STR:
1) Open http://webaudio.prototyping.bbc.co.uk/ring-modulator/#demo
2) Click on the speech tooltips.

Result: audio is altered and noisy.

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7a68ee30a57450602d7e445a5ba61c2cdbb0c07e&tochange=6ce98e37b52e63b3d1cb452e9bb61edc164f6ddb

Yanis Sellami — Bug 1118372 - Properly apply volume in WaveShaperNodeEngine. r=padenot
Reporter

Updated

4 years ago
Flags: needinfo?(yanisellami)
Thanks, Loic.  I think this may be an effect of bug 1190215.
Depends on: 1190215
Tracking for 42+. It is probably too late to take this in 41. 
Karl, how bad is this regression and how widespread are its effects?
Flags: needinfo?(karlt)
It's a bad bug, but things, including the url here, were bad before due to bug 1118372.  Basically WaveShaperNode only works in particular situations.

There are some situations where things may have worked before bug 1118372 was fixed, but don't now.  However, I think it works in more situations now than before, so I think backing out changes from bug 1118372 would not be an improvement.
Flags: needinfo?(karlt)
Priority: -- → P1
Too late to get a fix in 41. While this is a valid bug, I do not believe it is a release blocker for 41. It would be nice if we can get a fix soon, for 42.
No use tracking it for 41.
Maire, can you help find an owner for this bug? Thanks!
Flags: needinfo?(mreavy)
Assignee: nobody → padenot
Hi Liz, Paul is going to look at this and see how much worse we made things.  Depending on the fix and the risks/rewards, we'll see how far we want to uplift.
Rank: 15
Flags: needinfo?(mreavy)
You were right in comment 1, Karl, simply doing this fixes it. I'm using this
bug and duping the other one against it because this one is tracked by releng.

I'll be adding a test tomorrow.
Attachment #8670358 - Flags: review?(karlt)
This is an easy, not risky patch, that will have tests. We'll hopefully be able to uplift it.
Flags: needinfo?(yanisellami)
Duplicate of this bug: 1190215
Attachment #8670358 - Flags: review?(karlt)
Attachment #8670358 - Attachment is obsolete: true
Attachment #8670676 - Flags: review?(karlt) → review+
Comment on attachment 8670676 [details] [diff] [review]
Properly scale the input buffer before processing it with the curve. r=

>+      if (aInput.mVolume != 1.0) {

1.0f
Comment on attachment 8670688 [details] [diff] [review]
Test that waveshaper nodes don't corrupt their input buffer. r=

>+++ b/dom/media/webaudio/test/test_mediaElementAudioSourceNodeFidelity.html

>+++ b/dom/media/webaudio/test/webaudio.js

I guess changes to these files were accidentally included in this patch, but
please remove those as they are unrelated.  binIndexForFrequency() can be
moved if and when required.

The new test is fine, but some suggestions to make the test a bit
stronger below:

>+osc.connect(gain);
>+gain.connect(ws);
>+gain.connect(sp);

Whether this test detects the bug depends on the order of processing, which is
not defined, but may happen to depend on the order of connections here.

To be sure of detecting this failure, two waveshapers can be connected to the
same gain node and their outputs tested.  See test_analyserNodeWithGain.html, for example.

>+  if (max > 0.49) {
>+    ok(true, "Volume was handled properly.");
>+    sp.onaudioprocess = null;
>+    SimpleTest.finish();
>+  }

Something like ok(Math.abs(max - gain.gain.value) < 0.01, "[...]") would
tighten up the test a bit.

If the frequency of the oscillator were chosen appropriately for script
processor buffer size and sample rate, then this could be tested in the first
audioprocess event, without needing to wait until sufficiently large max is
detected.  An offline context is an option so that sample rate is
configurable, which may make the frequency choice easier.
Attachment #8670688 - Flags: review?(karlt) → review+
https://hg.mozilla.org/mozilla-central/rev/a61652a27e8c
https://hg.mozilla.org/mozilla-central/rev/240a3f4f4346
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Paul, could you fill the uplift request to aurora (and beta, if you care enough) :) ?
merci
Flags: needinfo?(padenot)
Comment on attachment 8670676 [details] [diff] [review]
Properly scale the input buffer before processing it with the curve. r=

Approval Request Comment
[Feature/regressing bug #]: bug 1118372
[User impact if declined]: Bad behavior of some part of the Web Audio API (audio distortion), break content out there
[Describe test coverage new/current, TreeHerder]: Test added, been on inbound/central for some time
[Risks and why]: no risk, this is very easy and has a test 
[String/UUID change made/needed]: none
Flags: needinfo?(padenot)
Attachment #8670676 - Flags: approval-mozilla-beta?
Attachment #8670676 - Flags: approval-mozilla-aurora?
Comment on attachment 8670688 [details] [diff] [review]
Test that waveshaper nodes don't corrupt their input buffer. r=

(See the comment above).
Attachment #8670688 - Flags: approval-mozilla-beta?
Attachment #8670688 - Flags: approval-mozilla-aurora?
Comment on attachment 8670676 [details] [diff] [review]
Properly scale the input buffer before processing it with the curve. r=

OK, thanks. Should be in 42 beta 6.
If there is any potential regressions, I will steal your new bike and use it as ransom until they are fixed! ;)
Attachment #8670676 - Flags: approval-mozilla-beta?
Attachment #8670676 - Flags: approval-mozilla-beta+
Attachment #8670676 - Flags: approval-mozilla-aurora?
Attachment #8670676 - Flags: approval-mozilla-aurora+
Attachment #8670688 - Flags: approval-mozilla-beta?
Attachment #8670688 - Flags: approval-mozilla-beta+
Attachment #8670688 - Flags: approval-mozilla-aurora?
Attachment #8670688 - Flags: approval-mozilla-aurora+
the second patch didn't apply cleanly to aurora:

grafting 307840:240a3f4f4346 "Bug 1203616 - Test that waveshaper nodes don't corrupt their input buffer. r=karlt"
merging dom/media/webaudio/test/mochitest.ini
warning: conflicts during merge.
merging dom/media/webaudio/test/mochitest.ini incomplete! (edit conflicts, then use 'hg resolve --mark')

paul, could you take a look, thanks!
Flags: needinfo?(padenot)
Attachment #8671940 - Attachment description: Test that waveshaper nodes don't corrupt their input buffer. r= → Test that waveshaper nodes don't corrupt their input buffer. (Aurora rebase)
Flags: needinfo?(padenot)
Carsten, here the rebased patch.
Flags: needinfo?(cbook)
Flags: needinfo?(cbook)
Reporter

Updated

4 years ago
Whiteboard: [mozfr-community]
You need to log in before you can comment on or make changes to this bug.