From 98c5c196d4e194e9c271fa50a9246b0edb342134 Mon Sep 17 00:00:00 2001 From: Jonathan Moore Liles Date: Fri, 22 Feb 2013 17:18:52 -0800 Subject: [PATCH] Timeline: Rework locking. --- timeline/src/Audio_Region.C | 2 + timeline/src/Control_Sequence.C | 12 +----- timeline/src/Engine/Disk_Stream.C | 21 +++------ timeline/src/Engine/Engine.C | 18 ++++---- timeline/src/Engine/Engine.H | 2 +- timeline/src/Engine/Playback_DS.C | 48 ++++++++++++++------- timeline/src/Engine/Track.C | 5 ++- timeline/src/Project.C | 4 +- timeline/src/Sequence.C | 15 +++---- timeline/src/Sequence_Widget.C | 6 +-- timeline/src/Timeline.C | 71 ++++++++++++++++++------------- timeline/src/Timeline.H | 8 ++-- timeline/src/Track.C | 50 ++++++++++++---------- timeline/src/Track.H | 1 + 14 files changed, 141 insertions(+), 122 deletions(-) diff --git a/timeline/src/Audio_Region.C b/timeline/src/Audio_Region.C index b2e56c6..b975233 100644 --- a/timeline/src/Audio_Region.C +++ b/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(); diff --git a/timeline/src/Control_Sequence.C b/timeline/src/Control_Sequence.C index 1c02a6a..7cae003 100644 --- a/timeline/src/Control_Sequence.C +++ b/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; } diff --git a/timeline/src/Engine/Disk_Stream.C b/timeline/src/Engine/Disk_Stream.C index 359a072..720218a 100644 --- a/timeline/src/Engine/Disk_Stream.C +++ b/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 ); diff --git a/timeline/src/Engine/Engine.C b/timeline/src/Engine/Engine.C index de92bea..fd02171 100644 --- a/timeline/src/Engine/Engine.C +++ b/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; diff --git a/timeline/src/Engine/Engine.H b/timeline/src/Engine/Engine.H index 4f8a726..569f91b 100644 --- a/timeline/src/Engine/Engine.H +++ b/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 */ diff --git a/timeline/src/Engine/Playback_DS.C b/timeline/src/Engine/Playback_DS.C index 022e189..ab22f51 100644 --- a/timeline/src/Engine/Playback_DS.C +++ b/timeline/src/Engine/Playback_DS.C @@ -33,6 +33,7 @@ #include "const.h" #include "debug.h" #include "Thread.H" +#include 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 diff --git a/timeline/src/Engine/Track.C b/timeline/src/Engine/Track.C index 3e9bb04..49f08b0 100644 --- a/timeline/src/Engine/Track.C +++ b/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 ) diff --git a/timeline/src/Project.C b/timeline/src/Project.C index a72286b..ad3526f 100644 --- a/timeline/src/Project.C +++ b/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 ); */ diff --git a/timeline/src/Sequence.C b/timeline/src/Sequence.C index e6405b2..6681271 100644 --- a/timeline/src/Sequence.C +++ b/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(); diff --git a/timeline/src/Sequence_Widget.C b/timeline/src/Sequence_Widget.C index 0503ed5..4da9dcf 100644 --- a/timeline/src/Sequence_Widget.C +++ b/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; diff --git a/timeline/src/Timeline.C b/timeline/src/Timeline.C index ec27fe8..96b0c18 100644 --- a/timeline/src/Timeline.C +++ b/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 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 */ diff --git a/timeline/src/Timeline.H b/timeline/src/Timeline.H index 2313fa1..1736b79 100644 --- a/timeline/src/Timeline.H +++ b/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; diff --git a/timeline/src/Track.C b/timeline/src/Track.C index 0134e14..29cdd63 100644 --- a/timeline/src/Track.C +++ b/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(); } } diff --git a/timeline/src/Track.H b/timeline/src/Track.H index 7bb6006..1c16566 100644 --- a/timeline/src/Track.H +++ b/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 );