From 278e05cbf3ac2d3952fc709e81b63439612730a6 Mon Sep 17 00:00:00 2001 From: Igor Kushnir Date: Thu, 22 Mar 2018 19:07:10 +0200 Subject: [PATCH] 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 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 | 46 ++++--------------- audioplayerfactory.cc | 28 +++++++++++- audioplayerfactory.hh | 8 ++++ audioplayerinterface.hh | 8 +++- externalaudioplayer.cc | 99 +++++++++++++++++++++++++++++++++++++++++ externalaudioplayer.hh | 44 ++++++++++++++++++ externalviewer.cc | 37 ++++++++++++--- externalviewer.hh | 19 ++++++-- ffmpegaudioplayer.hh | 3 +- goldendict.pro | 2 + 10 files changed, 242 insertions(+), 52 deletions(-) create mode 100644 externalaudioplayer.cc create mode 100644 externalaudioplayer.hh diff --git a/articleview.cc b/articleview.cc index aa7d006d..b9f63ee9 100644 --- a/articleview.cc +++ b/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 #include #include @@ -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" ) ); } diff --git a/audioplayerfactory.cc b/audioplayerfactory.cc index cf1d5e80..68184b29 100644 --- a/audioplayerfactory.cc +++ b/audioplayerfactory.cc @@ -1,11 +1,16 @@ /* This file is (c) 2018 Igor Kushnir * Part of GoldenDict. Licensed under GPLv3 or later, see the LICENSE file */ +#include +#include #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() ); +} diff --git a/audioplayerfactory.hh b/audioplayerfactory.hh index 482f385e..09fdf43a 100644 --- a/audioplayerfactory.hh +++ b/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; }; diff --git a/audioplayerinterface.hh b/audioplayerinterface.hh index 66b055bf..d9cd22e6 100644 --- a/audioplayerinterface.hh +++ b/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 ); }; diff --git a/externalaudioplayer.cc b/externalaudioplayer.cc new file mode 100644 index 00000000..7bda4164 --- /dev/null +++ b/externalaudioplayer.cc @@ -0,0 +1,99 @@ +/* This file is (c) 2018 Igor Kushnir + * 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(); +} diff --git a/externalaudioplayer.hh b/externalaudioplayer.hh new file mode 100644 index 00000000..c9a531aa --- /dev/null +++ b/externalaudioplayer.hh @@ -0,0 +1,44 @@ +/* This file is (c) 2018 Igor Kushnir + * Part of GoldenDict. Licensed under GPLv3 or later, see the LICENSE file */ + +#ifndef EXTERNALAUDIOPLAYER_HH_INCLUDED +#define EXTERNALAUDIOPLAYER_HH_INCLUDED + +#include +#include +#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 diff --git a/externalviewer.cc b/externalviewer.cc index 5a56f0a7..b89043a6 100644 --- a/externalviewer.cc +++ b/externalviewer.cc @@ -2,22 +2,21 @@ * Part of GoldenDict. Licensed under GPLv3 or later, see the LICENSE file */ #include +#include #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; +} diff --git a/externalviewer.hh b/externalviewer.hh index 832ed503..d2014d40 100644 --- a/externalviewer.hh +++ b/externalviewer.hh @@ -7,7 +7,6 @@ #include #include #include -#include #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 diff --git a/ffmpegaudioplayer.hh b/ffmpegaudioplayer.hh index 7de992f2..bfce56e6 100644 --- a/ffmpegaudioplayer.hh +++ b/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() diff --git a/goldendict.pro b/goldendict.pro index 0dd6cd5b..fa1505b2 100644 --- a/goldendict.pro +++ b/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 \