[Amarok] Clean up the EngineController logic.

[Leo Franchi]

On Friday 21 August 2009 17: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.

I think i already replied to this email on kde-commits :) in any case, i 
talked to alex about it, he said he'd take a look this weekend. I definitely 
tracked it down to this commit.

if it's not fixed by this weekend i'll revert till alex has some more time :)

leo


> 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();