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

Invert dependency graph for backends #209

Merged
merged 1 commit into from Mar 1, 2019

Conversation

Projects
None yet
4 participants
@jdm
Copy link
Member

jdm commented Feb 22, 2019

This change has several benefits:

  • it makes the backend selection explicit by the user of this library (which ensures that it's easy to test that the dummy backend compiles)
  • it moves the platform-based backend selection into its own optional crate and reduces duplication
  • it makes the backends depend on servo-media, rather than the reverse, which improves build times for Servo when the public media APIs are not modified

The downside is there are more trait objects instead of static types. My suspicion is that the performance hit of using virtual methods here is dwarfed by the other work being done, but I have not attempted to measure this at all.

Fixes #194.

@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Feb 22, 2019

The changes for Servo to use this new model are quite short: servo/servo@master...jdm:media-invert

@ferjm

ferjm approved these changes Feb 25, 2019

Copy link
Member

ferjm left a comment

Thanks for doing this!

r=me

@@ -12,7 +12,7 @@ use node::{BlockInfo, ChannelInfo};
use offline_sink::OfflineAudioSink;
use oscillator_node::OscillatorNode;
use panner_node::PannerNode;
use sink::{AudioSink, AudioSinkError, DummyAudioSink};
use sink::{AudioSink, AudioSinkError/*, DummyAudioSink*/};

This comment has been minimized.

@ferjm

ferjm Feb 25, 2019

Member

It seems that this can be removed now.

Err((_graph, e)) => {
error!("Could not start audio render thread due to error `{:?}`", e);
assert!(false);
/*error!(

This comment has been minimized.

@ferjm

ferjm Feb 25, 2019

Member

Remove the commented code, please.

DummyWebRtcController
}
pub trait Backend: Send + Sync {
fn make_player(&self) -> Box<Player>;

This comment has been minimized.

@ferjm

ferjm Feb 25, 2019

Member

uber-nit: s/make_player/create_player/g as all the other methods are create_*.

servo-media-audio = { path = "../../audio" }
servo-media-player = { path = "../../player" }
servo-media-streams = { path = "../../streams" }
servo-media-webrtc = { path = "../../webrtc" }

This comment has been minimized.

@Manishearth

Manishearth Feb 25, 2019

Member

newline at eof

@@ -18,5 +18,8 @@ path = "../streams"
[dependencies.servo-media-webrtc]
path = "../webrtc"

[target.'cfg(any(all(target_os = "android", target_arch = "arm"), target_arch = "x86_64"))'.dependencies.servo-media-gstreamer]
path = "../backends/gstreamer"
#[target.'cfg(any(all(target_os = "android", target_arch = "arm"), target_arch = "x86_64"))'.dependencies.servo-media-gstreamer]

This comment has been minimized.

@Manishearth

Manishearth Feb 25, 2019

Member

remove this?

@Manishearth

This comment has been minimized.

Copy link
Member

Manishearth commented Feb 25, 2019

The reason we initially chose the other model was mostly that it was easier, and we didn't expect other backends. But the dummy backend is turning out to be pretty important, so it's probably good to have it supported better.

I doubt the trait object costs are major here.

@jdm jdm force-pushed the jdm:dummy branch 2 times, most recently from c772bc2 to a3749da Mar 1, 2019

@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Mar 1, 2019

@bors-servo r=Manishearth,ferjm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 1, 2019

📌 Commit a3749da has been approved by Manishearth,ferjm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 1, 2019

⌛️ Testing commit a3749da with merge 3db7227...

bors-servo added a commit that referenced this pull request Mar 1, 2019

Auto merge of #209 - jdm:dummy, r=Manishearth,ferjm
Invert dependency graph for backends

This change has several benefits:
* it makes the backend selection explicit by the user of this library (which ensures that it's easy to test that the dummy backend compiles)
* it moves the platform-based backend selection into its own optional crate and reduces duplication
* it makes the backends depend on servo-media, rather than the reverse, which improves build times for Servo when the public media APIs are not modified

The downside is there are more trait objects instead of static types. My suspicion is that the performance hit of using virtual methods here is dwarfed by the other work being done, but I have not attempted to measure this at all.
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 1, 2019

💔 Test failed - checks-travis

@jdm jdm force-pushed the jdm:dummy branch from a3749da to 31d3ea1 Mar 1, 2019

@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Mar 1, 2019

@bors-servo r=Manishearth,ferjm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 1, 2019

📌 Commit 31d3ea1 has been approved by Manishearth,ferjm

bors-servo added a commit that referenced this pull request Mar 1, 2019

Auto merge of #209 - jdm:dummy, r=Manishearth,ferjm
Invert dependency graph for backends

This change has several benefits:
* it makes the backend selection explicit by the user of this library (which ensures that it's easy to test that the dummy backend compiles)
* it moves the platform-based backend selection into its own optional crate and reduces duplication
* it makes the backends depend on servo-media, rather than the reverse, which improves build times for Servo when the public media APIs are not modified

The downside is there are more trait objects instead of static types. My suspicion is that the performance hit of using virtual methods here is dwarfed by the other work being done, but I have not attempted to measure this at all.

Fixes #194.
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 1, 2019

⌛️ Testing commit 31d3ea1 with merge 18a5117...

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 1, 2019

💔 Test failed - checks-travis

@jdm jdm force-pushed the jdm:dummy branch from 31d3ea1 to a94a3b5 Mar 1, 2019

@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Mar 1, 2019

@bors-servo r=Manisheart,ferjm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 1, 2019

📌 Commit a94a3b5 has been approved by Manisheart,ferjm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 1, 2019

⌛️ Testing commit a94a3b5 with merge 029475e...

bors-servo added a commit that referenced this pull request Mar 1, 2019

Auto merge of #209 - jdm:dummy, r=Manisheart,ferjm
Invert dependency graph for backends

This change has several benefits:
* it makes the backend selection explicit by the user of this library (which ensures that it's easy to test that the dummy backend compiles)
* it moves the platform-based backend selection into its own optional crate and reduces duplication
* it makes the backends depend on servo-media, rather than the reverse, which improves build times for Servo when the public media APIs are not modified

The downside is there are more trait objects instead of static types. My suspicion is that the performance hit of using virtual methods here is dwarfed by the other work being done, but I have not attempted to measure this at all.

Fixes #194.
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Mar 1, 2019

☀️ Test successful - checks-travis
Approved by: Manisheart,ferjm
Pushing 029475e to master...

@bors-servo bors-servo merged commit a94a3b5 into servo:master Mar 1, 2019

1 check passed

homu Test successful
Details

@bors-servo bors-servo referenced this pull request Mar 1, 2019

Merged

Fix sink pad ordering #213

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.