Timeline: Rework locking.

timeline/src/Audio_Region.C

@@ -477,6 +477,8 @@ Audio_Region::draw_box( void )
 void
 Audio_Region::peaks_ready_callback ( void *v )
 {
+    /* this is called from the peak builder thread */
+
     DMESSAGE("Damaging region from peaks ready callback");
     Fl::lock();
     ((Audio_Region*)v)->redraw();

timeline/src/Control_Sequence.C

@@ -85,12 +85,8 @@ Control_Sequence::~Control_Sequence ( )
 
     log_destroy();
 
-    engine->lock();
-
     track()->remove( this );
 
-    engine->unlock();
-
     if ( _output )
     { 
         _output->shutdown();
@@ -144,8 +140,6 @@ Control_Sequence::name ( const char *s )
 void
 Control_Sequence::update_port_name ( void )
 {
-    engine->lock();
-
     bool needs_activation = false;
     if ( ! _output )
     {
@@ -170,8 +164,6 @@ Control_Sequence::update_port_name ( void )
             _output = NULL;
         }
     }
-
-    engine->unlock();
 }
 
 void
@@ -621,7 +613,7 @@ Control_Sequence::process_osc ( void )
 {
     if ( mode() != OSC )
         return;
-   
+
     header()->outputs_indicator->value( _osc_output() && _osc_output()->connected() );
 
     if ( _osc_output() && _osc_output()->connected() )
@@ -769,7 +761,7 @@ Control_Sequence::handle ( int m )
 
                 add( r );
 
-                timeline->unlock();
+               timeline->unlock();
 
                 return 1;
             }

timeline/src/Engine/Disk_Stream.C

@@ -1,4 +1,4 @@
-
+ 
 /*******************************************************************************/
 /* Copyright (C) 2008 Jonathan Moore Liles                                     */
 /*                                                                             */
@@ -73,7 +73,8 @@ Disk_Stream::Disk_Stream ( Track *track, float frame_rate, nframes_t nframes, in
 Disk_Stream::~Disk_Stream ( )
 {
     /* it isn't safe to do all this with the RT thread running */
-    engine->lock();
+
+    timeline->wrlock();
 
     shutdown();
 
@@ -84,7 +85,7 @@ Disk_Stream::~Disk_Stream ( )
     for ( int i = channels(); i--; )
         jack_ringbuffer_free( _rb[ i ] );
 
-    engine->unlock();
+    timeline->unlock();
 }
 
 
@@ -146,23 +147,13 @@ Disk_Stream::shutdown ( void )
         _terminate = true;
         
         /* try to wake the thread so it'll see that it's time to die */
-        int total_ms = 0;
         while ( _terminate )
         {
-            usleep( 1000 * 10 );
-            total_ms += 10;
+            usleep( 100 );
             block_processed();
-            
-            if ( total_ms > 100 )
-            {
-                WARNING("Disk_Stream thread has taken longer than %ims to respond to terminate signal. Canceling", total_ms );
-                _thread.cancel();
-                break;
-            }
         }
         
-        if ( ! _terminate )
-            _thread.join();
+        _thread.join();
 
         sem_destroy( &_blocks );
 

timeline/src/Engine/Engine.C

@@ -168,7 +168,11 @@ Engine::process ( nframes_t nframes )
     }
     else
     {
-        if ( ! trylock() )
+        if ( !timeline)
+        /* handle chicken/egg problem */
+            return 0;
+
+        if ( timeline->tryrdlock() )
         {
             /* the data structures we need to access here (tracks and
              * their ports, but not track contents) may be in an
@@ -178,14 +182,12 @@ Engine::process ( nframes_t nframes )
             return 0;
         }
 
-        /* handle chicken/egg problem */
-        if ( timeline )
-            /* this will initiate the process() call graph for the various
-             * number and types of tracks, which will in turn send data out
-             * the appropriate ports.  */
-            timeline->process( nframes );
+        /* this will initiate the process() call graph for the various
+         * number and types of tracks, which will in turn send data out
+         * the appropriate ports.  */
+        timeline->process( nframes );
 
-        unlock();
+        timeline->unlock();
     }
 
     return 0;

timeline/src/Engine/Engine.H

@@ -27,7 +27,7 @@ class Port;
 
 #include "Thread.H"
 
-class Engine : public JACK::Client, public Mutex
+class Engine : public JACK::Client
 {
     Thread _thread;                                            /* only used for thread checking */
 

timeline/src/Engine/Playback_DS.C

@@ -33,6 +33,7 @@
 #include "const.h"
 #include "debug.h"
 #include "Thread.H"
+#include <unistd.h>
 
 bool
 Playback_DS::seek_pending ( void )
@@ -76,29 +77,36 @@ Playback_DS::read_block ( sample_t *buf, nframes_t nframes )
 
     memset( buf, 0, nframes * sizeof( sample_t ) * channels() );
 
-    /* stupid chicken/egg */
-    if ( ! timeline )
-        return;
-
 //    printf( "IO: attempting to read block @ %lu\n", _frame );
 
-    timeline->rdlock();
+    if ( !timeline )
+        return;
 
-    if ( sequence() )
+    while ( ! _terminate )
     {
-        /* FIXME: how does this work if _delay is not a multiple of bufsize? */
-        
-        if ( _frame >= _delay )
+        if ( ! timeline->tryrdlock() )
         {
-            if ( ! sequence()->play( buf, _frame - _delay, nframes, channels() ) )
-                WARNING( "Programming error?" );
+            if ( sequence() )
+            {
+                
+                /* FIXME: how does this work if _delay is not a multiple of bufsize? */
+                
+                if ( _frame >= _delay )
+                {
+                    if ( ! sequence()->play( buf, _frame - _delay, nframes, channels() ) )
+                        WARNING( "Programming error?" );
+                }
+                
+                _frame += nframes;
+            }
+            
+            timeline->unlock();
+            
+            return;
         }
-        
-        _frame += nframes;
-        
+
+        usleep( 1000 * 10 );
     }
-    
-    timeline->unlock();
 }
 
 #define AVOID_UNNECESSARY_COPYING 1
@@ -146,6 +154,10 @@ Playback_DS::disk_thread ( void )
 
         read_block( buf, nframes );
 
+        /* might have received terminate signal while waiting for block */
+        if ( _terminate )
+            goto done;
+
 //        unlock(); // for seeking
 
         /* deinterleave the buffer and stuff it into the per-channel ringbuffers */
@@ -201,6 +213,8 @@ Playback_DS::disk_thread ( void )
 
     }
 
+done:
+
     DMESSAGE( "playback thread terminating" );
 
     delete[] buf;
@@ -209,6 +223,8 @@ Playback_DS::disk_thread ( void )
 #endif
 
     _terminate = false;
+
+    _thread.exit();
 }
 
 /** take a single block from the ringbuffers and send it out the

timeline/src/Engine/Track.C

@@ -72,7 +72,7 @@ Track::configure_outputs ( int n )
     if ( n == on )
         return true;
 
-//    engine->lock();
+    DMESSAGE( "Reconfiguring outputs for track %s", name() );
 
     if ( playback_ds )
     {
@@ -111,7 +111,6 @@ Track::configure_outputs ( int n )
     if ( output.size() )
         playback_ds = new Playback_DS( this, engine->frame_rate(), engine->nframes(), output.size() );
 
-//    engine->unlock();
     /* FIXME: bogus */
     return true;
 }
