Bug 106001 – Floating point precision error in AudioPannerNode

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Praveen Jadhav]

Please refer the sample code here below:


      test(function () {
            var context = window.AudioContext;
            var pannerNode = context.createPanner();
            pannerNode.maxDistance = 100.55;
            assert_true(pannerNode.maxDistance === 100.55, "AudioPannerNode.maxDistance is float value" + pannerNode.maxDistance);
        } );

Expected Result: Assert should return true.
Current Result: Assert is returning false. Because pannerNode.maxDistance is getting value as 100.55000305175781.
Analysis: While setMaxDistance() called, value is set as 100.55000305175781. The conversion is like double->float->double causing precision error.
Verified version: Webkitr-137862.

[Soo-Hyun Choi]

(In reply to comment #0)
> Please refer the sample code here below:
> 
> 
>       test(function () {
>             var context = window.AudioContext;
>             var pannerNode = context.createPanner();
>             pannerNode.maxDistance = 100.55;
>             assert_true(pannerNode.maxDistance === 100.55, "AudioPannerNode.maxDistance is float value" + pannerNode.maxDistance);
>         } );
> 
> Expected Result: Assert should return true.
> Current Result: Assert is returning false. Because pannerNode.maxDistance is getting value as 100.55000305175781.
> Analysis: While setMaxDistance() called, value is set as 100.55000305175781. The conversion is like double->float->double causing precision error.
> Verified version: Webkitr-137862.

This sounds like there is a bug in "Source/WebCore/platform/audio/Distance.h" whereby "setMaxDistance()" is trying to type-cast maxDistance variable into double (from what it is defined as float).

Any update?

[Praveen Jadhav]

Hi,

I have modified the files PannerNode.idl and PannerNode.h to have only double attribute types. This avoids float to double conversion in the code and hence avoids the precision error bug which happens because of the limitation.

Is it ok to update this patch?

Regards
Praveen

[Soo-Hyun Choi]

(In reply to comment #2)
> Hi,
> 
> I have modified the files PannerNode.idl and PannerNode.h to have only double attribute types. This avoids float to double conversion in the code and hence avoids the precision error bug which happens because of the limitation.
> 
> Is it ok to update this patch?
> 
> Regards
> Praveen

That perhaps needs to modify spec standard, but let us see your path first.

[Chris Dumez]

(In reply to comment #2)
> Hi,
> 
> I have modified the files PannerNode.idl and PannerNode.h to have only double attribute types. This avoids float to double conversion in the code and hence avoids the precision error bug which happens because of the limitation.
> 
> Is it ok to update this patch?
> 
> Regards
> Praveen

You probably shouldn't change PannerNode.idl as it needs to match the spec. The spec says to use float, not double. However, you can likely update DistanceEffect in Distance.h to use float instead of double.
DistanceEffect is used only by PannerNode so changing the type there should not be an issue.

[Praveen Jadhav]

Created attachment 184655 [details]
Avoiding precision error by making attributes of double datatype

[Praveen Jadhav]

(In reply to comment #4)
> (In reply to comment #2)
> > Hi,
> > 
> > I have modified the files PannerNode.idl and PannerNode.h to have only double attribute types. This avoids float to double conversion in the code and hence avoids the precision error bug which happens because of the limitation.
> > 
> > Is it ok to update this patch?
> > 
> > Regards
> > Praveen
> 
> You probably shouldn't change PannerNode.idl as it needs to match the spec. The spec says to use float, not double. However, you can likely update DistanceEffect in Distance.h to use float instead of double.
> DistanceEffect is used only by PannerNode so changing the type there should not be an issue.

Changing DistanceEffect in Distance.h to use float will not fix the actual bug reported here. HTML file will pass double data type and at the time of comparision, it will again convert it to double datatype. Thus the double->float->double conversion will be there.

I have attached my code modifications which resolves the problem. This avoids datatype conversion from double to float and back to double. Please check.

[Praveen Jadhav]

(In reply to comment #5)
> Created an attachment (id=184655) [details]
> Avoiding precision error by making attributes of double datatype

The attached patch is not proper. I will upload it again shortly.

[Praveen Jadhav]

Created attachment 184964 [details]
Updated patch attached

[Chris Dumez]

Comment on attachment 184964 [details]
Updated patch attached

View in context: https://bugs.webkit.org/attachment.cgi?id=184964&action=review

> Source/WebCore/ChangeLog:6
> +        Reviewed by Soo-Hyun Choi.

To my knowledge Soo-Hyun Choi is not a WebKit reviewer. You should leave it as it is when generated (by NOBODY...).

> Source/WebCore/ChangeLog:13
> +        No tests. No change in behaviour.

Actually, there is a change in behavior. You need to add a LayoutTest IMO.

[Praveen Jadhav]

Created attachment 185151 [details]
Patch updated based on previous comments.

[Praveen Jadhav]

Created attachment 185152 [details]
Patch updated based on previous comments.

[Chris Dumez]

If the patch is ready for review, do not forget to set review flag (r?).

[Chris Dumez]

Comment on attachment 185152 [details]
Patch updated based on previous comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=185152&action=review

> LayoutTests/ChangeLog:7
> +

Missing Changelog explanation.

[Praveen Jadhav]

Created attachment 185165 [details]
Brief explaination included in ChangeLog for LayoutTests

[Chris Dumez]

For clarity, it would be nice to mark your previous patches as obsolete.

[Kentaro Hara]

Comment on attachment 185165 [details]
Brief explaination included in ChangeLog for LayoutTests

View in context: https://bugs.webkit.org/attachment.cgi?id=185165&action=review

> Source/WebCore/Modules/webaudio/PannerNode.idl:52
> -    attribute float refDistance;
> -    attribute float maxDistance;
> -    attribute float rolloffFactor;
> +    attribute double refDistance;
> +    attribute double maxDistance;
> +    attribute double rolloffFactor;

It looks like the spec requires float: https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#PannerNode

What's the rationale for the change?

> LayoutTests/webaudio/pannernode-basic.html:48
> +    panner.refDistance = 270.52
> +    if (panner.refDistance === 270.52)
> +        testPassed("refDistance value is set to 270.52.");
> +    else
> +        testFailed("refDistance value should be set to 270.52.");
> +    

By using fast/js/resources/js-test-pre.js, you can write the test like:

  shouldBe("panner.refDistance", "270.52");

You can clean up these tests in a follow-up patch if you want.

[Praveen Jadhav]

(In reply to comment #16)
> (From update of attachment 185165 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185165&action=review
> 
> > Source/WebCore/Modules/webaudio/PannerNode.idl:52
> > -    attribute float refDistance;
> > -    attribute float maxDistance;
> > -    attribute float rolloffFactor;
> > +    attribute double refDistance;
> > +    attribute double maxDistance;
> > +    attribute double rolloffFactor;
> 
> It looks like the spec requires float: https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#PannerNode
> 
> What's the rationale for the change?
> 
> > LayoutTests/webaudio/pannernode-basic.html:48
> > +    panner.refDistance = 270.52
> > +    if (panner.refDistance === 270.52)
> > +        testPassed("refDistance value is set to 270.52.");
> > +    else
> > +        testFailed("refDistance value should be set to 270.52.");
> > +    
> 
> By using fast/js/resources/js-test-pre.js, you can write the test like:
> 
>   shouldBe("panner.refDistance", "270.52");
> 
> You can clean up these tests in a follow-up patch if you want.


The code changes indeed deviate from the specifications. However, the inherent precision error issue in the current platforms can be avoided if we use double instead of float. You may refer to the example I have mentioned while raising this bug.

About the LayoutTest scenarios, the changes are consistent with other test cases in pannernode-basic.html. If you insist on using shouldBe(), I will make the necessary changes and upload again. Please let me know.

[Kentaro Hara]

Comment on attachment 185165 [details]
Brief explaination included in ChangeLog for LayoutTests

View in context: https://bugs.webkit.org/attachment.cgi?id=185165&action=review

>>> Source/WebCore/Modules/webaudio/PannerNode.idl:52
>>> +    attribute double rolloffFactor;
>> 
>> It looks like the spec requires float: https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#PannerNode
>> 
>> What's the rationale for the change?
> 
> The code changes indeed deviate from the specifications. However, the inherent precision error issue in the current platforms can be avoided if we use double instead of float. You may refer to the example I have mentioned while raising this bug.
> 
> About the LayoutTest scenarios, the changes are consistent with other test cases in pannernode-basic.html. If you insist on using shouldBe(), I will make the necessary changes and upload again. Please let me know.

I'd like to delegate the decision to WebAudio experts. (I'm not sure it's OK to change the IDL.)

shouldBe() is just a nit. You can do it in a follow-up patch.

[Kenneth Russell]

Comment on attachment 185165 [details]
Brief explaination included in ChangeLog for LayoutTests

Sorry, but I think that making the IDL deviate from the Web Audio spec is the wrong way to fix this. I think the right way to fix it is to introduce an epsilon in the comparisons when retrieving these values.

[Praveen Jadhav]

Thank you all for your time. We will make the necessary changes in html file itself. This bug shall be INVALID.

[Chris Rogers]

Praveen, actually I think we *do* want to change the web audio spec to use doubles here, so maybe we don't need to give up on the patch.

We have an open W3C audio spec bug to switch as many APIs to use doubles.

[Chris Rogers]

spec changed:
https://dvcs.w3.org/hg/audio/rev/69a39a516e45

[Kentaro Hara]

The implementation of the patch looks good to me. Please add the spec link to ChangeLog.

[Praveen Jadhav]

Created attachment 186529 [details]
Updated spec URL included in the patch

I have updated the ChangeLog as requested. However, not all modifications done in webaudio spec are reflected in the source code. Further updates may be done after thorough testing.

[Kentaro Hara]

Comment on attachment 186529 [details]
Updated spec URL included in the patch

Per the discussion and the spec, it looks reasonable to make this change.

[Chris Dumez]

Comment on attachment 186529 [details]
Updated spec URL included in the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186529&action=review

> Source/WebCore/ChangeLog:6
> +        Reviewed by Chris Rogers.

It seems unusual to have 2 "reviewed by" lines.

> LayoutTests/ChangeLog:6
> +        Reviewed by Chris Rogers.

Ditto.

[Praveen Jadhav]

Created attachment 186561 [details]
Patch

[WebKit Review Bot]

Comment on attachment 186561 [details]
Patch

Rejecting attachment 186561 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[WebKit Review Bot]

Comment on attachment 186561 [details]
Patch

Clearing flags on attachment: 186561

Committed r141867: <http://trac.webkit.org/changeset/141867>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.