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

Moar API changes! #59

Closed
wants to merge 4 commits into from
Closed

Moar API changes! #59

wants to merge 4 commits into from

Conversation

@ferjm
Copy link
Member

ferjm commented Jun 26, 2018

Apparently a rendering thread can either be running or suspended, not closed.

The rest are exposed getters and a way to message AudioScheduledSourceNodes directly (and a generic way for a node to implement different message handlers).

r? @Manishearth

@ferjm ferjm requested a review from Manishearth Jun 26, 2018
Copy link
Member

Manishearth left a comment

I don't think we need getters for properties the DOM code can calculate itself.

The engines contain these properties for their own use; otherwise I see the DOM code already knowing these values.

@@ -4,6 +4,9 @@ macro_rules! make_message_handler(
fn message(&mut self, msg: ::audio::node::AudioNodeMessage, sample_rate: f32) {
match msg {
::audio::node::AudioNodeMessage::$node(m) => self.handle_message(m, sample_rate),
::audio::node::AudioNodeMessage::GetInputCount(tx) => tx.send(self.input_count()).unwrap(),

This comment has been minimized.

@Manishearth

Manishearth Jun 26, 2018

Member

We don't need these; the DOM will maintain these values itself I think.

All of the invariants will be upheld by the DOM side as well (aside from invariants that require maintaining an audio param timeline or a graph to validate)

@@ -94,6 +94,8 @@ pub enum AudioNodeMessage {
AudioScheduledSourceNode(AudioScheduledSourceNodeMessage),
GainNode(GainNodeMessage),
GetChannelCount(Sender<u8>),
GetChannelCountMode(Sender<ChannelCountMode>),
GetChannelInterpretation(Sender<ChannelInterpretation>),

This comment has been minimized.

@Manishearth

Manishearth Jun 26, 2018

Member

again, we don't need getters for these

@ferjm
Copy link
Member Author

ferjm commented Jun 26, 2018

I may be missing something, but TBH I don't see much value on duplicating this data on both sides :\

We mostly use this information on the servo-media side. The DOM bindings are mostly a proxy of this data and caching it there will add some extra complexity that IMHO seems unnecessary.

Note that we also need setters for some of these properties.

In general, I am trying to get as much data as possible from servo-media, so the DOM bindings are as thin as possible.

@Manishearth
Copy link
Member

Manishearth commented Jun 26, 2018

I may be missing something, but TBH I don't see much value on duplicating this data on both sides :\

To avoid unnecessary channel traffic. The DOM APIs are sync and should not have to block on the rendering process. There are some cases where this is inevitable but I'd like to minimize it 😄

There's a bunch of DOM-side stuff in the WebAudio APIs that involves calculating errors and such, for which it's best to have this info locally. To me this crate is a lower level abstraction that provides the actual audio processing engines.

This is how Gecko does it as well. The DOM side does all the checks (since it must report errors) and stores a local value, and sends updates to the nodes.

@ferjm ferjm closed this Jun 27, 2018
@ferjm ferjm deleted the ferjm:moar.api.changes branch Jun 27, 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.