-
Notifications
You must be signed in to change notification settings - Fork 960
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 optional sample rate conversion to audio::TargetFile and audio::BufferRecorderNode #1869
base: master
Are you sure you want to change the base?
Conversation
Really excited to see these changes. Just wanted to let you know that I'm onsite for a project and pretty busy for the next week and a half, but looking forward to checking these out when I have a moment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay on this, that week and a half project turned into a month.. but overall this looks great.
So as to the part about not implemented on MSW - I believe this is actually the only place where you explicitly need a ci::audio::dsp::Converter
, as MSW Media Foundation doesn't do resampling (in our simple usage, anyway). On the other hand, Core Audio's ExtAudioFile
does do more advanced stuff like resampling. With SourceFile
, I distinguished between implementations within built in samplerate conversion and those without with the virtual method supportsConversion()
(
Cinder/include/cinder/audio/Source.h
Line 70 in 649732c
virtual bool supportsConversion() { return false; } |
Going to test on my end since I'm on windows..
include/cinder/audio/Target.h
Outdated
size_t getSampleRate() const { return mSampleRate; } | ||
//! Returns the true sample rate of the target file. \note Actual input samplerate may differ. \see getSampleRate() | ||
size_t getSampleRateTarget() const { return mSampleRateTarget; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it's worth naming this getSampleRateNative()
to be consistent with SourceFile
? I can't say I like one name over the other, though it is slightly ambiguous that the class is called TargetFile
and the method has the noun 'target' in in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me both seem reasonable. I personally didn't find getSampleRateTarget()
ambiguous since I thought of it as "get the sample rate of the target [file]". But I can definitely see the consistency argument with SourceFile
.
src/cinder/audio/Target.cpp
Outdated
{ | ||
if ( ! mSampleRateTarget ) { | ||
mSampleRateTarget = mSampleRate; | ||
} else if ( mSampleRateTarget != mSampleRate) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: else
statements should begin on a new line (apologies, this wasn't in the CONTRIBUTING.md, but I'm going to add it as it is something we've all agreed upon AFAIK).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just fixed this with 89ad51f.
I've also removed spaces between if
/while
and their braces. This isn't mentioned in the CONTRIBUTING.md
either but is done very consistently in the code.
src/cinder/audio/Target.cpp
Outdated
auto numDestFrames = size_t( numSourceFrames * (float)getSampleRateTarget() / (float)getSampleRate() ); | ||
|
||
// copy buffer into temporary buffer to remove frame offset (needed for mConverter->convert) | ||
mConverterSourceBuffer.copyOffset( *buffer, numSourceFrames, 0, currFrame ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concerning this and your comment in the PR description about this being a bit hacky, I hope that in the future this problem can be alleviated once audio::BufferView
s are implemented (see #1701). I think for now a TODO comment linking back to that would suffice. The copy is quite cheap compared to the actual encoding, but obviously eventually we want to do as little extra copying as possible.
I can see how the I'll also try to adapt the |
This last commit adds
|
@richardeakin I don't have experience with resolving conflicts on GitHub. What's the process here? Can I do this since I don't have write access? Besides a merge conflict in |
You should probably locally merge master back into your koenaad:target_sample_rate branch, resolve conflicts, then push it back to your repo - it'll update this PR. You can also rebase and I started looking at how to use this on Windows but was only able to put a couple hours towards it. I'm hoping to revisit that soon. Basically my current thought that Windows and Linux will need to use a |
Okay, makes sense. Unfortunately, my macbook broke down this week (which has my development environment) so I will only be able to continue working on this over at least a week I think. |
No problem, sorry to hear about your macbook. I may continue with trying to get it working on the windows side, but I can replay my mods on top of your branch once it is fixed up. |
Hi @richardeakin, this should now be ready to be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking really good. I've synced with this branch and updated the windows side of things in https://github.com/richardeakin/Cinder/tree/target_sample_rate_msw, all seems to be working when testing with the SampleTest (note if you want to try that, open it via AudioTest.msw folder. Do you want to merge that into your branch so it appears on this PR? Alternatively I can merge those changes in separately but it'd be nice and tidy to get the changes all in one shot.
I think there are a couple small things left on the OS X / iOS side of things that we should do so that it uses its internal converter, one of the things I commented on inline. The other is that the file ASBD should use mSampleRateNative.
|
||
// Implement to write \a numFrames frames of \a buffer to file. The writing begins at \a frameOffset. | ||
virtual void performWrite( const Buffer *buffer, size_t numFrames, size_t frameOffset ) = 0; | ||
//! Implementations should override and return true if they can provide samplerate conversion. If false (default), a Converter will be used if needed. | ||
virtual bool supportsConversion() { return false; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think TargetFileCoreAudio
still needs to override this and return true.
Hi, sorry for the delay. I haven't had much time to look at this and won't have much in the future either (started a new job, my first actually). About the OS X / iOS side: I don't have any experience with these services so I won't be able to help much with this. I'd propose to continue with this (non-optimal) implementation for now and opening an issue as reference for future contributors. If you would like me to merge something, let me know and I'll help out as much as I can. But I won't be able to provide much development for now. |
All good, thanks for your what you've already contributed! I'll try to get to the OS X side the next time I'm on my macbook, which isn't often these days, but I'd like to try to get those changes in there before putting this down and moving on. |
As I was discussing with @richardeakin on the forum, I've added the abillty to configure the sample rate of
TargetFile
.The sample rate conversion is implemented in
audio::TargetFile
and exposed inaudio::BufferRecorderNode
.I propose the following API changes:
TargetFile::create
has an additional optional parametertargetSampleRate
.I placed it before
extension
since this parameter is (for some reason) not used in the implementation ofcreate
.TargetFileCoreAudio
.TargetFile::getSampleRateNative
similar as inSource
.BufferRecorderNode::writeToFile
has an additional optional parameterdestSampleRate
which is passed toTargetFile::create
.I've not implemented these changes on MSW since I work on macOS, but instead added an assert firing when sample rate conversion is required.
I feel like my implementation of
TargetFile::write
is bit hacky, becausedsp::Converter
1) processes data in fixed chunk sizes and 2) can't handle frame offsets in buffers. So I first have to copy data into a new buffer (removing the frame offset), convert it using a second buffer before writing it to file.So could that could be improved imo.