Closed Bug 865234 Opened 7 years ago Closed 7 years ago

Implement channel up-mixing and down-mixing rules for AudioNodes

Categories

(Core :: Web Audio, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(9 files, 8 obsolete files)

1.43 KB, patch
roc
: review+
Details | Diff | Splinter Review
19.39 KB, patch
roc
: review+
Details | Diff | Splinter Review
10.77 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.32 KB, patch
roc
: review+
Details | Diff | Splinter Review
26.01 KB, patch
Details | Diff | Splinter Review
1.92 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.54 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.51 KB, patch
roc
: review+
Details | Diff | Splinter Review
16.18 KB, patch
Details | Diff | Splinter Review
This includes implementing the channelCount, channelCountMode, and channelInterpretation properties for AudioNode.
Blocks: 866433
I have some patches for this stuff.
Assignee: nobody → ehsan
Attachment #743432 - Flags: review?(roc)
Attachment #743435 - Flags: review?(roc)
Comment on attachment 743432 [details] [diff] [review]
Part 1: Add DOM bindings for the channel mixing attributes

Review of attachment 743432 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webaudio/AudioDestinationNode.cpp
@@ +18,5 @@
>  AudioDestinationNode::AudioDestinationNode(AudioContext* aContext, MediaStreamGraph* aGraph)
> +  : AudioNode(aContext,
> +              2,
> +              mozilla::dom::ChannelCountMode::Explicit,
> +              mozilla::dom::ChannelInterpretation::Speakers)

I'm not sure this is a good choice --- I think "max" would be a better mode --- but that's a spec issue.

::: content/media/webaudio/PannerNode.cpp
@@ +161,5 @@
>  PannerNode::PannerNode(AudioContext* aContext)
> +  : AudioNode(aContext,
> +              2,
> +              mozilla::dom::ChannelCountMode::Clamped_max,
> +              mozilla::dom::ChannelInterpretation::Speakers)

mozilla::dom:: prefixes should not be needed here (or in the same places in other files)
Attachment #743432 - Flags: review?(roc) → review+
Comment on attachment 743434 [details] [diff] [review]
Part 2: Send the channel mixing information to the AudioNodeStream

Review of attachment 743434 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/AudioNodeStream.h
@@ +42,5 @@
> +  enum ChannelMixingMode {
> +    // ChannelCountMode
> +    MAX = 1 << 1,
> +    CLAMPED_MAX = 1 << 2,
> +    EXPLICIT = 1 << 3,

These could just be 0, 1, 2.

But is this enum really worth it? Why not just pass around two of the WebIDL generated enums? Or a struct containing them?

::: content/media/webaudio/AnalyserNode.cpp
@@ +87,5 @@
>  {
>    mStream = aContext->Graph()->CreateAudioNodeStream(new AnalyserNodeEngine(this),
>                                                       MediaStreamGraph::INTERNAL_STREAM);
>    AllocateBuffer();
> +  SendChannelMixingParametersToStream();

Instead of generating message traffic for every node construction, pass the mixing parameters as parameters to CreateAudioNodeStream.

Or better still, CreateAudioNodeStream can do it for all node types, getting the parameters via the Engine's reference to the AudioNode.
Comment on attachment 743439 [details] [diff] [review]
Part 7: Change the speaker up-mixing and down-mixing rules according to the Web Audio spec

Review of attachment 743439 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/AudioChannelFormat.cpp
@@ +149,5 @@
> +          aChannelArray->ElementAt(1) = surroundChannels[SURROUND_R];
> +          aChannelArray->ElementAt(2) = surroundChannels[SURROUND_C];
> +          aChannelArray->ElementAt(3) = surroundChannels[SURROUND_LFE];
> +          aChannelArray->ElementAt(4) = surroundChannels[SURROUND_SL];
> +          aChannelArray->ElementAt(5) = surroundChannels[SURROUND_SR];

I think all this code can be simplified a lot. :-)

In particular this can be data-driven using an array or something to express the mapping of surroundChannels elements into aChannelArray.
(In reply to comment #11)
> ::: content/media/webaudio/AudioDestinationNode.cpp
> @@ +18,5 @@
> >  AudioDestinationNode::AudioDestinationNode(AudioContext* aContext, MediaStreamGraph* aGraph)
> > +  : AudioNode(aContext,
> > +              2,
> > +              mozilla::dom::ChannelCountMode::Explicit,
> > +              mozilla::dom::ChannelInterpretation::Speakers)
> 
> I'm not sure this is a good choice --- I think "max" would be a better mode ---
> but that's a spec issue.

Do you want me to bring that up?

> ::: content/media/webaudio/PannerNode.cpp
> @@ +161,5 @@
> >  PannerNode::PannerNode(AudioContext* aContext)
> > +  : AudioNode(aContext,
> > +              2,
> > +              mozilla::dom::ChannelCountMode::Clamped_max,
> > +              mozilla::dom::ChannelInterpretation::Speakers)
> 
> mozilla::dom:: prefixes should not be needed here (or in the same places in
> other files)

They are needed in order to properly reference the names ChannelCountMode and ChannelInterpretation (which are both type names and method names.)
(In reply to comment #12)
> ::: content/media/AudioNodeStream.h
> @@ +42,5 @@
> > +  enum ChannelMixingMode {
> > +    // ChannelCountMode
> > +    MAX = 1 << 1,
> > +    CLAMPED_MAX = 1 << 2,
> > +    EXPLICIT = 1 << 3,
> 
> These could just be 0, 1, 2.

I need to be able to and with MAX.

> But is this enum really worth it? Why not just pass around two of the WebIDL
> generated enums? Or a struct containing them?

What would be the advantage of doing that?  All we need here is 5 bits!

> ::: content/media/webaudio/AnalyserNode.cpp
> @@ +87,5 @@
> >  {
> >    mStream = aContext->Graph()->CreateAudioNodeStream(new AnalyserNodeEngine(this),
> >                                                       MediaStreamGraph::INTERNAL_STREAM);
> >    AllocateBuffer();
> > +  SendChannelMixingParametersToStream();
> 
> Instead of generating message traffic for every node construction, pass the
> mixing parameters as parameters to CreateAudioNodeStream.
> 
> Or better still, CreateAudioNodeStream can do it for all node types, getting
> the parameters via the Engine's reference to the AudioNode.

OK, I'll do that.
With the second half of comment 12 addressed.
Attachment #743434 - Attachment is obsolete: true
Attachment #743434 - Flags: review?(roc)
Attachment #743635 - Flags: review?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> Comment on attachment 743439 [details] [diff] [review]
> Part 7: Change the speaker up-mixing and down-mixing rules according to the
> Web Audio spec
> 
> Review of attachment 743439 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/AudioChannelFormat.cpp
> @@ +149,5 @@
> > +          aChannelArray->ElementAt(1) = surroundChannels[SURROUND_R];
> > +          aChannelArray->ElementAt(2) = surroundChannels[SURROUND_C];
> > +          aChannelArray->ElementAt(3) = surroundChannels[SURROUND_LFE];
> > +          aChannelArray->ElementAt(4) = surroundChannels[SURROUND_SL];
> > +          aChannelArray->ElementAt(5) = surroundChannels[SURROUND_SR];
> 
> I think all this code can be simplified a lot. :-)
> 
> In particular this can be data-driven using an array or something to express
> the mapping of surroundChannels elements into aChannelArray.

How about this?  :-)
Attachment #743439 - Attachment is obsolete: true
Attachment #743439 - Flags: review?(roc)
Attachment #743726 - Flags: review?(roc)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #14)
> (In reply to comment #11)
> > I'm not sure this is a good choice --- I think "max" would be a better mode ---
> > but that's a spec issue.
> 
> Do you want me to bring that up?

If you like. I don't mind.

> > ::: content/media/webaudio/PannerNode.cpp
> > @@ +161,5 @@
> > >  PannerNode::PannerNode(AudioContext* aContext)
> > > +  : AudioNode(aContext,
> > > +              2,
> > > +              mozilla::dom::ChannelCountMode::Clamped_max,
> > > +              mozilla::dom::ChannelInterpretation::Speakers)
> > 
> > mozilla::dom:: prefixes should not be needed here (or in the same places in
> > other files)
> 
> They are needed in order to properly reference the names ChannelCountMode
> and ChannelInterpretation (which are both type names and method names.)

Ugh, that's horrible! Can't we avoid that somehow?
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #15)
> > But is this enum really worth it? Why not just pass around two of the WebIDL
> > generated enums? Or a struct containing them?
> 
> What would be the advantage of doing that?  All we need here is 5 bits!

Cleaner code! I think you can store both fields in a struct that uses bitfields to pack the enums.
Comment on attachment 743726 [details] [diff] [review]
Part 7: Change the speaker up-mixing and down-mixing rules according to the Web Audio spec

Review of attachment 743726 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/AudioNodeStream.cpp
@@ +290,2 @@
>          for (uint32_t i = 0; i < outputChannelCount; ++i) {
> +          outputChannels[i] = &downmixBuffer[i * WEBAUDIO_BLOCK_SIZE];

We shouldn't be shadowing 'i' here, that's confusing.

I'm not sure why you're changing this.
Blocks: 867086
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #14)
> > (In reply to comment #11)
> > > I'm not sure this is a good choice --- I think "max" would be a better mode ---
> > > but that's a spec issue.
> > 
> > Do you want me to bring that up?
> 
> If you like. I don't mind.

Done.

> > > ::: content/media/webaudio/PannerNode.cpp
> > > @@ +161,5 @@
> > > >  PannerNode::PannerNode(AudioContext* aContext)
> > > > +  : AudioNode(aContext,
> > > > +              2,
> > > > +              mozilla::dom::ChannelCountMode::Clamped_max,
> > > > +              mozilla::dom::ChannelInterpretation::Speakers)
> > > 
> > > mozilla::dom:: prefixes should not be needed here (or in the same places in
> > > other files)
> > 
> > They are needed in order to properly reference the names ChannelCountMode
> > and ChannelInterpretation (which are both type names and method names.)
> 
> Ugh, that's horrible! Can't we avoid that somehow?

Not sure...  Boris, is it possible for us to get the Web IDL codegen to rename enums?

(At any rate, I'd rather fix that as a follow-up and not hold up these patches for this.)
Flags: needinfo?(bzbarsky)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #15)
> > > But is this enum really worth it? Why not just pass around two of the WebIDL
> > > generated enums? Or a struct containing them?
> > 
> > What would be the advantage of doing that?  All we need here is 5 bits!
> 
> Cleaner code! I think you can store both fields in a struct that uses
> bitfields to pack the enums.

OK, I'll do that.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> Comment on attachment 743726 [details] [diff] [review]
> Part 7: Change the speaker up-mixing and down-mixing rules according to the
> Web Audio spec
> 
> Review of attachment 743726 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/AudioNodeStream.cpp
> @@ +290,2 @@
> >          for (uint32_t i = 0; i < outputChannelCount; ++i) {
> > +          outputChannels[i] = &downmixBuffer[i * WEBAUDIO_BLOCK_SIZE];
> 
> We shouldn't be shadowing 'i' here, that's confusing.

Yeah, I'll fix that.

> I'm not sure why you're changing this.

The previous code "borrowed" these buffers from aTmpChunk, which is not safe at all if you have more than one chunk, because in the second round of the outer loop we'll effectively overwrite the data that we had before.  The test case in part 8 helped me catch this.
Attachment #743941 - Flags: review?(roc)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #21)
> (At any rate, I'd rather fix that as a follow-up and not hold up these
> patches for this.)

OK.
> Not sure...  Boris, is it possible for us to get the Web IDL codegen to rename enums?

The perils of dropping the Get on getters...  :(

So right now there is no facility for renaming enums without renaming them in the IDL.  Which is totally doable, because unlike interfaces enums do not cause any properties anywhere content-visible with the enum's name.  The biggest risk would be if some place used the per-spec enum name and failed to compile, but we'd catch that at that point.

The other option here is that we do have a facility, with binaryNames stuff in the conf file, to rename _methods_ on the C++ side.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky (:bz) from comment #25)
> > Not sure...  Boris, is it possible for us to get the Web IDL codegen to rename enums?
> 
> The perils of dropping the Get on getters...  :(
> 
> So right now there is no facility for renaming enums without renaming them
> in the IDL.  Which is totally doable, because unlike interfaces enums do not
> cause any properties anywhere content-visible with the enum's name.  The
> biggest risk would be if some place used the per-spec enum name and failed
> to compile, but we'd catch that at that point.
> 
> The other option here is that we do have a facility, with binaryNames stuff
> in the conf file, to rename _methods_ on the C++ side.

Yeah, I should've though about binaryName!  I think that's the way to go here.
With the binaryNames stuff.  No need to review again.
Attachment #743432 - Attachment is obsolete: true
Now using the Web IDL generated enums, packing them into 16 bits each.
Attachment #743635 - Attachment is obsolete: true
Attachment #743635 - Flags: review?(roc)
Attachment #744033 - Flags: review?(roc)
Rebased on top of the latest version of part 2.
Attachment #743435 - Attachment is obsolete: true
Attachment #743435 - Flags: review?(roc)
Attachment #744034 - Flags: review?(roc)
Rebased on top of the latest version of part 2.
Attachment #744035 - Flags: review?(roc)
Attachment #743436 - Attachment is obsolete: true
Attachment #743436 - Flags: review?(roc)
Rebased.
Attachment #743437 - Attachment is obsolete: true
Attachment #743437 - Flags: review?(roc)
Attachment #744036 - Flags: review?(roc)
I think this is it, the other patches here should not need rebasing.
Comment on attachment 744033 [details] [diff] [review]
Part 2: Send the channel mixing information to the AudioNodeStream

Review of attachment 744033 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webaudio/DynamicsCompressorNode.cpp
@@ +182,4 @@
>                ChannelCountMode::Explicit,
>                ChannelInterpretation::Speakers)
> +              mozilla::dom::ChannelCountMode::Explicit,
> +              mozilla::dom::ChannelInterpretation::Speakers)

this ain't gonna build
Attachment #744033 - Flags: review?(roc) → review+
Comment on attachment 744034 [details] [diff] [review]
Part 3: Implement ChannelCountMode's effect when mixing inputs

Review of attachment 744034 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/AudioNodeStream.cpp
@@ +258,5 @@
>        continue;
>      }
>  
>      inputChunks.AppendElement(chunk);
> +    if (mMixingMode.mChannelCountMode != ChannelCountMode::Explicit) {

Slightly simpler to take out this conditional, switch on mChannelCountMode after the loop and if it's Explicit, just overwrite outputChannelCount to mNumberOfInputChannels.
Attachment #744034 - Flags: review?(roc) → review+
I was bound to mess up one of these rebases!
Attachment #744033 - Attachment is obsolete: true
Attachment #744041 - Flags: review?(roc)
Comment on attachment 744041 [details] [diff] [review]
Part 2: Send the channel mixing information to the AudioNodeStream

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #35)
> Created attachment 744041 [details] [diff] [review]
> Part 2: Send the channel mixing information to the AudioNodeStream
> 
> I was bound to mess up one of these rebases!

Well this just addresses comment 33!
Attachment #744041 - Flags: review?(roc)
Comment on attachment 743726 [details] [diff] [review]
Part 7: Change the speaker up-mixing and down-mixing rules according to the Web Audio spec

Review of attachment 743726 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that.

::: content/media/AudioNodeStream.cpp
@@ +286,4 @@
>        if (mMixingMode & SPEAKERS) {
>          nsAutoTArray<float*,GUESS_AUDIO_CHANNELS> outputChannels;
>          outputChannels.SetLength(outputChannelCount);
> +        downmixBuffer = new float[outputChannelCount * WEBAUDIO_BLOCK_SIZE];

How about making downmixBuffer an nsAutoTArray<float,GUESS_AUDIO_CHANNELS*WEBAUDIO_BLOCK_SIZE>? That's still only 1K of stack space so no problem. And here you'd just use SetLength().
Comment on attachment 744036 [details] [diff] [review]
Part 5: Don't mark a stream as finished when there are streams following it

Review of attachment 744036 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/AudioNodeStream.h
@@ +48,5 @@
>      : ProcessedMediaStream(nullptr),
>        mEngine(aEngine),
>        mKind(aKind),
> +      mNumberOfInputChannels(2),
> +      mMarkAsFinsihedAfterThisBlock(false)

"Finished"
Attachment #744036 - Flags: review?(roc) → review+
Depends on: 867511
Duplicate of this bug: 867086
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
You need to log in before you can comment on or make changes to this bug.