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

Improve player #823

Merged
merged 9 commits into from
Sep 20, 2021
Merged

Improve player #823

merged 9 commits into from
Sep 20, 2021

Conversation

JasonLG1979
Copy link
Contributor

@JasonLG1979 JasonLG1979 commented Jul 21, 2021

  • Make the decoders and player use the same math for converting between samples and milliseconds.

  • Remove unwraps, expects and panics from player, decoders and mod.

  • Reduce redundant math.

  • Simplify decoder errors with thiserror.

Make the decoders and player use the same math for converting between samples and milliseconds.
And a small change to lewton_decoder
Make decoder seek in pcm not ms. Everytime we call decoder.seek we also need position_pcm. We can skip some redundant math and just call seek with position_pcm.
@JasonLG1979
Copy link
Contributor Author

@roderickvd as you might have noticed, this commit pretty much goes hand in hand with #820. The 2 combined go a long way to better playback error handling. It really doesn't matter which is merged 1st. Either way I'll have to fix some conflicts.

And remove unwraps from the decoders and panics from mod
@roderickvd
Copy link
Member

Thanks for your work mate ❤️ as I've recently returned from holiday, I'll start my review somewhere the coming days.

@roderickvd roderickvd mentioned this pull request Aug 7, 2021
@JasonLG1979
Copy link
Contributor Author

Thanks for your work mate as I've recently returned from holiday, I'll start my review somewhere the coming days.

It's all good. I hope you had fun and are well rested.

@JasonLG1979
Copy link
Contributor Author

Hey @roderickvd not to press or anything but have you had a chance to look at any of this?

@roderickvd
Copy link
Member

You're quite right, you put effort into this and deserve no less than proper feedback and handling of this PR! Unfortunately though at this time "life is happening" over at my place. Rest assured I will be handling this but please give me some time to "get back at it again".

In the meantime other contributors and maintainers are warmly invited to review.

@JasonLG1979
Copy link
Contributor Author

You're quite right, you put effort into this and deserve no less than proper feedback and handling of this PR! Unfortunately though at this time "life is happening" over at my place.

Well whatever is going on I hope it's either a good thing or at least not horrible?

Rest assured I will be handling this but please give me some time to "get back at it again".

OK. No problem.

Copy link
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.

Here's my initial code review. Couple of questions if you could think along?

playback/src/audio_backend/jackaudio.rs Show resolved Hide resolved
playback/src/decoder/mod.rs Show resolved Hide resolved
playback/src/decoder/mod.rs Outdated Show resolved Hide resolved
playback/src/decoder/passthrough_decoder.rs Outdated Show resolved Hide resolved
playback/src/lib.rs Outdated Show resolved Hide resolved
playback/src/player.rs Show resolved Hide resolved
playback/src/player.rs Outdated Show resolved Hide resolved
@roderickvd
Copy link
Member

Hi @JasonLG1979 I think there's a few minor review points remaining, when you have time to address those, we can get this in and start rolling a whopper of a 0.3.0 release!

@JasonLG1979
Copy link
Contributor Author

@roderickvd I think that about covers it.

@JasonLG1979
Copy link
Contributor Author

Oh, geee, what a surprise Error: Process completed with exit code 101... LMFAO!!!

@roderickvd
Copy link
Member

Looking good, thanks mate. Do you want me to squash merge it, or do you want to squash and force push yourself? I'm fine with either, the latter offers a bit more attribution.

Oh, geee, what a surprise Error: Process completed with exit code 101... LMFAO!!!

Yeah we'll have to tackle that sometime soon. I haven't dug around deep enough, but I even think it's caused by one of the transitive dependencies? Anyway it doesn't stand in the way for this or any other commits as far as I'm concerned.

@roderickvd
Copy link
Member

On second though, I'm going ahead with this now. I see GitHub provides proper attribution either way. 👍

@roderickvd roderickvd merged commit 89577d1 into librespot-org:dev Sep 20, 2021
@roderickvd
Copy link
Member

57937a1 fixed the 101 exit codes.

@JasonLG1979 JasonLG1979 deleted the improve-player branch October 1, 2021 14:59
@roderickvd roderickvd linked an issue Nov 17, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic does not exit
3 participants