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

Allow specifying sample rate per output - add output resamplers #925

Merged
merged 10 commits into from
Jan 17, 2025

Conversation

WojciechBarczynski
Copy link
Member

@WojciechBarczynski WojciechBarczynski commented Jan 16, 2025

I tested it on RTP 24000HZ and MP4 44100 Hz and 48kHz and it seems to be working fine

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to repeat some of this code with one in decoder/resampler, since reusing it would overcomplicate it significantly.

Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming internal logic for resampling, so I did not check that code. Is this safe assumption?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, it's copy-paste adjusted to types.

@WojciechBarczynski WojciechBarczynski changed the title Allow specifying sample rate per output Allow specifying sample rate per output - add output resamplers Jan 16, 2025
@WojciechBarczynski WojciechBarczynski marked this pull request as ready for review January 16, 2025 12:36
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming internal logic for resampling, so I did not check that code. Is this safe assumption?

src/config.rs Outdated
const SUPPORTED_SAMPLE_RATES: [u32; 5] = [8_000, 12_000, 16_000, 24_000, 48_000];
const DEFAULT_OUTPUT_SAMPLE_RATE: u32 = 48_000;
let output_sample_rate: u32 = match env::var("LIVE_COMPOSITOR_OUTPUT_SAMPLE_RATE") {
const SUPPORTED_SAMPLE_RATES: [u32; 6] = [8_000, 12_000, 16_000, 24_000, 44_100, 48_000];
Copy link
Member

@wkozyra95 wkozyra95 Jan 17, 2025

Choose a reason for hiding this comment

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

maybe leave the config options as they are,

This something for the future to think about, but maybe we should consider dropping this option and setting 96_000 sample rate or something like this. Do you see any problems with that approach?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we use 96000, audio will always be resampled, potentially leading to reduced quality. With the current approach, if users have the same input and output sample rate, they can just set MIXING_SAMPLE_RATE to that value and avoid resampling. I haven't heard any noticeable degradation in quality when resampling to different sample rates, but it probably exists to some extent.

No strong opinion on that.

@WojciechBarczynski WojciechBarczynski force-pushed the @WojciechBarczynski/per_output_sample_rate branch from 8cb5365 to 43490f3 Compare January 17, 2025 11:58
@WojciechBarczynski WojciechBarczynski merged commit a576149 into master Jan 17, 2025
5 checks passed
@WojciechBarczynski WojciechBarczynski deleted the @WojciechBarczynski/per_output_sample_rate branch January 17, 2025 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants