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

FrameMapper Audio Crackles/Popping Fixes #653

Merged
merged 3 commits into from Apr 9, 2021
Merged

Conversation

jonoomph
Copy link
Member

@jonoomph jonoomph commented Apr 9, 2021

Added new unit test which distributes audio samples between many different frame rates (30/1, 24/1, 30000/1001, 119/4), and fixes a huge issue with mapping frame numbers incorrectly causing audio crackles/pops. Also fixes a bug which causes crashes on NON-STEREO channel layouts.

For example, when using a source video with a 119/4 frame rate (29.75 fps) and converting it to 30 fps (and positioning the Clip at position 3.77 seconds), it would generate a huge amount of high frequency pops, due to incorrect frame # adjustments inside FrameMapper.

…erent framerates (30/1, 24/1, 30000/1001, 119/4), and fixes a huge issue with mapping frame numbers incorrectly causing audio crackles/pops. Also fixes a bug which causes crashes on NON-STEREO channel layouts.
@@ -1683,7 +1681,7 @@ void FFmpegWriter::write_audio_packets(bool is_final) {
);

// Set remaining samples
remaining_frame_samples = nb_samples * av_get_bytes_per_sample(AV_SAMPLE_FMT_S16);
remaining_frame_samples = total_frame_samples;
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes a bug where MONO channel layout crashes libopenshot

int original_samples = Frame::GetSamplesPerFrame(AdjustFrameNumber(end_samples_frame), original, reader->info.sample_rate, reader->info.channels) - end_samples_position;
// Get original samples (with NO framerate adjustments)
// This is the original reader's frame numbers
int original_samples = Frame::GetSamplesPerFrame(end_samples_frame, original, reader->info.sample_rate, reader->info.channels) - end_samples_position;
Copy link
Member Author

Choose a reason for hiding this comment

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

Removing AdjustFrameNumber() method from original samples calculation, since this access the original reader, and is always in original frame #'s... never mapped to the Timeline offset.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great! Does this also fix the export issue? When we were getting deformed waveforms when exporting to different FPS?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably! But I would love to test that again. The new unit test had hundreds of thousands of failures without this fix. And zero failures with it. But I didn't notice the failures until the Timeline merged the audio, and some frames had the wrong # of samples, and thus introduced some high frequency pops.

@codecov
Copy link

codecov bot commented Apr 9, 2021

Codecov Report

Merging #653 (2e57d36) into develop (b232919) will increase coverage by 0.28%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #653      +/-   ##
===========================================
+ Coverage    51.75%   52.04%   +0.28%     
===========================================
  Files          151      151              
  Lines        12255    12315      +60     
===========================================
+ Hits          6343     6409      +66     
+ Misses        5912     5906       -6     
Impacted Files Coverage Δ
src/CVObjectDetection.cpp 0.00% <ø> (ø)
src/CVStabilization.cpp 88.17% <ø> (-0.31%) ⬇️
src/CVTracker.cpp 80.95% <ø> (-0.26%) ⬇️
src/FrameMapper.h 100.00% <ø> (ø)
src/effects/Stabilizer.cpp 0.00% <ø> (ø)
src/effects/Tracker.cpp 0.00% <ø> (ø)
src/FFmpegWriter.cpp 59.67% <100.00%> (-0.04%) ⬇️
src/FrameMapper.cpp 91.89% <100.00%> (+0.24%) ⬆️
tests/FrameMapper_Tests.cpp 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b232919...2e57d36. Read the comment docs.

@jonoomph jonoomph merged commit 862a91e into develop Apr 9, 2021
@jonoomph jonoomph deleted the audio-crackles-mapper branch April 9, 2021 04:01
@ferdnyc
Copy link
Contributor

ferdnyc commented Apr 9, 2021

@jonoomph
The new unit test — like the ones it's based on — leaks memory like a sieve, since it's allocating audio buffers in a loop, but never frees any of that memory. new without delete is always a red flag.

There needs to be a delete[] audio_buffer; at the end of each loop iteration, after the samples are copied into the Frame objects using AddAudio().

I fixed all of them while I was porting the tests to Catch2, so when that gets merged (hopefully ASAP, just had some fine-tuning to do) it'll be sorted out. Just an FYI.

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

3 participants