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

fix 96kbps playback issue #1434

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Conversation

fivebanger
Copy link
Contributor

Fixes 96kbps issues (fixes #1433 (comment))

@photovoltex
Copy link
Member

Oh damn it. These types where removed by the protobuf update. Anyhow, wouldn't it be better to re-add the types discussed here #1236 (comment)? And could we add an comment somewhere in the proto file, so that we don't remove it accidentally again?

@kingosticks
Copy link
Contributor

Maybe we could come up with a test that just checks all the format types arer there?

@fivebanger
Copy link
Contributor Author

Anyhow, wouldn't it be better to re-add the types discussed here #1236 (comment)? And could we add an comment somewhere in the proto file, so that we don't remove it accidentally again?

Unfortunately I still have no clue where all those type definitions come from and if they are valid or not. I thought the missing definitions have been removed by intention. I'll add

        AAC_160 = 10;
        AAC_320 = 11;
        MP4_128 = 12;
        OTHER5 = 13;

later on again.

@photovoltex
Copy link
Member

Is the unknown type still required then?

@fivebanger
Copy link
Contributor Author

fivebanger commented Dec 31, 2024

Is the unknown type still required then?

What I remember from the original discussion the UNKNOWN should act as a default value, otherwise we may run into issues again (especially for 96kbps, since that's the first value and gets overwritten by unknown types). Pls. check #1236 (comment) and following. Since the UNKNOWN seems not to harm anything I would vote for keeping this approach, even though I have checked that my examples would also work without the UNKNOWN definition. According to my understanding, any new (and therefore unknown) type definition which is not covered by the .proto definition would overwrite the 96kbps file_id again and 96kbps streams would not play.

Edit:
After recalling all the details from the original discussions, I think the question is not whether to keep or not to keep the UNKNOWN approach (we should keep it). The question is which definitions in the .proto files may suffer from the same issue in case Spotify returns a value that is not covered by a .proto type definition and gets mapped to default (thereby erroneously overwriting the first value).

@photovoltex
Copy link
Member

Personally I would prefer to not modify the proto files from the extracted version. But as long as it's working and is tested I would also be fine as it is.

So @fivebanger please add a small test that checks for the default value and the additional types so that we don't run into the problem again when we update the proto files. It would also be helpful to have a reference to the initial issue or PR somewhere in a comment around the test.

@photovoltex photovoltex self-assigned this Jan 6, 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.

Fail to play 96kbps
3 participants