Summary: | PannerNode's rolloffFactor should be clamped to [0, 1] internally when distanceModel is "linear" | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||
Component: | Web Audio | Assignee: | Chris Dumez <cdumez> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | clark_wang, darin, eric.carlson, ews-watchlist, ggaren, glenn, jer.noble, philipj, sergio, webkit-bug-importer, youennf | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
URL: | https://www.w3.org/TR/webaudio/#dom-pannernode-rollofffactor | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 212611 | ||||||||
Attachments: |
|
Description
Chris Dumez
2020-08-18 14:19:36 PDT
Created attachment 406814 [details]
Patch
Comment on attachment 406814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406814&action=review > Source/WebCore/platform/audio/Distance.cpp:72 > + auto clampedRolloffFactor = std::min(m_rolloffFactor, 1.0); This says clamped but it only does the 1.0 half of the clamping. I’d expect to see std::max or std::clamp here. Comment on attachment 406814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406814&action=review >> Source/WebCore/platform/audio/Distance.cpp:72 >> + auto clampedRolloffFactor = std::min(m_rolloffFactor, 1.0); > > This says clamped but it only does the 1.0 half of the clamping. I’d expect to see std::max or std::clamp here. to handle the 0.0 minimum. (In reply to Darin Adler from comment #3) > Comment on attachment 406814 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=406814&action=review > > >> Source/WebCore/platform/audio/Distance.cpp:72 > >> + auto clampedRolloffFactor = std::min(m_rolloffFactor, 1.0); > > > > This says clamped but it only does the 1.0 half of the clamping. I’d expect to see std::max or std::clamp here. > > to handle the 0.0 minimum. We already throw an exception in the setter if the JS tries to set a negative value. Therefore, we only need to make sure the value is no more than 1. (In reply to Chris Dumez from comment #4) > (In reply to Darin Adler from comment #3) > > Comment on attachment 406814 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=406814&action=review > > > > >> Source/WebCore/platform/audio/Distance.cpp:72 > > >> + auto clampedRolloffFactor = std::min(m_rolloffFactor, 1.0); > > > > > > This says clamped but it only does the 1.0 half of the clamping. I’d expect to see std::max or std::clamp here. > > > > to handle the 0.0 minimum. > > We already throw an exception in the setter if the JS tries to set a > negative value. Therefore, we only need to make sure the value is no more > than 1. From https://www.w3.org/TR/webaudio/#dom-pannernode-rollofffactor: "A RangeError exception MUST be thrown if this is set to a negative value." Comment on attachment 406814 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=406814&action=review > Source/WebCore/Modules/webaudio/PannerNode.cpp:282 > return Exception { RangeError, "rolloffFactor cannot be set to a negative value"_s }; Throwing happens here if value is negative. Created attachment 406819 [details]
Patch
Committed r265838: <https://trac.webkit.org/changeset/265838> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406819 [details]. |