Skip to content

Handle/Ignore unknown audio formats #1467

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

Merged
merged 4 commits into from
Apr 13, 2025

Conversation

photovoltex
Copy link
Member

Fixes the incorrect parsing when a audio format is unknown. See explanation here #1434 (comment).

Fixes #1433

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request addresses the handling of unknown audio formats to prevent incorrect parsing. The changes include using pattern matching on audio format values, logging ignored cases for both unknown and unspecified formats, and updating the changelog with the new behavior.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
metadata/src/audio/file.rs Updated audio format parsing to match and log unknown formats
CHANGELOG.md Added an entry summarizing the fix for incorrect audio format parsing

Copy link
Member

@roderickvd roderickvd left a comment

Choose a reason for hiding this comment

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

Ignoring unknown file formats is principally OK (although the Symfonia decoder may still be able to probe and find a correct codec for it, if it's compiled in), but:

w.r.t. the issue you're linking to, are there any other codecs we should add support for? I believe Spotify supports podcasts in MP3, M4A, and WAV. With M4A they probably mean AAC in an MP4 container.

@photovoltex
Copy link
Member Author

Ignoring unknown file formats is principally OK (although the Symfonia decoder may still be able to probe and find a correct codec for it, if it's compiled in)

The problem we have is that we previously mapped incorrect files to some formats. So if we tried to load file x with format z but it actually required y it wouldn't work. I think ignoring unknowns is the better way to go.

are there any other codecs we should add support for?

If we want to handle the unknown we should handle them inside this mapper. But I would like to keep the protobuf definitions as close to the original as possible to prevent these issues when updating the protobuf definitions.

I will take a look at supporting the remaining codecs that were removed with the protobuf update :)

@photovoltex
Copy link
Member Author

I believe Spotify supports podcasts in MP3, M4A, and WAV. With M4A they probably mean AAC in an MP4 container.

For now I just added the know types from #1236 (comment) back. With our own enum for the format it should be easier to add more unknown format types in the future if anyone finds one that he would like to add.

@roderickvd
Copy link
Member

Ignoring unknown file formats is principally OK (although the Symfonia decoder may still be able to probe and find a correct codec for it, if it's compiled in)

The problem we have is that we previously mapped incorrect files to some formats. So if we tried to load file x with format z but it actually required y it wouldn't work. I think ignoring unknowns is the better way to go.

I agree. Then (optionally?) compiling in other codecs with Symphonia automagically plays back whatever you throw at it.

@photovoltex
Copy link
Member Author

Hmm might be a good idea if that would work, but I don't think the current version has any logic for that in place or does it?

@photovoltex photovoltex requested a review from roderickvd March 23, 2025 14:06
@roderickvd
Copy link
Member

Hmm might be a good idea if that would work, but I don't think the current version has any logic for that in place or does it?

Yes, I see that currently we've hardcoded to either use an Ogg reader with Vorbis decoder, or an MPA reader/decoder. It would in fact be easier to just let Symphonia probe. That way, it loads whatever you throw at it, as long as you've got the reader/decoders compiled in.

Copy link
Member

@roderickvd roderickvd left a comment

Choose a reason for hiding this comment

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

LGTM - my previous comment can be saved for another PR. I still think it'd be a good idea to have support for WAV & M4A, because Spotify specifically mentions them as supported podcast formats.

@photovoltex photovoltex merged commit 59381cc into librespot-org:dev Apr 13, 2025
13 checks passed
@photovoltex photovoltex deleted the fix-incorrect-parsing branch April 13, 2025 06:37
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.

Fail to play 96kbps
2 participants