[Amarok] Clean up the EngineController logic.
Alex Merry
kde at randomguy3.me.uk
Sat Aug 22 11:37:44 CEST 2009
I've figured out why it happens: EngineController no longer emits the
engineStateChanged(Phonon::Playing, Phonon::Playing) notification when the
track changes (because it's not a state change - this was a hack to make the
window caption update). Various places were incorrectly listening for this to
tell them that a track changed.
Things like the current track applet should be listening to engineNewMetdata
instead (or as well as). I'm going to fix the MainWindow caption and the
context view stuff, but please bear this in mind in the future. Particularly
if you test with GStreamer, as GStreamer appears to emit state changes when
the track changes (I'm guessing Playing->Buffering->Playing).
Alex
On Saturday 22 August 2009 01:27:44 Seb Ruiz wrote:
> Hi Alex,
> Not sure if this is the commit behind this break, but it seems that
> Amarok doesn't register automatic track changes anymore.
>
> Anybody else have a clue if it isn't?
>
> Seb
>
> 2009/8/19 Alex Merry <kde at randomguy3.me.uk>:
> > commit 28b3e64d1dc5c7170d7b5d09e6cb4c8b3cdd4e02
> > Author: Alex Merry <kde at randomguy3.me.uk>
> > AuthorDate: Fri Aug 7 21:26:34 2009 +0100
> > Commit: Alex Merry <kde at randomguy3.me.uk>
> > CommitDate: Tue Aug 18 19:49:42 2009 +0100
> >
> > Clean up the EngineController logic.
> >
> > This commit has been sitting on my computer for too long.
> >
> > Leo: this does the fadeout logic the "right way" (and in doing so
> > makes it work properly on GStreamer), at the expense of not working on
> > CoreAudio (as I recall, there were some issues with keeping a
> > VolumeFaderEffect in the audio path on CoreAudio). You might want to try
> > re-enabling it and seeing if Phonon on CoreAudio is still broken.
> >
> > CCMAIL: lfranchi at kde.org
> >
> > diff --git a/src/EngineController.cpp b/src/EngineController.cpp
> > index 4911d65..f8b93e1 100644
> > --- a/src/EngineController.cpp
> > +++ b/src/EngineController.cpp
> > @@ -101,6 +101,14 @@ EngineController::~EngineController()
> > }
> >
> > void
> > +EngineController::createFadeoutEffect()
> > +{
> > + m_fader = new Phonon::VolumeFaderEffect( this );
> > + m_path.insertEffect( m_fader );
> > + m_fader->setFadeCurve( Phonon::VolumeFaderEffect::Fade9Decibel );
> > +}
> > +
> > +void
> > EngineController::initializePhonon()
> > {
> > DEBUG_BLOCK
> > @@ -109,6 +117,7 @@ EngineController::initializePhonon()
> > delete m_controller;
> > delete m_audio;
> > delete m_preamp;
> > + delete m_fader;
> >
> > PERF_LOG( "EngineController: loading phonon objects" )
> > m_media = new Phonon::MediaObject( this );
> > @@ -118,19 +127,27 @@ EngineController::initializePhonon()
> >
> > m_controller = new Phonon::MediaController( m_media );
> >
> > - // HACK we turn off replaygain manually on OSX, until the phonon
> > coreaudio backend is fixed. - // as the default is specified in the
> > .cfg file, we can't just tell it to be a different default on OSX + //
> > HACK we turn off replaygain and fader manually on OSX, until the phonon +
> > // coreaudio backend is fixed as the default is specified in the .cfg
> > file, + // we can't just tell it to be a different default on OSX
> > #ifdef Q_WS_MAC
> > AmarokConfig::setReplayGainMode(
> > AmarokConfig::EnumReplayGainMode::Off ); + AmarokConfig::setFadeout(
> > false );
> > #endif
> >
> > - // only create pre-amp if we have replaygain on, preamp can cause
> > phonon issues + // only create pre-amp if we have replaygain on,
> > VolumeFaderEffect can cause phonon issues if(
> > AmarokConfig::replayGainMode() != AmarokConfig::EnumReplayGainMode::Off )
> > {
> > m_preamp = new Phonon::VolumeFaderEffect( this );
> > m_path.insertEffect( m_preamp );
> > }
> >
> > + // only create fader if we have fadeout on, VolumeFaderEffect can
> > cause phonon issues + if( AmarokConfig::fadeout() &&
> > AmarokConfig::fadeoutLength() ) + {
> > + createFadeoutEffect();
> > + }
> > +
> > m_media->setTickInterval( 100 );
> > debug() << "Tick Interval (actual): " << m_media->tickInterval();
> > PERF_LOG( "EngineController: loaded phonon objects" )
> > @@ -149,13 +166,10 @@ EngineController::initializePhonon()
> > connect( m_controller, SIGNAL( titleChanged( int ) ), SLOT(
> > slotTitleChanged( int ) ) );
> >
> >
> > - //TODO: The xine engine does not support crossfading. Cannot get the
> > gstreamer engine to work, will test this once I do. -#if 0
> > if( AmarokConfig::trackDelayLength() > -1 )
> > m_media->setTransitionTime( AmarokConfig::trackDelayLength() );
> > // Also Handles gapless. else if( AmarokConfig::crossfadeLength() > 0 )
> > // TODO: Handle the possible options on when to crossfade.. the values
> > are not documented anywhere however m_media->setTransitionTime(
> > -AmarokConfig::crossfadeLength() ); -#endif
> > }
> >
> >
> > @@ -271,11 +285,15 @@ EngineController::play() //SLOT
> > {
> > DEBUG_BLOCK
> >
> > + // FIXME: what should we do in BufferingState?
> > if( m_media->state() == Phonon::PlayingState )
> > return;
> >
> > - if( m_fader )
> > - m_fader->deleteLater();
> > + if ( m_fader )
> > + {
> > + m_fadeoutTimer->stop();
> > + m_fader->setVolume(1.0);
> > + }
> >
> > if ( m_media->state() == Phonon::PausedState )
> > {
> > @@ -343,7 +361,6 @@ EngineController::playUrl( const KUrl &url, uint
> > offset ) slotStopFadeout();
> >
> > debug() << "URL: " << url.url();
> > - /// TODO: commented out since audiocd needs porting to new devicelib
> > framework, should not affect other urls
> >
> > if ( url.url().startsWith( "audiocd:/" ) )
> > {
> > @@ -440,22 +457,23 @@ EngineController::stop( bool forceInstant ) //SLOT
> > trackChangedNotify( Meta::TrackPtr( 0 ) );
> > }
> >
> > - // Stop instantly if fadeout is already running, or the media is
> > paused (i.e. pressing Stop twice) - if( m_fader || m_media->state() ==
> > Phonon::PausedState )
> > + // Stop instantly if fadeout is already running, or if the media is
> > not actually playing + if( m_fadeoutTimer->isActive() ||
> > m_media->state() != Phonon::PlayingState ) {
> > forceInstant = true;
> > }
> >
> > if( AmarokConfig::fadeout() && AmarokConfig::fadeoutLength() &&
> > !forceInstant ) {
> > - stateChangedNotify( Phonon::StoppedState, Phonon::PlayingState
> > ); //immediately disable Stop action + // WARNING: this can cause
> > a gap in playback on GStreamer... + if ( !m_fader )
> > + createFadeoutEffect();
> >
> > - m_fader = new Phonon::VolumeFaderEffect( this );
> > - m_path.insertEffect( m_fader );
> > - m_fader->setFadeCurve( Phonon::VolumeFaderEffect::Fade9Decibel
> > ); m_fader->fadeOut( AmarokConfig::fadeoutLength() );
> >
> > m_fadeoutTimer->start( AmarokConfig::fadeoutLength() + 1000 );
> > //add 1s for good measure, otherwise seems to cut off early (buffering..)
> > +
> > + stateChangedNotify( Phonon::StoppedState, m_media->state() );
> > }
> > else
> > m_media->stop();
> > @@ -477,12 +495,17 @@ EngineController::playPause() //SLOT
> > //this is used by the TrayIcon, PlayPauseAction and DBus
> > debug() << "PlayPause: phonon state" << m_media->state();
> >
> > - if( m_media->state() == Phonon::PausedState ||
> > - m_media->state() == Phonon::StoppedState ||
> > - m_media->state() == Phonon::LoadingState )
> > - play();
> > - else
> > - pause();
> > + switch ( m_media->state() )
> > + {
> > + case Phonon::PausedState:
> > + case Phonon::StoppedState:
> > + case Phonon::LoadingState:
> > + play();
> > + break;
> > + default:
> > + pause();
> > + break;
> > + }
> > }
> >
> > void
> > @@ -499,9 +522,10 @@ EngineController::seek( int ms ) //SLOT
> > if ( m_boundedPlayback )
> > seekTo = m_boundedPlayback->startPosition() + ms;
> > else
> > - seekTo = ms;
> > + seekTo = ms;
> >
> > m_media->seek( static_cast<qint64>( seekTo ) );
> > + // FIXME: is this correct for bounded playback?
> > trackPositionChangedNotify( seekTo, true ); /* User seek */
> > }
> > else
> > @@ -632,6 +656,15 @@ EngineController::setNextTrack( Meta::TrackPtr track
> > ) }
> > }
> >
> > +Phonon::State
> > +EngineController::state() const
> > +{
> > + if ( m_fadeoutTimer->isActive() )
> > + return Phonon::StoppedState;
> > + else
> > + return phononMediaObject()->state();
> > +}
> > +
> > bool
> > EngineController::isStream()
> > {
> > @@ -816,9 +849,6 @@ EngineController::slotNewTrackPlaying( const
> > Phonon::MediaSource &source ) else if( m_preamp )
> > m_preamp->setVolumeDecibel( 0.0 );
> >
> > - // state never changes if tracks are queued, but we need this to
> > update the caption - stateChangedNotify( m_media->state(),
> > m_media->state() );
> > -
> > trackChangedNotify( m_currentTrack );
> > newTrackPlaying();
> > }
> > @@ -828,10 +858,6 @@ EngineController::slotStateChanged( Phonon::State
> > newState, Phonon::State oldSta {
> > DEBUG_BLOCK
> >
> > - // Sanity checks:
> > - if( newState == oldState )
> > - return;
> > -
> > // HACK:
> > // The following check is an attempt to fix
> > http://bugs.kde.org/show_bug.cgi?id=180339 // ("amarok stops playing
> > tracks") and other issues with Phonon. @@ -848,6 +874,10 @@
> > EngineController::slotStateChanged( Phonon::State newState, Phonon::State
> > oldSta newState = Phonon::ErrorState; // Indicate error
> > }
> >
> > + // Sanity checks:
> > + if( newState == oldState )
> > + return;
> > +
> > if( newState == Phonon::ErrorState ) // If media is borked, skip to
> > next track {
> > warning() << "Phonon failed to play this URL. Error: " <<
> > m_media->errorString(); @@ -881,6 +911,16 @@
> > EngineController::slotStateChanged( Phonon::State newState, Phonon::State
> > oldSta The::playlistActions()->requestNextTrack();
> > }
> >
> > + if ( m_fadeoutTimer->isActive() )
> > + {
> > + // We've stopped already as far as the rest of Amarok is
> > concerned + if ( oldState == Phonon::PlayingState )
> > + oldState = Phonon::StoppedState;
> > +
> > + if ( oldState == newState )
> > + return;
> > + }
> > +
> > stateChangedNotify( newState, oldState );
> > }
> >
> > @@ -989,8 +1029,8 @@ EngineController::slotStopFadeout() //SLOT
> > m_fadeoutTimer->stop();
> >
> > if ( m_fader ) {
> > - m_fader->deleteLater();
> > m_media->stop();
> > + m_fader->setVolume( 1.0 );
> > }
> > }
> >
> > diff --git a/src/EngineController.h b/src/EngineController.h
> > index db91da0..d7e32c1 100644
> > --- a/src/EngineController.h
> > +++ b/src/EngineController.h
> > @@ -101,7 +101,7 @@ public:
> > /**
> > * The state of the engine
> > */
> > - Phonon::State state() const { return phononMediaObject()->state(); }
> > + Phonon::State state() const;
> >
> > /*enum Filetype { MP3 };*/ //assuming MP3 for time being
> > /*AMAROK_EXPORT*/ static bool installDistroCodec();
> > @@ -276,6 +276,8 @@ private:
> > */
> > void playUrl( const KUrl &url, uint offset );
> >
> > + void createFadeoutEffect();
> > +
> > static EngineController* s_instance;
> > EngineController();
> > ~EngineController();
>
--
Why have I got six monitors? Because I haven't got room for eight.
-- Terry Pratchett
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/amarok-devel/attachments/20090822/b76d339e/attachment.sig
More information about the Amarok-devel
mailing list