Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for setting channel information #61

Merged
merged 6 commits into from Jun 28, 2018
Merged

Add support for setting channel information #61

merged 6 commits into from Jun 28, 2018

Conversation

@Manishearth
Copy link
Member

Manishearth commented Jun 27, 2018

I'm trying a trait based trick for keeping message handling common. If you like this
we can use something similar for AudioScheduledSourceNode.

This is also used by the sink, and the example merges channels halfway through by forcing a mix on the sink.

based off #60

r? @ferjm

@Manishearth
Copy link
Member Author

Manishearth commented Jun 27, 2018

separate PR because this is somewhat WIP and is trying something out with the traits, and I don't want to block the other PR on this. Review and land #60 first

@Manishearth Manishearth force-pushed the chan-setting branch from 35797c6 to 9f539d4 Jun 27, 2018
@@ -92,6 +136,9 @@ pub enum AudioNodeMessage {
AudioBufferSourceNode(AudioBufferSourceNodeMessage),
GainNode(GainNodeMessage),
OscillatorNode(OscillatorNodeMessage),
SetChannelCount(u8),
SetChannelMode(ChannelCountMode),
SetChannelInterpretation(ChannelInterpretation),

This comment has been minimized.

@ferjm

ferjm Jun 27, 2018

Member

Should these setters get a Sender<()> so we can block from the consumer (DOM side in this case) until the property is actually set?

This comment has been minimized.

@Manishearth

Manishearth Jun 27, 2018

Author Member

We don't block for the other setters. I don't see a reason to block the DOM thread on the rendering thread.

@Manishearth Manishearth force-pushed the chan-setting branch from 9f539d4 to 23cb991 Jun 27, 2018
@Manishearth Manishearth force-pushed the chan-setting branch from 23cb991 to c904743 Jun 27, 2018
@Manishearth Manishearth force-pushed the chan-setting branch from c904743 to ee53441 Jun 27, 2018
@Manishearth
Copy link
Member Author

Manishearth commented Jun 27, 2018

@sdroege so it seems like if I change the channel count audio info on the app src, there's an audible delay. This isn't introduced by the code here (I tried changing the channel count of a different node and it's fine), it seems to be because the buffered up frames get dropped (??)

Is there something I should do here when the channel count changes? Wait for it to flush?

@sdroege
Copy link
Contributor

sdroege commented Jun 28, 2018

@sdroege so it seems like if I change the channel count audio info on the app src, there's an audible delay. This isn't introduced by the code here (I tried changing the channel count of a different node and it's fine), it seems to be because the buffered up frames get dropped (??)

You mean changing the number of channels takes a while to take effect? Or that there is generally latency involved if a non-default number of channels is set already from the beginning?

@Manishearth
Copy link
Member Author

Manishearth commented Jun 28, 2018

No, changing channels causes some latency, with an audible gap of silence.

@sdroege
Copy link
Contributor

sdroege commented Jun 28, 2018

So it does not only take a while to take effect, but there is actually silence for a while? How can I reproduce this to take a look myself?

@Manishearth
Copy link
Member Author

Manishearth commented Jun 28, 2018

from the examples folder; cargo run --bin channels

@Manishearth Manishearth merged commit 78f65a3 into master Jun 28, 2018
@Manishearth
Copy link
Member Author

Manishearth commented Jun 28, 2018

Merging for now, the main approach was reviewed and the extra commits are cleanups or are hooking it up to the sink. Feel free to review later; but I don't want this to be causing lots of merge conflicts 😄

@Manishearth Manishearth deleted the chan-setting branch Jun 28, 2018
@sdroege
Copy link
Contributor

sdroege commented Jun 29, 2018

I'll take a look in the next days, travelling currently. I'll let you know here what I find or send a PR directly

@sdroege
Copy link
Contributor

sdroege commented Jul 1, 2018

@Manishearth the problem here is that you not only change the channel configuration, but you let this change the configuration on the audio sink. This requires re-initializing the hardware, and that takes a moment.

A solution for that would be to configure the audio sink with one specific format (configurable? the first one that arrives?) and then have conversion in software before the audio sink.

Unrelated to that, I saw the demo video and it seems there's quite some latency. You can reduce the latency by with the buffer-time and latency-time properties on the sink. The first is the size of the buffer and defines the latency, the second is the chunk size in which the audio is shoveled to the hardware (and it obviously needs to be lower than buffer-time). A smaller buffer-time makes buffer underruns more likely, a smaller latency-time increases CPU usage, so this needs to be carefully configured. But the default of 200ms is relatively high :)

@Manishearth
Copy link
Member Author

Manishearth commented Jul 2, 2018

Is there a way to get the max channels supported by the hardware? We can set the caps to that, and then do software conversion.

@sdroege
Copy link
Contributor

sdroege commented Jul 2, 2018

Is there a way to get the max channels supported by the hardware? We can set the caps to that, and then do software conversion.

Yes (set the sink to READY state and do Pad::query_caps(None) on its sinkpad) but it's not as easy as that. Channels have meanings, like their positions and e.g. whether it's LFE.

@Manishearth
Copy link
Member Author

Manishearth commented Jul 2, 2018

Yeah I guess we need to match that up with our interpretations.

@sdroege
Copy link
Contributor

sdroege commented Jul 3, 2018

Yeah I guess we need to match that up with our interpretations.

Do you have a link? And how does it work exactly? I would've assumed the WebAudio app to select what channel layout it wants to produce, not the hardware deciding what is wanted

@philn how are you handling this in webkit? Short summary is that switching the numbers of channels causes a short pause and click during hardware reconfiguration.

@Manishearth
Copy link
Member Author

Manishearth commented Jul 3, 2018

https://webaudio.github.io/web-audio-api/#ChannelLayouts

WebAudio specifies what the channel layouts should be interpreted as for 1,2,4,6 channels in speaker mode.

@philn
Copy link
Collaborator

philn commented Jul 3, 2018

I'm not sure dynamic reconfiguration is well handled in WebKit... The audio channels are configured one time (when the src element has been constructed) from the AudioBus channels layout. Then the internal task of the element pushes data to separate appsrc elements (one per channel) and that goes to an interleave element.

@Manishearth
Copy link
Member Author

Manishearth commented Jul 3, 2018

That seems to be similar to what I'm planning here -- configure it based on the layout, and the destinationnode's channel count is just an intermediate mixing interface, like a 1-gain GainNode

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.