-
Notifications
You must be signed in to change notification settings - Fork 4k
[feat] Add sample rate parameter to st.audio_input component
#12272
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
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
✅ PR preview is ready!
|
📉 Frontend coverage change detectedThe frontend unit test (vitest) coverage has decreased by 0.0900%
💡 Consider adding more unit tests to maintain or improve coverage. |
st.audio_input component
1db321e to
1545062
Compare
frontend/lib/src/components/widgets/AudioInput/convertAudioToWav.ts
Outdated
Show resolved
Hide resolved
frontend/lib/src/components/widgets/AudioInput/convertAudioToWav.ts
Outdated
Show resolved
Hide resolved
frontend/lib/src/components/widgets/AudioInput/convertAudioToWav.test.ts
Show resolved
Hide resolved
aa9bbff to
60ec96a
Compare
frontend/lib/src/components/widgets/AudioInput/convertAudioToWav.ts
Outdated
Show resolved
Hide resolved
…ion and recording start
…prove readability and avoid race conditions
…plugin setup for improved clarity and performance
…ing URL handling for improved functionality
…eletion handling, and improve plugin management
…andling, and enhance cleanup logic for improved clarity and performance
…and streamline logic for improved clarity
…dio permission issues
90d11de to
02dbf67
Compare
frontend/lib/src/components/widgets/AudioInput/convertAudioToWav.test.ts
Show resolved
Hide resolved
| arrayBuffer = await fileBlob.arrayBuffer() | ||
| } catch (error) { | ||
| LOG.error("Failed to read blob as ArrayBuffer", error) | ||
| void audioContext.close() |
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.
question: Should we await for the .close() to succeed before returning?
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.
Hmm are you thinking perhaps it's only not an issue right now cause fast computer? I was thinking it works while not waiting for it to finish (and we no longer depend on it) so why make the user wait for it.
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.
Those are good points! My original question was more towards the syntax code smell of using void vs await. I would expect to use await in most scenarios, and only use void when we really don't care about the side-effect. I see your perspective from your last comment, so I think this is fine in practice.
frontend/lib/src/components/widgets/AudioInput/convertAudioToWav.ts
Outdated
Show resolved
Hide resolved
…g to prevent unnecessary re-renders.
and
setRecordingTime(formatTime(0))
…av.ts Co-authored-by: Bob Nisco <[email protected]>
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.
Pull Request Overview
Copilot reviewed 8 out of 12 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
frontend/lib/src/components/widgets/AudioInput/convertAudioToWav.ts:1
- The global regex replacement
/:/gwill replace all colons in the timestamp, but ISO timestamps contain multiple colons (e.g., '2025-01-15T10:30:45'). This could result in malformed filenames. Consider using a more specific replacement or sanitizing the entire timestamp for filename safety.
/**
sfc-gh-bnisco
left a comment
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.
Thanks for the revisions!
|
Thanks for making this! I haven't had the time to test it, but reading bits and pieces of code I see the right digital signal processing terminology, and the usual of by one bug fix so commonly occurring in this type of work. |
Configurable audio sample rates for
st.audio_inputOverview
Adds support for configurable audio sample rates in
st.audio_input, letting apps pick recording quality per use case (e.g., 16 kHz for ASR, 48 kHz for music).While I'm here, this PR also fixes #12446
Changes
Python API
sample_rate(optional) added tost.audio_input(...), default 16 kHz.8000, 16000, 22050, 44100, 48000Hz.StreamlitAPIExceptionon invalid values.Frontend
sampleRate: { ideal: targetSampleRate }togetUserMedia.OfflineAudioContextto convert to the exact target rate.ideal; resampling enforces consistency.Technical details
audioBitsPerSecondusage (bitrate ≠ sample rate).audioContextoption fromRecordPlugin.startRecording()(not the plugin constructor).Testing
Example usage
Files changed
lib/streamlit/elements/widgets/audio_input.py— Python APIfrontend/lib/src/components/widgets/AudioInput/AudioInput.tsx— React updatesfrontend/lib/src/components/widgets/AudioInput/convertAudioToWav.ts— resamplingfrontend/lib/src/components/widgets/AudioInput/convertAudioToWav.test.ts— new testslib/tests/streamlit/elements/audio_input_test.py— Python testse2e_playwright/st_audio_input*.py— E2E coverageNotes
sampleRateis an idealgetUserMediaconstraint; actual device rate may differ.