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

Added error handling to Preset and Profile file reading operations #3342

Merged
merged 4 commits into from
May 16, 2020
Merged

Added error handling to Preset and Profile file reading operations #3342

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ad94c71
Added error handling to Preset and Profile file reading operations
kartchnb Apr 3, 2020
ba17cc6
Merge branch 'develop' into develop
ferdnyc Apr 4, 2020
4ad1d72
Merge branch 'develop' into develop
jonoomph May 9, 2020
09207c2
Merge branch 'develop' into develop
jonoomph May 15, 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
33 changes: 19 additions & 14 deletions src/classes/project_data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,21 +280,26 @@ def new(self):
# Loop through profiles
for profile_folder in [info.USER_PROFILES_PATH, info.PROFILES_PATH]:
for file in os.listdir(profile_folder):
# Load Profile and append description
profile_path = os.path.join(profile_folder, file)
profile = openshot.Profile(profile_path)

if default_profile == profile.info.description:
log.info("Setting default profile to %s" % profile.info.description)

# Update default profile
self._data["profile"] = profile.info.description
self._data["width"] = profile.info.width
self._data["height"] = profile.info.height
self._data["fps"] = {"num": profile.info.fps.num, "den": profile.info.fps.den}
self._data["display_ratio"] = {"num": profile.info.display_ratio.num, "den": profile.info.display_ratio.den}
self._data["pixel_ratio"] = {"num": profile.info.pixel_ratio.num, "den": profile.info.pixel_ratio.den}
break
try:
# Load Profile and append description
profile = openshot.Profile(profile_path)

if default_profile == profile.info.description:
log.info("Setting default profile to %s" % profile.info.description)

# Update default profile
self._data["profile"] = profile.info.description
self._data["width"] = profile.info.width
self._data["height"] = profile.info.height
self._data["fps"] = {"num": profile.info.fps.num, "den": profile.info.fps.den}
self._data["display_ratio"] = {"num": profile.info.display_ratio.num, "den": profile.info.display_ratio.den}
self._data["pixel_ratio"] = {"num": profile.info.pixel_ratio.num, "den": profile.info.pixel_ratio.den}
break

except RuntimeError as e:
# This exception occurs when there's a problem parsing the Profile file - display a message and continue
log.error("Failed to parse file '%s' as a profile: %s" % (profile_path, e))

# Get the default audio settings for the timeline (and preview playback)
default_sample_rate = int(s.get("default-samplerate"))
Expand Down
233 changes: 128 additions & 105 deletions src/windows/export.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
import time
import xml.dom.minidom as xml
import tempfile

from xml.parsers.expat import ExpatError

from PyQt5.QtCore import *
from PyQt5.QtWidgets import *
Expand Down Expand Up @@ -196,14 +196,19 @@ def __init__(self):
self.profile_paths = {}
for profile_folder in [info.USER_PROFILES_PATH, info.PROFILES_PATH]:
for file in os.listdir(profile_folder):
# Load Profile
profile_path = os.path.join(profile_folder, file)
profile = openshot.Profile(profile_path)
try:
# Load Profile
profile = openshot.Profile(profile_path)

# Add description of Profile to list
profile_name = "%s (%sx%s)" % (profile.info.description, profile.info.width, profile.info.height)
self.profile_names.append(profile_name)
self.profile_paths[profile_name] = profile_path

# Add description of Profile to list
profile_name = "%s (%sx%s)" % (profile.info.description, profile.info.width, profile.info.height)
self.profile_names.append(profile_name)
self.profile_paths[profile_name] = profile_path
except RuntimeError as e:
# This exception occurs when there's a problem parsing the Profile file - display a message and continue
log.error("Failed to parse file '%s' as a profile: %s" % (profile_path, e))

# Sort list
self.profile_names.sort()
Expand All @@ -227,11 +232,17 @@ def __init__(self):
# ********* Simple Project Type **********
# load the simple project type dropdown
presets = []
for preset_path in [info.EXPORT_PRESETS_PATH, info.USER_PRESETS_PATH]:
for file in os.listdir(preset_path):
xmldoc = xml.parse(os.path.join(preset_path, file))
type = xmldoc.getElementsByTagName("type")
presets.append(_(type[0].childNodes[0].data))
for preset_folder in [info.EXPORT_PRESETS_PATH, info.USER_PRESETS_PATH]:
for file in os.listdir(preset_folder):
preset_path = os.path.join(preset_folder, file)
try:
xmldoc = xml.parse(preset_path)
type = xmldoc.getElementsByTagName("type")
presets.append(_(type[0].childNodes[0].data))

