GH-60729: Add IEEE format wave audio support#145384
Conversation
This adds support for floating point wav files and fix python#60729.
'format' is the term used in the wave audio specification
Per the RIFF/WAVE Rev. 3 documentation, non-PCM formats require a fact chunk, while PCM does not. This is also what libsdnfile/audacity do Reference: https://mmsp.ece.mcgill.ca/Documents/AudioFormats/WAVE/WAVE.html (see the 'fact Chunk' section and linked Rev. 3 RIFF docs).
These methods are public but were previously not documented. This caused the unit tests to fail when I mentioned changes to getparams() in the whats new ;-)
Misc/NEWS.d/next/Library/2023-03-10-13-10-06.gh-issue-60729.KCCHTe.rst
Outdated
Show resolved
Hide resolved
…CHTe.rst Co-authored-by: AN Long <aisk@users.noreply.github.com>
lkoenig
left a comment
There was a problem hiding this comment.
Thanks for pushing that forward.
encukou
left a comment
There was a problem hiding this comment.
Thank you for the PR!
I didn't finish reviewing today; sending the comments I have.
Doc/library/wave.rst
Outdated
| This is one of ``WAVE_FORMAT_PCM``, ``WAVE_FORMAT_IEEE_FLOAT``, or | ||
| ``WAVE_FORMAT_EXTENSIBLE``. |
There was a problem hiding this comment.
Could you document these using .. data::, and link to them using :data:?
Doc/library/wave.rst
Outdated
| framerate, nframes, comptype, compname, format)``, equivalent to output | ||
| of the ``get*()`` methods. |
There was a problem hiding this comment.
This is a backwards-incompatible change: before someone could write:
nc, sw, fr, nf, ct, cn = wav.getparams()This could be solved by adding format as a named-only attribute (which, unfortunately, isn't easy with namedtuple), or maybe an argument to getparams.
You might want to leave getparams alone for now & tackle this in a focused follow-up PR.
There was a problem hiding this comment.
Sure, I reverted those changes now
Lib/test/test_wave.py
Outdated
| class MiscTestCase(unittest.TestCase): | ||
| def test__all__(self): | ||
| not_exported = {'WAVE_FORMAT_PCM', 'WAVE_FORMAT_EXTENSIBLE', 'KSDATAFORMAT_SUBTYPE_PCM'} | ||
| not_exported = {'WAVE_FORMAT_PCM', 'WAVE_FORMAT_IEEE_FLOAT', 'WAVE_FORMAT_EXTENSIBLE', 'KSDATAFORMAT_SUBTYPE_PCM'} |
There was a problem hiding this comment.
Please export the WAVE_FORMAT_ ones (add to __all__) instead, and remove them here.
No point in hiding them; they're needed for documented functionality.
There was a problem hiding this comment.
(I'd keep KSDATAFORMAT_SUBTYPE_PCM hidden until it's documented; that seems out of scope for this PR.)
There was a problem hiding this comment.
Sure, they're exported now
| try: | ||
| self._fact_sample_count_pos = self._file.tell() | ||
| except (AttributeError, OSError): | ||
| self._fact_sample_count_pos = None |
There was a problem hiding this comment.
Hmm, what's the story about supporting files without tell?
There was a problem hiding this comment.
I’m not trying to extend unseekable support in this PR. I’m keeping the existing behavior: if output is seekable, header fields (including fact) are patched as needed; for unseekable outputs, writing works when header values are already final, and mismatches can still fail as before.
Co-authored-by: Petr Viktorin <encukou@gmail.com>
…ieee-wave-audio-support # Conflicts: # Doc/library/wave.rst
|
|
||
| .. data:: WAVE_FORMAT_IEEE_FLOAT | ||
|
|
||
| Format code for IEEE floating-point audio. |
There was a problem hiding this comment.
What is the size of a sample? 32-bit? Or does it accept different sizes?
There was a problem hiding this comment.
for IEEE Float it can only be 32-bit or 64-bit. I've amended the documentation and added raising errors in case of setting sampwidth to different values for IEEE or setting format to IEEE if sampwidth is incorrect values
This is also similar to what libsndfile does
eeee8cd to
6d62e66
Compare
encukou
left a comment
There was a problem hiding this comment.
Looks good to me, thank you!
Will merge tomorrow if there are no objections.
|
awesome and thanks in advance! While you're at it I'd like to ask you to also consider these (smaller) PRs I made for wave.py that did not get much attention yet:
There's also this but it might warrant some rework after this IEEE PR is merged, I might want to make a new one for that |
|
I put them on my list :) |
This is an updated version of #102574, the original PR where @lkoenig added support for IEEE Wave Audio.
As requested by @encukou
I've
formatto getparamsCloses: #102574
📚 Documentation preview 📚: https://cpython-previews--145384.org.readthedocs.build/