- 
                Notifications
    You must be signed in to change notification settings 
- Fork 735
Add TorchScript-able "save" func to sox_io backend #732
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
fcc7e45    to
    5ed66a3      
    Compare
  
    e96727e    to
    c9fd168      
    Compare
  
    | Codecov Report
 @@            Coverage Diff             @@
##           master     #732      +/-   ##
==========================================
- Coverage   89.23%   89.02%   -0.22%     
==========================================
  Files          32       32              
  Lines        2517     2532      +15     
==========================================
+ Hits         2246     2254       +8     
- Misses        271      278       +7     
 Continue to review full report at Codecov. 
 | 
2b50660    to
    1f49bd9      
    Compare
  
    e88aba5    to
    b9f8732      
    Compare
  
    | elif ext in ['ogg', 'vorbis']: | ||
| compression = 3. | ||
| else: | ||
| raise RuntimeError(f'Unsupported file type: "{ext}"') | 
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.
@vincentqb I thought of calling for opening an issue for adding support for other audio formats, but differentiating the valid extensions that are supported by libsox from invalid ones is not trivial and there are many formats that require additional libraries to be installed. Meaning that adding support on our get_signalinfo function may not be enough for users to actually use a new format, depending on the format. So I think we want to be more cautious and not giving high expectations to users casually. Therefore I decided not to include call for opening a PR in the error message.
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.
Can you remind me how the current backend exposes more backend? From our discussion, the user had to provide extra internal information, right?
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.
The existing implementation asks users to construct the following data structure, which requires users to know the internal of libsox.
https://fossies.org/dox/sox-14.4.2/structsox__signalinfo__t.html
https://fossies.org/dox/sox-14.4.2/structsox__encodinginfo__t.html
| # note: torchaudio can load large vorbis file, but cannot save large volbis file | ||
| # the following test causes Segmentation fault | 
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.
Is this meant to be fixed eventually?
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.
Nope I do not think this can be fixed. However, I wanted to leave a record of know limitation.
| elif ext in ['ogg', 'vorbis']: | ||
| compression = 3. | ||
| else: | ||
| raise RuntimeError(f'Unsupported file type: "{ext}"') | 
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.
Can you remind me how the current backend exposes more backend? From our discussion, the user had to provide extra internal information, right?
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.
LGTM. I've added minor clarifying questions below.
| thanks | 
* fix loss calculation for RNN * fixes loss for both RNN & Transformer
This is a part of PRs to add new "sox_io" backend. #726 and depends on #718, #728 and #731.
This PR adds
savefunction to "sox_io" backend, which can save Tensor to a file with the following audio formats;wavmp3flacogg/vorbis** Note The current binary distribution of torchaudio does not containogg/vorbiscodecs. To handle these files, one needs to build torchaudio from the source.