72871 2011-11-21 04:15:39 -0800 Incorrect calculation for DistanceEffect linearGain 2011-12-19 11:25:00 -0800 1 1 1 Unclassified WebKit Web Audio 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 0 davidgaleano rtoy crogers kbr rtoy webkit.review.bot oldest_to_newest 506863 0 davidgaleano 2011-11-21 04:15:39 -0800 I think the following code in "WebCore/platform/audio/Distance.cpp" is incorrect: double DistanceEffect::linearGain(double distance) 73 { 74 return (1.0 - m_rolloffFactor * (distance - m_refDistance)) / (m_maxDistance - m_refDistance); 75 } When the distance is equal or lower to the reference distance the output gain should be one but if you resolve the equation for that case: (1.0 - m_rolloffFactor * (m_refDistance - m_refDistance)) / (m_maxDistance - m_refDistance) (1.0 - m_rolloffFactor * 0) / (m_maxDistance - m_refDistance) (1.0 - 0) / (m_maxDistance - m_refDistance) 1.0 / (m_maxDistance - m_refDistance) Which is not one in a majority of cases. I think the parenthesis are wrong and the correct code should be: double DistanceEffect::linearGain(double distance) 73 { 74 return (1.0 - (m_rolloffFactor * (distance - m_refDistance) / (m_maxDistance - m_refDistance)); 75 } 507060 1 rtoy 2011-11-21 10:34:48 -0800 Yes, I agree. You can easily hear the effect using http://chromium.googlecode.com/svn/trunk/samples/audio/simple.html Bring up the javascript console and enter panner.distanceModel = 0 to get the linear gain model. Now you can't hear anything because the gain value even at the reference distance is approximately 1/10000 (because the maxDistance is 10000 and refDistance is 1). 509224 2 davidgaleano 2011-11-25 11:16:00 -0800 Do you think my suggested code is correct? do you guys want a patch or something? At the moment the linear model is a bit unusable because of this bug. 509954 3 rtoy 2011-11-28 10:54:28 -0800 (In reply to comment #2) > Do you think my suggested code is correct? do you guys want a patch or something? > At the moment the linear model is a bit unusable because of this bug. No, no patch is needed. Your suggested correction looks like the correct solution. I actually don't know exactly what linearGain is specified to do, but this looks like the most reasonable answer. 518973 4 crogers 2011-12-11 19:16:20 -0800 Hi David, sorry for jumping into this so late. I just returned from vacation... It looks like you're exactly right. Thanks for catching this! The distance models are supposed to be defined exactly as in OpenAL: http://connect.creativelabs.com/openal/Documentation/OpenAL%201.1%20Specification.pdf Ray, would you like to put in a patch for this? 519299 5 118813 rtoy 2011-12-12 10:14:41 -0800 Created attachment 118813 Patch 519343 6 rtoy 2011-12-12 10:52:47 -0800 The patch adds a reference to the OpenAL document. I didn't do that in this patch, but perhaps a reference to that document should also be included for the other distance formulas. And what about the webaudio spec itself? Should it just reference the OpenAL document or include equivalent text? 519381 7 118813 crogers 2011-12-12 12:38:31 -0800 Comment on attachment 118813 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118813&action=review > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) This line needs to be updated. Either add a test for this, or say that tests still must be written for all distance models. This does not add new API. (or something like that) > Source/WebCore/platform/audio/Distance.cpp:74 > + // See Open AL 1.1 Specification, section 3.4.4. If we include this comment, then we should include the URL and hoist it out of this specific function and put it somewhere else more common, like the .h or .idl > Source/WebCore/platform/audio/Distance.cpp:77 > + distance = std::min(std::max(distance, m_refDistance), m_maxDistance); This clamping is not specific to linearGain. Also, this logic is already in DistanceEffect::gain() which is taking into account m_isClamped so it should be removed from here 519457 8 118845 rtoy 2011-12-12 14:22:21 -0800 Created attachment 118845 Patch 519461 9 118813 rtoy 2011-12-12 14:24:16 -0800 Comment on attachment 118813 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118813&action=review >> Source/WebCore/ChangeLog:8 >> + No new tests. (OOPS!) > > This line needs to be updated. Either add a test for this, or say that tests still must be written for all distance models. This does not add new API. > > (or something like that) Updated. >> Source/WebCore/platform/audio/Distance.cpp:74 >> + // See Open AL 1.1 Specification, section 3.4.4. > > If we include this comment, then we should include the URL and hoist it out of this specific function and put it somewhere else more common, like the .h or .idl Moved to .h file. >> Source/WebCore/platform/audio/Distance.cpp:77 >> + distance = std::min(std::max(distance, m_refDistance), m_maxDistance); > > This clamping is not specific to linearGain. Also, this logic is already in DistanceEffect::gain() which is taking into account m_isClamped so it should be removed from here Removed. 519606 10 crogers 2011-12-12 15:40:00 -0800 Looks fine. 519701 11 118845 kbr 2011-12-12 17:02:58 -0800 Comment on attachment 118845 Patch OK. rs=me 521182 12 118845 webkit.review.bot 2011-12-14 12:33:23 -0800 Comment on attachment 118845 Patch Clearing flags on attachment: 118845 Committed r102810: <http://trac.webkit.org/changeset/102810> 521183 13 webkit.review.bot 2011-12-14 12:33:27 -0800 All reviewed patches have been landed. Closing bug. 524055 14 118813 eric 2011-12-19 11:25:00 -0800 Comment on attachment 118813 Patch Cleared review? from obsolete attachment 118813 so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again). 118813 2011-12-12 10:14:41 -0800 2011-12-19 11:25:00 -0800 Patch bug-72871-20111212101440.patch text/plain 1826 rtoy U3VidmVyc2lvbiBSZXZpc2lvbjogMTAyMzgwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggN2NmMmNiM2YzNDEzNGVm OWUxNzYyYjBhNWQzZjZiMTZmZTE1MTE5YS4uMjYwOWFkODhhMTllZWMyOTA4OWVjZjkwZTRlODZi M2ZlZjE2NzNjMCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDExLTEyLTEyICBSYXlt b25kIFRveSAgPHJ0b3lAZ29vZ2xlLmNvbT4KKworICAgICAgICBJbmNvcnJlY3QgY2FsY3VsYXRp b24gZm9yIERpc3RhbmNlRWZmZWN0IGxpbmVhckdhaW4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndl YmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTcyODcxCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzLiAoT09QUyEpCisKKyAgICAgICAg KiBwbGF0Zm9ybS9hdWRpby9EaXN0YW5jZS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpEaXN0YW5j ZUVmZmVjdDo6bGluZWFyR2Fpbik6IEltcGxlbWVudCBjb3JyZWN0CisgICAgICAgIGxpbmVhckdh aW4gZnVuY3Rpb24uICAoRml4IHByb3Bvc2VkIGJ5IGRhdmlkZ2FsZWFub0Bob3RtYWlsLmNvbS4p CisKIDIwMTEtMTItMDggIEVyaWsgQXJ2aWRzc29uICA8YXJ2QGNocm9taXVtLm9yZz4KIAogICAg ICAgICBDb2RlR2VuZXJhdG9yVjg6IEZpeCBpc3N1ZSB3aXRoIG92ZXJsb2FkZWQgc3RhdGljIGNv bmRpdGlvbmFsIG1ldGhvZHMKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2F1 ZGlvL0Rpc3RhbmNlLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2F1ZGlvL0Rpc3RhbmNl LmNwcAppbmRleCAwNGUyOGY5YjQ4ZTcxMGNmMzhkMGIwYjg1MDRmNDI0NmVlMWY5OGE4Li5mMjI0 N2Q3ODNhYjk2MzEwMzZkOWUwYzhiZTliM2IyZTliZmU2YjQ0IDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViQ29yZS9wbGF0Zm9ybS9hdWRpby9EaXN0YW5jZS5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUv cGxhdGZvcm0vYXVkaW8vRGlzdGFuY2UuY3BwCkBAIC03MSw3ICs3MSwxNCBAQCBkb3VibGUgRGlz dGFuY2VFZmZlY3Q6OmdhaW4oZG91YmxlIGRpc3RhbmNlKQogCiBkb3VibGUgRGlzdGFuY2VFZmZl Y3Q6OmxpbmVhckdhaW4oZG91YmxlIGRpc3RhbmNlKQogewotICAgIHJldHVybiAoMS4wIC0gbV9y b2xsb2ZmRmFjdG9yICogKGRpc3RhbmNlIC0gbV9yZWZEaXN0YW5jZSkpIC8gKG1fbWF4RGlzdGFu Y2UgLSBtX3JlZkRpc3RhbmNlKTsKKyAgICAvLyBTZWUgT3BlbiBBTCAxLjEgU3BlY2lmaWNhdGlv biwgc2VjdGlvbiAzLjQuNC4KKyAgICAKKyAgICAvLyBDbGFtcCBkaXN0YW5jZSB0byBiZSBiZXR3 ZWVuIHxtX3JlZkRpc3RhbmNlfCBhbmQgfG1fbWF4RGlzdGFuY2V8LgorICAgIGRpc3RhbmNlID0g c3RkOjptaW4oc3RkOjptYXgoZGlzdGFuY2UsIG1fcmVmRGlzdGFuY2UpLCBtX21heERpc3RhbmNl KTsKKworICAgIC8vIFdlIHdhbnQgYSBnYWluIHRoYXQgZGVjcmVhc2VzIGxpbmVhcmx5IGZyb20g bV9yZWZEaXN0YW5jZSB0bworICAgIC8vIG1fbWF4RGlzdGFuY2UuIFRoZSBnYWluIGlzIDEgYXQg bV9yZWZEaXN0YW5jZS4KKyAgICByZXR1cm4gKDEuMCAtIG1fcm9sbG9mZkZhY3RvciAqIChkaXN0 YW5jZSAtIG1fcmVmRGlzdGFuY2UpIC8gKG1fbWF4RGlzdGFuY2UgLSBtX3JlZkRpc3RhbmNlKSk7 CiB9CiAKIGRvdWJsZSBEaXN0YW5jZUVmZmVjdDo6aW52ZXJzZUdhaW4oZG91YmxlIGRpc3RhbmNl KQo= 118845 2011-12-12 14:22:21 -0800 2011-12-14 12:33:23 -0800 Patch bug-72871-20111212142220.patch text/plain 2461 rtoy U3VidmVyc2lvbiBSZXZpc2lvbjogMTAyMzgwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggN2NmMmNiM2YzNDEzNGVm OWUxNzYyYjBhNWQzZjZiMTZmZTE1MTE5YS4uYzc3MTVjOWMzNjA4N2RmYzkyMGYyZDFkY2FiZWNi MzM1ZDJhNmQ1YiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIxIEBACisyMDExLTEyLTEyICBSYXlt b25kIFRveSAgPHJ0b3lAZ29vZ2xlLmNvbT4KKworICAgICAgICAqIHBsYXRmb3JtL2F1ZGlvL0Rp c3RhbmNlLmggKFdlYkNvcmUpOiAKKworICAgICAgICBJbmNvcnJlY3QgY2FsY3VsYXRpb24gZm9y IERpc3RhbmNlRWZmZWN0IGxpbmVhckdhaW4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5v cmcvc2hvd19idWcuY2dpP2lkPTcyODcxCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgVGVzdHMgc3RpbGwgbmVlZCB0byBiZSB3cml0dGVuIGZvciBhbGwg ZGlzdGFuY2UgbW9kZWxzLiAgVGhpcworICAgICAgICBkb2VzIG5vdCBhZGQgYSBuZXcgQVBJIGFu ZCBqdXN0IGNvcnJlY3RzIGFuIGltcGxlbWVudGF0aW9uIGVycm9yLiAKKworICAgICAgICAqIHBs YXRmb3JtL2F1ZGlvL0Rpc3RhbmNlLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkRpc3RhbmNlRWZm ZWN0OjpsaW5lYXJHYWluKTogSW1wbGVtZW50IGNvcnJlY3QKKyAgICAgICAgbGluZWFyR2FpbiBm dW5jdGlvbi4gIChGaXggcHJvcG9zZWQgYnkgZGF2aWRnYWxlYW5vQGhvdG1haWwuY29tLikKKyAg ICAgICAgKiBwbGF0Zm9ybS9hdWRpby9EaXN0YW5jZS5oOiBBZGQgbGluayB0byBPcGVuIEFMIHNw ZWNpZmljYXRpb24uIAorICAgICAgICAKKwogMjAxMS0xMi0wOCAgRXJpayBBcnZpZHNzb24gIDxh cnZAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIENvZGVHZW5lcmF0b3JWODogRml4IGlzc3VlIHdp dGggb3ZlcmxvYWRlZCBzdGF0aWMgY29uZGl0aW9uYWwgbWV0aG9kcwpkaWZmIC0tZ2l0IGEvU291 cmNlL1dlYkNvcmUvcGxhdGZvcm0vYXVkaW8vRGlzdGFuY2UuY3BwIGIvU291cmNlL1dlYkNvcmUv cGxhdGZvcm0vYXVkaW8vRGlzdGFuY2UuY3BwCmluZGV4IDA0ZTI4ZjliNDhlNzEwY2YzOGQwYjBi ODUwNGY0MjQ2ZWUxZjk4YTguLmYzZDk4NzMyZjk1YmExMWFhNTEyNWY3Zjc3MDM3YWZkMjYyMWM2 MmEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2F1ZGlvL0Rpc3RhbmNlLmNw cAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9hdWRpby9EaXN0YW5jZS5jcHAKQEAgLTcx LDcgKzcxLDkgQEAgZG91YmxlIERpc3RhbmNlRWZmZWN0OjpnYWluKGRvdWJsZSBkaXN0YW5jZSkK IAogZG91YmxlIERpc3RhbmNlRWZmZWN0OjpsaW5lYXJHYWluKGRvdWJsZSBkaXN0YW5jZSkKIHsK LSAgICByZXR1cm4gKDEuMCAtIG1fcm9sbG9mZkZhY3RvciAqIChkaXN0YW5jZSAtIG1fcmVmRGlz dGFuY2UpKSAvIChtX21heERpc3RhbmNlIC0gbV9yZWZEaXN0YW5jZSk7CisgICAgLy8gV2Ugd2Fu dCBhIGdhaW4gdGhhdCBkZWNyZWFzZXMgbGluZWFybHkgZnJvbSBtX3JlZkRpc3RhbmNlIHRvCisg ICAgLy8gbV9tYXhEaXN0YW5jZS4gVGhlIGdhaW4gaXMgMSBhdCBtX3JlZkRpc3RhbmNlLgorICAg IHJldHVybiAoMS4wIC0gbV9yb2xsb2ZmRmFjdG9yICogKGRpc3RhbmNlIC0gbV9yZWZEaXN0YW5j ZSkgLyAobV9tYXhEaXN0YW5jZSAtIG1fcmVmRGlzdGFuY2UpKTsKIH0KIAogZG91YmxlIERpc3Rh bmNlRWZmZWN0OjppbnZlcnNlR2Fpbihkb3VibGUgZGlzdGFuY2UpCmRpZmYgLS1naXQgYS9Tb3Vy Y2UvV2ViQ29yZS9wbGF0Zm9ybS9hdWRpby9EaXN0YW5jZS5oIGIvU291cmNlL1dlYkNvcmUvcGxh dGZvcm0vYXVkaW8vRGlzdGFuY2UuaAppbmRleCBjN2VkZGVkYjU5MDRmODU5YmEyOGE1YzlmN2E3 NWU3Y2RiMTFhN2YyLi5jNWI2Y2M5NDljY2E3ZjJlOTNlOTJiYTU0ZDQ4MTQxY2VjZDQ2MWY1IDEw MDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9hdWRpby9EaXN0YW5jZS5oCisrKyBi L1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2F1ZGlvL0Rpc3RhbmNlLmgKQEAgLTMxLDcgKzMxLDgg QEAKIAogbmFtZXNwYWNlIFdlYkNvcmUgewogCi0vLyBEaXN0YW5jZSBtb2RlbHMgYXJlIGRlZmlu ZWQgYWNjb3JkaW5nIHRvIHRoZSBPcGVuQUwgc3BlY2lmaWNhdGlvbgorLy8gRGlzdGFuY2UgbW9k ZWxzIGFyZSBkZWZpbmVkIGFjY29yZGluZyB0byB0aGUgT3BlbkFMIHNwZWNpZmljYXRpb246Cisv LyBodHRwOi8vY29ubmVjdC5jcmVhdGl2ZWxhYnMuY29tL29wZW5hbC9Eb2N1bWVudGF0aW9uL09w ZW5BTCUyMDEuMSUyMFNwZWNpZmljYXRpb24uaHRtLgogCiBjbGFzcyBEaXN0YW5jZUVmZmVjdCB7 CiBwdWJsaWM6Cg==