Run a single external audio player process at a time

External and internal audio players work similarly now. Fixes #950.

* inherit a new ExternalAudioPlayer class from AudioPlayerInterface;
* use an existing ExternalViewer class to implement ExternalAudioPlayer;
* take (const char *, int) instead of std::vector<char> in
  ExternalViewer constructor to fit into AudioPlayerInterface;
* extend ExternalViewer API to let ExternalAudioPlayer stop superseded
  audio player processes;
* make AudioPlayerInterface::play() return an error message string to
  allow reporting immediate failures from derived classes;
* Document AudioPlayerInterface API;
* Document AudioPlayerFactory::player();
* use the common audio interface exclusively in ArticleView.

articleview.cc

@@ -2,7 +2,6 @@
  * Part of GoldenDict. Licensed under GPLv3 or later, see the LICENSE file */
 
 #include "articleview.hh"
-#include "externalviewer.hh"
 #include <map>
 #include <QMessageBox>
 #include <QWebHitTestResult>
@@ -269,11 +268,7 @@ void ArticleView::setGroupComboBox( GroupComboBox const * g )
 ArticleView::~ArticleView()
 {
   cleanupTemp();
-
-#ifndef DISABLE_INTERNAL_PLAYER
-  if ( audioPlayer )
-    audioPlayer->stop();
-#endif
+  audioPlayer->stop();
 
 #if QT_VERSION >= QT_VERSION_CHECK(4, 6, 0)
   ui.definition->ungrabGesture( Gestures::GDPinchGestureType );
@@ -285,12 +280,8 @@ void ArticleView::showDefinition( QString const & word, unsigned group,
                                   QString const & scrollTo,
                                   Contexts const & contexts_ )
 {
-
-#ifndef DISABLE_INTERNAL_PLAYER
   // first, let's stop the player
-  if ( audioPlayer )
-    audioPlayer->stop();
-#endif
+  audioPlayer->stop();
 
   QUrl req;
   Contexts contexts( contexts_ );
@@ -354,11 +345,8 @@ void ArticleView::showDefinition( QString const & word, QStringList const & dict
   if( dictIDs.isEmpty() )
     return;
 
-#ifndef DISABLE_INTERNAL_PLAYER
   // first, let's stop the player
-  if ( audioPlayer )
-    audioPlayer->stop();
-#endif
+  audioPlayer->stop();
 
   QUrl req;
 
@@ -1910,28 +1898,10 @@ void ArticleView::resourceDownloadFinished()
              Dictionary::WebMultimediaDownload::isAudioUrl( resourceDownloadUrl ) )
         {
           // Audio data
-#ifndef DISABLE_INTERNAL_PLAYER
-          if ( audioPlayer )
-          {
-            connect( audioPlayer.data(), SIGNAL( error( QString ) ), this, SLOT( audioPlayerError( QString ) ), Qt::UniqueConnection );
-            audioPlayer->play( data.data(), data.size() );
-          }
-          else
-#endif
-          {
-            // Use external viewer to play the file
-            try
-            {
-              ExternalViewer * viewer = new ExternalViewer( this, data, "wav", cfg.preferences.audioPlaybackProgram.trimmed() );
-
-              // Once started, it will erase itself
-              viewer->start();
-            }
-            catch( ExternalViewer::Ex & e )
-            {
-              QMessageBox::critical( this, "GoldenDict", tr( "Failed to run a player to play sound file: %1" ).arg( e.what() ) );
-            }
-          }
+          connect( audioPlayer.data(), SIGNAL( error( QString ) ), this, SLOT( audioPlayerError( QString ) ), Qt::UniqueConnection );
+          QString errorMessage = audioPlayer->play( data.data(), data.size() );
+          if( !errorMessage.isEmpty() )
+            QMessageBox::critical( this, "GoldenDict", tr( "Failed to play sound file: %1" ).arg( errorMessage ) );
         }
         else
         {
@@ -1987,7 +1957,7 @@ void ArticleView::resourceDownloadFinished()
 
 void ArticleView::audioPlayerError( QString const & message )
 {
-  emit statusBarMessage( tr( "WARNING: FFmpeg Audio Player: %1" ).arg( message ),
+  emit statusBarMessage( tr( "WARNING: Audio Player: %1" ).arg( message ),
                          10000, QPixmap( ":/icons/error.png" ) );
 }
 

audioplayerfactory.cc

@@ -1,11 +1,16 @@
 /* This file is (c) 2018 Igor Kushnir <igorkuo@gmail.com>
  * Part of GoldenDict. Licensed under GPLv3 or later, see the LICENSE file */
 
+#include <QScopedPointer>
+#include <QObject>
 #include "audioplayerfactory.hh"
 #include "ffmpegaudioplayer.hh"
+#include "externalaudioplayer.hh"
+#include "gddebug.hh"
 
 AudioPlayerFactory::AudioPlayerFactory( Config::Preferences const & p ) :
-  useInternalPlayer( p.useInternalPlayer )
+  useInternalPlayer( p.useInternalPlayer ),
+  audioPlaybackProgram( p.audioPlaybackProgram )
 {
   reset();
 }
@@ -15,8 +20,20 @@ void AudioPlayerFactory::setPreferences( Config::Preferences const & p )
   if( p.useInternalPlayer != useInternalPlayer )
   {
     useInternalPlayer = p.useInternalPlayer;
+    audioPlaybackProgram = p.audioPlaybackProgram;
     reset();
   }
+  else
+  if( !useInternalPlayer && p.audioPlaybackProgram != audioPlaybackProgram )
+  {
+    audioPlaybackProgram = p.audioPlaybackProgram;
+    ExternalAudioPlayer * const externalPlayer =
+        qobject_cast< ExternalAudioPlayer * >( playerPtr.data() );
+    if( externalPlayer )
+      setAudioPlaybackProgram( *externalPlayer );
+    else
+      gdWarning( "External player was expected, but it does not exist.\n" );
+  }
 }
 
 void AudioPlayerFactory::reset()
@@ -27,6 +44,13 @@ void AudioPlayerFactory::reset()
   else
 #endif
   {
-    playerPtr.reset();
+    QScopedPointer< ExternalAudioPlayer > externalPlayer( new ExternalAudioPlayer );
+    setAudioPlaybackProgram( *externalPlayer );
+    playerPtr.reset( externalPlayer.take() );
   }
 }
+
+void AudioPlayerFactory::setAudioPlaybackProgram( ExternalAudioPlayer & externalPlayer )
+{
+  externalPlayer.setPlayerCommandLine( audioPlaybackProgram.trimmed() );
+}

audioplayerfactory.hh

@@ -7,17 +7,25 @@
 #include "audioplayerinterface.hh"
 #include "config.hh"
 
+class ExternalAudioPlayer;
+
 class AudioPlayerFactory
 {
   Q_DISABLE_COPY( AudioPlayerFactory )
 public:
   explicit AudioPlayerFactory( Config::Preferences const & );
   void setPreferences( Config::Preferences const & );
+  /// The returned reference to a smart pointer is valid as long as this object
+  /// exists. The pointer to the owned AudioPlayerInterface may change after the
+  /// call to setPreferences(), but it is guaranteed to never be null.
   AudioPlayerPtr const & player() const { return playerPtr; }
 
 private:
   void reset();
+  void setAudioPlaybackProgram( ExternalAudioPlayer & externalPlayer );
+
   bool useInternalPlayer;
+  QString audioPlaybackProgram;
   AudioPlayerPtr playerPtr;
 };
 

audioplayerinterface.hh

@@ -12,10 +12,16 @@ class AudioPlayerInterface : public QObject
 {
   Q_OBJECT
 public:
-  virtual void play( const char * data, int size ) = 0;
+  /// Stops current playback if any, copies the audio buffer at [data, data + size),
+  /// then plays this buffer. It is safe to invalidate \p data after this function call.
+  /// Returns an error message in case of immediate failure; an empty string
+  /// in case of success.
+  virtual QString play( const char * data, int size ) = 0;
+  /// Stops current playback if any.
   virtual void stop() = 0;
 
 signals:
+  /// Notifies of asynchronous errors.
   void error( QString message );
 };
 

externalaudioplayer.cc

@@ -0,0 +1,99 @@
+/* This file is (c) 2018 Igor Kushnir <igorkuo@gmail.com>
+ * Part of GoldenDict. Licensed under GPLv3 or later, see the LICENSE file */
+
+#include "externalaudioplayer.hh"
+#include "externalviewer.hh"
+
+ExternalAudioPlayer::ExternalAudioPlayer() : exitingViewer( 0 )
+{
+}
+
+ExternalAudioPlayer::~ExternalAudioPlayer()
+{
+  // Destroy viewers immediately to prevent memory and temporary file leaks
+  // at application exit.
+
+  // Set viewer to null first and foremost to make sure that onViewerDestroyed()
+  // doesn't attempt to start viewer or mess the smart pointer up.
+  stopAndDestroySynchronously( viewer.take() );
+
+  stopAndDestroySynchronously( exitingViewer );
+}
+
+void ExternalAudioPlayer::setPlayerCommandLine( QString const & playerCommandLine_ )
+{
+  playerCommandLine = playerCommandLine_;
+}
+
+QString ExternalAudioPlayer::play( const char * data, int size )
+{
+  stop();
+
+  Q_ASSERT( !viewer && "viewer must be null at this point for exception safety." );
+  try
+  {
+    // Our destructor properly destroys viewers we remember about.
+    // In the unlikely case that we call viewer.reset() during the application
+    // exit, ~QObject() prevents leaks as this class is a parent of all viewers.
+    viewer.reset( new ExternalViewer( data, size, "wav", playerCommandLine, this ) );
+  }
+  catch( const ExternalViewer::Ex & e )
+  {
+    return e.what();
+  }
+
+  if( exitingViewer )
+    return QString(); // Will start later.
+  return startViewer();
+}
+
+void ExternalAudioPlayer::stop()
+{
+  if( !exitingViewer && viewer && !viewer->stop() )
+  {
+    // Give the previous viewer a chance to stop before starting a new one.
+    // This prevents overlapping audio and possible conflicts between
+    // external program instances.
+    // Graceful stopping is better than calling viewer.reset() because:
+    //   1) the process gets a chance to clean up and save its state;
+    //   2) there is no event loop blocking and consequently no (short) UI freeze
+    //      while synchronously waiting for the external process to exit.
+    exitingViewer = viewer.take();
+  }
+  else // viewer is either not started or already stopped -> simply destroy it.
+    viewer.reset();
+}
+
+void ExternalAudioPlayer::onViewerDestroyed( QObject * destroyedViewer )
+{
+  if( exitingViewer == destroyedViewer )
+  {
+    exitingViewer = 0;
+    if( viewer )
+    {
+      QString errorMessage = startViewer();
+      if( !errorMessage.isEmpty() )
+        emit error( errorMessage );
+    }
+  }
+  else
+  if( viewer.data() == destroyedViewer )
+    viewer.take(); // viewer finished and died -> release ownership.
+}
+
+QString ExternalAudioPlayer::startViewer()
+{
+  Q_ASSERT( !exitingViewer && viewer );
+  connect( viewer.data(), SIGNAL( destroyed( QObject * ) ),
+           this, SLOT( onViewerDestroyed( QObject * ) ) );
+  try
+  {
+    viewer->start();
+  }
+  catch( const ExternalViewer::Ex & e )
+  {
+    viewer.reset();
+    return e.what();
+  }
+  return QString();
+}

externalaudioplayer.hh

@@ -0,0 +1,44 @@
+/* This file is (c) 2018 Igor Kushnir <igorkuo@gmail.com>
+ * Part of GoldenDict. Licensed under GPLv3 or later, see the LICENSE file */
+
+#ifndef EXTERNALAUDIOPLAYER_HH_INCLUDED
+#define EXTERNALAUDIOPLAYER_HH_INCLUDED
+
+#include <QScopedPointer>
+#include <QString>
+#include "audioplayerinterface.hh"
+
+class ExternalViewer;
+
+class ExternalAudioPlayer : public AudioPlayerInterface
+{
+  Q_OBJECT
+public:
+  ExternalAudioPlayer();
+  ~ExternalAudioPlayer();
+  /// \param playerCommandLine_ Will be used in future play() calls.
+  void setPlayerCommandLine( QString const & playerCommandLine_ );
+
+  virtual QString play( const char * data, int size );
+  virtual void stop();
+
+private slots:
+  void onViewerDestroyed( QObject * destroyedViewer );
+
+private:
+  QString startViewer();
+
+  QString playerCommandLine;
+  ExternalViewer * exitingViewer; ///< If not null: points to the previous viewer,
+                                  ///< the current viewer (if any) is not started yet
+                                  ///< and waits for exitingViewer to be destroyed first.
+
+  struct ScopedPointerDeleteLater
+  {
+    static void cleanup( QObject * p ) { if( p ) p->deleteLater(); }
+  };
+  // deleteLater() is safer because viewer actively participates in the QEventLoop.
+  QScopedPointer< ExternalViewer, ScopedPointerDeleteLater > viewer;
+};
+
+#endif // EXTERNALAUDIOPLAYER_HH_INCLUDED

externalviewer.cc

@@ -2,22 +2,21 @@
  * Part of GoldenDict. Licensed under GPLv3 or later, see the LICENSE file */
 
 #include <QDir>
+#include <QTimer>
 #include "externalviewer.hh"
 #include "parsecmdline.hh"
 #include "gddebug.hh"
 
-using std::vector;
-
-ExternalViewer::ExternalViewer( QObject * parent, vector< char > const & data,
-                                QString const & extension,
-                                QString const & viewerCmdLine_ )
+ExternalViewer::ExternalViewer( const char * data, int size,
+                                QString const & extension, QString const & viewerCmdLine_,
+                                QObject * parent)
     throw( exCantCreateTempFile ):
   QObject( parent ),
   tempFile( QDir::temp().filePath( QString( "gd-XXXXXXXX." ) + extension ) ),
   viewer( this ),
   viewerCmdLine( viewerCmdLine_ )
 {
-  if ( !tempFile.open() || (size_t) tempFile.write( &data.front(), data.size() ) != data.size() )
+  if ( !tempFile.open() || tempFile.write( data, size ) != size )
     throw exCantCreateTempFile();
 
   tempFileName = tempFile.fileName(); // For some reason it loses it after it was closed()
@@ -53,3 +52,29 @@ void ExternalViewer::start() throw( exCantRunViewer )
   else
     throw exCantRunViewer( tr( "the viewer program name is empty" ).toUtf8().data() );
 }
+
+bool ExternalViewer::stop()
+{
+  if( viewer.state() == QProcess::NotRunning )
+    return true;
+  viewer.terminate();
+  QTimer::singleShot( 1000, &viewer, SLOT( kill() ) ); // In case terminate() fails.
+  return false;
+}
+
+void ExternalViewer::stopSynchronously()
+{
+  // This implementation comes straight from QProcess::~QProcess().
+  if( viewer.state() == QProcess::NotRunning )
+    return;
+  viewer.kill();
+  viewer.waitForFinished();
+}
+
+void stopAndDestroySynchronously( ExternalViewer * viewer )
+{
+  if( !viewer )
+    return;
+  viewer->stopSynchronously();
+  delete viewer;
+}

externalviewer.hh

@@ -7,7 +7,6 @@
 #include <QObject>
 #include <QTemporaryFile>
 #include <QProcess>
-#include <vector>
 #include "ex.hh"
 
 /// An external viewer, opens resources in other programs
@@ -26,14 +25,26 @@ public:
   DEF_EX( exCantCreateTempFile, "Couldn't create temporary file.", Ex )
   DEF_EX_STR( exCantRunViewer, "Couldn't run external viewer:", Ex )
 
-  ExternalViewer( QObject * parent, std::vector< char > const & data,
-                  QString const & extension, QString const & viewerCmdLine )
+  ExternalViewer( const char * data, int size,
+                  QString const & extension, QString const & viewerCmdLine,
+                  QObject * parent = 0 )
     throw( exCantCreateTempFile );
 
   // Once this is called, the object will be deleted when it's done, even if
   // the function throws.
   void start() throw( exCantRunViewer );
+
+  /// If the external process is running, requests its termination and returns
+  /// false - expect the QObject::destroyed() signal to be emitted soon.
+  /// If the external process is not running, returns true, the object
+  /// destruction is not necessarily scheduled in this case.
+  bool stop();
+  /// Kills the process if it is running and waits for it to finish.
+  void stopSynchronously();
 };
 
-#endif
+/// Call this function instead of simply deleting viewer to prevent the
+/// "QProcess: Destroyed while process X is still running." warning in log.
+void stopAndDestroySynchronously( ExternalViewer * viewer );
 
+#endif

ffmpegaudioplayer.hh

@@ -22,9 +22,10 @@ public:
              this, SIGNAL( error( QString ) ) );
   }
 
-  virtual void play( const char * data, int size )
+  virtual QString play( const char * data, int size )
   {
     AudioService::instance().playMemory( data, size );
+    return QString();
   }
 
   virtual void stop()

goldendict.pro

@@ -267,6 +267,7 @@ HEADERS += folding.hh \
     audioplayerinterface.hh \
     audioplayerfactory.hh \
     ffmpegaudioplayer.hh \
+    externalaudioplayer.hh \
     externalviewer.hh \
     wordfinder.hh \
     groupcombobox.hh \
@@ -398,6 +399,7 @@ SOURCES += folding.cc \
     scanpopup.cc \
     articleview.cc \
     audioplayerfactory.cc \
+    externalaudioplayer.cc \
     externalviewer.cc \
     wordfinder.cc \
     groupcombobox.cc \