924286 - Handle AudioChunk::mVolume (from GainNode) in BiquadFilterNodeEngine

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: handle non-unit mVolume

Comment 2

[Karl Tomlinson (:karlt)]

Will include testcase in bug 924288.

Comment 3

[(no longer active)]

Comment on attachment 814651 [details] [diff] [review]
handle non-unit mVolume

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

Sorry that I forgot about this.  I think I forgot to handle mVolume in about every single engine that I implemented.  And I did audit them all at some point but perhaps it was before BiquadFilterNode.

Comment 4

[Karl Tomlinson (:karlt)]

:) I'm bit surprised it took us so long to notice.  I've checked all the nodes that use the blink subdir, and Bug 924290 is the only remaining.

Comment 5

[Karl Tomlinson (:karlt)]

Er, I mean bug 924289.

Comment 6

[(no longer active)]

(In reply to comment #5)
> Er, I mean bug 924289.

\o/

Comment 7

[Karl Tomlinson (:karlt)]

Oh, sorry.  Good news:  blink/DynamicsCompressor.cpp already handles that (since bug 864613).

Comment 8

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d53c32505943

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d53c32505943

Comment 10

[Karl Tomlinson (:karlt)]

Test landed in bug 924288
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8fe38e7b4f7

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 814651 [details] [diff] [review]
handle non-unit mVolume

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Web Audio BiquadFilterNode bug 865246
User impact if declined: 
There are some situations (single non-null input BiquadFilterNode) where an author's gain control will be completely ineffective.  For example, attempts to silence a stream will fail.

Testing completed (on m-c, etc.): on m-c, aurora; in testsuite
Risk to taking this patch (and alternatives if risky): 
small patch, small influence, low risk
String or IDL/UUID changes made by this patch: none

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bb69444071a9

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/bb69444071a9