GH-60729: Add IEEE format wave audio support#145384
GH-60729: Add IEEE format wave audio support#145384mbeijen wants to merge 8 commits intopython:mainfrom
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 <[email protected]>
encukou
left a comment
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.
| 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:?
| 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.
| .. method:: getnchannels() | ||
|
|
||
| Return the number of channels. |
There was a problem hiding this comment.
Would you mind sending docs for existing methods in a separate PR, so they can be backported separately?
There was a problem hiding this comment.
Thank you!
Please merge (don't rebase) the main branch; pushing a rebase tends to mess PRs up.
| 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.)
| 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?
Co-authored-by: Petr Viktorin <[email protected]>
4 participants
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/