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

Jump to bottom

WIP: Improved Profile file parsing and error handling #493

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

WIP: Improved Profile file parsing and error handling #493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3343dd2
Improved Profile parsing
kartchnb Apr 3, 2020
ccc31c1
Rethought my changes to KeyFrame
kartchnb Apr 3, 2020
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
5 changes: 4 additions & 1 deletion src/KeyFrame.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,15 @@ namespace {

double InterpolateBetween(Point const & left, Point const & right, double target, double allowed_error) {
assert(left.co.X < target);
assert(target <= right.co.X);
assert(target <= right.co.X);
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop this change, please (addition of extraneous whitespace)

switch (right.interpolation) {
case CONSTANT: return left.co.Y;
case LINEAR: return InterpolateLinearCurve(left, right, target);
case BEZIER: return InterpolateBezierCurve(left, right, target, allowed_error);
}

// Control should never reach this point
return 0.0;
}


Expand Down
57 changes: 32 additions & 25 deletions src/Profiles.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,7 @@ using namespace openshot;
// @brief Constructor for Profile.
// @param path The folder path / location of a profile file
Profile::Profile(std::string path) {

bool read_file = false;

try
{
try {
// Initialize info values
info.description = "";
info.height = 0;
Expand All @@ -54,21 +50,32 @@ Profile::Profile(std::string path) {
info.display_ratio.den = 0;
info.interlaced_frame = false;

// Open the file for reading
QFile inputFile(path.c_str());
if (inputFile.open(QIODevice::ReadOnly))
if (!inputFile.open(QIODevice::ReadOnly)) {
throw std::runtime_error("Failed to open the file for reading");
}

// Iterate over each line in the file
QTextStream in(&inputFile);
while (!in.atEnd())
{
QTextStream in(&inputFile);
while (!in.atEnd())
{
QString line = in.readLine();
QString line = in.readLine();

// Trim any extra whitespace from the line
line = line.trimmed();
Comment on lines +63 to +66
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be simpler to do this all in one go, like with the splitted parts a few lines down.

Suggested change
QString line = in.readLine();
// Trim any extra whitespace from the line
line = line.trimmed();
QString line = in.readLine().trimmed();


if (line.length() <= 0)
continue;
// If this line is not empty, or a comment
if (line.length() > 0 && line.at(0) != QChar('#')) {
// If the line does not contain an '=' character, it's invalid
if (line.contains(QChar('=')) == false) {
throw std::runtime_error("Invalid line encountered");
}

// Split current line
QStringList parts = line.split( "=" );
std::string setting = parts[0].toStdString();
std::string value = parts[1].toStdString();
std::string setting = parts[0].trimmed().toStdString();
std::string value = parts[1].trimmed().toStdString();
Comment on lines +71 to +78
Copy link
Contributor

@ferdnyc ferdnyc Apr 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Despite my previous comments about ignoring unrecognized settings, I'm fine with requiring that each line (except comments) contain an = character, since that can reasonably be considered part of the basic file format.

In fact, another way to go, rather than checking for .contains('='), would be to just always .split("=") the line, and then check that the resulting QStringList contains exactly two parts — no more, no less, as either would be an error.

Because a file accidentally left like this:

width=1280height=720
progressive=1
frame_rate_num=25
frame_rate_den=1

...should probably cause some sort of alarm to be raised.

(Unless the line is a description= line, maybe? Might be nice to special-case those so they're allowed to have arguments containing = signs. Hmm. I don't know, have to think on that.)

int value_int = 0;

// update struct (based on line number)
Expand Down Expand Up @@ -113,23 +120,23 @@ Profile::Profile(std::string path) {
else if (setting == "colorspace") {
std::stringstream(value) >> value_int;
info.pixel_format = value_int;
}
else {
// Any other setting makes the file invalid
std::ostringstream oss;
oss << "Unrecognized setting \"" << setting << "\" encountered";
throw std::runtime_error(oss.str());
Comment on lines +124 to +128
Copy link
Contributor

@ferdnyc ferdnyc Apr 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Honestly, I'm pretty wary of this. Logging an unrecognized setting (using ZmqLogger::Instance()->AppendDebugMethod()) would be OK, but not bailing on the entire file.

If an unrecognized parameter is an error, then whenever we next need to extend the format with new parameters, we'll instantly have a problem with older libopenshot versions breaking if they're fed any of the new files.

For backwardsforwards-compatibility, far better if the parser simply assumes that anything it doesn't know how to process is valid data that just isn't meant for its consumption, and continue processing the rest of the file.

}
}
read_file = true;

inputFile.close();
}

}
catch (const std::exception& e)
{
// Error parsing profile file
throw InvalidFile("Profile could not be found or loaded (or is invalid).", path);
// If a runtime error was thrown, pass it along as an InvalidFile exception
catch (const std::exception& e) {
throw InvalidFile(e.what(), path);
}

// Throw error if file was not read
if (!read_file)
// Error parsing profile file
throw InvalidFile("Profile could not be found or loaded (or is invalid).", path);
}

// Generate JSON string of this object
Expand Down