Refactored to have apply_volume in a specifix mixer by romerod · Pull Request #148 · plietar/librespot

romerod · 2017-01-20T19:54:27Z

This is still work in progress, are you interested in having multiple mixer support?

I started by refactoring the code to have a Mixer trait which is passed to Player.

I verified the code on rpi2 and it works, i'm a complete rust newb so don't hang me for weird code.

TODO:

call init(), inuse() and release on the mixer
add possibility for the mixer to notify the player that the volume changed.
add mixer implementation which allows external process to be responsible for mixing (any ideas for IPC?)

plietar · 2017-01-20T21:59:33Z

Thanks for working on this, this is an often requested feature.

I don't think the API as it is now would allow for hardware mixers. The apply_volume doesn't make any sense in that case.

The best is probably to merge AudioBackend and Mixer. They are closely linked. AudioBackend's which don't support hardware mixing can just fallback to software as it is now.

romerod · 2017-01-20T22:47:44Z

I know that apply_volume doesn't make sense in any but the SoftwareMixer case, that's way the default implementation just returns the unaltered data.

I don't like the idea of putting the audio backend and the mixer together. I've the same use case as @herrernst in #43 where volume will be controlled on an amplifier which I'll remote control with CEC.

Having them separated makes it possible to run different mixers with different backend.

Having the possibility to delegate the "mixing" to an external process would make it possible to use amixer for example without the need to manually implement that.

plietar · 2017-01-22T16:23:19Z

Right, I hadn't thought of that use case and it makes more sense now. Thanks for clarifying. I think the API is reasonable like that.

It may make sense the split the Mixer trait into a VolumeControl and a VolumeApplier (terrible names), which share an Arc<AtomicUsize>. The former is owned by SpircManager and the later by the Player. This way the player does not have to worry about volume control, and this can be left to the SpircManager.

add possibility for the mixer to notify the player that the volume changed.

add mixer implementation which allows external process to be responsible for mixing (any ideas for IPC?)

I think this should be left for other PRs. I also have a refactor coming up which would impact the way these are done.

I don't have a candidate for IPC. Is there a precedent for volume control ? Is writing the volume to the process' stdin enough ?

plietar · 2017-01-22T16:14:04Z

src/mixer/mod.rs

Call that set_volume

plietar · 2017-01-22T16:15:10Z

src/mixer/mod.rs

Is this necessary ? Initialisation can happen when constructing the Mixer object

plietar · 2017-01-22T16:15:54Z

src/mixer/mod.rs

start/stop seem like better names

romerod · 2017-01-26T11:42:16Z

I've started splitting it up a bit differently. I introduced the StreamEditor trait which can be optionally passed to the Player. The current volume is owned by the Mixer. Are you ok with this? Currently the state of the device isn't read anymore by the SpircManager [WIP].

plietar

Overall looks good, couple of nits.
AudioFilter sounds like a better name than StreamEditor. Maybe one day we can have other type of filters.

I'm confused about the changes in src/spirc.rs and why they're needed, beyond just adding a mixer: Box<Mixer + Send> to the SpircManager and calling it when appropriate.

Finally, a device can disable volume changing (probably by not advertising the kVolumeSteps in spirc). This could be done if none is used as a mixer.
That can be left for another PR though.

plietar · 2017-01-26T14:09:51Z

src/main.rs

mixer::find should take an Option<&str>, and take care of choosing a default rather than picking it here, similar to how the audio backend is done.

plietar · 2017-01-26T14:10:26Z

src/main.rs

Use .expect("Invalid mixer") so it is more obvious in case of an error

plietar · 2017-01-26T14:10:48Z

src/main.rs

Move mixer.get_stream_editor() to a variable

plietar · 2017-01-26T14:11:49Z

src/mixer/mod.rs

Brace should go at the end of the line.
If the line is too long, use

