Skip to content

Better errors in PulseAudio backend#801

Merged
roderickvd merged 10 commits intolibrespot-org:devfrom
JasonLG1979:fix_pulseaudio_backend
Jun 30, 2021
Merged

Better errors in PulseAudio backend#801
roderickvd merged 10 commits intolibrespot-org:devfrom
JasonLG1979:fix_pulseaudio_backend

Conversation

@JasonLG1979
Copy link
Copy Markdown
Contributor

@JasonLG1979 JasonLG1979 commented Jun 18, 2021

  • Use F32 if a user requests F64 (F64 is not supported by PulseAudio)
  • Move all code that can fail to start where errors can be returned to prevent a panic!
  • Remove unneeded unwraps.

Combined with the ability to gracefully exit on error player in #797 this should about do it for for the whole "never panic!" idea as far as the PulseAudio backend is concerned.

@JasonLG1979
Copy link
Copy Markdown
Contributor Author

JasonLG1979 commented Jun 18, 2021

@roderickvd In future PR's I can do about the same and have open contain no code that can actually fail and just use it to set the fields thereby avoiding having to refactor to make open be able to return an error?

@roderickvd roderickvd self-assigned this Jun 18, 2021
roderickvd
roderickvd reviewed Jun 18, 2021
View reviewed changes
Comment thread CHANGELOG.md Outdated
roderickvd
roderickvd requested changes Jun 18, 2021
View reviewed changes
Comment thread playback/src/audio_backend/pulseaudio.rs Outdated
Comment thread playback/src/audio_backend/pulseaudio.rs
@roderickvd
Copy link
Copy Markdown
Member

roderickvd commented Jun 18, 2021

@roderickvd In future PR's I can do about the same and have open contain no code that can actually fail and just use it to set the fields thereby avoiding having to refactor to make open be able to return an error?

As a a quick win, yes, because it's an improvement over what it is, and open can't return an Error today.

For future work, I would prefer that open would be able to fail, following what the sound system API is doing. For example, when possible we should get hold of the device in open or fail when the requested device doesn't exist or cannot be accessed. Then in start we can open the stream.

For some API's this is a two-in-one operation, but when it can be split, it's better that way because in the meantime we can allow other players to take over (e.g. in Volumio and moOde where librespot is always running but users may switch to mpd or shairpoint-sync playback and back again).

@JasonLG1979 JasonLG1979 requested a review from roderickvd June 18, 2021 10:20
roderickvd
roderickvd requested changes Jun 18, 2021
View reviewed changes
Comment thread playback/src/audio_backend/pulseaudio.rs
Comment thread playback/src/audio_backend/pulseaudio.rs Outdated
Comment thread playback/src/audio_backend/pulseaudio.rs Outdated
@JasonLG1979
Fix PulseAudio backend
6d373c1 
* Use F32 if a user requests F64 (F64 is not supported by PulseAudio)
* Move all code that can fail to `start` where errors can be returned to prevent a panic!
* Remove unneeded unwraps.
@JasonLG1979
Merge branch 'librespot-org:dev' into fix_pulseaudio_backend
79c4db7
@roderickvd roderickvd changed the title Fix PulseAudio backend Better errors in PulseAudio backend Jun 19, 2021
@JasonLG1979
Various fixes addressing review comments and use drain in stop
8c37afe 
* Move the pulse::Sample::Format logic down to start.
* More idiomatic Rust.
* More meaningful error messages.
* Use drain in stop to drain the buffer in stop.
@JasonLG1979
Merge branch 'librespot-org:dev' into fix_pulseaudio_backend
86f5bb2
@JasonLG1979
update change log
c9d912f
@JasonLG1979
Copy link
Copy Markdown
Contributor Author

JasonLG1979 commented Jun 20, 2021

Oops. I forgot about the change log in my CI PR,lol!!!

@JasonLG1979
Copy link
Copy Markdown
Contributor Author

JasonLG1979 commented Jun 20, 2021

Weird. It shouldn't trigger the CI since it's a **.md?

@JasonLG1979
Copy link
Copy Markdown
Contributor Author

JasonLG1979 commented Jun 20, 2021 •
edited
Loading

I think that covers your code reviews?

@roderickvd
Copy link
Copy Markdown
Member

roderickvd commented Jun 21, 2021

Yes I think so. Pending discussion on the amount and detail of the error types over at #808.

@JasonLG1979
Copy link
Copy Markdown
Contributor Author

JasonLG1979 commented Jun 21, 2021

Yes I think so. Pending discussion on the amount and detail of the error types over at #808.

What's to discuss?

@roderickvd
Copy link
Copy Markdown
Member

roderickvd commented Jun 21, 2021

Why did you close this PR? I did not say "no" only that I also saw a couple of different error types, of which I was wondering whether they shouldn't be merged to more generic ones.

