Use block/chunk abstraction for processing; add gainnode #17

Manishearth · 2018-05-22T21:40:53Z

No description provided.


        Use correct block size


        Add block and chunk abstraction


        Rewrite to use chunks


        Remove hashmap, add GainNode

Manishearth · 2018-05-22T21:59:13Z

I replaced the hashmap with a vec because there's no guaranteed order for hashmap iteration.

When we add graph support we'll replace it with a petgraph graph.

sdroege · 2018-05-23T06:10:15Z

src/backends/gstreamer/audio_sink.rs

+                debug_assert!(chunks.len() == 1);
+                let data = &mut chunks.blocks[0].data;
+                let data = data.as_mut_byte_slice().expect("casting failed");
+                buffer.copy_from_slice(0, data).expect("copying failed");


This looks like a potentially unneeded copy. You can wrap a buffer around any AsRef<[u8]> or AsMut<[u8]> if that helps anything here

How would I do that?

Currently there is only minimal support for custom memory types in the bindings (see here), but it's probably sufficient for what you need. From a mutable slice and from an immutable slice

Ah. Due to the way rustdoc deals with typedefs those functions don't show up in the docs where you'd expect 😄

Yeah, personally I'm not using rustdoc because of that for anything built on top of gtk-rs. Easier to navigate the sources :)

Okay so it looks like I'd need to convert between Box<[u8]> and Box<[f32]> here (and in future versions, between vecs), and byte-slice-cast doesn't support that yet. Filing a bug at sdroege/byte-slice-cast#1 . It's not hard to write but I don't want to do one-off unsafe code in servo-media for it.

Manishearth · 2018-05-23T06:43:27Z

Do you have a list of features that would make rustdoc pleasant here? I can try and implement them on the side, or mentor others to do so. Not urgent, but worth fixing anyway. The typedef one has been a constant paper cut for me in multiple codebases. On May 22, 2018 11:39 PM, "Sebastian Dröge" <notifications@github.com> wrote: *@sdroege* commented on this pull request. ------------------------------ In src/backends/gstreamer/audio_sink.rs <#17 (comment)>:

