1443224 - ChannelMerger should throw errors on invalid values of count and mode

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P2)

Description

[toy.raymond]

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

Comment 1

[ovidiu boca[:Ovidiu]]

Tested on Mac OS X 10.12 with FF Nightly 60.0a1(2018-03-07) and FF 58 and I can reproduce it.

Comment 2

[Paul Adenot (:padenot)]

Attachment: Bug 1443224 - Throw correct exceptions when trying to set the channelCount, the channelCountMode and the channelInterpretation on a ChannelSplitterNode.

Spec: https://webaudio.github.io/web-audio-api/#dom-audionode-channelcount
Google has written a web platform test this is going to be merged soon (we
currently fail it).

Review commit: https://reviewboard.mozilla.org/r/226440/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226440/

Comment 3

[Andrea Marchesini [:baku]]

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.

Comment 4

[Paul Adenot (:padenot)]

Attachment: fix test

Review commit: https://reviewboard.mozilla.org/r/226986/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226986/

Comment 5

[Paul Adenot (:padenot)]

Comment on attachment 8957588 [details]
Bug 1443224 - Throw correct exceptions when trying to set the channelCount, the channelCountMode and the channelInterpretation on a ChannelSplitterNode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226440/diff/1-2/

Comment 6

[Paul Adenot (:padenot)]

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

Comment 7

[Paul Adenot (:padenot)]

Comment on attachment 8957588 [details]
Bug 1443224 - Throw correct exceptions when trying to set the channelCount, the channelCountMode and the channelInterpretation on a ChannelSplitterNode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226440/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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 9

[Paul Adenot (:padenot)]

Attachment: Bug 1443224 - Fix bustage by adding override on the definition.

Review commit: https://reviewboard.mozilla.org/r/226988/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226988/

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 8958050 [details]
Bug 1443224 - Fix bustage by adding override on the definition.

https://reviewboard.mozilla.org/r/226988/#review232722

Comment 11

[Mozilla Autoland]

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.

Comment 12

[Cosmin Sabou [:CosminS]]

Backed out for build bustages on dom/ChannelSplitterNode.h

Treeherder push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7386deb98f9a909d8c6f4dea701e351bbd266e94&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=167363685

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=167363685&repo=autoland&lineNumber=16323

Backout link: https://hg.mozilla.org/integration/autoland/rev/fe6bc4234078855432012d333b7e2ccd9a31abd8

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/6d9ebda1ad07

Comment 15

[ovidiu boca[:Ovidiu]]

I tested this on Mac OS X 10.12 with FF Nightly 61.0a1(2018-04-25) and the issue is still reproducible.