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.
(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?
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
(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.
(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.
Created attachment 184655 [details] Avoiding precision error by making attributes of double datatype
(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.
(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.
Created attachment 184964 [details] Updated patch attached
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.
Created attachment 185151 [details] Patch updated based on previous comments.
Created attachment 185152 [details] Patch updated based on previous comments.
If the patch is ready for review, do not forget to set review flag (r?).
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.
Created attachment 185165 [details] Brief explaination included in ChangeLog for LayoutTests
For clarity, it would be nice to mark your previous patches as obsolete.
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.
(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.
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.
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.
Thank you all for your time. We will make the necessary changes in html file itself. This bug shall be INVALID.
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.
spec changed: https://dvcs.w3.org/hg/audio/rev/69a39a516e45
The implementation of the patch looks good to me. Please add the spec link to ChangeLog.
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.
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.
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.
Created attachment 186561 [details] Patch
Comment on attachment 186561 [details] Patch Rejecting attachment 186561 [details] from review queue. praveen.j@samsung.com 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.
Comment on attachment 186561 [details] Patch Clearing flags on attachment: 186561 Committed r141867: <http://trac.webkit.org/changeset/141867>
All reviewed patches have been landed. Closing bug.