Implement ChannelSplitterNode

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
mozilla23
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 6 obsolete attachments)

6.09 KB, patch
Details | Diff | Splinter Review
2.43 KB, patch
Details | Diff | Splinter Review
17.00 KB, patch
roc
: review+
Details | Diff | Splinter Review
14.04 KB, patch
roc
: review+
Details | Diff | Splinter Review
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.