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 failing reference tests 3341/13 and 3341/14 #10

Closed
sdroege opened this issue Aug 12, 2020 · 4 comments
Closed

Fix failing reference tests 3341/13 and 3341/14 #10

sdroege opened this issue Aug 12, 2020 · 4 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@sdroege
Copy link
Owner

sdroege commented Aug 12, 2020

See #9

@sdroege sdroege added bug Something isn't working help wanted Extra attention is needed labels Aug 12, 2020
@l0calh05t
Copy link
Contributor

l0calh05t commented Sep 5, 2020

I had a quick look into this, and I have a question: Where does your chunk size of 100 ms come from? I ask, because if I go down to a chunk size of 10 ms (20 ms also seem to work, anything larger causes the tests to fail again) like so:

        for chunk in samples.chunks(2 * 48_000 / 100) {
            e.add_frames_i32(chunk).expect("Failed to analyze samples");

            let loudness = e
                .loudness_momentary()
                .expect("Failed to get momentary loudness");
            if loudness > max_loudness {
                max_loudness = loudness;
            }
        }

The tests pass (the 8s in the second test have to be replaced by 80s obviously).

l0calh05t added a commit to l0calh05t/ebur128 that referenced this issue Sep 5, 2020
@l0calh05t
Copy link
Contributor

Just for clarification, I looked at the chunk size because these tests ask for a maximum momentary loudness and while even momentary loudness has a 400ms window, it is still a sliding window and the maximum can occur at any point. loudness_momentary returns the current loudness at the end of a chunk, but the maximum could have been at any point between.

(I didn't check the i%8 expressions in the second test in too much detail, it may no longer represent exactly what the spec mentions)

@sdroege
Copy link
Owner Author

sdroege commented Sep 5, 2020

Makes sense. I didn't put much thought into the chunk size :) can you update the PR to use 10ms? That seems like a nicer number to understand the calculations around it.

Thanks!

l0calh05t added a commit to l0calh05t/ebur128 that referenced this issue Sep 5, 2020
@l0calh05t
Copy link
Contributor

Makes sense. I didn't put much thought into the chunk size :) can you update the PR to use 10ms? That seems like a nicer number to understand the calculations around it.

Thanks!

Done 🙂

@sdroege sdroege closed this as completed in cceff80 Sep 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants