Bug 127316

Not a bug, but just not implemented.

Created attachment 16491 [details]
Patch that enables fadeout on stop, too...

Created attachment 16500 [details]
Now it does at least something with crossfade length...

Comment on attachment 16500 [details]
Now it does at least something with crossfade length...

Okay, its closer to right value if it's m_xfadeLenght * 150...

I just wanted to add here:
I hate it too when I press stop, and the music stops immediately!
But I also hate it, when songs are faded in... the beginning of a song is _not_ to be faded in my opinion.

My suggestion is: On a manual stop, music should be faded out! On a manual start, music should fade in, IF the song was stopped in the middle.

A normal behaviour in my opinion would be:
If i select a song while listening to another -> fade the first one out, the second one should start immediately!
If I press 'stop'/'pause'-> fade out
If I press play, from a paused position -> fade in.

If this could be implemented this would be really perfect!!
Thanks.

Created attachment 17742 [details]
Rewrote the patch, should be quite clear now...

Sorry Tuomas, we can't use this patch. fadeOut() is blocking the GUI thread (as it is running in the GUI thread). It needs some processEvents() instead of sleep().

Created attachment 17750 [details]
New version of the patch, now doesn't lock the GUI, works for pause, too, doesn't cause any major harm to press play while fading out...

Created attachment 17753 [details]
New version, this one handles continuing playing during fadeout a bit cleaner...

Created attachment 17754 [details]
Okay, now it's even more bits better...

