-
Notifications
You must be signed in to change notification settings - Fork 261
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,7 +36,7 @@ using namespace std; | |
using namespace openshot; | ||
|
||
FrameMapper::FrameMapper(ReaderBase *reader, Fraction target, PulldownType target_pulldown, int target_sample_rate, int target_channels, ChannelLayout target_channel_layout) : | ||
reader(reader), target(target), pulldown(target_pulldown), is_dirty(true), avr(NULL) | ||
reader(reader), target(target), pulldown(target_pulldown), is_dirty(true), avr(NULL), parent_position(0.0) | ||
{ | ||
// Set the original frame rate from the reader | ||
original = Fraction(reader->info.fps.num, reader->info.fps.den); | ||
|
@@ -112,6 +112,16 @@ void FrameMapper::Init() | |
fields.clear(); | ||
frames.clear(); | ||
|
||
// Find parent position (if any) | ||
Clip *parent = (Clip *) ParentClip(); | ||
if (parent) { | ||
parent_position = parent->Position(); | ||
parent_start = parent->Start(); | ||
} else { | ||
parent_position = 0.0; | ||
parent_start = 0.0; | ||
} | ||
|
||
// Mark as not dirty | ||
is_dirty = false; | ||
|
||
|
@@ -263,8 +273,9 @@ void FrameMapper::Init() | |
|
||
while (remaining_samples > 0) | ||
{ | ||
// get original samples | ||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removing There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
// Enough samples | ||
if (original_samples >= remaining_samples) | ||
|
@@ -395,9 +406,20 @@ std::shared_ptr<Frame> FrameMapper::GetFrame(int64_t requested_frame) | |
// Create a scoped lock, allowing only a single thread to run the following code at one time | ||
const GenericScopedLock<CriticalSection> lock(getFrameCriticalSection); | ||
|
||
// Check if mappings are dirty (and need to be recalculated) | ||
// Find parent properties (if any) | ||
Clip *parent = (Clip *) ParentClip(); | ||
if (parent) { | ||
float position = parent->Position(); | ||
float start = parent->Start(); | ||
if (parent_position != position || parent_start != start) { | ||
// Force dirty if parent clip has moved or been trimmed | ||
// since this heavily affects frame #s and audio mappings | ||
is_dirty = true; | ||
} | ||
} | ||
|
||
// Check if mappings are dirty (and need to be recalculated) | ||
if (is_dirty) | ||
// Recalculate mappings | ||
Init(); | ||
|
||
// Check final cache a 2nd time (due to potential lock already generating this frame) | ||
|
There was a problem hiding this comment.
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