Investigate test_oscillatorNodeNegativeFrequency.html failures on Android

RESOLVED FIXED

Status

()

RESOLVED FIXED
4 years ago
8 months ago

People

(Reporter: padenot, Assigned: padenot)

Tracking

32 Branch
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Comment hidden (empty)
(Assignee)

Updated

4 years ago
Assignee: nobody → padenot
Keywords: leave-open
(Assignee)

Comment 3

4 years ago
Created attachment 8546739 [details]
log-osc-arm.txt
(Assignee)

Comment 4

4 years ago
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).
(Assignee)

Comment 5

4 years ago
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+
(Assignee)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.