Bug 215625 – PannerNode's rolloffFactor should be clamped to [0, 1] internally when distanceModel is "linear"

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

[Chris Dumez]

PannerNode.rolloffFactor setter should clamp value to [0, 1] when distanceModel is "linear":
- https://www.w3.org/TR/webaudio/#dom-pannernode-rollofffactor

[Chris Dumez]

Created attachment 406814 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

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.

[Chris Dumez]

(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.

[Chris Dumez]

(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."

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 406819 [details]
Patch

[EWS]

Committed r265838: <https://trac.webkit.org/changeset/265838>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406819 [details].

[Radar WebKit Bug Importer]

<rdar://problem/67360120>