@@ -124,6 +123,8 @@ Track::configure_inputs ( int n )
     if ( n == on )
         return true;
 
+    DMESSAGE( "Reconfiguring inputs for track %s", name() );
+
 //    engine->lock();
 
     if ( record_ds )

timeline/src/Project.C

@@ -204,7 +204,9 @@ Project::close ( void )
         return false;
     
     timeline->wrlock();
+
     Loggable::close();
+
     timeline->unlock();
 
 //    write_info();
@@ -311,13 +313,11 @@ Project::open ( const char *name )
     if ( ! engine )
         make_engine();
  
-    timeline->wrlock();
     {
         Block_Timer timer( "Replayed journal" );
         if ( ! Loggable::open( "history" ) )
             return E_INVALID;
     }
-    timeline->unlock();
 
     /* /\* really a good idea? *\/ */
     /* timeline->sample_rate( rate ); */

timeline/src/Sequence.C

@@ -76,14 +76,14 @@ Sequence::~Sequence (  )
 {
     DMESSAGE( "destroying sequence" );
 
-    if ( _name )
-        free( _name );
-
     if ( _widgets.size() )
         FATAL( "programming error: leaf destructor must call Sequence::clear()!" );
 
     if ( parent() )
         parent()->remove( this );
+
+    if ( _name )
+        free( _name );
 }
 
 
@@ -155,10 +155,10 @@ void
 Sequence::handle_widget_change ( nframes_t start, nframes_t length )
 {
     /* this might be invoked from Capture or GUI thread */
-    Fl::lock();
+//    Fl::lock();
     sort();
     timeline->damage_sequence();
-    Fl::unlock();
+//    Fl::unlock();
 
 //    timeline->update_length( start + length );
 }
