Assignee: nobody → padenot
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef7d3c854571 This disable the test for now.
Created attachment 8546740 [details] 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).
Created 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 just interpolate between the first and second value of the wave table, instead of interpolating backward.
Attachment #8547529 - Flags: review?(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).
Attachment #8547529 - Flags: review?(karlt) → review+
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
You need to log in before you can comment on or make changes to this bug.