Volume, play/pause, and fix sink dropping #83

Xaeroxe · 2017-01-23T19:58:30Z

Fixed the set_volume function of a Sink and adds a get_volume function.

tomaka · 2017-01-23T21:21:09Z

src/engine.rs

+    pending_sounds: Mutex<Vec<HandleHandle>>,
+}
+
+// Handle to a handle I guess.


Same two remarks as #84

Ok, there is no such thing as an AtomicF32 though so I assume the volume level should still be a Mutex

Xaeroxe · 2017-01-23T23:18:17Z

The other two have been merged into this request and the requested changes have been made.

Xaeroxe · 2017-01-24T00:05:40Z

@tomaka Ready for review.

tomaka · 2017-01-24T07:41:27Z

Hmmm, unfortunately I'm not a fan of the fact that we lock the volume's mutex for each sample.
But properly handling this requires a lot of work.

tomaka · 2017-01-24T07:44:51Z

Please load the volume in a temporary variable outside of the iterator (for example between lines 126 and 127) so that we only lock the mutex once from time to time. That should be okay-ish I think.

tomaka · 2017-01-24T07:45:14Z

src/lib.rs

+    /// The value `1.0` is the "normal" volume (unfiltered input). Any value other than 1.0 will
+    /// multiply each sample by this value.
+    #[inline]
+    pub fn get_volume(&self) -> f32 {


Please rename this to just volume().

tomaka · 2017-01-24T07:45:23Z

src/engine.rs

@@ -206,18 +250,42 @@ impl Handle {
        self.next_sounds.lock().unwrap().push((source, Some(tx)));
    }

+    /// Gets the volume of the sound played by this sink.
+    #[inline]
+    pub fn get_volume(&self) -> f32 {


tomaka · 2017-01-24T07:46:02Z

Sorry, I didn't review your volume PR carefully. This time it should be good after these changes.

Xaeroxe · 2017-01-24T16:27:11Z

Ok, so I agree it is not desirable for the sampling thread to block, locking can lead to blocking. However I do want the volume to be fetched for every sample as that makes the set_volume function feel the most responsive. It's not possible to make the f32 atomic. So the approach I'm currently thinking of would use a channel, we'd store the Sender in the Handle and the Receiver in the HandleHandle, then whenever the Handle wishes to change the volume level it will send an f32 through the channel, HandleHandle would store an f32 called volume and in the sample iterator (lines 128-133) we would call try_recv to fetch any new volume level sent by Handle, and store it in HandleHandle. This will not be contained in a while loop, it will only attempt to receive one volume level per sample, which prevents the sampling thread from being DDOSed by a large influx of volume change requests.

I'll create and push an implementation of this on my lunch break, let me know if you find any fault in it.

tomaka · 2017-01-24T16:48:09Z

I don't think it's better to use a channel.

The iterator that builds the samples is a code that is very easy to optimize for the compiler. The fact that we're using the Relaxed ordering means that in theory it can fetch the value of "paused" only from time to time and build the samples through SIMD for example.

If you're introducing a channel read or a mutex in there, you're not only paying the price of the channel or mutex read, but by introducing a memory fence in the loop you're also totally killing any potential for a compiler optimisation on this code.

In fact I have already doubts about the fact that paused doesn't kill all optimisations already, but I thought it would be "good enough" while waiting for a more robust rewrite of the engine's code.

EDIT: When it comes to being responsive, I don't think you'd notice a 5 milliseconds lag for example (since AFAIK that's more or less the best possible value for real-time audio processing right now), and by fetching the volume only every 5 milliseconds for example you'd avoid between 2200 and 4400 mutex locks.

Xaeroxe · 2017-01-24T17:16:35Z

I have a difficult time imagining how the engine could have the features in this request in a more optimized manner, unless we were to make changes to cpal. That being said, currently on line 126 of engine.rs I am dropping the sound if it was declared dead, and that seems to be responsive enough in my own testing, so on second thought I'm not opposed to loading temporary volume variables here.

tomaka · 2017-01-24T17:26:16Z

I have a difficult time imagining how the engine could have the features in this request in a more optimized manner, unless we were to make changes to cpal.

You could put the Arc<Mutex<f32>> of the volume in the source of samples itself, alongside with a local copy of the volume. The source of samples would then load the value from the mutex to its local copy from time to time.

Xaeroxe · 2017-01-24T17:37:49Z

Ok, that makes sense. I'd like to see if I can accomplish that, should "from time to time" just be approximately every 5ms?

Xaeroxe · 2017-01-24T17:49:55Z

Some context: I'm using rodio in a game engine project and I want to give users of my engine more control over the playback of their sounds, those features that I need to do that are in this PR. I don't have anywhere near the same level of experience manipulating audio devices as you do but I gave it my best attempt. I'm trying to avoid having an audio engine that is just "good enough for now" I'd like it to be at this ideal performance level you describe. So I'm willing to tackle the idea of finding a good place and way to do more infrequent fetching of the variables into a local copy.

Xaeroxe · 2017-01-24T18:04:37Z

Alternatively if you want to do a rewrite of the engine I could just add my temporary variable after line 126 as you originally suggested and just be done with it.

tomaka · 2017-01-24T18:13:15Z

I'd like to rewrite some parts of the engine but I don't know if it's going to happen in the next month or in the next year.

Basically the ideal solution would be to add a filter (similar to the ones in src/source) whose constructor takes a source and an Arc<Mutex<f32>>.
The filter would calculate approximately the number of samples that are equal to 5 ms, and re-read from the mutex once that number of samples has been processed.
The actual duration of 5ms could perhaps be customized with another constructor.

Then the engine could wrap all sound sources passed to it around that filter.

Xaeroxe · 2017-01-24T18:25:30Z

Ok, do you also feel we should be storing local copies for the AtomicBools in this filter or is the current implementation sufficient for those?

tomaka · 2017-01-24T18:26:53Z

@Xaeroxe23 Yeah I think that would be better.
Once you have written one of the filters, adding the others should be very easy after some copy-pasting.

Xaeroxe · 2017-01-24T21:05:14Z

Resolved in another PR, closing.

Xaeroxe added 4 commits January 23, 2017 12:23

Introducing HandleHandle struct for future changes

7037f67

Fix volume setting and add get_volume

09ee5dd

Fix Sink drop implementation

143fea2

Enhance Sink with pausing and resuming

755a5ea

tomaka requested changes Jan 23, 2017

View reviewed changes

Xaeroxe added 2 commits January 23, 2017 16:11

Merge pause and volume

d47adc7

Merge in sink drop

96aa0b6

Make requested changes

cc6364d

Xaeroxe changed the title ~~Volume~~ Volume, play/pause, and fix sink dropping Jan 23, 2017

tomaka approved these changes Jan 24, 2017

View reviewed changes

tomaka requested changes Jan 24, 2017

View reviewed changes

Xaeroxe mentioned this pull request Jan 24, 2017

Added implementation as source filters #85

Merged

Xaeroxe closed this Jan 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Volume, play/pause, and fix sink dropping #83

Volume, play/pause, and fix sink dropping #83

Xaeroxe commented Jan 23, 2017

tomaka Jan 23, 2017

Xaeroxe Jan 23, 2017

Xaeroxe commented Jan 23, 2017

Xaeroxe commented Jan 24, 2017

tomaka commented Jan 24, 2017

tomaka commented Jan 24, 2017

tomaka Jan 24, 2017

tomaka Jan 24, 2017

tomaka commented Jan 24, 2017

Xaeroxe commented Jan 24, 2017 •

edited

Loading

tomaka commented Jan 24, 2017 •

edited

Loading

Xaeroxe commented Jan 24, 2017

tomaka commented Jan 24, 2017

Xaeroxe commented Jan 24, 2017

Xaeroxe commented Jan 24, 2017

Xaeroxe commented Jan 24, 2017

tomaka commented Jan 24, 2017

Xaeroxe commented Jan 24, 2017 •

edited

Loading

tomaka commented Jan 24, 2017

Xaeroxe commented Jan 24, 2017

Volume, play/pause, and fix sink dropping #83

Volume, play/pause, and fix sink dropping #83

Conversation

Xaeroxe commented Jan 23, 2017

tomaka Jan 23, 2017

Choose a reason for hiding this comment

Xaeroxe Jan 23, 2017

Choose a reason for hiding this comment

Xaeroxe commented Jan 23, 2017

Xaeroxe commented Jan 24, 2017

tomaka commented Jan 24, 2017

tomaka commented Jan 24, 2017

tomaka Jan 24, 2017

Choose a reason for hiding this comment

tomaka Jan 24, 2017

Choose a reason for hiding this comment

tomaka commented Jan 24, 2017

Xaeroxe commented Jan 24, 2017 • edited Loading

tomaka commented Jan 24, 2017 • edited Loading

Xaeroxe commented Jan 24, 2017

tomaka commented Jan 24, 2017

Xaeroxe commented Jan 24, 2017

Xaeroxe commented Jan 24, 2017

Xaeroxe commented Jan 24, 2017

tomaka commented Jan 24, 2017

Xaeroxe commented Jan 24, 2017 • edited Loading

tomaka commented Jan 24, 2017

Xaeroxe commented Jan 24, 2017

Xaeroxe commented Jan 24, 2017 •

edited

Loading

tomaka commented Jan 24, 2017 •

edited

Loading

Xaeroxe commented Jan 24, 2017 •

edited

Loading