@@ -211,7 +211,6 @@ Sequence::remove ( Sequence_Widget *r )
     _widgets.remove( r );
 
     handle_widget_change( r->start(), r->length() );
-    
 }
 
 static nframes_t
@@ -400,9 +399,7 @@ Sequence::handle ( int m )
                         /* accept objects dragged from other sequences of this type */
                         
                         timeline->wrlock();
-                        
                         add( Sequence_Widget::pushed() );
-                        
                         timeline->unlock();
                         
                         damage( FL_DAMAGE_USER1 );
@@ -487,7 +484,6 @@ Sequence::handle ( int m )
                 Loggable::block_start();
 
                 timeline->wrlock();
-
                 while ( _delete_queue.size() )
                 {
 
@@ -504,7 +500,6 @@ Sequence::handle ( int m )
 
                     delete t;
                 }
-
                 timeline->unlock();
 
                 Loggable::block_end();

timeline/src/Sequence_Widget.C

@@ -168,7 +168,7 @@ Sequence_Widget::begin_drag ( const Drag &d )
 {
     _drag = new Drag( d );
 
-    timeline->rdlock();
+    timeline->wrlock();
 
     /* copy current values */
     
@@ -400,9 +400,7 @@ Sequence_Widget::handle ( int m )
             /* deletion */
             if ( test_press( FL_BUTTON3 + FL_CTRL ) )
             {
-                timeline->wrlock();
                 remove();
-                timeline->unlock();
 
                 return 1;
             }
@@ -444,7 +442,9 @@ Sequence_Widget::handle ( int m )
             if ( test_press( FL_BUTTON1 + FL_CTRL ) && ! _drag->state )
             {
                 /* duplication */
+                timeline->wrlock();
                 sequence()->add( this->clone() );
+                timeline->unlock();
 
                 _drag->state = 1;
                 return 1;

timeline/src/Timeline.C

@@ -41,7 +41,6 @@
 #include "Annotation_Sequence.H"
 #include "Track.H"
 #include "Transport.H"
-#include "Engine/Engine.H" // for lock()
 
 #include "FL/menu_popup.H"
 
@@ -362,6 +361,10 @@ Timeline::range ( nframes_t start, nframes_t length )
 void
 Timeline::add_take_for_armed_tracks ( void )
 {
+    THREAD_ASSERT( UI );
+
+    wrlock();
+
     for ( int i = tracks->children(); i-- ; )
     {
         Track *t = (Track*)tracks->child( i );
@@ -369,6 +372,8 @@ Timeline::add_take_for_armed_tracks ( void )
         if ( t->armed() && t->sequence()->_widgets.size() )
             t->sequence( new Audio_Sequence( t ) );
     }
+
+    unlock();
 }
 
 void
@@ -938,10 +943,12 @@ Timeline::draw_measure_cb ( nframes_t frame, const BBT &bbt, void *v )
         return;
 
     if ( bbt.beat )
+    {
         if ( o->panzoomer->zoom() > 12 )
             return;
         else
             c = FL_DARK1;
+    }
 
     fl_color( fl_color_add_alpha( c, 64 ) );
     
@@ -1246,6 +1253,8 @@ Timeline::draw_cursors ( void ) const
 void
 Timeline::draw ( void )
 {
+    THREAD_ASSERT( UI );
+
     int X, Y, W, H;
 
     int bdx = 0;
@@ -1869,14 +1878,10 @@ Timeline::add_track ( Track *track )
 {
     DMESSAGE( "added new track to the timeline" );
 
-    engine->lock();
-
     tracks->add( track );
 
 //    update_track_order();
 
-    engine->unlock();
-
     /* FIXME: why is this necessary? doesn't the above add do DAMAGE_CHILD? */
     redraw();
 
@@ -1888,16 +1893,12 @@ Timeline::insert_track ( Track *track, int n )
     if ( n > tracks->children() || n < 0 )
         return;
 
-    engine->lock();
-
     tracks->insert( *track, n );
 
     update_track_order();
 
     tracks->redraw();
 
-    engine->unlock();
-
     /* FIXME: why is this necessary? doesn't the above add do DAMAGE_CHILD? */
 //    redraw();    
 }
@@ -1912,7 +1913,7 @@ compare_tracks ( Track *a, Track *b )
 void
 Timeline::apply_track_order ( void )
 {
-    engine->lock();
+    /* wrlock(); */
 
     std::list<Track*> tl;
     
@@ -1931,7 +1932,7 @@ Timeline::apply_track_order ( void )
 
     update_track_order();
 
-    engine->unlock();
+    /* unlock(); */
 }
 
 void
@@ -1947,17 +1948,6 @@ Timeline::find_track ( const Track *track ) const
     return tracks->find( *track );
 }
 
-void
-Timeline::move_track_up ( Track *track )
-{
-    insert_track( track, find_track( track ) - 1 );
-}
-
-void
-Timeline::move_track_down ( Track *track )
-{
-    insert_track( track, find_track( track ) + 2 );
-}
 
 /** remove /track/ from the timeline */
 void
@@ -1965,15 +1955,11 @@ Timeline::remove_track ( Track *track )
 {
     DMESSAGE( "removed track from the timeline" );
 
-    engine->lock();
-
     /* FIXME: what to do about track contents? */
     tracks->remove( track );
 
     update_track_order();
 
-    engine->unlock();
-
     /* FIXME: why is this necessary? doesn't the above add do DAMAGE_CHILD? */
     redraw();
 }
@@ -1982,6 +1968,30 @@ Timeline::remove_track ( Track *track )
 /* Commands */
 /************/
 
+void
+Timeline::command_move_track_up ( Track *track )
+{
+    wrlock();
+    insert_track( track, find_track( track ) - 1 );
+    unlock();
+}
+
+void
+Timeline::command_move_track_down ( Track *track )
+{
+    wrlock();
+    insert_track( track, find_track( track ) + 2 );
+    unlock();
+}
+
+void
+Timeline::command_remove_track ( Track *track )
+{
+    wrlock();
+    remove_track(track);
+    unlock();
+}
+
 void
 Timeline::command_quit ( )
 {
@@ -1998,8 +2008,10 @@ Timeline::command_load ( const char *name, const char *display_name )
     if ( ! name )
         return false;
   
+    wrlock();
     int r = Project::open( name );
-  
+    unlock();
+
     if ( r < 0 )
     {
   	const char *s = Project::errstr( r );
@@ -2145,7 +2157,7 @@ Timeline::handle_peer_scan_complete ( void *o )
 void
 Timeline::connect_osc ( void )
 {
-    wrlock();
+//    rdlock();
 
     /* try to (re)connect OSC signals */
     for ( int i = tracks->children(); i-- ; )
@@ -2155,7 +2167,7 @@ Timeline::connect_osc ( void )
         t->connect_osc();
     }
 
-    unlock();
+//    unlock();
 }
 
 void
@@ -2183,7 +2195,6 @@ Timeline::process_osc ( void )
 {
     THREAD_ASSERT( OSC );
 
-    /* FIXME: is holding this lock a problem for recording? */
     rdlock();
 
     /* reconnect OSC signals */

timeline/src/Timeline.H

@@ -57,6 +57,8 @@ class Cursor_Point;
 class Fl_Panzoomer;
 class Fl_Tile;
 
+#include "RWLock.H"
+
 namespace OSC { class Endpoint; }
 
 #define USE_WIDGET_FOR_TIMELINE
@@ -83,7 +85,6 @@ struct Rectangle
     Rectangle ( int X, int Y, int W, int H ) : x( X ), y( Y ), w( W ), h( H ) {}
 };
 
-#include "RWLock.H"
 
 #ifdef USE_WIDGET_FOR_TIMELINE
 class Timeline : public Fl_Group, public RWLock
@@ -252,9 +253,10 @@ public:
 
     void add_track ( Track *track );
     void remove_track ( Track *track );
+    void command_remove_track ( Track *track );
 
-    void move_track_up ( Track *track );
-    void move_track_down ( Track *track );
+    void command_move_track_up ( Track *track );
+    void command_move_track_down ( Track *track );
 
     int find_track ( const Track * track ) const;
 

timeline/src/Track.C

@@ -36,8 +36,6 @@
 #include "FL/Fl_Scalepack.H"
 #include "FL/Fl_Blink_Button.H"
 
-#include "Engine/Engine.H" // for lock()
-
 #include "Control_Sequence.H"
 #include "Annotation_Sequence.H"
 
@@ -639,12 +637,8 @@ Track::remove ( Control_Sequence *t )
     if ( ! control )
         return;
 
-    engine->lock();
-
     control->remove( t );
 
-    engine->unlock();
-
     adjust_size();
 }
 
@@ -681,8 +675,6 @@ Track::add ( Control_Sequence *t )
 {
     DMESSAGE( "adding control sequence" );
 
-    engine->lock();
-
     t->track( this );
 
     t->color( random_color() );
@@ -690,8 +682,6 @@ Track::add ( Control_Sequence *t )
 //    control->insert( *t, 0 );
     control->add( t );
     
-    engine->unlock();
-
     adjust_size();
 }
 
@@ -741,6 +731,16 @@ Track::menu_cb ( Fl_Widget *w, void *v )
     ((Track*)v)->menu_cb( (Fl_Menu_*) w );
 }
 
+void
+Track::command_configure_channels ( int n )
+{
+    /* due to locking this should only be invoked by direct user action */
+    timeline->wrlock();
+    configure_inputs( n );
+    configure_outputs( n );
+    timeline->unlock();
+}
+
 void
 Track::menu_cb ( const Fl_Menu_ *m )
 {
@@ -754,18 +754,15 @@ Track::menu_cb ( const Fl_Menu_ *m )
 
     if ( ! strcmp( picked, "Type/Mono" ) )
     {
-        configure_inputs( 1 );
-        configure_outputs( 1 );
+        command_configure_channels( 1 );
     }
     else if ( ! strcmp( picked, "Type/Stereo" ) )
     {
-        configure_inputs( 2 );
-        configure_outputs( 2 );
+        command_configure_channels( 2 );
     }
     else if ( ! strcmp( picked, "Type/Quad" ) )
     {
-        configure_inputs( 4 );
-        configure_outputs( 4 );
+        command_configure_channels( 4 );
     }
     else if ( ! strcmp( picked, "Type/..." ) )
     {
@@ -779,8 +776,7 @@ Track::menu_cb ( const Fl_Menu_ *m )
                 fl_alert( "Invalid number of channels." );
             else
             {
-                configure_inputs( c );
-                configure_outputs( c );
+                command_configure_channels(c);
             }
         }
     }
@@ -789,7 +785,9 @@ Track::menu_cb ( const Fl_Menu_ *m )
         /* add audio track */
         char *name = get_unique_control_name( "Control" );
 
+        timeline->wrlock();
         new Control_Sequence( this, name );
+        timeline->unlock();
     }
     else if ( ! strcmp( picked, "/Overlay controls" ) )
     {
@@ -846,8 +844,8 @@ Track::menu_cb ( const Fl_Menu_ *m )
 
         if ( r == 2 )
         {
-            timeline->remove_track( this );
-            Fl::delete_widget( this );
+            timeline->command_remove_track( this );
+             Fl::delete_widget( this );
         }
     }
     else if ( ! strcmp( picked, "/Rename" ) )
