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

Jump to bottom

Supporting extensible format(PCM) for wave.open(read-mode) #77171

Closed
acelletti mannequin opened this issue Mar 3, 2018 · 6 comments
Closed

Supporting extensible format(PCM) for wave.open(read-mode) #77171

acelletti mannequin opened this issue Mar 3, 2018 · 6 comments
Labels
3.12 bugs and security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@acelletti
Copy link
Mannequin

acelletti mannequin commented Mar 3, 2018

BPO 32990
Nosy @ned-deily, @serhiy-storchaka, @ZackerySpytz, @acelletti
PRs
  • bpo-32990: Support WAVE_FORMAT_EXTENSIBLE in the wave module #9515
    		•
    Files
  • pluck-pcm24-ext.wav: PCM 24b with FORMAT_EXTENSIBLE
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = None
created_at = <Date 2018-03-03.09:25:58.701>
labels = ['3.8', 'type-feature', 'library']
title = 'Supporting extensible format(PCM) for wave.open(read-mode)'
updated_at = <Date 2018-09-23.13:35:55.962>
user = 'https://github.com/acelletti'

    bugs.python.org fields:

    activity = <Date 2018-09-23.13:35:55.962>
actor = 'ZackerySpytz'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2018-03-03.09:25:58.701>
creator = 'acelletti'
dependencies = []
files = ['47467']
hgrepos = []
issue_num = 32990
keywords = ['patch']
message_count = 4.0
messages = ['313183', '313190', '313191', '325477']
nosy_count = 4.0
nosy_names = ['ned.deily', 'serhiy.storchaka', 'ZackerySpytz', 'acelletti']
pr_nums = ['9515']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue32990'
versions = ['Python 3.8']
    @acelletti
    Copy link
    Mannequin Author

    acelletti mannequin commented Mar 3, 2018

    The wave.Wave_read class currently supports 8, 16, 24, and 32 bit PCM files.
    Wave files are only supported if the wFormatTag in the format chunk matches the flag WAVE_FORMAT_PCM, which is correct but incomplete for 24 bit files.

    According to the specification the WAVE_FORMAT_EXTENSIBLE format should be used whenever the actual number of bits/sample is not equal to the container size. Based on this specification, most applications export 24 bit PCM with the WAVE_FORMAT_EXTENSIBLE flag since 24 is stored in container size 32.
    Importing these files causes wave.open to raise an exception.

    The specification also explains how to detect 24PCM exported in this fashion as "The first two bytes of the GUID form the sub-code specifying the data format code, e.g. WAVE_FORMAT_PCM.". In essence, we have to look at the first two bytes of the SubFormat tag and that will tell us if this file is PCM.

    Based on this premise, it appears to me that there is no reason for not adding support for both format specification as the rest of the file is exactly the same for both.

    I am attaching a file that can be used to test the exception being raised.

    Source: http://www-mmsp.ece.mcgill.ca/Documents/AudioFormats/WAVE/WAVE.html

    @acelletti acelletti mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Mar 3, 2018
    @serhiy-storchaka serhiy-storchaka added the 3.8 only security fixes label Mar 3, 2018
    @acelletti
    Copy link
    Mannequin Author

    acelletti mannequin commented Mar 3, 2018

    I am currently working on a patch for this.

    When done can I try pushing this in 3.7 rather than 3.8?

    It is not much of an enhancement but rather fixing the code that raises an error for a perfectly valid PCM wave file (bugfix).

    @acelletti acelletti mannequin removed the 3.8 only security fixes label Mar 3, 2018
    @ned-deily
    Copy link
    Member

    Thanks for working on this. Please follow the process in our Developers Guide and submit a PR against the master branch (for 3.8). After a core developer reviews it and if it is accepted, we can then decide about backports to other release branches. Also, if you haven't already, please make sure to read the section on licensing and sign a contributor agreement:

    https://devguide.python.org/pullrequest/

    @ned-deily ned-deily added the 3.8 only security fixes label Mar 3, 2018
    @ZackerySpytz
    Copy link
    Mannequin

    ZackerySpytz mannequin commented Sep 16, 2018

    Andrea, are you still working on this issue? If not, I will submit a PR.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @bedevere-bot bedevere-bot mentioned this issue Sep 12, 2022
    Merged
    @zooba zooba closed this as completed Sep 14, 2022
    zooba pushed a commit that referenced this issue Sep 14, 2022
    @ykadowak @ZackerySpytz
    gh-77171 Support WAVE_FORMAT_EXTENSIBLE in the wave module (GH-96777)
    92338f8 
    The test file, a modified version of Lib/test/audiodata/pluck-pcm24.wav, was provided by Andrea Celletti on the bug tracker.

Co-authored-by: Zackery Spytz <zspytz@gmail.com>
    @zooba
    Copy link
    Member

    zooba commented Sep 23, 2022

    Reopening this to update docs to limit our support to only PCM files.

    @zooba zooba reopened this Sep 23, 2022
    @zooba zooba added 3.12 bugs and security fixes and removed 3.8 only security fixes labels Sep 23, 2022
    @zooba
    Copy link
    Member

    zooba commented Sep 23, 2022

    Also to fix the SubFormat check. On further reading, I see that we need to check the entire SubFormat GUID, not just the last two bytes, as it could potentially be a non-standard format.

    zooba added a commit to zooba/cpython that referenced this issue Sep 23, 2022
    @zooba
    pythongh-77171: Fixes SubFormat check to compare the entire value. Ad…
    791b3f2 
    …d docs
    This was referenced Sep 23, 2022
    Merged
    Merged
    zooba added a commit to zooba/cpython that referenced this issue Sep 23, 2022
    @zooba
    pythongh-77171: Document that wave only supports simple PCM files
    d1abc59
    zooba added a commit that referenced this issue Sep 23, 2022
    @zooba
    gh-77171: Document that wave only supports simple PCM files (GH-97510)
    dc9065f
    miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 23, 2022
    @zooba @miss-islington
    pythongh-77171: Document that wave only supports simple PCM files (py…
    bda5623 
    …thonGH-97510)

(cherry picked from commit dc9065f)

Co-authored-by: Steve Dower <steve.dower@python.org>
    @bedevere-bot bedevere-bot mentioned this issue Sep 23, 2022
    Merged
    miss-islington added a commit that referenced this issue Sep 23, 2022
    @miss-islington @zooba
    gh-77171: Document that wave only supports simple PCM files (GH-97510)
    2a50772 
    (cherry picked from commit dc9065f)

Co-authored-by: Steve Dower <steve.dower@python.org>
    zooba added a commit that referenced this issue Sep 23, 2022
    @zooba
    gh-77171: Fixes SubFormat check to compare the entire value. Add docs (…
    a4ac14f 
    …GH-97509)
    @zooba zooba closed this as completed Sep 23, 2022
    pablogsal pushed a commit that referenced this issue Oct 22, 2022
    @zooba @pablogsal
    gh-77171: Document that wave only supports simple PCM files (GH-97510)
    00e8a27
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.12 bugs and security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @zooba @serhiy-storchaka @ned-deily