Closed Bug 865234 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Web Audio, defect)

Product:
Core
Component:
Web Audio
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla23

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(9 files, 8 obsolete files)

Part 6: Round up the |when| argument AudioBufferSouceNode.start
1.43 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 8: Add a test case for channel mixing rules
19.39 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 7: Change the speaker up-mixing and down-mixing rules according to the Web Audio spec
10.77 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 9: Avoid variable shadowing
2.32 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 1: Add DOM bindings for the channel mixing attributes
26.01 KB, patch
Details | Diff | Splinter Review
Part 3: Implement ChannelCountMode's effect when mixing inputs
1.92 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 4: Implement ChannelInterpretation's effect when mixing inputs
3.54 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 5: Don't mark a stream as finished when there are streams following it
3.51 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 2: Send the channel mixing information to the AudioNodeStream
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)
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: