Skip to content
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

Improve make_waveform #4570

Merged
merged 8 commits into from Jun 20, 2023
Merged

Improve make_waveform #4570

merged 8 commits into from Jun 20, 2023

Conversation

akx
Copy link
Contributor

@akx akx commented Jun 20, 2023

Description

This PR fixes make_waveform to

  1. check that the file exists (e.g. if an user passes in a path, it's an actual existing file)
  2. not use an insecure method to craft the ffmpeg command. As a side effect, this also makes make_waveform work when the input filename contains a space.

The latter doesn't seem to generally be an issue for gradio component users, as machinery exists to clean up the more evil characters from uploaded filepaths, but can be if one uses make_waveform directly.

Checklist:

  • I have performed a self-review of my own code
  • I have added a short summary of my change to the CHANGELOG.md
  • My code follows the style guidelines of this project
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@akx akx marked this pull request as ready for review June 20, 2023 08:11
@gradio-pr-bot
Copy link
Contributor

All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-4570-all-demos

@abidlabs
Copy link
Member

Thanks @akx, taking a look at this now. Just a reminder that we appreciate if you can create an issue first with a repro instead of opening the PR directly.

@abidlabs
Copy link
Member

abidlabs commented Jun 20, 2023

I tested out this PR and I agree with the security fix (by which I assume you mean dropping shell=True). However, I don't see how this relates to the issue of input filenames containing whitespaces.

Testing this on main already works:

import gradio as gr

gr.make_waveform("test 2.mp3")

Can you give an example that is not working for you on main?

@akx
Copy link
Contributor Author

akx commented Jun 20, 2023

However, I don't see how this relates to the issue of input filenames containing whitespaces.

A path with a space in it will be split into two shell arguments and simply won't work.

Sorry, not at a terminal right now but try dropping a file with a space in the name on a gr.Audio(type="filepath") on main; IIRC that broke things with ffmpeg complaining it couldn't find /tmp/gradio/.../foo for a foo bar.mp3.

Right, at a (Windows) terminal now 😁

You can repro this by

  • change demo/waveform/run.py so gr.Audio() is gr.Audio(type="filepath")
  • run the demo
  • drop in a file with a space in it (12 inch hoover.wav in my case)
  • Get an error in the console and no waveform
> python demo\waveform\run.py
Running on local URL:  http://127.0.0.1:7860

To create a public link, set `share=True` in `launch()`.
ffmpeg version 5.0.1-full_build-www.gyan.dev Copyright (c) 2000-2022 the FFmpeg developers
  built with gcc 11.2.0 (Rev7, Built by MSYS2 project)
[...]
Input #0, png_pipe, from 'C:\Users\X\AppData\Local\Temp\tmpb3kf1qt9.png':
  Duration: N/A, bitrate: N/A
  Stream #0:0: Video: png, rgba(pc), 1000x200, 25 fps, 25 tbr, 25 tbn
C:\Users\X\AppData\Local\Temp\gradio\488f44dac3c83fa15aca9f51997a58b45d365f7e\12: No such file or directory

See how 12 inch hoover.wav has been truncated to 12?

@abidlabs
Copy link
Member

abidlabs commented Jun 20, 2023

Thanks @akx, was able to reproduce! LGTM

Pushed a test as well. Should be good to merge once CI passes

@abidlabs abidlabs merged commit c6c545c into gradio-app:main Jun 20, 2023
11 checks passed
@akx akx deleted the shello-waveform branch June 27, 2023 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants