Closed Bug 865247 Opened 8 years ago Closed 8 years ago

Implement ChannelSplitterNode

Categories

(Core :: Web Audio, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(4 files, 6 obsolete files)

No description provided.
Kevin wants this, so I'm going to implement it.  Should be pretty easy.
Assignee: nobody → ehsan
Attachment #745449 - Flags: review?(roc)
Bobby, this is the patch series which will change the ProduceAudioBlock API, in case you're curious.  Look at part 3 to see how you need to change your implementation -- should be pretty simple.
Actually it looks like the input and output numbers could be uint16_t.
Comment on attachment 745448 [details] [diff] [review]
Part 3: Change the ProduceAudioBlock to make it aware of multiple input and output ports for simultaneous processing

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

Since Channel*Nodes are the only ones that take more than one numbered input/output I wonder whether it would be better to avoid modifying the ProduceAudioBlock API.

We could keep the existing single input/output virtual method and add a separate method for multi-input/output, calling the former when there's only one input and output.

::: content/media/AudioNodeStream.cpp
@@ +242,5 @@
> +{
> +  uint32_t inputCount = mInputs.Length();
> +  uint32_t maxInputPort = 0;
> +  for (uint32_t i = 0; i < inputCount; ++i) {
> +    maxInputPort = std::max(maxInputPort, mInputs[i]->InputNumber());

Looping over the input ports isn't good. Why not just copy the maximum number of inputs and outputs into the AudioNodeEngine and access it directly?

::: content/media/webaudio/ScriptProcessorNode.cpp
@@ +184,5 @@
>  
>    virtual void ProduceAudioBlock(AudioNodeStream* aStream,
> +                                 const OutputChunks& aInput,
> +                                 OutputChunks& aOutput,
> +                                 bool* aFinished)

Why are you removing MOZ_OVERRIDE?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> Comment on attachment 745448 [details] [diff] [review]
> Part 3: Change the ProduceAudioBlock to make it aware of multiple input and
> output ports for simultaneous processing
> 
> Review of attachment 745448 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Since Channel*Nodes are the only ones that take more than one numbered
> input/output I wonder whether it would be better to avoid modifying the
> ProduceAudioBlock API.
> 
> We could keep the existing single input/output virtual method and add a
> separate method for multi-input/output, calling the former when there's only
> one input and output.

Before calling ProduceAudioBlock API we don't know if the engine will produce multiple outputs, so while this could work for channel merger since we will have multiple input ports, it won't work for channel splitter.  I guess I can add a bool flag to AudioNodeEngine and set it to true for splitters and mergers and use that to decide which API to call.  Does that sound good?

> ::: content/media/AudioNodeStream.cpp
> @@ +242,5 @@
> > +{
> > +  uint32_t inputCount = mInputs.Length();
> > +  uint32_t maxInputPort = 0;
> > +  for (uint32_t i = 0; i < inputCount; ++i) {
> > +    maxInputPort = std::max(maxInputPort, mInputs[i]->InputNumber());
> 
> Looping over the input ports isn't good. Why not just copy the maximum
> number of inputs and outputs into the AudioNodeEngine and access it directly?

OK, I can do that.  Actually that takes care of what I said above as well.

> ::: content/media/webaudio/ScriptProcessorNode.cpp
> @@ +184,5 @@
> >  
> >    virtual void ProduceAudioBlock(AudioNodeStream* aStream,
> > +                                 const OutputChunks& aInput,
> > +                                 OutputChunks& aOutput,
> > +                                 bool* aFinished)
> 
> Why are you removing MOZ_OVERRIDE?

My mistake.
Attachment #745449 - Attachment is obsolete: true
Forgot to add the test case.
Attachment #745548 - Attachment is obsolete: true
Attachment #745553 - Flags: review?(roc)
Attachment #745553 - Attachment is obsolete: true
Attachment #745553 - Flags: review?(roc)
Attachment #745559 - Flags: review?(roc)
Having two virtual overloads and overriding one of them warns in gcc.  I guess I need to rename the new ProduceAudioBlock.  :(
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.