fn get_stream_editor(&self) -> Option<Box<StreamEditor + Send>> {

plietar · 2017-01-26T14:12:01Z

src/mixer/mod.rs

Why is init needed ?

plietar · 2017-01-26T14:16:16Z

src/mixer/softmixer.rs

Just store the Arc<RwLock<u16>> in there, no need for a closure

There is no reason to allow the SoftVolumeApplier to modify the value, i was looking to be able to get a ReadLock from the RwLock but didn't find one. That's why i decided to use the closure

The privacy boundary is usually the module. As long as it isn't possible to modify the volume unexpectedly from outside the module, that's fine.
I find the closure a lot more confusing than storing the Arc<RwLock<u16>>

plietar · 2017-01-26T14:16:24Z

src/mixer/softmixer.rs

That line is too long

plietar · 2017-01-26T14:18:57Z

src/player.rs

Why did you remove this ?

It's tied to the changes i made in spirc, there is a new struct "State" which contains the whole system state (Player and Mixer) so I thought the update_time belongs to the State not the PlayerState

plietar · 2017-01-26T14:20:29Z

src/player.rs

This could just be editor.modify_stream(&mut packet.data) to modify in place.
(See my comment above)

plietar · 2017-01-26T14:20:56Z

src/spirc.rs

Why is this needed ?

see my previous answer about why I removed the update_time, I think when I continue with the changes it will become clear

plietar

Why do you need a channel to send volume change updates back to the SpircManager ?
Volume change is only done by spirc anyway, so you can just do self.notify(false, None); when a kMessageTypeVolume message has been received.

There shouldn't be any changes needed to mercury, and it should definitely not depend on the spirc module.
I'm also not sure what the implement_sender macro does and why it's needed.
I'm also still not convinced about why the State struct in player is needed, and why this existing code wasn't sufficient.

Sorry for requesting all these changes, but I found the implementation very confusing and overly complicated.

plietar · 2017-02-02T00:04:53Z

src/player.rs

Hmm, using Some(OK(mut packet)) should be enough to avoid the copy.

I will try that, thanks

romerod · 2017-02-03T09:39:54Z

There are multiple reasons I used a channel to send volume change notifications:

The end goal I still have (not for this PR) is to be able to create a mixer which controls an external amplifier, the external amplifier is controllable by other means too, like the remote control
I didn't want to alter the current way it works. Until now the player was called from the spirc to change the volume, and the player informed the spirc thru the add_observer functionality.
I didn't want to introduce an additional thread to listen to this changes and I like the way you used the channel to communicate between the spirc and mercury. I extended this approach to be able to share the channel between the spirc and multiple senders, with multiple message types. By following this approach it would be possible to get rid of the Arc<Mutex<>> arround the SpircInternal and just keep a reference to a "CommandSender" and pass a "PlayerStateSender" to the player without week pointers. If you want to take a look at how I came up with the macro take a look at https://repl.it/F1PY/9

I know I modified quite a lot of things, but I didn't want to introduce additional threads and weak pointers.

The dependencies between spirc and mercury were less than ideal, I know, i'd suggest something like the last commit, what do you think? It might be cleaner to move the SpircMessage enum back to spirc, but then the macro would need to create a trait which would need to get boxed to be passed arround etc. I thought it is simpler like that.

Don't worry about asking to much changes, the idea is to get something good out of it, right?

The state part is not finished, TODO on line 435 of spirc.

…ge types to one channel

…Sender implemented by macro

…olumeApplier

plietar · 2017-02-03T13:01:44Z

Ok the changes to channel stuff make more sense. It's a shame the select! macro isn't stable.
I have a refactor in progress to use tokio and future-rs which would make this a lot simpler though.

I suggest leaving out the volume updates from this PR, especially since it is not needed for software volume. It can be implemented in a much cleaner way once I'm done with the refactor.

…results in 0

romerod · 2017-02-03T13:38:37Z

Hmm, looked at the select! macro, but I don't think it serializes incoming messages as the solution does so you would still have to use locks, which make it much more difficult to know at what point who has to wait on what. Do you see the possible simplification of passing a PlayerStateSender to the instead of the observable stuff and removing the Arc<Mutex<>>?

romerod · 2017-02-03T16:17:38Z

I removed that part of the code (it hurts a bit ;-) ).

As long as the player is playing changes in the mixer will still be reflected as updates are sent to the server, so I can live with that.

romerod · 2017-02-19T16:37:56Z

Is there something missing?

sashahilton00 · 2017-02-19T23:12:19Z

