-
Notifications
You must be signed in to change notification settings - Fork 435
feat(android): add input_preset
to StreamConfig
#995
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
base: master
Are you sure you want to change the base?
Changes from all commits
c907081
34ca87b
f792b87
2a4b375
7f98979
d673bff
51d0a00
42a897c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -263,6 +263,8 @@ pub struct StreamConfig { | |
pub channels: ChannelCount, | ||
pub sample_rate: SampleRate, | ||
pub buffer_size: BufferSize, | ||
#[cfg(all(target_os = "android", feature = "android-input-preset"))] | ||
pub input_preset: platform::AudioInputPreset, | ||
} | ||
|
||
/// Describes the minimum and maximum supported buffer size for the device | ||
|
@@ -410,6 +412,8 @@ impl SupportedStreamConfig { | |
channels: self.channels, | ||
sample_rate: self.sample_rate, | ||
buffer_size: BufferSize::Default, | ||
#[cfg(all(target_os = "android", feature = "android-input-preset"))] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make sense to rename the feature flag to something more specific like "android-voice-recognition" or "android-aec"? Because it seems like there are more input presets that one could choose, if not today then in the future. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it better to keep the name because AEC is not the only function of the flag. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's what I was trying to say. Looking at the AudioInputPreset enum there are six variants today. How can we make the feature flag clearly convey which preset gets enabled? Edit: what would be a good way going forward to support the other variants as well? |
||
input_preset: platform::AudioInputPreset::VoiceRecognition, | ||
roderickvd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
} | ||
|
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 see this requires
api-level-28
where the normal feature requiresapi-level-26
. That OK?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 don't see any conflicts between the features in
ndk
document and didn't find any errors about api levels when I used the branch of this PR in my own project.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.
Well, if it works, it works. As it does bump the minimum Android version, it would be a good thing to document. Would you document the feature flag, as well as add an entry to the changelog?