except ExpatError as e:
# This indicates an invalid Preset file - display an error and continue
log.error("Failed to parse file '%s' as a preset: %s" % (preset_path, e))

# Exclude duplicates
type_index = 0
Expand Down Expand Up @@ -370,30 +381,36 @@ def cboSimpleProjectType_index_changed(self, widget, index):
# parse the xml files and get targets that match the project type
project_types = []
acceleration_types = {}
for preset_path in [info.EXPORT_PRESETS_PATH, info.USER_PRESETS_PATH]:
for file in os.listdir(preset_path):
xmldoc = xml.parse(os.path.join(preset_path, file))
type = xmldoc.getElementsByTagName("type")

if _(type[0].childNodes[0].data) == selected_project:
titles = xmldoc.getElementsByTagName("title")
videocodecs = xmldoc.getElementsByTagName("videocodec")
for title in titles:
project_types.append(_(title.childNodes[0].data))
for codec in videocodecs:
codec_text = codec.childNodes[0].data
if "vaapi" in codec_text and openshot.FFmpegWriter.IsValidCodec(codec_text):
acceleration_types[_(title.childNodes[0].data)] = QIcon(":/hw/hw-accel-vaapi.svg")
elif "nvenc" in codec_text and openshot.FFmpegWriter.IsValidCodec(codec_text):
acceleration_types[_(title.childNodes[0].data)] = QIcon(":/hw/hw-accel-nvenc.svg")
elif "dxva2" in codec_text and openshot.FFmpegWriter.IsValidCodec(codec_text):
acceleration_types[_(title.childNodes[0].data)] = QIcon(":/hw/hw-accel-dx.svg")
elif "videotoolbox" in codec_text and openshot.FFmpegWriter.IsValidCodec(codec_text):
acceleration_types[_(title.childNodes[0].data)] = QIcon(":/hw/hw-accel-vtb.svg")
elif "qsv" in codec_text and openshot.FFmpegWriter.IsValidCodec(codec_text):
acceleration_types[_(title.childNodes[0].data)] = QIcon(":/hw/hw-accel-qsv.svg")
elif openshot.FFmpegWriter.IsValidCodec(codec_text):
acceleration_types[_(title.childNodes[0].data)] = QIcon(":/hw/hw-accel-none.svg")
for preset_folder in [info.EXPORT_PRESETS_PATH, info.USER_PRESETS_PATH]:
for file in os.listdir(preset_folder):
preset_path = os.path.join(preset_folder, file)
try:
xmldoc = xml.parse(preset_path)
type = xmldoc.getElementsByTagName("type")

if _(type[0].childNodes[0].data) == selected_project:
titles = xmldoc.getElementsByTagName("title")
videocodecs = xmldoc.getElementsByTagName("videocodec")
Comment on lines +399 to +405
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here, re: nesting

Suggested change
try:
xmldoc = xml.parse(preset_path)
type = xmldoc.getElementsByTagName("type")
if _(type[0].childNodes[0].data) == selected_project:
titles = xmldoc.getElementsByTagName("title")
videocodecs = xmldoc.getElementsByTagName("videocodec")
try:
xmldoc = xml.parse(preset_path)
type = xmldoc.getElementsByTagName("type")
if _(type[0].childNodes[0].data) != selected_project:
continue
titles = xmldoc.getElementsByTagName("title")
videocodecs = xmldoc.getElementsByTagName("videocodec")

for title in titles:
project_types.append(_(title.childNodes[0].data))
Comment on lines +406 to +407
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for title in titles:
project_types.append(_(title.childNodes[0].data))
project_types.extend([_(t.childNodes[0].data) for t in titles])

Oops, only just noticed this one. Far more Pythonic to say...

Copy link
Contributor

Choose a reason for hiding this comment

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

(I realize you didn't write that line originally, but if we're touching it, might as well make it pretty.)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is c++ innovation that were adopted in Python, so it is not "Pythonic" in any way, while it still may look like this. This kind of code is harder to read than the original one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Masking append to extend behind "beautify" of the code.

Copy link
Contributor

@ferdnyc ferdnyc May 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

When the list is constructed by a loop regardless, I agree it's less impactful. however .extend() isn't just a different way of writing .append() in a loop. List concatenation is more efficient than repeatedly appending individual items, primarily because the memory allocation required to enlarge the destination list can be done all at once.

The larger the list you're adding to, and the larger the list of items you're extending it with, the more benefits this has. And while you benefit the most if the lists are preexisting, even when looping to construct the list of additions, doing that first and then adding it to the destination list is preferable.

