1119266 - Investigate test_oscillatorNodeNegativeFrequency.html failures on Android

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Comment 1

[Paul Adenot (:padenot)]

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

This disable the test for now.

Comment 2

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

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

Comment 3

[Paul Adenot (:padenot)]

Attachment: log-osc-arm.txt

Comment 4

[Paul Adenot (:padenot)]

Attachment: log-osc-x86.txt

Those two file have been produced by:

+      printf("j1: %u, j2: %u, sampleInterpolationFactor: %f, basePhaseIncrement: %f, mFinalFrequency: %f\n", j1, j2, sampleInterpolationFactor, basePhaseIncrement, mFinalFrequency);

after the mPhase update in ComputeCustom.

(and __android_log_print for Android of course).

Comment 5

[Paul Adenot (:padenot)]

Attachment: Properly wrap the table index in ComputeCustom on ARM. r=

On ARM, casting a negative float to an unsigned integer yields zero, whereas on
x86, it wraps. When the OscillatorNode had a negative frequency, the code would
just interpolate between the first and second value of the wave table, instead
of interpolating backward.

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 8547529 [details] [diff] [review]
Properly wrap the table index in ComputeCustom on ARM. r=

>On ARM, casting a negative float to an unsigned integer yields zero, whereas on
>x86, it wraps. When the OscillatorNode had a negative frequency, the code would

Thanks for the nice analysis.  Conversion from float to unsigned (or signed) integer has undefined behavior when outside the range of the destination type.

>+      j1Signed += periodicWaveSize;

Can you remove this line please?

There is nothing to ensure that j1Signed is >= -periodicWaveSize, so I'd
prefer not to have this line implying that it is.

The line is unnecessary because periodicWaveSize is a power of 2 and here

>+      uint32_t j1 = j1Signed & indexMask;

the implicit promotion from j1Signed to uint32_t (when int does not have more
bits than uint32_t - perhaps either indexMask should be unsigned int or an
explicit conversion to uint32_t should be applied, to avoid 2s complement
assumptions if int ever has more bits than uint32_t) for the bitwise & is
defined to follow the expected modulo behavior.  Adding a power of 2 that is
greater than indexMask will have no effect on the result (unless signed
overflow introduces technically undefined behavior).

Comment 7

[Paul Adenot (:padenot)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=208e4ff27ff9

Comment 8

[Paul Adenot (:padenot)]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.