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

Add missing surround sound identifiers #449

Merged
merged 3 commits into from
Feb 3, 2025

Conversation

messmerd
Copy link
Contributor

@messmerd messmerd commented Feb 1, 2025

This PR adds two missing surround sound identifiers: "Side Left Height" (or top middle left) and "Side Right Height" (or top middle right) as seen in the Dolby 7.1.6 specification which was mentioned in #407.

"Top Center" (CLAP_SURROUND_TC) already exists so I don't think a 3rd identifier is needed.
This addition should be backwards compatible.

As a disclaimer, I do not know anything about surround sound, so someone familiar with it should look over this PR.

Closes #407

@abique
Copy link
Contributor

abique commented Feb 3, 2025

Thanks!

@abique
Copy link
Contributor

abique commented Feb 3, 2025

@roelofvkr maybe you want to approve this change?

@abique abique self-requested a review February 3, 2025 10:28
@abique abique self-assigned this Feb 3, 2025
@roelofvkr
Copy link
Contributor

Thanks for checking! This also looks good to me and the current spec be enough to support up to 9.1.6.

I did see dolby has one more spec for 11.1.8, which would require more additions (it seems like they have another set of rear speakers and another set of top rear/side speakers), but we don't support that and I couldn't really find any other software or plugin format that does.

For me this is good, but I did want to mention the 11.1.8 spec for completeness' sake.

@messmerd
Copy link
Contributor Author

messmerd commented Feb 3, 2025

Do you think any of the identifier comments should be changed? (whether for this PR's additions or older identifiers)

I ask because there are abbreviations like "TSL" which stands for "Top Side Left", but the comment next to it says "Side Left Height". "Top"/"Height" and "Back"/"Rear" are used interchangeably.

@abique
Copy link
Contributor

abique commented Feb 3, 2025

Do you think any of the identifier comments should be changed? (whether for this PR's additions or older identifiers)

I ask because there are abbreviations like "TSL" which stands for "Top Side Left", but the comment next to it says "Side Left Height". "Top"/"Height" and "Back"/"Rear" are used interchangeably.

I think they should be updated so they include both common descriptions.

@messmerd
Copy link
Contributor Author

messmerd commented Feb 3, 2025

I think they should be updated so they include both common descriptions.

Done

@abique
Copy link
Contributor

abique commented Feb 3, 2025

Thank you very much, great work :-)

@abique abique merged commit 3179414 into free-audio:next Feb 3, 2025
4 checks passed
@messmerd messmerd deleted the surround-sound-mask branch February 4, 2025 04:20
abique pushed a commit that referenced this pull request Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants