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 WavDecoder::total_duration. #699

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

IohannRabeson
Copy link
Contributor

It was ignoring the channels count.

@roderickvd
Copy link
Collaborator

D'oh! Pointy hat to me, good one. I probably copied the same mistake in the lewton FLAC decoder too:

let sample_rate = sample_rate as u64;

@IohannRabeson
Copy link
Contributor Author

Ok, I'm checking and I will fix it.

@dvdsk
Copy link
Collaborator

dvdsk commented Feb 10, 2025

D'oh! Pointy hat to me, good one. I probably copied the same mistake in the lewton FLAC decoder too:

you've got this (very nice) comment there that points to the opposite:

        // `samples` in FLAC means "inter-channel samples" aka frames
        // so we do not divide by `self.channels` here.

It was ignoring the channels count.
@IohannRabeson
Copy link
Contributor Author

I renamed sample_rate to data_rate and I removed the commit that was modifying FlacDecoder.

@roderickvd
Copy link
Collaborator

D'oh! Pointy hat to me, good one. I probably copied the same mistake in the lewton FLAC decoder too:

you've got this (very nice) comment there that points to the opposite:

        // `samples` in FLAC means "inter-channel samples" aka frames
        // so we do not divide by `self.channels` here.

🤦‍♂️ I will go to bed now 😄

@dvdsk
Copy link
Collaborator

dvdsk commented Feb 10, 2025

I'm gonna go and add some integration tests to prevent this from ever regressing. Looks good to merge for me

@dvdsk
Copy link
Collaborator

dvdsk commented Feb 10, 2025

🤦‍♂️ I will go to bed now 😄

I should follow your example, I just spend some time writing total duration tests before realizing I already did that... three days ago (to be merged as part of #697).

@dvdsk dvdsk merged commit 70d01da into RustAudio:master Feb 10, 2025
9 checks passed
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