1267579 - Unexpected result when using OscillatorNode with custom wave shape

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P2)

Description

[sebpiq]

Attachment: bugreport-ff.html

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160325004402

Steps to reproduce:

Try to get a Cos wave of frequency 0 with web audio OscillatorNode, in order to have a node outputting only ones. See attachment for a test case.


Actual results:

The OscillatorNode outputs only zeros.


Expected results:

The OscillatorNode should have output ones.

Comment 1

[Loic]

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=74166e
0168385eba427642965b93c7ad010662ca&tochange=3785489fa210d38b890ab73f21202b94c552
8c1f

Regressed by Bug 1222405.

Dan, your thoughts?

Comment 2

[Dan Minor [:dminor]]

Having a look now.

Comment 3

[Dan Minor [:dminor]]

Thanks for your report!

Comment 4

[Dan Minor [:dminor]]

Attachment: MozReview Request: Bug 1267579 - Unexpected result when using OscillatorNode with custom wave shape; r?padenot

This fixes some division by zero errors introduced by Bug 1222405 when the
fundamental frequency is zero.

Review commit: https://reviewboard.mozilla.org/r/48987/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48987/

Comment 5

[Dan Minor [:dminor]]

Attachment: MozReview Request: Bug 1267579 - Add test case; r?padenot

Review commit: https://reviewboard.mozilla.org/r/48989/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48989/

Comment 6

[Paul Adenot (:padenot)]

Comment on attachment 8745347 [details]
MozReview Request: Bug 1267579 - Unexpected result when using OscillatorNode with custom wave shape; r?padenot

https://reviewboard.mozilla.org/r/48987/#review45819

Comment 7

[Paul Adenot (:padenot)]

Comment on attachment 8745348 [details]
MozReview Request: Bug 1267579 - Add test case; r?padenot

https://reviewboard.mozilla.org/r/48989/#review45821

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b165d4c2ee46
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6e1c4c2b2c4

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b165d4c2ee46
https://hg.mozilla.org/mozilla-central/rev/a6e1c4c2b2c4

Comment 10

[Maire Reavy [:mreavy]]

Dan, Paul -- Does it make sense to uplift this?  If so, how far?  I'm marking Fx46 as wontfix, but Fx47 is still very early in the cycle -- so it would get basically the entire Beta cycle to bake there if we ask for uplift this week.  Let me know your thoughts.  Thanks!

Comment 11

[Dan Minor [:dminor]]

This is an edge case that only occurs when the fundamental frequency is set to zero, which I don't think is likely to affect that many people. On the other hand, it's also low risk. I'll defer to Paul :)

Comment 12

[Paul Adenot (:padenot)]

It's still fixing a division by zero.

Also, admittedly, to have a DC offset, it's more efficient to have an AudioBufferSourceNode with a buffer with a length of 1, with the only element set to 1.

Very low-risk indeed, maybe it's worth it because it's fixing a correctness issue.

Comment 13

[Dan Minor [:dminor]]

Comment on attachment 8745347 [details]
MozReview Request: Bug 1267579 - Unexpected result when using OscillatorNode with custom wave shape; r?padenot

Given this is low risk, I think it makes sense to ask for uplift to 48.

Approval Request Comment
[Feature/regressing bug #]: Bug 1267579
[User impact if declined]: Minimal, as Paul mentions there is a more efficient way of generating the desired signal.
[Describe test coverage new/current, TreeHerder]: A new test case has been written.
[Risks and why]: Very low risk - this only affects cases where the requested fundamental frequency is zero, which isn't a common use case.
[String/UUID change made/needed]: None

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8745347 [details]
MozReview Request: Bug 1267579 - Unexpected result when using OscillatorNode with custom wave shape; r?padenot

Recent regression, Beta47+, Aurora48+

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello sebpiq, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f60b4b71a319
https://hg.mozilla.org/releases/mozilla-aurora/rev/db6cfb052942

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/98501c412edc
https://hg.mozilla.org/releases/mozilla-beta/rev/22dbad80fbab

Comment 18

[sebpiq]

Yep works in latest nightly (49.0a1). Thanks for fixing this!