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

Implement basic AudioParams #34

Merged
merged 12 commits into from Jun 1, 2018
Merged

Implement basic AudioParams #34

merged 12 commits into from Jun 1, 2018

Conversation

@Manishearth
Copy link
Member

Manishearth commented May 31, 2018

This implements the set and ramp AudioParam events and most of the framework for the harder ones.

r? @ferjm

@Manishearth Manishearth requested a review from ferjm May 31, 2018
@ferjm
ferjm approved these changes Jun 1, 2018
Copy link
Member

ferjm left a comment

LGTM with the comments/questions addressed. Thanks


/// A tick, i.e. the time taken for a single frame
#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Debug)]
pub struct Tick(pub u32);

This comment has been minimized.

@ferjm

ferjm Jun 1, 2018

Member

We are using Tick as the type of AudioRenderThread.current_frame, should we use u64 here? It can grow ad infinitum.

This comment has been minimized.

@ferjm

ferjm Jun 1, 2018

Member

The spec uses unsigned long long for AudioWorkletGlobalScope.currentFrame, which is supposed to be equal to the value of BaseAudioContext's internal current frame. So yes, u64 is probably more appropriate here.

// change frequency at 0.5s and 1s
// change gain at 0.75s and 1.25s
graph.message_node(0, AudioNodeMessage::SetAudioParamEvent(UserAutomationEvent::SetValueAtTime(110., 0.5)));
graph.message_node(0, AudioNodeMessage::SetAudioParamEvent(UserAutomationEvent::SetValueAtTime(220., 1.)));

This comment has been minimized.

@ferjm

ferjm Jun 1, 2018

Member

This works for now, but we need a way to differentiate AudioParams. For instance, oscillator nodes have frequency and detune.

I proposed a different set of messages here. I can change that to use AudioParams if this PR is merged first.

This comment has been minimized.

@Manishearth

Manishearth Jun 1, 2018

Author Member

Yeah so the plan is to have an enum full of variants for each kind of param. I just didn't implement that here since it's trivial to do any time and we don't yet have anything with more than one param.

current_event = next;
}
}
}

This comment has been minimized.

@ferjm

ferjm Jun 1, 2018

Member

Should we remove old events from self.events?

This comment has been minimized.

@Manishearth

Manishearth Jun 1, 2018

Author Member

Eventually yeah. I want to wait till I implement the rest of the events because some of them have some lookbehind and I'm not clear on how that's going to work exactly so I thought I'd handle cleanup later

return false;
}

// println!("Curr {:?}", self.current_event);

This comment has been minimized.

@ferjm

ferjm Jun 1, 2018

Member

Delete this, please.



let current_tick = block.absolute_tick(tick);
// println!("curr_tick {:?}", current_tick);

This comment has been minimized.

@ferjm

ferjm Jun 1, 2018

Member

Delete this one too, please.

@ferjm
ferjm approved these changes Jun 1, 2018
@Manishearth Manishearth merged commit 25da7ab into servo:master Jun 1, 2018
@Manishearth Manishearth deleted the Manishearth:audioparam 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.