@@ -87,9 +61,13 @@ impl AudioSink for GStreamerAudioSink {

.into(); buffer.set_pts(pts); buffer.set_duration(next_pts - pts); - let mut map = buffer.map_writable().unwrap(); - let data = map.as_mut_slice(); - graph_.process(data, rate); + + let mut chunks = graph_.process(rate); + debug_assert!(chunks.len() == 1); + let data = &mut chunks.blocks[0].data; + let data = data.as_mut_byte_slice().expect("casting failed"); + buffer.copy_from_slice(0, data).expect("copying failed"); Yeah, personally I'm not using rustdoc because of that for anything built on top of gtk-rs. Easier to navigate the sources :) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#17 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABivSNjaCrdul_IiuoXU7T6dmm4Hccsvks5t1QQXgaJpZM4UJb8Q> .

sdroege · 2018-05-23T06:55:38Z

Do you have a list of features that would make rustdoc pleasant here?

The typedef one is the most annoying one, I'll try remembering others. Guillaume Gomez probably remembers some more too :) It improved quite a bit over the last months though.

Manishearth · 2018-05-23T14:14:27Z

e? @ferjm

ferjm

Looks great and ready to be merged after addressing @sdroege feedback. Thanks!

ferjm · 2018-05-23T14:25:33Z

Cargo.toml

+
+[dependencies.num-traits]
+optional = true
+version = "0.1"


Note that I removed some of these dependencies in #16

ferjm · 2018-05-23T14:25:33Z

examples/play.rs

 use std::{thread, time};

 fn main() {
    if let Ok(servo_media) = ServoMedia::get() {
        let mut graph = servo_media.create_audio_graph().unwrap();
        graph.create_node(AudioNodeType::OscillatorNode(Default::default()));
+        let mut options = GainNodeOptions::default();
+        options.gain = 0.5;
+        graph.create_node(AudioNodeType::GainNode(options));


\o/ glad to see more nodes

ferjm · 2018-05-23T14:25:33Z

src/audio/graph_thread.rs

 use std::sync::mpsc::Receiver;
 use std::sync::Arc;

 #[cfg(feature = "gst")]
 use backends::gstreamer::audio_sink::GStreamerAudioSink;

 pub enum AudioGraphThreadMsg {
-    CreateNode(usize, AudioNodeType),
+    CreateNode(AudioNodeType),
    ResumeProcessing,
    PauseProcessing,
 }

 pub struct AudioGraphThread {
    // XXX Test with a hash map for now. This should end up


Not a big deal, but the comment needs to be removed or updated.

ferjm · 2018-05-23T14:25:33Z

src/audio/graph_thread.rs

+            AudioNodeType::GainNode(options) => {
+                let node = Box::new(GainNode::new(options));
+                let mut nodes = self.nodes.borrow_mut();
+                nodes.push(node);
            }
            _ => unimplemented!(),
        }


nit:

let node: Box<AudioNodeEngine> = match node_type { AudioNodeType::OscillatorNode(options) => Box::new(OscillatorNode::new(options)), AudioNodeType::GainNode(options) => Box::new(GainNode::new(options)), _ => unimplemented()!, }; let mut nodes = self.nodes.borrow_mut(); nodes.push(node)

ferjm · 2018-05-23T15:21:15Z

src/audio/graph_thread.rs

    ResumeProcessing,
    PauseProcessing,
 }

 pub struct AudioGraphThread {
    // XXX Test with a hash map for now. This should end up
    // being a graph, like https://docs.rs/petgraph/0.4.12/petgraph/.
-    nodes: RefCell<HashMap<usize, Box<AudioNodeEngine>>>,
+    nodes: RefCell<Vec<Box<AudioNodeEngine>>>,


Sorry, I forgot to mention this. Can we also leave the HashMap until we use something like petgraph.GraphMap, please? We need it to implement node methods like #21.

We can't, hashmaps don't have ordered iteration. I was getting nondeterministic panics because sometimes the gain node came before the oscillator.

For node methods I was thinking we'd just use indices for now. The caller knows what the indices will be.

I meant leaving both, the HashMap and the Vec. Or maybe using something like linked_hash_map. But I guess Vec indices also works for now.

Yeah I don't want to tune up the abstraction here too much since we'll replace it soon anyway.


        mention buffer::from_slice


        review fixes


        Remove some unused dependencies

Manishearth changed the title ~~Use block/chunk abstraction for processing~~ Use block/chunk abstraction for processing; add gainnode May 22, 2018

Manishearth added 4 commits May 22, 2018

Use correct block size

852e785

Add block and chunk abstraction

6b09210

Rewrite to use chunks

94899e3

Remove hashmap, add GainNode

b19048f

Manishearth force-pushed the blocks branch from c5c6cc4 to b19048f May 22, 2018

Manishearth mentioned this pull request May 22, 2018

Channel support #18

Closed

sdroege reviewed May 23, 2018

View changes

ferjm approved these changes May 23, 2018

View changes

ferjm reviewed May 23, 2018

View changes

Manishearth added 3 commits May 23, 2018

mention buffer::from_slice

1d22545

review fixes

3aa86a2

Remove some unused dependencies

fc4ef84

Manishearth merged commit 3c486eb into master May 23, 2018

Manishearth mentioned this pull request May 23, 2018

Remove unused extern crates #16

Closed

Manishearth deleted the blocks branch May 23, 2018

Manishearth mentioned this pull request May 23, 2018

Play silence by default #15

Merged

servo / media

Use block/chunk abstraction for processing; add gainnode #17

Use block/chunk abstraction for processing; add gainnode #17

Manishearth commented May 22, 2018

Manishearth commented May 22, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented May 23, 2018

sdroege commented May 23, 2018

Manishearth commented May 23, 2018

ferjm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

servo / media

Join GitHub today

Use block/chunk abstraction for processing; add gainnode #17

Use block/chunk abstraction for processing; add gainnode #17

Conversation

Manishearth commented May 22, 2018

Manishearth commented May 22, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented May 23, 2018

sdroege commented May 23, 2018

Manishearth commented May 23, 2018

ferjm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ferjm May 23, 2018 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ferjm May 23, 2018 • edited

This comment has been minimized.

ferjm May 23, 2018 •

edited

ferjm May 23, 2018 •

edited