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
4549 autoplay #4705
4549 autoplay #4705
Conversation
🎉 The demo notebooks match the run.py files! 🎉 |
beforeAll(() => { | ||
window.HTMLMediaElement.prototype.play = vi.fn(); | ||
window.HTMLMediaElement.prototype.pause = vi.fn(); | ||
}); |
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 tired to mock this in a slightly more clever way but it didn't work at all, this does work but I'd like to figure out a batter way to do it in the future.
afterEach(() => cleanup()); | ||
|
||
test("renders provided value and label", async () => { | ||
const { getByTestId, queryAllByText } = render(Audio, { | ||
const { getByTestId, queryAllByText } = await render(Audio, { |
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.
render
is now async because it ensure the DOM is updated before resolving. We need to await it to ensure this has all happened.
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.
Interesting - the same function can be be simultaneously async and not async? 🤯
All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-4705-all-demos |
|
||
const startButton = getByTestId<HTMLAudioElement>("dynamic-dynamic-audio"); | ||
const fn = spyOn(startButton, "play"); | ||
startButton.dispatchEvent(new Event("loadeddata")); |
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 need to trigger this even because this is the heuristic we use to start 'autoplaying'. See above about better mocks, i'd like to clean this up in the future.
js/audio/src/Audio.svelte
Outdated
async function handle_playback(): Promise<void> { | ||
if (autoplay) { | ||
node.pause(); | ||
await node.play(); | ||
} | ||
} | ||
|
||
node.addEventListener("loadeddata", handle_playback); | ||
node.addEventListener("timeupdate", clamp_playback); | ||
|
||
return { | ||
destroy: () => node.removeEventListener("timeupdate", clamp_playback) | ||
destroy(): void { | ||
node.removeEventListener("loadeddata", handle_playback); | ||
node.removeEventListener("timeupdate", clamp_playback); | ||
} |
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.
This is basically the fix.
js/audio/src/StaticAudio.svelte
Outdated
function loaded(node: HTMLAudioElement): ActionReturn { | ||
async function handle_playback(): Promise<void> { | ||
if (autoplay) { | ||
await node.play(); | ||
} | ||
} | ||
|
||
node.addEventListener("loadeddata", handle_playback); | ||
|
||
return { | ||
destroy: () => node.removeEventListener("loadeddata", handle_playback) | ||
}; | ||
} |
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.
Actually I'll deupe all of this.
js/gallery/src/Gallery.svelte
Outdated
@@ -161,6 +161,8 @@ | |||
) | |||
.join(" "); | |||
} | |||
|
|||
$: console.log(container_element?.tagName); |
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.
remove
Not happy now I've reviewed, will change to wip. |
This can be tested with the following snippet: import gradio as gr
TEST_AUDIO_A = "a.mp3"
TEST_AUDIO_B = "b.wav"
audio_files = [TEST_AUDIO_A, TEST_AUDIO_B]
a_idx = 0
TEST_VIDEO_A = "a.mp4"
TEST_VIDEO_B = "b.mp4"
video_files = [TEST_VIDEO_A, TEST_VIDEO_B]
v_idx = 0
def output_audio():
global a_idx
file = audio_files[a_idx]
a_idx = 1 if a_idx == 0 else 0
return file
def output_video():
global v_idx
file = video_files[v_idx]
v_idx = 1 if v_idx == 0 else 0
return file
with gr.Blocks() as demo:
audio = gr.Audio(type="filepath", label="Audio", autoplay=True)
button = gr.Button(value="play audio")
button.click(output_audio, outputs=[audio])
video = gr.Video(type="filepath", label="Video", autoplay=True)
button_two = gr.Button(value="play video")
button_two.click(output_video, outputs=[video])
demo.launch() Use some audio files from the demos folder. When swapping between them they should autoplay |
We didn't really have any video tests so i added a few. |
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.
Thank you for the fix @pngwn ! Verified it works. Left some nits - mostly questions for me.
Description
Closes #4549. Closes #4707
Updating the src now triggers autoplay as expected. Tests added to prevent regression.
I also experienced some flake so I have adjusted one of our test utilities a little to prevent that in the future.