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

Add support for 24-bit signed LPCM wav in sox_io backend #1389

Merged
merged 1 commit into from
Mar 15, 2021

Conversation

iseessel
Copy link
Contributor

No description provided.

@iseessel
Copy link
Contributor Author

@mthrok Let me know what you think!

Copy link
Collaborator

@mthrok mthrok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @iseessel

Thanks for working on this. It looks good overall.

case 24: // Cast 24-bit to 32-bit. That is each data chunk will
// implicitly perform 8 left-shifts.
std::cout << "Warning: Casting 24-bit signed PCM to torch::kInt32";
return torch::kInt32;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that adding a warning here will issue warning regardless of the value of normalize. Ideally, the warning should be issued only when normalize=False and the input format is 24-bit PCM WAV, but let's keep that from this PR. So, let's simplify the case for 24-bit.

case 24:
case 32:
  return torch::kInt32;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you like me to add warnings to apply_effects_file and apply_effects_fileobj?

torchaudio/backend/sox_io_backend.py Outdated Show resolved Hide resolved
torchaudio/backend/sox_io_backend.py Outdated Show resolved Hide resolved
torchaudio/csrc/sox/utils.cpp Outdated Show resolved Hide resolved
@mthrok mthrok linked an issue Mar 14, 2021 that may be closed by this pull request
@iseessel iseessel force-pushed the support-signed-24-bit-int branch from 803cc7a to 6a8a05f Compare March 15, 2021 15:01
@iseessel
Copy link
Contributor Author

iseessel commented Mar 15, 2021

Changes have been addressed.

  1. Would you like me to add warnings to apply_effects_file and apply_effects_fileobj?
  2. Should I put in a follow up PR to remove the dead code?
    @mthrok

@mthrok
Copy link
Collaborator

mthrok commented Mar 15, 2021

  1. Would you like me to add warnings to apply_effects_file and apply_effects_fileobj?

If you have time and interest, please do so. The following is the direction I think works.

  1. In apply_effects_file and apply_effects_fileobj, change the logic of get_dtype to consider normalize option as well. (return float32 if normalize=True).
  2. Add warning if normalize=False and the input WAV is 24-bit. We should be able to use TORCH_WARN_ONCE
  3. Remove normalize option from convert_to_tensor.
  1. Should I put in a follow up PR to remove the dead code?

If you have time, please do so. I have filed a task T86709147 as well.

@mthrok mthrok changed the title Add support for signed-24-bit-int wav files in sox_io_backend Add support for 24-bit signed LPCM wav in sox_io backend Mar 15, 2021
@mthrok mthrok merged commit ed9020c into pytorch:master Mar 15, 2021
@mthrok
Copy link
Collaborator

mthrok commented Mar 15, 2021

Thanks!

vincentqb pushed a commit to vincentqb/audio that referenced this pull request Mar 17, 2021
vincentqb added a commit that referenced this pull request Mar 17, 2021
Co-authored-by: Isaac Seessel <iseessel@oberlin.edu>
mthrok pushed a commit to mthrok/audio that referenced this pull request Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support 24bit wave in sox_io load function.
3 participants