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

BUG: read tries to handle 20-bit WAV files but shouldn't #5990

Merged
merged 1 commit into from
Mar 22, 2016

Conversation

endolith
Copy link
Member

Audition can save WAV files as "24-bit packed int (type 1, 20-bit)" format where each sample is 3 bytes and BitsPerSample = 0x14 = 20. SciPy tries to open it and interprets it as 16-bit and it's all screwed up. Changed the handling to only accept bits per sample that align with numpy int types.

I guess it would be better if it read the byte lengths from numpy itself instead of a hardcoded list?

Though better yet would be to read any bit depth of file and store it as the next-highest numpy int type.

(And should also have a norm=True or something option that converts every type to normalized floating point from -1 to +1 so you don't have to do that yourself, same as scikits.audiolab.)

Currently it only handles np.int* types.
@endolith endolith changed the title BUG: read cannot handle 20-bit WAV files BUG: read tries to handle 20-bit WAV files but shouldn't Mar 22, 2016
@codecov-io
Copy link

@@            master   #5990   diff @@
======================================
  Files          238     238       
  Stmts        43757   43757       
  Branches      8218    8218       
  Methods          0       0       
======================================
  Hit          34182   34182       
  Partial       2604    2604       
  Missed        6971    6971       

Review entire Coverage Diff as of ba6666c

Powered by Codecov. Updated on successful CI builds.

@larsoner
Copy link
Member

Though better yet would be to read any bit depth of file and store it as the next-highest numpy int type.

Agreed.

And should also have a norm=True or something option that converts every type to normalized floating point from -1 to +1 so you don't have to do that yourself, same as scikits.audiolab

By normalizing by MAX_INT for the appropriate type? Agreed.

PRs welcome when you get time.

@larsoner
Copy link
Member

I think the manual list is okay for now. Hopefully you can make it better soon :)

Good to merge from your end?

@larsoner larsoner added scipy.signal defect A clear bug or issue that prevents SciPy from being installed or used as expected labels Mar 22, 2016
@endolith
Copy link
Member Author

By normalizing by MAX_INT for the appropriate type? Agreed.

Actually normalizing to BitsPerSample from the headers, in case it's not a multiple of 8, and shifting from unsigned for BitsPerSample from 1 to 8.

and if it doesn't normalize, it should return the bit depth as well as fs and data.

Good to merge from your end?

yes

@larsoner
Copy link
Member

and if it doesn't normalize, it should return the bit depth as well as fs and data.

This one would be tougher because it would be a backward-incompatible change, but I suppose you could add a return_depth=False (by default) argument.

larsoner added a commit that referenced this pull request Mar 22, 2016
BUG: read tries to handle 20-bit WAV files but shouldn't
@larsoner larsoner merged commit f2ba715 into scipy:master Mar 22, 2016
@larsoner
Copy link
Member

Thanks @endolith

@endolith endolith deleted the patch-1 branch March 23, 2016 00:33
@ev-br ev-br added this to the 0.18.0 milestone Mar 26, 2016
rgommers added a commit to rgommers/scipy that referenced this pull request May 22, 2016
This resulted in 24 test errors on my 32-bit Python install.
Introduced in scipygh-5990.
@endolith
Copy link
Member Author

This one would be tougher because it would be a backward-incompatible change, but I suppose you could add a return_depth=False (by default) argument.

Actually maybe that should be a separate function? This one is only for converting to exact equivalent dtypes, when possible?

@larsoner
Copy link
Member

This one is only for converting to exact equivalent dtypes, when possible?

A normalize=True could determine which dtypes can be read in. Can't decide if that's too magical, though. Seems like it would be okay if it's documented properly.

nmayorov pushed a commit to nmayorov/scipy that referenced this pull request Jun 5, 2016
This resulted in 24 test errors on my 32-bit Python install.
Introduced in scipygh-5990.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.signal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants