GTK progress optimizations and properly closing gst.pipeline

soundconverter/gstreamer/converter.py

@@ -237,15 +237,17 @@ def _query_position(self):
     def get_progress(self):
         """Fraction of how much of the task is completed."""
         duration = self.sound_file.duration
-        if not self.done:
-            position = self._query_position()
-            if duration is None:
-                return 0
-            progress = position / duration
-            progress = min(max(progress, 0.0), 1.0)
-            return progress, duration
 
-        return 1, duration
+        if self.done:
+            return 1, duration
+
+        if self.pipeline is None or duration is None:
+            return 0, duration
+
+        position = self._query_position()
+        progress = position / duration
+        progress = min(max(progress, 0.0), 1.0)
+        return progress, duration
 
     def cancel(self):
         """Cancel execution of the task."""
@@ -265,6 +267,14 @@ def resume(self):
             return
         self.pipeline.set_state(Gst.State.PLAYING)
 
+    def _cleanup(self):
+        """Delete the pipeline."""
+        bus = self.pipeline.get_bus()
+        bus.disconnect(self.watch_id)
+        bus.remove_signal_watch()
+        self.pipeline.set_state(Gst.State.NULL)
+        self.pipeline = None
+
     def _stop_pipeline(self):
         # remove partial file
         if self.temporary_filename is not None:
@@ -279,11 +289,7 @@ def _stop_pipeline(self):
         if not self.pipeline:
             logger.debug('pipeline already stopped!')
             return
-        self.pipeline.set_state(Gst.State.NULL)
-        bus = self.pipeline.get_bus()
-        bus.disconnect(self.watch_id)
-        bus.remove_signal_watch()
-        self.pipeline = None
+        self._cleanup()
 
     def _convert(self):
         """Run the gst pipeline that converts files.
@@ -323,13 +329,6 @@ def _conversion_done(self):
         if newname is None:
             raise AssertionError('the conversion was not started')
 
-        if not vfs_exists(self.temporary_filename):
-            self.error = 'Expected {} to exist after conversion.'.format(
-                self.temporary_filename
-            )
-            self.callback()
-            return
-
         if self.error:
             logger.debug('error in task, skipping rename: {}'.format(
                 self.temporary_filename
@@ -341,6 +340,13 @@ def _conversion_done(self):
             self.callback()
             return
 
+        if not vfs_exists(self.temporary_filename):
+            self.error = 'Expected {} to exist after conversion.'.format(
+                self.temporary_filename
+            )
+            self.callback()
+            return
+
         # rename temporary file
         logger.debug('{} -> {}'.format(
             beautify_uri(self.temporary_filename), beautify_uri(newname)
@@ -408,6 +414,7 @@ def _conversion_done(self):
         self.output_uri = newname
         self.done = True
         self.callback()
+        self._cleanup()
 
     def run(self):
         """Call this in order to run the whole Converter task."""

soundconverter/interface/ui.py

@@ -114,8 +114,8 @@ def __init__(self, builder):
         self.secondary = builder.get_object('secondary_error_label')
 
     def show_error(self, primary, secondary):
-        self.primary.set_markup(primary)
-        self.secondary.set_markup(secondary)
+        self.primary.set_markup(str(primary))
+        self.secondary.set_markup(str(secondary))
         try:
             sys.stderr.write(_('\nError: %s\n%s\n') % (primary, secondary))
         except Exception:
@@ -445,7 +445,7 @@ def format_cell(self, sound_file):
     def set_row_progress(self, number, progress):
         """Update the progress bar of a single row/file."""
         self.progress_column.set_visible(True)
-        if self.model[number][2] == 1.0:
+        if self.model[number][2] == progress * 100:
             return
 
         self.model[number][2] = progress * 100.0

tests/testcases/integration.py

@@ -29,7 +29,7 @@
 import sys
 import shutil
 import urllib.parse
-from gi.repository import Gtk, GLib
+from gi.repository import Gtk, GLib, Gst
 from importlib.util import spec_from_loader, module_from_spec
 from importlib.machinery import SourceFileLoader
 
@@ -527,8 +527,11 @@ def test_conversion(self):
 
         window.on_convert_button_clicked()
 
-        # wait for the assertions until all files are converted
         queue = window.converter_queue
+
+        pipeline = queue.all_tasks[0].pipeline
+
+        # wait for the assertions until all files are converted
         while not queue.finished:
             gtk_iteration()
 
@@ -578,6 +581,11 @@ def test_conversion(self):
         self.assertIn('empty/a', window.filelist.invalid_files_list)
         self.assertIn('empty/b/c', window.filelist.invalid_files_list)
 
+        # cleans up at the end. Important because otherwise it will crash
+        # because too many files are open.
+        self.assertEqual(pipeline.get_state(0).state, Gst.State.NULL)
+        self.assertIsNone(queue.all_tasks[0].pipeline)
+
     def test_pause_resume(self):
         gio_settings = get_gio_settings()
         gio_settings.set_int(
@@ -646,7 +654,6 @@ def test_pause_resume(self):
         converter_queue = window.converter_queue
         self.assertEqual(len(converter_queue.done), len(expected_filelist))
 
-        print(os.path.realpath('tests/tmp/a.opus'))
         self.assertTrue(os.path.isfile('tests/tmp/a.opus'))
 
         errors = sum([1 for task in converter_queue.done if task.error])
@@ -655,6 +662,95 @@ def test_pause_resume(self):
         self.assertFalse(window.filelist.progress_column.get_visible())
         self.assertEqual(len(window.filelist.invalid_files_list), 0)
 
+    def test_cancel(self):
+        gio_settings = get_gio_settings()
+        gio_settings.set_int(
+            'opus-bitrate',
+            get_quality('audio/ogg; codecs=opus', 3)
+        )
+
+        launch([
+            'tests/test data/audio/a.wav'
+        ])
+        self.assertEqual(settings['main'], 'gui')
+        self.assertEqual(settings['debug'], False)
+
+        window = win[0]
+
+        window.prefs.change_mime_type('audio/ogg; codecs=opus')
+
+        window.on_convert_button_clicked()
+        gtk_iteration(True)
+        queue = window.converter_queue
+        pipeline = queue.all_tasks[0].pipeline
+
+        # quick check if the conversion correctly started
+        self.assertEqual(len(queue.running), 1)
+        pipeline_state = pipeline.get_state(Gst.CLOCK_TIME_NONE).state
+        self.assertEqual(pipeline_state, Gst.State.PLAYING)
+
+        window.on_button_cancel_clicked()
+
+        # the task should not be running anymore
+        self.assertEqual(len(queue.running), 0)
+        # the running task is put back into pending
+        self.assertEqual(queue.pending.qsize(), 1)
+        pipeline_state = pipeline.get_state(Gst.CLOCK_TIME_NONE).state
+        self.assertEqual(pipeline_state, Gst.State.NULL)
+
+        # my computer needs ~0.03 seconds to convert it. So sleep some
+        # significantly longer time than that to make sure cancel actually
+        # cancels the conversion.
+        time.sleep(0.5)
+        gtk_iteration()
+        self.assertFalse(window.filelist.progress_column.get_visible())
+        self.assertEqual(len(queue.running), 0)
+        self.assertEqual(len(queue.done), 0)
+        self.assertEqual(queue.pending.qsize(), 1)
+        self.assertFalse(os.path.isfile('tests/tmp/a.opus'))
+        pipeline_state = pipeline.get_state(Gst.CLOCK_TIME_NONE).state
+        self.assertEqual(pipeline_state, Gst.State.NULL)
+
+        window.on_convert_button_clicked()
+        gtk_iteration(True)
+        new_queue = window.converter_queue
+        new_pipeline = new_queue.all_tasks[0].pipeline
+
+        # the new queue is running now instead
+        self.assertEqual(len(new_queue.running), 1)
+        new_pipeline_state = new_pipeline.get_state(Gst.CLOCK_TIME_NONE).state
+        self.assertEqual(new_pipeline_state, Gst.State.PLAYING)
+        # the old one is not running
+        old_pipeline_state = pipeline.get_state(Gst.CLOCK_TIME_NONE).state
+        self.assertEqual(old_pipeline_state, Gst.State.NULL)
+        self.assertEqual(len(queue.running), 0)
+        self.assertEqual(queue.pending.qsize(), 1)
+
+        gtk_iteration()
+        self.assertTrue(window.filelist.progress_column.get_visible())
+
+        start = time.time()
+        while not new_queue.finished:
+            gtk_iteration()
+        if time.time() - start > 0.4:
+            print(
+                'The test may not work as intended because the conversion'
+                'may take longer than the cancel duration.'
+            )
+
+        # the old queue object didn't finish anything
+        self.assertEqual(len(queue.done), 0)
+        self.assertEqual(queue.pending.qsize(), 1)
+
+        # the new queue finished
+        self.assertEqual(len(new_queue.running), 0)
+        self.assertEqual(len(new_queue.done), 1)
+        self.assertEqual(new_queue.pending.qsize(), 0)
+        self.assertEqual(new_queue.get_progress()[0], 1)
+        self.assertEqual(new_pipeline.get_state(0).state, Gst.State.NULL)
+
+        self.assertTrue(os.path.isfile('tests/tmp/a.opus'))
+
     def test_conversion_pattern(self):
         gio_settings = get_gio_settings()
         gio_settings.set_int('aac-quality', get_quality('audio/x-m4a', 3))