@@ -856,11 +854,11 @@ Track::menu_cb ( const Fl_Menu_ *m )
     }
     else if ( ! strcmp( picked, "/Move Up" ) )
     {
-        timeline->move_track_up( this );
+        timeline->command_move_track_up( this );
     }
     else if ( ! strcmp( picked, "/Move Down" ) )
     {
-        timeline->move_track_down( this );
+        timeline->command_move_track_down( this );
     }
     else if ( !strcmp( picked, "Takes/Show all takes" ) )
     {
@@ -868,7 +866,9 @@ Track::menu_cb ( const Fl_Menu_ *m )
     }
     else if ( !strcmp( picked, "Takes/New" ) )
     {
+        timeline->wrlock();
         sequence( (Audio_Sequence*)sequence()->clone_empty() );
+        timeline->unlock();
     }
     else if ( !strcmp( picked, "Takes/Remove" ) )
     {
@@ -876,12 +876,16 @@ Track::menu_cb ( const Fl_Menu_ *m )
             {
                 Loggable::block_start();
 
+                timeline->wrlock();
+
                 Audio_Sequence *s = sequence();
 
                 sequence( (Audio_Sequence*)takes->child( 0 ) );
 
                 delete s;
 
+                timeline->unlock();
+
                 Loggable::block_end();
             }
     }
@@ -900,7 +904,9 @@ Track::menu_cb ( const Fl_Menu_ *m )
     {
         Audio_Sequence* s = (Audio_Sequence*)m->mvalue()->user_data();
 
+        timeline->wrlock();
         sequence( s );
+        timeline->unlock();
     }
 }
 

timeline/src/Track.H

@@ -111,6 +111,7 @@ private:
 
     bool configure_outputs ( int n );
     bool configure_inputs ( int n );
+    void command_configure_channels ( int n );
 
     void update_port_names ( void );
     const char *name_for_port( JACK::Port::type_e type, int n );