@JasonLG1979
Copy link
Copy Markdown
Contributor Author

JasonLG1979 commented Jun 21, 2021 •
edited
Loading

Why did you close this PR? I did not say "no" only that I also saw a couple of different error types, of which I was wondering whether they shouldn't be merged to more generic ones.

Sorry wrong PR I meant to close the Alsa PR. I'm not a fan of generic errors. There aren't that many in this PR. I want for myself and other's to know exactly where the errors are being triggered in the code base. Generic errors are useless. We might as well just not have errors and just wrap the binding's error strings unchanged in a io::Error if we're just going to wrap them in a couple generic errors of our own.

@JasonLG1979 JasonLG1979 reopened this Jun 21, 2021
@roderickvd
Copy link
Copy Markdown
Member

roderickvd commented Jun 22, 2021

I can see both sides. What do others think?

@JasonLG1979
Copy link
Copy Markdown
Contributor Author

JasonLG1979 commented Jun 25, 2021

Ok, I closed #808. Are there any specific changes you'd like me to make to get this merged?

@roderickvd
Copy link
Copy Markdown
Member

roderickvd commented Jun 25, 2021

Since no one else has chimed in, I can only give my opinion on both this one and #808:

  1. I'd like there to be parity between them, i.e. if there's a DrainFailure in one, and the other can have one too, it should be there.
  2. I'd like to find a middle ground between meaningless generic errors, and having too much error types. Moving it all one level up, ParsingName and ParsingDesc is a ParsingError. PcmNoneOnStop and PcmNoneOnWrite is a PcmNoneError. NotConnectedOnStop and NotConnectedOnWrite is a NotConnectedError. And so on. From the context from which the call is made, it can already be inferred where that error occurred, it's not necessary to be explicit in the error type.

@JasonLG1979
Copy link
Copy Markdown
Contributor Author

JasonLG1979 commented Jun 25, 2021

  • I'd like there to be parity between them, i.e. if there's a DrainFailure in one, and the other can have one too, it should be there.
  • I'd like to find a middle ground between meaningless generic errors, and having too much error types. Moving it all one level up, ParsingName and ParsingDesc is a ParsingError. PcmNoneOnStop and PcmNoneOnWrite is a PcmNoneError. NotConnectedOnStop and NotConnectedOnWrite is a NotConnectedError. And so on. From the context from which the call is made, it can already be inferred where that error occurred, it's not necessary to be explicit in the error type.

I can do that.

@JasonLG1979
Copy link
Copy Markdown
Contributor Author

JasonLG1979 commented Jun 25, 2021 •
edited
Loading

I'd rather rework this one and after/if you merge #811 I can redo and put in a new PR for errors in the ALSA backend to make them match. As rebasing the 1st PR would be a PITA if/after you merged #811.

@JasonLG1979
Copy link
Copy Markdown
Contributor Author

JasonLG1979 commented Jun 25, 2021 •
edited
Loading

For PulseAudio PcmNoneError could/would equal ServerNoneError But you also can't end errors with Error or clippy with throw a fit because it's redundant so it would actually be PcmNone and ServerNone. The parsing errors would just be Parsing since it would be AlsaError::Parsing.

@JasonLG1979
Copy link
Copy Markdown
Contributor Author

JasonLG1979 commented Jun 26, 2021

There. When/if you merge this and maybe #811 I will put in a new PR to bring parity as much as possible to AlsaSink.

@JasonLG1979
Copy link
Copy Markdown
Contributor Author

JasonLG1979 commented Jun 27, 2021

@roderickvd I did what you asked. I think this is done?

roderickvd
roderickvd requested changes Jun 28, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@roderickvd roderickvd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was occupied over the weekend. Final points here.

Comment thread playback/src/audio_backend/pulseaudio.rs Outdated
Comment thread playback/src/audio_backend/pulseaudio.rs Outdated
Comment thread playback/src/audio_backend/pulseaudio.rs Outdated
@roderickvd roderickvd merged commit 9ff3398 into librespot-org:dev Jun 30, 2021
@JasonLG1979 JasonLG1979 deleted the fix_pulseaudio_backend branch June 30, 2021 22:05
paulfariello pushed a commit to paulfariello/librespot that referenced this pull request Sep 23, 2025
@JasonLG1979
Better errors in PulseAudio backend (librespot-org#801)
9f8e52a 
* More meaningful error messages
* Use F32 if a user requests F64 (F64 is not supported by PulseAudio)
* Move all code that can fail to `start` where errors can be returned to prevent panics
* Use drain in `stop`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roderickvd roderickvd roderickvd approved these changes

Assignees

@roderickvd roderickvd

Labels

audio

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@JasonLG1979 @roderickvd