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.

Sign up for GitHub

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

Jump to bottom

Add DSP settings info #142

Merged
merged 31 commits into from
Jan 4, 2025
Merged

Add DSP settings info #142

merged 31 commits into from
Jan 4, 2025

Conversation

OzGav
Copy link
Contributor

@OzGav OzGav commented Jan 2, 2025

No description provided.

@OzGav
Copy link
Contributor Author

OzGav commented Jan 2, 2025

@maximmaxim345

maximmaxim345
maximmaxim345 reviewed Jan 2, 2025
View reviewed changes
docs/player-support/index.md Outdated Show resolved Hide resolved
docs/player-support/index.md Outdated Show resolved Hide resolved
docs/player-support/index.md Outdated Show resolved Hide resolved
docs/player-support/index.md Outdated Show resolved Hide resolved
maximmaxim345
maximmaxim345 reviewed Jan 3, 2025
View reviewed changes
Copy link

@maximmaxim345 maximmaxim345 left a comment

Choose a reason for hiding this comment

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

I’ve added a few more small comments for minor improvements, but overall, I quite like the docs!
Nice work.

docs/dsp/parametriceq.md Outdated Show resolved Hide resolved
docs/dsp/parametriceq.md Outdated Show resolved Hide resolved
docs/dsp/parametriceq.md Outdated Show resolved Hide resolved
@OzGav
Copy link
Contributor Author

OzGav commented Jan 3, 2025

@maximmaxim345 I have now added the tone controls page. I looked at the code and then made an educated guess as to what is actually happening. Let me know if I guessed wrong!

maximmaxim345
maximmaxim345 reviewed Jan 3, 2025
View reviewed changes
Copy link

@maximmaxim345 maximmaxim345 left a comment

Choose a reason for hiding this comment

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

The Tone Controls function identically to the old EQ usable on Airplay (and others).

@OzGav
Copy link
Contributor Author

OzGav commented Jan 4, 2025

@maximmaxim345 Are you happy to get the other guys to review this now with a view to merging it?

@maximmaxim345
Copy link

@maximmaxim345 Are you happy to get the other guys to review this now with a view to merging it?

Yea, LGTM. But looks like docs/assets/screenshots/tone-control.jpg can now be deleted (since tone-controls.jpg replaced it).

@OzGav
Copy link
Contributor Author

OzGav commented Jan 4, 2025

Thanks. Thought i had fixed that. It's deleted now. @jozefKruszynski can you review and approve if appropriate?

@OzGav OzGav requested a review from jozefKruszynski January 4, 2025 10:09
marcelveldt
marcelveldt approved these changes Jan 4, 2025
View reviewed changes
Copy link
Member

@marcelveldt marcelveldt left a comment

Choose a reason for hiding this comment

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

Nice work!
I left one small comment if we can maybe add a small note somewhere since which MA version the feature is available but otherwise perfect!

@OzGav OzGav merged commit c342d80 into music-assistant:main Jan 4, 2025
@OzGav OzGav deleted the DSP branch January 4, 2025 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maximmaxim345 maximmaxim345 maximmaxim345 left review comments

@marcelveldt marcelveldt marcelveldt approved these changes

@jozefKruszynski jozefKruszynski Awaiting requested review from jozefKruszynski

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@OzGav @maximmaxim345 @marcelveldt