@plietar pinging incase of forgotten PR.

plietar

Sorry, I thought this was still in WIP.
I still don't understand why you need an extra State type ?

plietar · 2017-02-20T21:03:51Z

src/mixer/mod.rs

+
+pub fn find<T: AsRef<str>>(name: Option<T>) -> Option<Box<Mixer + Send>> {
+  match name {
+    _ => Some(Box::new(SoftMixer::new())),


Only return SoftMixer if soft was given as an argument (or None for default).

plietar · 2017-02-20T21:05:35Z

src/player.rs


 impl Player {
-    pub fn new<F>(session: Session, sink_builder: F) -> Player
+    pub fn new<F>(session: Session, stream_editor: Option<Box<AudioFilter + Send>>, sink_builder: F) -> Player


Rename the variable to audio_filter

plietar · 2017-02-20T21:05:51Z

src/player.rs


 impl PlayerInternal {
-    fn run(self, mut sink: Box<Sink>) {
+    fn run(self, mut sink: Box<Sink>, stream_editor: Option<Box<AudioFilter + Send>>) {


Same, change variable name

plietar · 2017-02-21T21:51:13Z

I've made a few fixes and merged. Thanks @romerod

romerod · 2017-02-22T00:27:14Z

Cool, thank you

core API review changes.

plietar reviewed Jan 22, 2017

View reviewed changes

plietar requested changes Jan 26, 2017

View reviewed changes

plietar requested changes Feb 2, 2017

View reviewed changes

romerod added 16 commits February 3, 2017 13:22

Refactored to have apply_volume in a specifix mixer

347bf05

Remove mixer from Player and add it to SpircManager

59398b3

Split mixer into Mixer and StreamEditor

464e85e

Select mixer dynamically

6df2af0

Use stream_editor in player

636de3f

Changes code review

e547a0c

Add macro to be able to create wrapped senders to send multiple messa…

3791633

…ge types to one channel

Create channel in spirc instead of in mercury and use MercuryResponse…

c8ee086

…Sender implemented by macro

Send update messages from mixer to SpircManager

134239d

SoftMixer: Change volume to AtomicUsize and pass AtomicUsize to SoftV…

2de5d10

…olumeApplier

AudioFilter: Modify fn modify_stream(&self, data: &mut [i16]);

da537b5

Move structs which are send across threads to own module

48a43f4

Avoid copying the data array in the player

2c187eb

Add documentation for channel macro

ec0e81f

SpircManager: Update state from player and mixer before sending info

5ef28bf

Call start and stop on mixer

44467a4

SoftVolumeMixer: move factor in again, otherwise i32 division always …

a7aba5c

…results in 0

Remove code to notify spirc manager from the mixer

10f9da4

romerod changed the title ~~[WIP] Refactored to have apply_volume in a specifix mixer~~ Refactored to have apply_volume in a specifix mixer Feb 3, 2017

plietar mentioned this pull request Feb 9, 2017

Add websockets server to librespot #152

Closed

plietar requested changes Feb 20, 2017

View reviewed changes

plietar merged commit 10f9da4 into plietar:master Feb 21, 2017

plietar added a commit that referenced this pull request Feb 21, 2017

Fixes for PR #148

5974cf7

plietar mentioned this pull request Mar 6, 2017

Volume issues #150

Open

sashahilton00 mentioned this pull request Jan 29, 2018

Volume issues librespot-org/librespot#41

Closed

mjaggard pushed a commit to mjaggard/librespot that referenced this pull request Mar 21, 2018

Merge pull request plietar#148 from awiouy/sisyphus

1de8049

core API review changes.

Comments

Conversation

romerod commented Jan 20, 2017

Uh oh!

plietar commented Jan 20, 2017

Uh oh!

romerod commented Jan 20, 2017

Uh oh!

plietar commented Jan 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romerod commented Jan 26, 2017

Uh oh!

plietar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romerod Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plietar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romerod commented Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plietar commented Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plietar commented Jan 22, 2017 •

edited

Loading

plietar left a comment •

edited

Loading

romerod Feb 1, 2017 •

edited

Loading

romerod commented Feb 3, 2017 •

edited

Loading

plietar commented Feb 3, 2017 •

edited

Loading