IOW, if your objection is primarily to the comprehension, then this version without it is still preferable, because it will avoid repeated small allocations to increase the size of project_types

title_types = []
for t in titles:
    title_types.append(_(t.childNodes[0].data))
project_types.extend(title_types)

The comprehension is just a cleaner way of writing that concisely, though I realize not everyone cares for them.

Copy link
Contributor

Choose a reason for hiding this comment

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

listA.extend(listB) is literally ten times as fast as .append() in a loop. And even if you loop to create the second list, doing that first and .extend()ing afterwards takes only half as long as the looped .append() on the destination list.

These timeit runs use exaggeratedly large lists, so real-world this won't have nearly as big of an impact, but...

# listA = a million integers from 0...999999
# listB = another million, from 2000000...2999999
#
# Looping listA.append() for each item in listB
>>> timeit.repeat('for n in listB:\n    listA.append(n)', setup='listA = [x for x in range(0, 1000000)]\nlistB = [x for x in range(2000000, 3000000)]', number=100, globals=globals())
[6.985341801017057, 7.013206199044362, 6.989441379031632, 7.000579444982577, 6.9871047679916956]

# listA.extend() with the entire contents of listB
>>> timeit.repeat('listA.extend(listB)', setup='listA = [x for x in range(0, 1000000)]\nlistB = [x for x in range(2000000, 3000000)]', number=100, globals=globals())
[0.6946912699495442, 0.6940702060237527, 0.694431746029295, 0.6942416160018183, 0.6937777320272289]

# listA.extend() with a list-comprehension copy of listB:
>>> timeit.repeat('listA.extend([n for n in listB])', setup='listA = [x for x in range(0, 1000000)]\nlistB = [x for x in range(2000000, 3000000)]', number=100, globals=globals())
[3.4050588610116392, 3.4058208650094457, 3.4050478630233556, 3.404870489030145, 3.405109421000816]

Copy link
Contributor

Choose a reason for hiding this comment

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

Now, again, I'm not saying this actually matters in this particular case. It doesn't. We're talking about small lists here, where it really doesn't make the tiniest bit of difference how you build them. But certain code patterns have more efficient alternatives that are typically preferable. Taking a blanket position of, "append() is clearer than extend()" ignores the fact that they're not even close to equivalent in terms of performance/efficiency, because there are times when that will matter.

for codec in videocodecs:
codec_text = codec.childNodes[0].data
if "vaapi" in codec_text and openshot.FFmpegWriter.IsValidCodec(codec_text):
acceleration_types[_(title.childNodes[0].data)] = QIcon(":/hw/hw-accel-vaapi.svg")
elif "nvenc" in codec_text and openshot.FFmpegWriter.IsValidCodec(codec_text):
acceleration_types[_(title.childNodes[0].data)] = QIcon(":/hw/hw-accel-nvenc.svg")
elif "dxva2" in codec_text and openshot.FFmpegWriter.IsValidCodec(codec_text):
acceleration_types[_(title.childNodes[0].data)] = QIcon(":/hw/hw-accel-dx.svg")
elif "videotoolbox" in codec_text and openshot.FFmpegWriter.IsValidCodec(codec_text):
acceleration_types[_(title.childNodes[0].data)] = QIcon(":/hw/hw-accel-vtb.svg")
elif "qsv" in codec_text and openshot.FFmpegWriter.IsValidCodec(codec_text):
acceleration_types[_(title.childNodes[0].data)] = QIcon(":/hw/hw-accel-qsv.svg")
elif openshot.FFmpegWriter.IsValidCodec(codec_text):
acceleration_types[_(title.childNodes[0].data)] = QIcon(":/hw/hw-accel-none.svg")
Comment on lines +408 to +421
Copy link
Contributor

Choose a reason for hiding this comment

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

Urrrrvery single one of these thing.childNodes[0].data references here, above, and below, can all be just thing.firstChild.data, FWIW.

(Also not your code originally, of course. But you just had to go and touch it, so now you get the commentary. 😉 )


except ExpatError as e:
# This indicates an invalid Preset file - display an error and continue
log.error("Failed to parse file '%s' as a preset: %s" % (preset_path, e))

# Add all targets for selected project type
preset_index = 0
Expand Down Expand Up @@ -468,77 +485,83 @@ def cboSimpleTarget_index_changed(self, widget, index):
# parse the xml to return suggested profiles
profile_index = 0
all_profiles = False
for preset_path in [info.EXPORT_PRESETS_PATH, info.USER_PRESETS_PATH]:
for file in os.listdir(preset_path):
xmldoc = xml.parse(os.path.join(preset_path, file))
title = xmldoc.getElementsByTagName("title")
if _(title[0].childNodes[0].data) == selected_target:
profiles = xmldoc.getElementsByTagName("projectprofile")

# get the basic profile
all_profiles = False
if profiles:
# if profiles are defined, show them
for profile in profiles:
profiles_list.append(_(profile.childNodes[0].data))
else:
# show all profiles
all_profiles = True
for profile_name in self.profile_names:
profiles_list.append(profile_name)

# get the video bit rate(s)
videobitrate = xmldoc.getElementsByTagName("videobitrate")
for rate in videobitrate:
v_l = rate.attributes["low"].value
v_m = rate.attributes["med"].value
v_h = rate.attributes["high"].value
self.vbr = {_("Low"): v_l, _("Med"): v_m, _("High"): v_h}

# get the audio bit rates
audiobitrate = xmldoc.getElementsByTagName("audiobitrate")
for audiorate in audiobitrate:
a_l = audiorate.attributes["low"].value
a_m = audiorate.attributes["med"].value
a_h = audiorate.attributes["high"].value
self.abr = {_("Low"): a_l, _("Med"): a_m, _("High"): a_h}

# get the remaining values
vf = xmldoc.getElementsByTagName("videoformat")
self.txtVideoFormat.setText(vf[0].childNodes[0].data)
vc = xmldoc.getElementsByTagName("videocodec")
self.txtVideoCodec.setText(vc[0].childNodes[0].data)
sr = xmldoc.getElementsByTagName("samplerate")
self.txtSampleRate.setValue(int(sr[0].childNodes[0].data))
c = xmldoc.getElementsByTagName("audiochannels")
self.txtChannels.setValue(int(c[0].childNodes[0].data))
c = xmldoc.getElementsByTagName("audiochannellayout")

# check for compatible audio codec
ac = xmldoc.getElementsByTagName("audiocodec")
audio_codec_name = ac[0].childNodes[0].data
if audio_codec_name == "aac":
# Determine which version of AAC encoder is available
if openshot.FFmpegWriter.IsValidCodec("libfaac"):
self.txtAudioCodec.setText("libfaac")
elif openshot.FFmpegWriter.IsValidCodec("libvo_aacenc"):
self.txtAudioCodec.setText("libvo_aacenc")
elif openshot.FFmpegWriter.IsValidCodec("aac"):
self.txtAudioCodec.setText("aac")
for preset_folder in [info.EXPORT_PRESETS_PATH, info.USER_PRESETS_PATH]:
for file in os.listdir(preset_folder):
preset_path = os.path.join(preset_folder, file)
try:
xmldoc = xml.parse(preset_path)
title = xmldoc.getElementsByTagName("title")
if _(title[0].childNodes[0].data) == selected_target:
profiles = xmldoc.getElementsByTagName("projectprofile")
Comment on lines +506 to +510
Copy link
Contributor

Choose a reason for hiding this comment

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

With the try: added in, this is starting to get really deeply nested, so the only thing I'd suggest is maybe going with:

Suggested change
try:
xmldoc = xml.parse(preset_path)
title = xmldoc.getElementsByTagName("title")
if _(title[0].childNodes[0].data) == selected_target:
profiles = xmldoc.getElementsByTagName("projectprofile")
try:
xmldoc = xml.parse(preset_path)
title = xmldoc.getElementsByTagName("title")
if _(title[0].childNodes[0].data) != selected_target:
continue
profiles = xmldoc.getElementsByTagName("projectprofile")

Then everything that follows can be outdented a level. (Or, outdented back to its previous level, really.)


# get the basic profile
all_profiles = False
if profiles:
# if profiles are defined, show them
for profile in profiles:
profiles_list.append(_(profile.childNodes[0].data))
Comment on lines +516 to +517
Copy link
Contributor

@ferdnyc ferdnyc Apr 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
for profile in profiles:
profiles_list.append(_(profile.childNodes[0].data))
profiles_list.extend([_(p.firstChild.data) for p in profiles])

Copy link
Contributor

Choose a reason for hiding this comment

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

Second time you masking append to extend This isn't good.

else:
# show all profiles
all_profiles = True
for profile_name in self.profile_names:
profiles_list.append(profile_name)
Comment on lines +521 to +522
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for profile_name in self.profile_names:
profiles_list.append(profile_name)
profiles_list.extend([n for n in self.profile_names])


# get the video bit rate(s)
videobitrate = xmldoc.getElementsByTagName("videobitrate")
for rate in videobitrate:
v_l = rate.attributes["low"].value
v_m = rate.attributes["med"].value
v_h = rate.attributes["high"].value
self.vbr = {_("Low"): v_l, _("Med"): v_m, _("High"): v_h}

# get the audio bit rates
audiobitrate = xmldoc.getElementsByTagName("audiobitrate")
for audiorate in audiobitrate:
a_l = audiorate.attributes["low"].value
a_m = audiorate.attributes["med"].value
a_h = audiorate.attributes["high"].value
self.abr = {_("Low"): a_l, _("Med"): a_m, _("High"): a_h}

# get the remaining values
vf = xmldoc.getElementsByTagName("videoformat")
self.txtVideoFormat.setText(vf[0].childNodes[0].data)
vc = xmldoc.getElementsByTagName("videocodec")
self.txtVideoCodec.setText(vc[0].childNodes[0].data)
sr = xmldoc.getElementsByTagName("samplerate")
self.txtSampleRate.setValue(int(sr[0].childNodes[0].data))
c = xmldoc.getElementsByTagName("audiochannels")
self.txtChannels.setValue(int(c[0].childNodes[0].data))
c = xmldoc.getElementsByTagName("audiochannellayout")

# check for compatible audio codec
ac = xmldoc.getElementsByTagName("audiocodec")
audio_codec_name = ac[0].childNodes[0].data
if audio_codec_name == "aac":
# Determine which version of AAC encoder is available
if openshot.FFmpegWriter.IsValidCodec("libfaac"):
self.txtAudioCodec.setText("libfaac")
elif openshot.FFmpegWriter.IsValidCodec("libvo_aacenc"):
self.txtAudioCodec.setText("libvo_aacenc")
elif openshot.FFmpegWriter.IsValidCodec("aac"):
self.txtAudioCodec.setText("aac")
else:
# fallback audio codec
self.txtAudioCodec.setText("ac3")
else:
# fallback audio codec
self.txtAudioCodec.setText("ac3")
else:
# fallback audio codec
self.txtAudioCodec.setText(audio_codec_name)

layout_index = 0
for layout in self.channel_layout_choices:
if layout == int(c[0].childNodes[0].data):
self.cboChannelLayout.setCurrentIndex(layout_index)
break
layout_index += 1

self.txtAudioCodec.setText(audio_codec_name)

layout_index = 0
for layout in self.channel_layout_choices:
if layout == int(c[0].childNodes[0].data):
self.cboChannelLayout.setCurrentIndex(layout_index)
break
layout_index += 1

except ExpatError as e:
# This indicates an invalid Preset file - display an error and continue
log.error("Failed to parse file '%s' as a preset: %s" % (preset_path, e))
Comment on lines +580 to +582
Copy link
Contributor

@ferdnyc ferdnyc Apr 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This won't actually display an error anywhere except the console output — which means only in the logfile, for Windows users, since they don't even get console output. (Ditto macOS and Linux users who launch OpenShot graphically, but at least they have the option.) That's probably a good thing, most of the time — we don't really want to be throwing error dialogs up at users because of a profile parsing issue. There are already spots in the code that show dialog messages for errors loading icons, something I've argued against because what is the user expected to do about that!?

But in this case, especially if it's one of the user's own profiles, it might be good to give them some indication that there's an issue, because that's something they can correct. So, it might be worth showing an unobtrusive error message (localized, also) in the status bar of the main window. It's a sadly-underutilized means of communicating information without interrupting the user or forcing them to deal with the message immediately. You can see it used in the main window's code for handling the save frame / snapshot preview feature, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that's right, and also in the Recent Project list handling. This bit here:

openshot-qt/src/windows/main_window.py

Lines 537 to 540 in a22fa5d

log.info("File not found at {}".format(file_path))
self.statusBar.showMessage(_("Project {} is missing (it may have been moved or deleted). It has been removed from the Recent Projects menu.".format(file_path)), 5000)
self.remove_recent_project(file_path)
self.load_recent_menu()

Copy link
Contributor

@ferdnyc ferdnyc Apr 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

There's code in __init__, I believe, that sets up self.statusBar for the class.

Come to think of it, you could just use get_app().win.statusBar to access it from anywhere. Only thing I'm not 100% sure about is whether that'll work while the export dialog is open. If not, then meh, the log message is fine.


# init the profiles combo
for item in sorted(profiles_list):
self.cboSimpleVideoProfile.addItem(self.getProfileName(self.getProfilePath(item)), self.getProfilePath(item))
Expand Down
Loading