Feature: Add settings validation to FFmpegConfigManager
#95
Conversation
Adds settings validation for currently implemented FFmpeg options in `FFmpegConfigManager`. Specifically, the current audio options were tested against FFmpeg to determine which invalid settings would result in an error. In cases where FFmpeg would proceed despite an otherwise invalid codec option (ex. 0 channels), `MediaProcessor` will also decline to throw an error and allow FFmpeg to set sane defaults. In cases where FFmpeg would not allow a user to proceed we throw a runtime error with a helpful message for the user.
There was a problem hiding this comment.
Hi @andcscott, thanks a lot for submitting this!
I'd say it's in a pretty good shape already. Only serious thing we should take a look at is the setInputFilePath function, that I commented on. The rest are more for general styling and usage uniformity across the core. Please take a look and let me know if you have any feedback!
Should be good to go following these changes. Thanks again !
| void FFmpegConfigManager::setInputFilePath(const fs::path inputFilePath) { | ||
| m_globalSettings.inputFilePath = inputFilePath; | ||
| int read = access((inputFilePath.string().c_str()), R_OK); | ||
| if (read < 0 && errno == EACCES) { | ||
| throw std::runtime_error("Insufficient permissions to read input file!"); | ||
| } else if (read < 0) { | ||
| throw std::runtime_error("Unable to read input file, are you sure it exists?"); | ||
| } else { | ||
| m_globalSettings.inputFilePath = inputFilePath; | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you use the file system API here instead? this should have all you'd need to do the check.
| if (codec != VideoCodec::COPY) { | ||
| std::cerr << "Warning: Consider setting video codec to COPY if quality or conversion time are " | ||
| "unacceptable" << std::endl; | ||
| } |
There was a problem hiding this comment.
This took me a second to get it, so perhaps consider something like "Consider setting the video codec to COPY for higher quality and faster conversion times."
| // Validate Settings | ||
| void FFmpegConfigManager::validateAudio() { | ||
| switch (m_audioSettings.codec) { | ||
| case AudioCodec::AAC: | ||
| validateOption(m_audioSettings.numChannels, AAC_MAX_CHANNELS); | ||
| validateOption(m_audioSettings.sampleRate, AAC_SAMPLE_RATES); | ||
| break; | ||
| case AudioCodec::MP3: | ||
| validateOption(m_audioSettings.numChannels, MP3_MAX_CHANNELS); | ||
| validateOption(m_audioSettings.sampleRate, MP3_SAMPLE_RATES); | ||
| break; | ||
| case AudioCodec::FLAC: | ||
| validateOption(m_audioSettings.numChannels, FLAC_MAX_CHANNELS); | ||
| validateOption(m_audioSettings.sampleRate, FLAC_MAX_SAMPLE_RATE); | ||
| break; | ||
| case AudioCodec::OPUS: | ||
| validateOption(m_audioSettings.numChannels, OPUS_MAX_CHANNELS); | ||
| validateOption(m_audioSettings.sampleRate, OPUS_SAMPLE_RATES); | ||
| break; | ||
| case AudioCodec::UNKNOWN: | ||
| setAudioCodec(AudioCodec::AAC); | ||
| setAudioChannels(2); | ||
| setAudioSampleRate(48000); | ||
| std::cerr << "Warning: Audio codec cannot be UNKNOWN, default to AAC @ 48000 Hz with 2 " | ||
| "channels" | ||
| << std::endl; | ||
| break; | ||
| default: | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Quite okay to use switch case as we don't expect this list to grow (not too much at least), but mostly to reduce verbosity and make it more in line with the rest of the core, please consider implementing this with an unordered map, e.g. codecValidationRules that would map each codec to a struct that would hold the data needed for your validation (like max channels and sample rate).
The benefit would then be to simply try to find it in the map and handle the case it isn't found. This would also add some flexibility for when we might need to add rules for codec validation.
| template <typename T, std::size_t N> | ||
| void FFmpegConfigManager::validateOption(const T optionValue, | ||
| const std::array<T, N>& validOptions) { | ||
| for (const T& valid : validOptions) { | ||
| if (m_audioSettings.sampleRate == valid) return; | ||
| } | ||
| std::string codec = Utils::enumToString<AudioCodec>(getAudioCodec(), getAudioCodecAsString()); | ||
| std::ostringstream ssError; | ||
| ssError << "Invalid " + codec + " sample rate: FFmpeg supports "; | ||
| for (const T& opt : validOptions) { | ||
| ssError << opt << " "; | ||
| } | ||
| throw std::runtime_error(ssError.str()); | ||
| } |
There was a problem hiding this comment.
I would go with vector here for validOptions as it would make it a bit more flexible and keep the template parameters cleaner. Vector should be almost as good, with such a small data set and likely infrequent access patterns.
omeryusufyagci
added
improvement
Hi @omeryusufyagci, hope you're doing well!
This PR relates to #56 by adding settings validation for the current FFmpeg options available in
FFmpegConfigManager. More specifically, it attempts to address issues where the sample rate or number of audio channels could be unacceptable to FFmpeg. I reviewed the codecs rather thoroughly, but I did not consider other audio/video options for this PR primarily because they aren't currently needed/I'm unsure which you'd eventually like to add.In addition to referencing the current specs for the available codecs (AAC, MP3, FLAC, Opus) I also did some testing with FFmpeg to determine when it continue even when given an option outside the spec. In this way, the config manager should only throw an error when FFmpeg would truly panic, not when it proceeds with its own set of assumptions. I also reviewed cases where FFmpeg could throw an error even though the spec states it is valid (255 Opus channels could require a very complicated rematrix!).
Last, two template functions were added to help streamline the process of adding new options later. These templates address the two primary ways in which FFmpeg conceives of a valid option: either a maximum unsigned integer, or a list of acceptable unsigned integers.