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

Add getSampleRate() #24

Open
wants to merge 1 commit into
base: default
Choose a base branch
from

Conversation

bmatherly
Copy link
Contributor

This is a convenience function to allow applications to easily know
what sample rate the stretcher was initalized with to check if it
needs to be recreated.

This is a convenience function to allow applications to easily know
what sample rate the stretcher was initalized with to check if it
needs to be recreated.
@cannam
Copy link
Member

cannam commented Feb 14, 2020

I am in two minds about this one. The content of the commit of course looks fine, but I'm not convinced that it is worth adding to the API for.

The stretcher has two sorts of construction parameters - fixed ones like sample rate and channel count, and variable ones like ratios and options. At the moment you can query the channel count after construction (even though it never changes), but not the sample rate, so there is undeniably an inconsistency there. However, I'm not entirely sure it is a good thing to be able to query the channel count either, and I slightly regret including that method already. I think the stretcher object should be contained (in application code) in a context that fixes these construction parameters, so its sample rate and channel count are known explicitly, rather than expecting to query these values and make a new stretcher if they don't match - which may be prone to cause hard-to-diagnose errors.

Can you persuade me otherwise?

@bmatherly
Copy link
Contributor Author

Hi. Thank you for your thoughtful consideration.

I implemented a filter wrapper for rubberband in the MLT framework. In that framework, a filter does not know the sample rate of the audio that it will be receiving at construction time. So the filter needs to detect the rate when it receives its first audio. In some cases, for example, if the user changes the project configuration, the filter could spontaneously receive audio frames with different channel count or sampling frequency.

Here is the line of code that runs with every frame to detect the changing conditions:
https://github.com/mltframework/mlt/blob/master/src/modules/rubberband/filter_rbpitch.cpp#L111
As you can see, the channel count change can be detected by querying the stretcher. But for the sampling frequency, I am forced to save a member variable to remember the initalized sampling frequency:
https://github.com/mltframework/mlt/blob/master/src/modules/rubberband/filter_rbpitch.cpp#L35
The sole purpose of that member variable is to detect when the frequency changes because I can not query the stretcher.

I understand that it is not difficult or expensive to keep a member variable to remember the frequency. So this change is only a convenience in this scenario.

From my perspective, the reason to accept the patch would be:

  1. Consistency: since the channel function already exists
  2. Small convenience for situations like I describe above

I do not feel strongly about the patch. I only offer it because it was easy for me to produce and test in my environment. So if you reject the patch, my application will still be fine. But if you accept it, I will probably change my code to use the new function.

Base automatically changed from master to default February 11, 2021 14:06
@luzpaz
Copy link

luzpaz commented Jun 28, 2023

Any verdict on this ?

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.

3 participants