Closed Bug 1443224 Opened 6 years ago Closed 6 years ago

ChannelMerger should throw errors on invalid values of count and mode

Categories

(Core :: Web Audio, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: toy.raymond, Assigned: padenot)

References

Details

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.106 Safari/537.36

Steps to reproduce:

Run this in the console:

c = new AudioContext()
n = new ChannelMergerNode(c)
n.channelCount = 3
n.channelCountMode = "max"



Actual results:

The channelCount is set to 3 and channelCountMode is set to "max". 


Expected results:

In both cases, an error MUST be thrown because these attributes are fixed.  See https://webaudio.github.io/web-audio-api/#dfn-channelcount-constraints and https://webaudio.github.io/web-audio-api/#dfn-channelcountmode-constraints
Tested on Mac OS X 10.12 with FF Nightly 60.0a1(2018-03-07) and FF 58 and I can reproduce it.
Status: UNCONFIRMED → NEW
Has STR: --- → yes
Component: Untriaged → Web Audio
Ever confirmed: true
Product: Firefox → Core
Version: 57 Branch → Trunk
Comment on attachment 8957588 [details]
Bug 1443224 - Throw correct exceptions when trying to set the channelCount, the channelCountMode and the channelInterpretation on a ChannelSplitterNode.

https://reviewboard.mozilla.org/r/226440/#review232404

::: dom/media/webaudio/AudioNode.cpp:94
(Diff revision 1)
>        return;
>      }
>    }
>  
>    if (aOptions.mChannelInterpretation.WasPassed()) {
> -    SetChannelInterpretationValue(aOptions.mChannelInterpretation.Value());
> +    SetChannelInterpretationValue(aOptions.mChannelInterpretation.Value(), aRv);

Just in case we add something more in this method,
what about if we add here:

if (NS_WARN_IF(aRv.Failed())) {
  return;
}

::: dom/media/webaudio/ChannelSplitterNode.h:34
(Diff revision 1)
>                const ChannelSplitterOptions& aOptions, ErrorResult& aRv)
>    {
>      return Create(aAudioContext, aOptions, aRv);
>    }
>  
> +  virtual void SetChannelCount(uint32_t aChannelCount, ErrorResult& aRv)

This method is final. No virtual here.
Attachment #8957588 - Flags: review?(amarchesini) → review+
Assignee: nobody → padenot
Rank: 15
Priority: -- → P2
Attachment #8958045 - Attachment is obsolete: true
Comment on attachment 8957588 [details]
Bug 1443224 - Throw correct exceptions when trying to set the channelCount, the channelCountMode and the channelInterpretation on a ChannelSplitterNode.

https://reviewboard.mozilla.org/r/226440/#review232712
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/autoland/rev/7386deb98f9a
Throw correct exceptions when trying to set the channelCount, the channelCountMode and the channelInterpretation on a ChannelSplitterNode. r=baku
Comment on attachment 8958050 [details]
Bug 1443224 - Fix bustage by adding override on the definition.

https://reviewboard.mozilla.org/r/226988/#review232722
Attachment #8958050 - Flags: review+
We're sorry - something has gone wrong while rewriting or rebasing your commits. The commits being pushed no longer match what was requested. Please file a bug.
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d9ebda1ad07
Throw correct exceptions when trying to set the channelCount, the channelCountMode and the channelInterpretation on a ChannelSplitterNode. r=baku
https://hg.mozilla.org/mozilla-central/rev/6d9ebda1ad07
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
I tested this on Mac OS X 10.12 with FF Nightly 61.0a1(2018-04-25) and the issue is still reproducible.
You need to log in before you can comment on or make changes to this bug.