Bug 213992 – Set Restrictions for channelCount, channelCountMode for PannerNode

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

[Clark Wang]

Add setter methods to PannerNode that will handle exceptions for setting channelCount, channelCountMode. Phased out sampleRate from AudioNode, using BaseAudioContext's sampleRate instead for processIfNecessary method instead.

[Clark Wang]

Also separated ChannelCountMode, ChannelCount enums into their own files

[Clark Wang]

Created attachment 403800 [details]
Patch

[Chris Dumez]

Comment on attachment 403800 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Added setter methods to PannerNode that handle exceptions for channelCount, channelCountMode, according to spec: https://www.w3.org/TR/webaudio/#dom-audionode-channelcount.

Please wrap lines properly so they are not too long like this.

> Source/WebCore/ChangeLog:9
> +        Phased out sampleRate from AudioNode, using BaseAudioContext's sampleRate for processIfNecessary method. Moved ChannelCount, ChannelCountMode enums into their own files.

Ditto.

> Source/WebCore/Modules/webaudio/AudioNode.cpp:295
> +ChannelCountMode AudioNode::channelCountMode()

This could be inlined in the header now.

> Source/WebCore/Modules/webaudio/AudioNode.cpp:300
> +ExceptionOr<void> AudioNode::setChannelCountMode(const ChannelCountMode mode)

const is not needed.

> Source/WebCore/Modules/webaudio/AudioNode.cpp:309
> +    if (mode == Max)

The whole implementation of this method makes no sense anymore. Should be something like:
m_channelCountMode = mode;

> Source/WebCore/Modules/webaudio/AudioNode.cpp:324
> +ChannelInterpretation AudioNode::channelInterpretation()

This could be inlined in the header now.

> Source/WebCore/Modules/webaudio/AudioNode.cpp:336
> +    if (interpretation == ChannelInterpretation::Speakers)

The implementation of this method does not make sense anymore.

> Source/WebCore/Modules/webaudio/AudioNode.cpp:381
> +            m_lastNonSilentTime = (context().currentSampleFrame() + framesToProcess) / static_cast<double>(context().sampleRate());

Nothing to do with the enum string. Please split unrelated changes in separate patches.

> Source/WebCore/Modules/webaudio/AudioNode.h:180
> +    virtual ExceptionOr<void> setChannelCountMode(const ChannelCountMode);

No need for const.

> Source/WebCore/Modules/webaudio/AudioNode.h:182
> +    ChannelInterpretation channelInterpretation();

should be const.

> Source/WebCore/Modules/webaudio/AudioNode.h:185
>      ChannelCountMode internalChannelCountMode() const { return m_channelCountMode; }

This method seems identical to channelCountMode() now. It should be dropped.

> Source/WebCore/Modules/webaudio/AudioNode.h:186
> +    ChannelInterpretation internalChannelInterpretation() const { return m_channelInterpretation; }

ditto.

> Source/WebCore/Modules/webaudio/AudioNodeInput.cpp:148
> +    ChannelCountMode mode = node()->internalChannelCountMode();

auto mode

> Source/WebCore/Modules/webaudio/AudioNodeInput.cpp:161
> +    if (mode == ClampedMax)

Please make that enum an enum class.

> Source/WebCore/Modules/webaudio/AudioNodeInput.cpp:199
> +    ChannelInterpretation interpretation = node()->internalChannelInterpretation();

auto

> Source/WebCore/Modules/webaudio/ChannelCountMode.h:30
> +enum ChannelCountMode {

Should be an enum class.

> Source/WebCore/Modules/webaudio/PannerNode.cpp:342
> +        return Exception { NotSupportedError };

Please provide an explanation string...

> Source/WebCore/Modules/webaudio/PannerNode.cpp:344
> +    auto result = this->AudioNode::setChannelCount(channelCount);

this-> is not needed.

Also, seems like this could be return as:
return AudioNode::setChannelCount(channelCount);

> Source/WebCore/Modules/webaudio/PannerNode.cpp:352
> +ExceptionOr<void> PannerNode::setChannelCountMode(const ChannelCountMode mode)

No const.

> Source/WebCore/Modules/webaudio/PannerNode.cpp:357
> +    auto result = this->AudioNode::setChannelCountMode(mode);

Same comments as above.

> Source/WebCore/Modules/webaudio/PannerNode.h:131
> +    ExceptionOr<void> setChannelCount(unsigned) override;

s/override/final

> Source/WebCore/Modules/webaudio/PannerNode.h:132
> +    ExceptionOr<void> setChannelCountMode(const ChannelCountMode) override;

s/override/final

[Clark Wang]

Created attachment 403806 [details]
Patch

[Clark Wang]

Created attachment 403807 [details]
Patch

[Clark Wang]

Created attachment 403808 [details]
Patch

[Chris Dumez]

Comment on attachment 403808 [details]
Patch

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

Patch does not build in debug.

> Source/WebCore/Modules/webaudio/AudioNode.cpp:299
> +    if (mode != ChannelCountMode::Max && mode != ChannelCountMode::ClampedMax && mode != ChannelCountMode::Explicit)

Why not we need this check? These seem to be the only values of ChannelCountMode.

> Source/WebCore/Modules/webaudio/AudioNode.cpp:317
> +    if (interpretation != ChannelInterpretation::Speakers && interpretation != ChannelInterpretation::Discrete)

Ditto.

> Source/WebCore/Modules/webaudio/AudioNode.h:176
> +    unsigned channelCount() { return m_channelCount; }

const

> Source/WebCore/Modules/webaudio/AudioNode.h:179
> +    ChannelCountMode channelCountMode() { return m_channelCountMode; }

const

[Chris Dumez]

Comment on attachment 403808 [details]
Patch

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

>> Source/WebCore/Modules/webaudio/AudioNode.cpp:299
>> +    if (mode != ChannelCountMode::Max && mode != ChannelCountMode::ClampedMax && mode != ChannelCountMode::Explicit)
> 
> Why not we need this check? These seem to be the only values of ChannelCountMode.

Why *do* we need this check?

[Chris Dumez]

Comment on attachment 403808 [details]
Patch

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

> Source/WebCore/Modules/webaudio/AudioNode.cpp:310
> +ExceptionOr<void> AudioNode::setChannelInterpretation(const ChannelInterpretation interpretation)

Drop the const.

[Clark Wang]

Created attachment 403864 [details]
Patch

[Chris Dumez]

Comment on attachment 403864 [details]
Patch

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

r=me with nit fix

> Source/WebCore/Modules/webaudio/AudioNode.h:183
> +    ExceptionOr<void> setChannelInterpretation(const ChannelInterpretation);

Drop the const.

[Clark Wang]

Created attachment 403868 [details]
Patch

[EWS]

Committed r264176: <https://trac.webkit.org/changeset/264176>

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

[Radar WebKit Bug Importer]

<rdar://problem/65275132>