Created attachment 17758 [details]
Try 146: Now stop/pause during fadeout is safely ignored (it's not the best option, but I am not the best coder either, fair enough)...

SVN commit 584340 by markey:

* Fade-out for xine-engine when pressing Stop or Pause. Patch by
  Tuomas Nurmi <tnurmi@edu.kauhajoki.fi>. (BR 127316)

Everyone please enable this and test extensively! Do we even have a configure option for it? Maybe not.

FEATURE: 127316


 M  +2 -0      ChangeLog  
 M  +104 -30   src/engine/xine/xine-engine.cpp  
 M  +15 -0     src/engine/xine/xine-engine.h  


--- trunk/extragear/multimedia/amarok/ChangeLog #584339:584340
@@ -4,6 +4,8 @@
 
 VERSION 1.4.4
   FEATURES:
+    * Fade-out for xine-engine when pressing Stop or Pause. Patch by 
+      Tuomas Nurmi <tnurmi@edu.kauhajoki.fi>. (BR 127316)
     * Support downloading of files from an MTP device.
     * Purged podcast episodes can be readded by increasing the purge
       number.
--- trunk/extragear/multimedia/amarok/src/engine/xine/xine-engine.cpp #584339:584340
@@ -64,6 +64,7 @@
 //static inline QCString configPath() { return QFile::encodeName(KStandardDirs().localkdedir() + KStandardDirs::kde_default("data") + "amarok/xine-config"); }
 static inline QCString configPath() { return QFile::encodeName(locate( "data", "amarok/") + "xine-config" ); }
 static Fader *s_fader = 0;
+static OutFader *s_outfader = 0;
 
 
 XineEngine::XineEngine()
@@ -75,6 +76,7 @@
         , m_post( 0 )
         , m_preamp( 1.0 )
         , m_stopFader( false )
+        , m_fadeOutRunning ( false )
         , m_equalizerEnabled( false )
 {
     addPluginProperty( "HasConfigure", "true" );
@@ -94,25 +96,13 @@
         s_fader->wait();
         delete s_fader;
     }
+    if( s_outfader ) {
+        s_outfader->wait();
+        delete s_outfader;
+    }
 
-    // NOTE The fadeout gets stuck when the EQ is active, so we skip it then
-    if( !m_equalizerEnabled && m_stream && state() == Engine::Playing )
-    {
-        const int volume = xine_get_param( m_stream, XINE_PARAM_AUDIO_AMP_LEVEL );
-        const double D = 300000 * std::pow( (double)volume, -0.4951 );
+    fadeOut();
 
-        debug() << "Sleeping: " << D << ", " << volume << endl;
-
-        for( int v = volume - 1; v >= 1; v-- ) {
-            xine_set_param( m_stream, XINE_PARAM_AUDIO_AMP_LEVEL, v );
-
-            const int sleep = int(D * (-log10( v + 1 ) + 2));
-
-            ::usleep( sleep );
-        }
-        xine_stop( m_stream );
-    }
-
     if( m_xine )       xine_config_save( m_xine, configPath() );
 
     if( m_stream )     xine_close( m_stream );
@@ -361,29 +351,30 @@
 {
     if ( !m_stream )
        return;
+    if( !m_fadeOutRunning || state() == Engine::Paused )
+    {
+        s_outfader = new OutFader( this, true, true );
+        s_outfader->start();
+        ::usleep( 100 ); //to be sure engine state won't be changed before it is checked in fadeOut()
+        m_url = KURL(); //to ensure we return Empty from state()
 
-    m_url = KURL(); //to ensure we return Empty from state()
+        std::fill( m_scope.begin(), m_scope.end(), 0 );
 
-    std::fill( m_scope.begin(), m_scope.end(), 0 );
-
-    xine_stop( m_stream );
-    xine_close( m_stream );
-    xine_set_param( m_stream, XINE_PARAM_AUDIO_CLOSE_DEVICE, 1);
-
-    emit stateChanged( Engine::Empty );
+        emit stateChanged( Engine::Empty );
+    }
 }
 
 void
 XineEngine::pause()
 {
-    if( xine_get_param( m_stream, XINE_PARAM_SPEED ) )
+    if( xine_get_param( m_stream, XINE_PARAM_SPEED ) && state() != Engine::Paused )
     {
-        xine_set_param( m_stream, XINE_PARAM_SPEED, XINE_SPEED_PAUSE );
-        xine_set_param( m_stream, XINE_PARAM_AUDIO_CLOSE_DEVICE, 1);
+        s_outfader = new OutFader( this, false );
+        s_outfader->start();
+        ::usleep( 100 ); //to be sure engine state won't be changed before it is checked in fadeOut()
         emit stateChanged( Engine::Paused );
+    } else if ( !m_fadeOutRunning ) {
 
-    } else {
-
         xine_set_param( m_stream, XINE_PARAM_SPEED, XINE_SPEED_NORMAL );
         emit stateChanged( Engine::Playing );
     }
@@ -482,6 +473,44 @@
 }
 
 void
+XineEngine::fadeOut()
+{
+    if( m_fadeOutRunning ) //Let us not start another fadeout...
+        return;
+    m_fadeOutRunning = !m_fadeOutRunning;
+    // NOTE The fadeout gets stuck when the EQ is active, so we skip it then
+    if( m_xfadeLength > 0 && !m_equalizerEnabled && m_stream && state() == Engine::Playing )
+    {
+        // fader-class doesn't work in this spot as is, so some parts need to be copied here... (ugly)
+        uint stepsCount = m_xfadeLength < 1000 ? m_xfadeLength / 10 : 100;
+        uint stepSizeUs = (int)( 1000.0 * (float)m_xfadeLength / (float)stepsCount );
+
+        ::usleep( stepSizeUs );
+        QTime t;
+        t.start();
+        float mix = 0.0;
+        while ( mix < 1.0 )
+        {
+            ::usleep( stepSizeUs );
+            float vol = Engine::Base::makeVolumeLogarithmic( m_volume ) * m_preamp;
+            float mix = (float)t.elapsed() / (float)m_xfadeLength;
+            if ( mix > 1.0 )
+            {
+                break;
+            }
+            if ( m_stream )
+            {
+                float v = 4.0 * (1.0 - mix) / 3.0;
+                xine_set_param( m_stream, XINE_PARAM_AUDIO_AMP_LEVEL, (uint)( v < 1.0 ? vol * v : vol ) );
+            }
+        }
+    }
+    if( m_fadeOutRunning )
+        xine_set_param( m_stream, XINE_PARAM_AUDIO_AMP_LEVEL, (uint)(m_volume * m_preamp) );
+    m_fadeOutRunning = !m_fadeOutRunning;
+}
+
+void
 XineEngine::setEqualizerEnabled( bool enable )
 {
     if ( !m_stream )
@@ -1044,6 +1073,51 @@
 }
 
 
+//////////////////
+/// class OutFader
+//////////////////
+
+OutFader::OutFader( XineEngine *engine, bool stop, bool force )
+   : QObject( engine )
+   , QThread()
+   , m_engine( engine )
+   , m_stop( stop )
+   , m_force( force )
+{
+}
+
+OutFader::~OutFader()
+{
+     wait();
+
+     DEBUG_FUNC_INFO
+
+     s_outfader = 0;
+}
+
+void
+OutFader::run()
+{
+    m_engine->fadeOut();
+    if( m_engine->m_fadeOutRunning == false || m_force )
+    {
+        if( m_stop )
+        {
+            xine_stop( m_engine->m_stream );
+            xine_close( m_engine->m_stream );
+            xine_set_param( m_engine->m_stream, XINE_PARAM_AUDIO_CLOSE_DEVICE, 1);
+        }
+        else
+        {
+            xine_set_param( m_engine->m_stream, XINE_PARAM_SPEED, XINE_SPEED_PAUSE );
+            xine_set_param( m_engine->m_stream, XINE_PARAM_AUDIO_CLOSE_DEVICE, 1);
+        }
+    }
+    QThread::sleep( 3 );
+    deleteLater();
+}
+
+
 bool XineEngine::metaDataForUrl(const KURL &url, Engine::SimpleMetaBundle &b)
 {
     bool result = false;
--- trunk/extragear/multimedia/amarok/src/engine/xine/xine-engine.h #584339:584340
@@ -27,6 +27,7 @@
     Q_OBJECT
 
     friend class Fader;
+    friend class OutFader;
 
    ~XineEngine();
 
@@ -51,6 +52,7 @@
     virtual void setEqualizerEnabled( bool );
     virtual void setEqualizerParameters( int preamp, const QValueList<int>& );
     virtual void setVolumeSW( uint );
+    virtual void fadeOut();
 
     static  void XineEventListener( void*, const xine_event_t* );
     virtual void customEvent( QCustomEvent* );
@@ -73,6 +75,7 @@
     float               m_preamp;
 
     bool                m_stopFader;
+    bool                m_fadeOutRunning;
 
     QString             m_currentAudioPlugin; //to see if audio plugin has been changed
     XineConfigDialog*   m_configDialog;
@@ -110,4 +113,16 @@
    ~Fader();
 };
 
+class OutFader : public QObject, public QThread
+{
+    XineEngine         *m_engine;
+    bool               m_stop;
+    bool               m_force;
+
+    virtual void run();
+public:
+    OutFader( XineEngine *, bool stop, bool force = false );
+    ~OutFader();
+};
+
 #endif

Some remarks to the patch:
- Pressing Pause, and then during the xfade, pressing play does not have 
any effect.
I would have assumed that the song gets immediately played again.
Similar thing when pressing pause, and then during the xfade duration 
pressing stop has also no effect.
I don't think that this is very intuitive.

- When pressing pause, the time display is immediately stopped (but the 
song is going on until it is faded out).
When pressing play (after xfade has finished), the time is hopping with 
the amount of the xfade duration.
This is too not very intuitive.

- Now when pressing pause during the crossfade period of two songs (the 
merging period when one song is fading out, and the next song is fading 
in), the sound gets distorted. This is no wonder as the new outfade 
instance is doing the same job as the regular fade instance at the same 
time.
Too bug 122514 is not solved, the song plays on after the xfade duration 
is over (see also my patch of bug 122514 solving that issue)

Another remark:
I'm listening mainly to a shuffled playlist.
Thus I've set the crossfade duration to a rather high value of 8 sec, which performs well with this 
kind of playlists.
But 8 seconds are much too high for a pause or stop fade out.
So a separate configurable pause/stop xfade duration would be welcome.

 So a separate configurable pause/stop xfade duration would be welcome. 

Sure, but it would need tinkering with UI files, which I do not dare to do, because I'd probably break something with my lack of Qt skill. If anyone else is willing to do this, go ahead...

SVN commit 586419 by markey:

We've decided that fadeout on pause isn't very useful, so I'm removing it. Fadeout on stop stays.

CCBUG: 127316


 M  +5 -5      xine-engine.cpp  


--- trunk/extragear/multimedia/amarok/src/engine/xine/xine-engine.cpp #586418:586419
@@ -367,14 +367,14 @@
 void
 XineEngine::pause()
 {
-    if( xine_get_param( m_stream, XINE_PARAM_SPEED ) && state() != Engine::Paused )
+    if( xine_get_param( m_stream, XINE_PARAM_SPEED ) )
     {
-        s_outfader = new OutFader( this, false );
-        s_outfader->start();
-        ::usleep( 100 ); //to be sure engine state won't be changed before it is checked in fadeOut()
+        xine_set_param( m_stream, XINE_PARAM_SPEED, XINE_SPEED_PAUSE ); 
+        xine_set_param( m_stream, XINE_PARAM_AUDIO_CLOSE_DEVICE, 1);
         emit stateChanged( Engine::Paused );
-    } else if ( !m_fadeOutRunning ) {
 
+    } else {
+
         xine_set_param( m_stream, XINE_PARAM_SPEED, XINE_SPEED_NORMAL );
         emit stateChanged( Engine::Playing );
     }

Tuomas, please have a look at this report: BUG 134274

SVN commit 586440 by markey:

Regression fix: With fadeout enabled, context browser would no longer switch back to idle view after pressing stop. This was because the browser was accessing Engine::state(), which still returned Playing.

CCBUG: 127316


 M  +5 -2      xine-engine.cpp  


--- trunk/extragear/multimedia/amarok/src/engine/xine/xine-engine.cpp #586439:586440
@@ -392,7 +392,7 @@
 Engine::State
 XineEngine::state() const
 {
-    if ( !m_stream )
+    if ( !m_stream || m_fadeOutRunning )
        return Engine::Empty;
 
     switch( xine_get_status( m_stream ) )
@@ -486,9 +486,12 @@
 {
     if( m_fadeOutRunning ) //Let us not start another fadeout...
         return;
+
     m_fadeOutRunning = !m_fadeOutRunning;
+    const bool isPlaying = m_stream && ( xine_get_status( m_stream ) == XINE_STATUS_PLAY );
+
     // NOTE The fadeout gets stuck when the EQ is active, so we skip it then
-    if( m_xfadeLength > 0 && !m_equalizerEnabled && m_stream && state() == Engine::Playing )
+    if( m_xfadeLength > 0 && !m_equalizerEnabled && isPlaying )
     {
         // fader-class doesn't work in this spot as is, so some parts need to be copied here... (ugly)
         uint stepsCount = m_xfadeLength < 1000 ? m_xfadeLength / 10 : 100;

When I'm playing really loud, cutting off the music abruptly on pause is quite a shock. So, could you please bring back fade on pause? I'd probably use it with small values like 500msec, but this makes all the difference.
Reopening to get attention :)

No, sorry. We have decided against fadeout-on-pause.