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 Frame::GetSamplesPerFrame when channels = 0 #930

Merged
merged 1 commit into from
Jun 2, 2023

Conversation

kxxt
Copy link
Contributor

@kxxt kxxt commented May 30, 2023

This function is relying on the implementation defined behavior of x86_64 to work properly.

On x86_64, fmod returns -NaN when channels = 0, it is handled by if (samples_per_frame < 0).

But on other architectures(like riscv64), fmod may return a positive NaN which will cause this function to return a bad value.

It is causing several tests to spin forever on riscv64.

This PR fixes it by directly return 0 when channels == 0 so that we do not need to deal with NaNs.

This function is relying on the implementation defined
behavior of x86_64 to work properly.

On x86_64,  `fmod` returns -NaN when `channels = 0`, it is handled by
`if (samples_per_frame < 0)`.

But on other architectures(like riscv64), `fmod` may return a positive
NaN which will cause this function to return a bad value.

It is causing several tests to spin forever on riscv64.

This PR fixes it by directly return 0 when `channels == 0` so that we do
not need to deal with NaNs.
kxxt added a commit to kxxt/archriscv-packages that referenced this pull request May 30, 2023
felixonmars pushed a commit to felixonmars/archriscv-packages that referenced this pull request May 31, 2023
@jonoomph
Copy link
Member

jonoomph commented Jun 2, 2023

Thanks! LGTM!

@jonoomph jonoomph merged commit 919f2aa into OpenShot:develop Jun 2, 2023
7 checks passed
@kxxt kxxt deleted the fix/Frame/GetSamplesPerFrame branch June 15, 2023 13:20
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.

None yet

2 participants