Skip to content

bpo-32990: Support WAVE_FORMAT_EXTENSIBLE in the wave module #9515

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

ZackerySpytz
Copy link
Contributor

@ZackerySpytz ZackerySpytz commented Sep 23, 2018

The test file, a modified version of
Lib/test/audiodata/pluck-pcm24.wav, was provided by Andrea Celletti
on the bug tracker.

https://bugs.python.org/issue32990

The test file, a modified version of
Lib/test/audiodata/pluck-pcm24.wav, was provided by Andrea Celletti
on the bug tracker.
@ZackerySpytz ZackerySpytz force-pushed the bpo-32990-wave-format-extensible branch from 482bb86 to 80b360a Compare September 23, 2018 13:48
if wFormatTag == WAVE_FORMAT_EXTENSIBLE:
# Only the first 2 bytes (of 16) of SubFormat are needed.
cbSize, wValidBitsPerSample, dwChannelMask, \
SubFormatFmt = struct.unpack_from('<HHLH', chunk.read(10))
Copy link
Member

Choose a reason for hiding this comment

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

Could you reformat this line in a way that makes it easier to read? Right now it looks like two separate statements.

Using parentheses around the left hand side is fine, and then indent the second line.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, we should also catch struct.error here and raise, like we do above.

wFormatTag, self._nchannels, self._framerate, dwAvgBytesPerSec, wBlockAlign = struct.unpack_from('<HHLLH', chunk.read(14))
wFormatTag, self._nchannels, self._framerate, dwAvgBytesPerSec, \
wBlockAlign, wBitsPerSample = \
struct.unpack_from('<HHLLHH', chunk.read(16))
Copy link
Member

Choose a reason for hiding this comment

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

Previously if there was no wBitsPerSample value and wFormatTag was something other than WAVE_FORMAT_PCM, we may get a different error.

Can we be sure that the longer chunk.read will always return 16 bytes? Or should we keep deferring the second read until after we've checked wFormatTag

@tiran
Copy link
Member

tiran commented Sep 24, 2018

FYI, we are planning to deprecate the wave module in 3.8 and move it out of the stdlib in 3.9 or 3.10. Does it make sense to add new features at all?

@ZackerySpytz
Copy link
Contributor Author

@tiran As stated on the bug tracker, this could be considered a bug fix. It would be good to hear @zooba's opinion.

@jjuod
Copy link

jjuod commented Nov 15, 2019

Has there been a final decision on this? It seems the plan is to keep wave in, as of PEP 594, so maybe it's possible to finish this merge? We're using this through wavio to deal with various WAVs and could contribute a new PR, but this seems pretty close to merging already...

@csabella
Copy link
Contributor

@ZackerySpytz, please address the code review. Thank you!

@evenbrenden
Copy link

Has there been a final decision on this? It seems the plan is to keep wave in, as of PEP 594, so maybe it's possible to finish this merge? We're using this through wavio to deal with various WAVs and could contribute a new PR, but this seems pretty close to merging already...

I'm in the same boat. Any objections to starting a new PR?

@ZackerySpytz
Copy link
Contributor Author

I will update this PR. A new one doesn't need to be created.

evenbrenden added a commit to evenbrenden/SampleScanner that referenced this pull request Apr 17, 2021
python/cpython#9515 only adds support for
reading WAVE_FORMAT_EXTENSIBLE, not writing.
@zooba
Copy link
Member

zooba commented Sep 14, 2022

Same change was merged in #96777

@zooba zooba closed this Sep 14, 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.

9 participants