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

AudioScheduledSourceNode #31

Merged
merged 5 commits into from Jun 1, 2018

Conversation

@ferjm
Copy link
Member

ferjm commented May 28, 2018

Fixes #21.

@ferjm ferjm requested a review from Manishearth May 28, 2018
@ferjm
Copy link
Member Author

ferjm commented May 28, 2018

@Manishearth
Copy link
Member

Manishearth commented May 29, 2018

Overall looks good.

Also, is it possible to schedule multiple events? If so we'd need to store these as a list of events (in a smallvec probably) and check them each time.

@ferjm
Copy link
Member Author

ferjm commented May 29, 2018

Also, is it possible to schedule multiple events? If so we'd need to store these as a list of events (in a smallvec probably) and check them each time

Good point! Yes, I believe it's possible.

@Manishearth
Copy link
Member

Manishearth commented May 29, 2018

@ferjm ferjm force-pushed the ferjm:audioscheduledsourcenode.start.stop branch 2 times, most recently from ee445de to 1a9608b May 30, 2018
@Manishearth
Copy link
Member

Manishearth commented Jun 1, 2018

Given that we now have ticks, can we internally use ticks for this? We can convert time floats to ticks at the user API boundary

@ferjm ferjm force-pushed the ferjm:audioscheduledsourcenode.start.stop branch from 1a9608b to 8429e84 Jun 1, 2018
@ferjm ferjm force-pushed the ferjm:audioscheduledsourcenode.start.stop branch from 8429e84 to 3748b6b Jun 1, 2018
@ferjm
Copy link
Member Author

ferjm commented Jun 1, 2018

@Manishearth I think this is ready for a final review. I checked the spec and found that we cannot schedule more than one start or stop times, so my believes here where incorrect.

fn message(&mut self, msg: AudioNodeMessage, sample_rate: f32) {
match msg {
AudioNodeMessage::SetAudioParamEvent(event) => {
self.gain.insert_event(event.to_event(sample_rate))

This comment has been minimized.

@Manishearth

Manishearth Jun 1, 2018

Member

This reverts the AudioParam changes for gainnode

This comment has been minimized.

@ferjm

ferjm Jun 1, 2018

Author Member

They are moved here

This comment has been minimized.

@Manishearth

Manishearth Jun 1, 2018

Member

Oh, I see what you did.

(sorry, reviewing on a phone)

I guess this is ok. A lot of the messages are common between nodes so I thought we'd have common messages. But this works.

pub enum AudioNodeMessage {
SetAudioParamEvent(UserAutomationEvent)
OscillatorNode(OscillatorNodeMessage),
GainNode(GainNodeMessage),

This comment has been minimized.

@Manishearth

Manishearth Jun 1, 2018

Member

I think we should be able to send all kinds of messages to all nodes, and nodes filter what they can actually handle.

Also, this reverts the AudioParam changes.

So you have SetAudioParam, Start, and Stop.

@Manishearth Manishearth merged commit 0a89fab into servo:master Jun 1, 2018
@ferjm ferjm deleted the ferjm:audioscheduledsourcenode.start.stop branch Jun 1, 2018
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

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