Skip to content

Improve Player File Callback Handling #83

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.

Sign up for GitHub

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

Closed
joelmacx wants to merge 1 commit into from
Closed

Improve Player File Callback Handling #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions common/components/src/AudioFilePlayer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,9 +265,23 @@ void AudioFilePlayer::valueTreePropertyChanged(
triggerAsyncUpdate();
} else if (property == FilePlayback::kPlaybackFile) {
attemptCreatePlaybackEngine();
} else if (property == FileExport::kExportCompleted &&
fer_.get().getExportCompleted()) {
attemptCreatePlaybackEngine();
} else if (property == FileExport::kExportCompleted) {
// When this property is false a new export is starting, so we want to
// destroy the player and wait until export is complete.
// When this property is true we want to attempt to create the playback
// engine again.
if (fer_.get().getExportCompleted()) {
attemptCreatePlaybackEngine();
} else {
// Destroy the playback engine
playbackEngine_ = nullptr;
// Join any existing creation thread
isBeingDestroyed_ = true;
if (playbackEngineLoaderThread_.joinable()) {
playbackEngineLoaderThread_.join();
}
isBeingDestroyed_ = false;
}
}
}

Expand Down Expand Up @@ -300,6 +314,12 @@ void AudioFilePlayer::attemptCreatePlaybackEngine() {

if (kFileToLoad.empty() || kFileToLoad.extension() != ".iamf" ||
!std::filesystem::exists(kFileToLoad)) {
// If the file doesn't exist or it's a new file, we set the player to a
Copy link
Collaborator

@BrandenAvalonCx BrandenAvalonCx Dec 10, 2025

Choose a reason for hiding this comment

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

I don't understand how this is checking for a "new" file. Do you mean an empty file?

Copy link
Collaborator Author

@joelmacx joelmacx Dec 10, 2025

Choose a reason for hiding this comment

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

If the path to the given output file does not exist, export must be to a new file not rewriting an existing file.
How would an empty iamf file exist here besides maybe a crash during export?

// stopped state
auto playbackState = fpbr_.get();
playbackState.setPlayState(FilePlayback::kStop);
fpbr_.update(playbackState);
playbackEngine_ = nullptr;
return;
}
createPlaybackEngine(kFileToLoad);
Expand Down Expand Up @@ -346,7 +366,8 @@ void AudioFilePlayer::onPlaybackEngineCreated(IAMFPlaybackDevice::Result res) {
auto fpb = fpbr_.get();
fpb.setPlayState(FilePlayback::kDisabled);
fpbr_.update(fpb);
} else if (res.error == IAMFPlaybackDevice::kEarlyAbortRequested) {
} else if (res.error == IAMFPlaybackDevice::kEarlyAbortRequested ||
isBeingDestroyed_) {
// Do nothing - destruction was requested
} else {
playbackEngine_ = std::move(res.device);
Expand Down
13 changes: 3 additions & 10 deletions rendererplugin/src/screens/FileExportScreen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,6 @@ FileExportScreen::FileExportScreen(MainEditor& editor,
return;
}
config.setManualExport(!config.getManualExport());
repository_->update(config);
if (config.getManualExport()) {
startTimer_.setEnabled(false);
endTimer_.setEnabled(false);
Expand All @@ -501,7 +500,7 @@ FileExportScreen::FileExportScreen(MainEditor& editor,
exportVideoFolder_.setEnabled(false);
browseVideoButton_.setEnabled(false);
browseVideoSourceButton_.setEnabled(false);

config.setExportCompleted(false);
} else {
startTimer_.setEnabled(true);
endTimer_.setEnabled(true);
Expand All @@ -517,15 +516,9 @@ FileExportScreen::FileExportScreen(MainEditor& editor,
exportVideoFolder_.setEnabled(true);
browseVideoButton_.setEnabled(true);
browseVideoSourceButton_.setEnabled(true);

if (!config.getExportCompleted()) {
Copy link
Collaborator Author

@joelmacx joelmacx Dec 9, 2025

Choose a reason for hiding this comment

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

I removed this to be consistent with how we update the exportCompleted field. I didn't write this so I'm unsure if this a problem for any DAWs @BrandenAvalonCx?

Copy link
Collaborator

@BrandenAvalonCx BrandenAvalonCx Dec 10, 2025

Choose a reason for hiding this comment

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

This is specifically needed for Premiere Pro. It looks like this change may be fine, but it should be tested on Premiere Pro to verify that it's okay to just update to true without checking the exportCompleted flag has been set.

// Normally this is handled by the file export processor
// But in manual button cases where the processor is destroyed we need
// to flag it here
config.setExportCompleted(true);
repository_->update(config);
}
config.setExportCompleted(true);
}
repository_->update(config);
repaint();
};
LOG_ANALYTICS(RendererProcessor::instanceId_, "FileExportScreen initiated.");
Expand Down