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.

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

Feature: Add settings validation to FFmpegConfigManager #95

Open
wants to merge 1 commit into
base: 60-feature-add-ffmpegcontroller-to-core
Choose a base branch
from
Open

Feature: Add settings validation to FFmpegConfigManager #95

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
87 changes: 81 additions & 6 deletions MediaProcessor/src/FFmpegConfigManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
#include "FFmpegConfigManager.h"

#include <unistd.h>

#include <filesystem>
#include <iostream>
#include <stdexcept>

namespace MediaProcessor {

FFmpegConfigManager::FFmpegConfigManager() {
Expand All @@ -9,11 +15,9 @@ FFmpegConfigManager::FFmpegConfigManager() {
{AudioCodec::OPUS, "opus"},
{AudioCodec::UNKNOWN, "unknown"}};

m_videoCodecAsString = {{VideoCodec::H264, "libx264"},
{VideoCodec::H265, "libx265"},
{VideoCodec::VP8, "libvpx"},
{VideoCodec::VP9, "libvpx-vp9"},
{VideoCodec::UNKNOWN, "unknown"}};
m_videoCodecAsString = {{VideoCodec::H264, "libx264"}, {VideoCodec::H265, "libx265"},
{VideoCodec::VP8, "libvpx"}, {VideoCodec::VP9, "libvpx-vp9"},
{VideoCodec::COPY, "copy"}, {VideoCodec::UNKNOWN, "unknown"}};

m_codecStrictnessAsString = {{CodecStrictness::VERY, "very"},
{CodecStrictness::STRICT, "strict"},
Expand All @@ -32,10 +36,18 @@ void FFmpegConfigManager::setCodecStrictness(CodecStrictness strictness) {
}

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;
}
}
Comment on lines 38 to 47
Copy link
Owner

Choose a reason for hiding this comment

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

Can you use the file system API here instead? this should have all you'd need to do the check.


void FFmpegConfigManager::setOutputFilePath(const fs::path outputFilePath) {
Utils::ensureDirectoryExists(outputFilePath);
m_globalSettings.outputFilePath = outputFilePath;
}

Expand Down Expand Up @@ -84,6 +96,10 @@ int FFmpegConfigManager::getAudioChannels() const {
// Video Setters
void FFmpegConfigManager::setVideoCodec(VideoCodec codec) {
m_videoSettings.codec = codec;
if (codec != VideoCodec::COPY) {
std::cerr << "Warning: Consider setting video codec to COPY if quality or conversion time are "
"unacceptable" << std::endl;
}
Comment on lines +99 to +102
Copy link
Owner

Choose a reason for hiding this comment

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

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."

}

// Video Getters
Expand Down Expand Up @@ -116,4 +132,63 @@ void FFmpegConfigManager::updateSettings(const struct FFmpegGlobalSettings& glob
m_videoSettings = videoSettings;
}

// Validate Settings
template <typename T>
void FFmpegConfigManager::validateOption(const T optionValue, const T optionMax) {
if (optionValue > optionMax) {
std::string codec =
Utils::enumToString<AudioCodec>(getAudioCodec(), getAudioCodecAsString());
throw std::runtime_error("Invalid Audio Option \"" + std::to_string(optionValue) +
"\" : FFmpeg accepts a maximum of " + std::to_string(optionMax) +
" for " + codec);
}
}

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());
}
Comment on lines +147 to +160
Copy link
Owner

Choose a reason for hiding this comment

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

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.


// 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;
}
}
Comment on lines +162 to +192
Copy link
Owner

Choose a reason for hiding this comment

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

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.


} // namespace MediaProcessor
30 changes: 28 additions & 2 deletions MediaProcessor/src/FFmpegConfigManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,14 @@
#include <vector>

#include "ConfigManager.h"
#include "Utils.h"

namespace fs = std::filesystem;

namespace MediaProcessor {

enum class AudioCodec { AAC, MP3, FLAC, OPUS, UNKNOWN };
enum class VideoCodec { H264, H265, VP8, VP9, UNKNOWN };
enum class VideoCodec { H264, H265, VP8, VP9, COPY, UNKNOWN };
enum class CodecStrictness { VERY, STRICT, NORMAL, UNOFFICIAL, EXPERIMENTAL };

/**
Expand Down Expand Up @@ -82,7 +83,7 @@ class FFmpegConfigManager {
struct FFmpegVideoSettings {
VideoCodec codec;

FFmpegVideoSettings() : codec(VideoCodec::H264) {}
FFmpegVideoSettings() : codec(VideoCodec::COPY) {}
} m_videoSettings;

// Update Global, Audio, or Video Settings
Expand All @@ -94,6 +95,31 @@ class FFmpegConfigManager {
std::unordered_map<AudioCodec, std::string> m_audioCodecAsString;
std::unordered_map<VideoCodec, std::string> m_videoCodecAsString;
std::unordered_map<CodecStrictness, std::string> m_codecStrictnessAsString;

// Channel # allowed options
static constexpr const int AAC_MAX_CHANNELS = 8;
static constexpr const int MP3_MAX_CHANNELS = 2;
static constexpr const int FLAC_MAX_CHANNELS = 8;
static constexpr const int OPUS_MAX_CHANNELS = 8;

// Sample rate allowed options
static constexpr const std::array<int, 13> AAC_SAMPLE_RATES = {
7350, 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000, 64000, 88200, 96000};
static constexpr const std::array<int, 9> MP3_SAMPLE_RATES = {8000, 12000, 11025, 16000, 24000,
22050, 32000, 44100, 48000};
static constexpr const std::array<int, 5> OPUS_SAMPLE_RATES = {8000, 12000, 16000, 24000,
48000};
static constexpr const int FLAC_MAX_SAMPLE_RATE = 1048575;

template <typename T, std::size_t N>
void validateOption(const T optionValue, const std::array<T, N>& validOptions);

template <typename T>
void validateOption(const T optionValue, const T optionMax);

void validateAudio();

void validateVideo();
};

} // namespace MediaProcessor
Expand Down
2 changes: 0 additions & 2 deletions MediaProcessor/src/FFmpegController.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ bool FFmpegController::extractAudio() {
}

std::vector<fs::path> FFmpegController::splitMedia(int numChunks, double overlapDuration) {
Utils::ensureDirectoryExists(m_ffmpegConfig.getOutputFilePath());

std::vector<double> chunkStartTimes;
std::vector<double> chunkDurations;
std::vector<fs::path> chunkPathCol;
Expand Down