Muxing Refactor and Coverage #81
Conversation
|
|
BrandenAvalonCx
left a comment
BrandenAvalonCx
left a comment
There was a problem hiding this comment.
Overall I like this approach but I think there's a bit of cleanup needed here
| const juce::String outputMuxdFile = exportData.getVideoExportFolder(); | ||
| // Checks for errors in the muxed file using ffmpeg and verifies presence of an | ||
| // IAMF audio stream and video stream | ||
| bool validateMuxedFile(const juce::String& path) { |
There was a problem hiding this comment.
Seems like this should be part of unit testing rather then in one of the main files?
There was a problem hiding this comment.
Worth noting that popen and pclose can't be used in Windows, so this won't build. Maybe this can be moved to a MacOS only build for now, or we can use something cross platform, though cross platform application execution is one of the reasons I don't really like this type of command execution.
| // Checks for errors in the muxed file using ffmpeg and verifies presence of an | ||
| // IAMF audio stream and video stream | ||
| bool validateMuxedFile(const juce::String& path) { | ||
| #ifndef ECLIPSA_FFMPEG_AVAILABLE |
There was a problem hiding this comment.
It's a bit odd to compile this in or out here rather then including or removing the unit tests. If you want to check here I would check in a non-compile based way, but I have no problem with not checking here either and letting the validator fail if FFMPEG is not present.
|
|
||
| FILE* pipe = popen(ffmpegCommand.toRawUTF8(), "r"); | ||
| if (!pipe) { | ||
| std::cout << "Failed to execute ffmpeg command" << std::endl; |
There was a problem hiding this comment.
Should these be log statements?
| std::string errStr = "IAMF Muxing: Failed to load audio file: " + | ||
| inputAudioFile.toStdString(); | ||
| LOG_ERROR(0, errStr); | ||
| LOG_ERROR(0, "IAMF Audio Muxing: Failed to load audio file."); |
There was a problem hiding this comment.
Can we output information about the file we're failing to load, to make debugging easier
| std::string errStr = "IAMF Muxing: Failed to load video file: " + | ||
| inputVideoFile.toStdString(); | ||
| LOG_ERROR(0, errStr); | ||
| LOG_ERROR(0, "Video Interleaving: Failed to load video file."); |
There was a problem hiding this comment.
Can we output information about the video file we're failing to load?
| if (!std::filesystem::exists(inputVideoFile.toStdString())) { | ||
| LOG_ERROR(0, "IAMF Muxing: Input video file" + | ||
| inputVideoFile.toStdString() + " does not exist."); | ||
| std::cout << "IAMF Muxing: Input video file" + |
There was a problem hiding this comment.
Shouldn't logging be enough here?
There was a problem hiding this comment.
Is something supposed to be here?
There was a problem hiding this comment.
Put this in third_party/gpac?
2 participants
Description
The old GPAC API usage was sometimes silently corrupting output muxed video files. This seems to have been somewhat dependent on the shape of the input IAMF files.
Although the GPAC CLI uses identical filters as the API here, it split muxing into 2 stages: audio writes then video writes. This PR replicates that flow in the API usage. This seems to resolve NAL errors in output MP4s.
Changes
ffprobeto better verify output muxed files..mp4and.movcontainers with H264, H265, and AV1 encoded video streams.Validation and Acceptance Criteria