-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Hide the waveform when playing recorded audio if show_recording_waveform
is False
#10405
Conversation
🪼 branch checks and previews
Install Gradio from this PR pip install https://gradio-pypi-previews.s3.amazonaws.com/1dcf118600dc276755104f02df520cb39df6ec30/gradio-5.13.0-py3-none-any.whl Install Gradio Python Client from this PR pip install "gradio-client @ git+https://github.com/gradio-app/gradio@1dcf118600dc276755104f02df520cb39df6ec30#subdirectory=client/python" Install Gradio JS Client from this PR npm install https://gradio-npm-previews.s3.amazonaws.com/1dcf118600dc276755104f02df520cb39df6ec30/gradio-client-1.10.0.tgz Use Lite from this PR <script type="module" src="https://gradio-lite-previews.s3.amazonaws.com/1dcf118600dc276755104f02df520cb39df6ec30/dist/lite.js""></script> |
🦄 change detectedThis Pull Request includes changes to the following packages.
With the following changelog entry.
Maintainers or the PR author can modify the PR title to modify this entry.
|
gradio/components/audio.py
Outdated
skip_length: The percentage (between 0 and 100) of the audio to skip when clicking on the skip forward / skip backward buttons. Defaults to 5. | ||
sample_rate: The output sample rate (in Hz) of the audio after editing. Defaults to 44100. | ||
show_recording_waveform: Whether to show the waveform when recording audio or playing audio. | ||
show_controls: Whether to show the standard HTML audio player below the waveform when recording audio or playing audio. By default, this is set to False if `show_recording_waveform` is True, and vice versa. |
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 this is the most sensible way to handle the case when show_recording_waveform=False
without adding a new parameter.
@@ -36,7 +36,10 @@ | |||
export let show_download_button: boolean; | |||
export let show_share_button = false; | |||
export let editable = true; | |||
export let waveform_options: WaveformOptions = {}; | |||
export let waveform_options: WaveformOptions = { |
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.
Added to preserve behavior for unit tests
@@ -195,6 +195,10 @@ | |||
border: 1px solid var(--block-border-color); | |||
} | |||
|
|||
.duration-button { |
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.
79794df
to
9922b12
Compare
a0cdd44
to
00f3622
Compare
a0a92fe
to
16770c2
Compare
@@ -54,7 +54,10 @@ | |||
export let file_count: "single" | "multiple" | "directory" = "multiple"; | |||
export let max_plain_text_length = 1000; | |||
export let waveform_settings: Record<string, any>; | |||
export let waveform_options: WaveformOptions = {}; | |||
export let waveform_options: WaveformOptions = { |
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.
waveform_options
and waveform_settings
shouldn't even be props here, but when I started refactoring this, the PR quickly got out of hand. Will refactor the audio out of multimodaltextbox here in a separate PR
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.
We should add waveform_options
as a param to multimodaltextbox
.
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.
Tested all combinations and look good to me.
One small nit: when both show_controls
and show_recording_waveform
areTrue
, when changing the playback speed on one, it doesn't show the new playback speed on the others' controls.
@@ -54,7 +54,10 @@ | |||
export let file_count: "single" | "multiple" | "directory" = "multiple"; | |||
export let max_plain_text_length = 1000; | |||
export let waveform_settings: Record<string, any>; | |||
export let waveform_options: WaveformOptions = {}; | |||
export let waveform_options: WaveformOptions = { |
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.
We should add waveform_options
as a param to multimodaltextbox
.
Also not sure when anyone would have both |
Thanks @dawoodkhan82, good points, I'll just deprecate |
Ok made the change, thanks @dawoodkhan82! |
For large audio files, its useful to provide a way to use the default browser controls to reduce latency and memory consumption.
Closes: #6801
Test all the reasonable combinations: