From 8d282617e6dd2c2a0f55a595bab09979cdfad84c Mon Sep 17 00:00:00 2001 From: Jonathan Moore Liles Date: Fri, 1 Mar 2013 17:05:42 -0800 Subject: [PATCH] Timeline: More locking fixes. Also, build peaks cache levels in threads launched by UI thread, instead of in the record diskthreads. --- timeline/src/Audio_Region.C | 9 +- timeline/src/Engine/Disk_Stream.C | 8 +- timeline/src/Engine/Peaks.C | 190 +++++++++++++++--------------- timeline/src/Engine/Peaks.H | 8 +- timeline/src/Engine/Timeline.C | 21 +++- timeline/src/Engine/Track.C | 15 ++- timeline/src/Project.C | 6 - timeline/src/Sequence.C | 40 +++---- timeline/src/TLE.fl | 2 +- timeline/src/Timeline.C | 21 +++- timeline/src/Timeline.H | 1 + 11 files changed, 177 insertions(+), 144 deletions(-) diff --git a/timeline/src/Audio_Region.C b/timeline/src/Audio_Region.C index b975233..14888e2 100644 --- a/timeline/src/Audio_Region.C +++ b/timeline/src/Audio_Region.C @@ -618,13 +618,12 @@ Audio_Region::draw ( void ) oend = end; } } - else + + if ( _clip->peaks()->needs_more_peaks() && ! transport->rolling ) { - if ( ! transport->rolling ) - { - /* create a thread to make the peaks */ + /* maybe create a thread to make the peaks */ + /* this function will just return if there's nothing to do. */ _clip->peaks()->make_peaks_asynchronously( Audio_Region::peaks_ready_callback, this ); - } } } else diff --git a/timeline/src/Engine/Disk_Stream.C b/timeline/src/Engine/Disk_Stream.C index 720218a..eb1e00f 100644 --- a/timeline/src/Engine/Disk_Stream.C +++ b/timeline/src/Engine/Disk_Stream.C @@ -74,7 +74,7 @@ Disk_Stream::~Disk_Stream ( ) { /* it isn't safe to do all this with the RT thread running */ - timeline->wrlock(); +// timeline->wrlock(); shutdown(); @@ -85,7 +85,7 @@ Disk_Stream::~Disk_Stream ( ) for ( int i = channels(); i--; ) jack_ringbuffer_free( _rb[ i ] ); - timeline->unlock(); +// timeline->unlock(); } @@ -144,13 +144,15 @@ Disk_Stream::shutdown ( void ) { if ( _thread.running() ) { + DMESSAGE( "Sending terminate signal to diskthread." ); + _terminate = true; /* try to wake the thread so it'll see that it's time to die */ while ( _terminate ) { - usleep( 100 ); block_processed(); + usleep( 1000 ); } _thread.join(); diff --git a/timeline/src/Engine/Peaks.C b/timeline/src/Engine/Peaks.C index b5ae7c5..9e9e6d2 100644 --- a/timeline/src/Engine/Peaks.C +++ b/timeline/src/Engine/Peaks.C @@ -104,9 +104,7 @@ class Peakfile FILE *_fp; nframes_t _chunksize; int _channels; /* number of channels this peakfile represents */ -// nframes_t _length; /* length, in frames, of the clip this peakfile represents */ off_t _offset; -// int _blocks; struct block_descriptor { @@ -127,14 +125,17 @@ class Peakfile public: + int nblocks ( void ) const + { + return blocks.size(); + } + Peakfile ( ) { -// _blocks = 0; _fp = NULL; _offset = 0; _chunksize = 0; _channels = 0; -// _length = 0; } ~Peakfile ( ) @@ -143,6 +144,11 @@ public: close(); } + void rescan ( void ) + { + blocks.clear(); + } + /* int blocks ( void ) const { return blocks.size(); } */ /** find the best block for /chunksize/ */ void @@ -184,9 +190,6 @@ public: if ( ! blocks.size() ) FATAL( "Peak file contains no blocks!" ); - -// DMESSAGE( "peakfile has %d blocks.", blocks.size() ); - blocks.sort(); /* fall back on the smallest chunksize */ @@ -204,7 +207,6 @@ public: } // DMESSAGE( "using peakfile block for chunksize %lu", _chunksize ); -// _blocks = blocks.size(); _offset = ftello( _fp ); } @@ -239,6 +241,7 @@ public: bool open ( const char *name, int channels, nframes_t chunksize ) { + assert( ! _fp ); // _chunksize = 0; _channels = channels; @@ -263,6 +266,8 @@ public: bool open ( FILE *fp, int channels, nframes_t chunksize ) { + assert( ! _fp ); + _fp = fp; _chunksize = 0; _channels = channels; @@ -364,7 +369,9 @@ public: Peaks::Peaks ( Audio_File *c ) { - _pending = false; + _rescan_needed = false; + _first_block_pending = false; + _mipmaps_pending = false; _clip = c; _peak_writer = NULL; _peakfile = new Peakfile(); @@ -412,39 +419,50 @@ Peaks::ready ( nframes_t s, nframes_t npeaks, nframes_t chunksize ) const bool Peaks::peakfile_ready ( void ) const { - return current() && ! _pending; + if ( _rescan_needed ) + { + DMESSAGE( "Rescanning peakfile" ); + _peakfile->rescan(); + if ( _peakfile->open( _clip->filename(), _clip->channels(), 256 ) ) + _peakfile->close(); + + _rescan_needed = false; + } + + return current() && ! _first_block_pending; } +/** start building peaks and/or peak mipmap in another thread. It is + * safe to call this again before the thread finishes. /callback/ will + * be called with /userdata/ FROM THE PEAK BUILDING THREAD when the + * peaks are finished. */ void Peaks::make_peaks_asynchronously ( void(*callback)(void*), void *userdata ) const { /* already working on it... */ - if( _pending ) + if( _first_block_pending || _mipmaps_pending ) return; - -// make_peaks(); - - _pending = true; - + + /* maybe still building mipmaps... */ + _first_block_pending = _peakfile->nblocks() < 1; + _mipmaps_pending = _peakfile->nblocks() <= 1; + peak_thread_data *pd = new peak_thread_data(); - + pd->callback = callback; pd->userdata = userdata; pd->peaks = const_cast(this); - + _make_peaks_thread.clone( &Peaks::make_peaks, pd ); _make_peaks_thread.detach(); + + DMESSAGE( "Starting new peak building thread" ); } nframes_t Peaks::read_peakfile_peaks ( Peak *peaks, nframes_t s, nframes_t npeaks, nframes_t chunksize ) const { - /* if ( _pending ) */ - /* return 0; */ - -// Peakfile _peakfile; - - if ( ! _peakfile->open( _clip->filename(), _clip->channels(), chunksize ) ) + if ( ! _peakfile->open( _clip->filename(), _clip->channels(), chunksize ) ) { DMESSAGE( "Failed to open peakfile!" ); return 0; @@ -545,7 +563,7 @@ Peaks::current ( void ) const { char *pn = peakname( _clip->filename() ); - bool b = ! newer( _clip->filename(), pn ); + bool b = newer( pn, _clip->filename() ); free( pn ); @@ -557,11 +575,14 @@ void * Peaks::make_peaks ( void *v ) { peak_thread_data *pd = (peak_thread_data*)v; - - pd->peaks->make_peaks(); - if ( pd->callback ) - pd->callback( pd->userdata ); + if ( pd->peaks->make_peaks() ) + { + if ( pd->callback ) + pd->callback( pd->userdata ); + + pd->peaks->_rescan_needed = true; + } delete pd; @@ -569,35 +590,25 @@ Peaks::make_peaks ( void *v ) } bool -Peaks::make_peaks ( void ) const +Peaks::needs_more_peaks ( void ) const { - Peaks::Builder pb( this ); - - int b = pb.make_peaks(); - - _pending = false; - - return b; -} - -/* thread entry point */ -void * -Peaks::make_peaks_mipmap ( void *v ) -{ - ((Peaks*)v)->make_peaks_mipmap(); - - return NULL; + return _peakfile->nblocks() <= 1 && ! ( _first_block_pending || _mipmaps_pending ); } bool -Peaks::make_peaks_mipmap ( void ) const +Peaks::make_peaks ( void ) const { Peaks::Builder pb( this ); - bool b = pb.make_peaks_mipmap(); - - _pending = false; + /* make the first block */ + int b = pb.make_peaks(); + _first_block_pending = false; + + b = pb.make_peaks_mipmap(); + + _mipmaps_pending = false; + return b; } @@ -638,8 +649,6 @@ Peaks::finish_writing ( void ) delete _peak_writer; _peak_writer = NULL; - - make_peaks_mipmap(); } void @@ -792,15 +801,12 @@ Peaks::Builder::write_block_header ( nframes_t chunksize ) fflush( fp ); } - - - /** generate additional cache levels for a peakfile with only 1 block (ie. that of a new capture) */ bool Peaks::Builder::make_peaks_mipmap ( void ) { if ( ! Peaks::mipmapped_peakfiles ) - return true; + return false; Audio_File *_clip = _peaks->_clip; @@ -895,10 +901,7 @@ Peaks::Builder::make_peaks_mipmap ( void ) } while ( len > 0 && s < _clip->length() ); - DMESSAGE( "Last sample was %lu", s ); - - /* fflush( fp ); */ - /* fsync( fileno( fp ) ); */ + DMESSAGE( "Last sample was %lu", (unsigned long)s ); pf.leave_open(); } @@ -918,43 +921,46 @@ Peaks::Builder::make_peaks ( void ) const char *filename = _clip->filename(); - DMESSAGE( "building peaks for \"%s\"", filename ); - - char *pn = peakname( filename ); - - if ( ! ( fp = fopen( pn, "w+" ) ) ) + if ( _peaks->_peakfile && _peaks->_peakfile->nblocks() > 1 ) { - free( pn ); + /* this peakfile already has enough blocks */ return false; } + else + { + DMESSAGE( "building peaks for \"%s\"", filename ); + + char *pn = peakname( filename ); + + if ( ! ( fp = fopen( pn, "w+" ) ) ) + { + free( pn ); + return false; + } + + free( pn ); + + _clip->seek( 0 ); + + Peak buf[ _clip->channels() ]; + + DMESSAGE( "building level 1 peak cache" ); + + write_block_header( Peaks::cache_minimum ); + + /* build first level from source */ + off_t len; + do { + len = _peaks->read_source_peaks( buf, 1, Peaks::cache_minimum ); + + fwrite( buf, sizeof( buf ), len, fp ); + } + while ( len ); + + fclose( fp ); - free( pn ); - - _clip->seek( 0 ); - - Peak buf[ _clip->channels() ]; - - DMESSAGE( "building level 1 peak cache" ); - - write_block_header( Peaks::cache_minimum ); - - /* build first level from source */ - off_t len; - do { - len = _peaks->read_source_peaks( buf, 1, Peaks::cache_minimum ); - - fwrite( buf, sizeof( buf ), len, fp ); + DMESSAGE( "done building peaks" ); } - while ( len ); - - /* reopen for reading */ - /* fflush( fp ); */ - /* fsync( fileno( fp ) ); */ - fclose( fp ); - - make_peaks_mipmap(); - - DMESSAGE( "done building peaks" ); return true; } diff --git a/timeline/src/Engine/Peaks.H b/timeline/src/Engine/Peaks.H index ead005f..c8d7612 100644 --- a/timeline/src/Engine/Peaks.H +++ b/timeline/src/Engine/Peaks.H @@ -35,7 +35,9 @@ class Peakfile; class Peaks { - mutable volatile bool _pending; + /* true if first block is still being built */ + mutable volatile bool _first_block_pending; + mutable volatile bool _mipmaps_pending; mutable Thread _make_peaks_thread; mutable Thread _make_peaks_mipmap_thread; @@ -110,6 +112,8 @@ class Peaks mutable float _fpp; + volatile mutable bool _rescan_needed; + nframes_t read_peaks ( nframes_t s, nframes_t npeaks, nframes_t chunksize ) const; nframes_t read_source_peaks ( Peak *peaks, nframes_t s, nframes_t npeaks, nframes_t chunksize ) const; nframes_t read_source_peaks ( Peak *peaks, nframes_t npeaks, nframes_t chunksize ) const; @@ -145,11 +149,11 @@ public: bool ready ( nframes_t s, nframes_t npeaks, nframes_t chunksize ) const; bool make_peaks ( void ) const; - bool make_peaks_mipmap ( void ) const; void make_peaks_asynchronously ( void(*callback)(void*), void *userdata ) const; void prepare_for_writing ( void ); void finish_writing ( void ); void write ( sample_t *buf, nframes_t nframes ); + bool needs_more_peaks ( void ) const; }; diff --git a/timeline/src/Engine/Timeline.C b/timeline/src/Engine/Timeline.C index 66f8fba..8ef71d0 100644 --- a/timeline/src/Engine/Timeline.C +++ b/timeline/src/Engine/Timeline.C @@ -34,11 +34,16 @@ bool Timeline::record ( void ) { + THREAD_ASSERT( UI ); + + DMESSAGE( "Initiating recording." ); + /* FIXME: right place for this? */ if ( transport->automatically_create_takes() && ! _created_new_takes ) { + DMESSAGE( "Creating new takes." ); add_take_for_armed_tracks(); _created_new_takes = true; } @@ -53,12 +58,16 @@ Timeline::record ( void ) if ( transport->punch_enabled() ) { + DMESSAGE( "Finding next punch region following frame %lu...", (unsigned long)frame); + const Sequence_Widget *w = punch_cursor_track->next( frame ); if ( w && w->start() >= frame ) { frame = w->start(); _punch_out_frame = w->start() + w->length(); + + DMESSAGE( "Punch enabled... Will punch in at frame %lu.", (unsigned long)frame ); } } @@ -94,6 +103,8 @@ Timeline::punch_in ( nframes_t frame ) void Timeline::punch_out ( nframes_t frame ) { + THREAD_ASSERT( UI ); + for ( int i = tracks->children(); i-- ; ) { Track *t = (Track*)tracks->child( i ); @@ -102,9 +113,11 @@ Timeline::punch_out ( nframes_t frame ) t->record_ds->stop( frame ); } - /* wait until finalization is complete before continuing */ + DMESSAGE( "Waiting for record threads to shutdown." ); + + /* none of the record threads need to call Fl::lock, because we're + * holding up the UI thread waiting for them to join.*/ - DMESSAGE( "Waiting for record threads to shutdown" ); for ( int i = tracks->children(); i-- ; ) { Track *t = (Track*)tracks->child( i ); @@ -112,6 +125,8 @@ Timeline::punch_out ( nframes_t frame ) if ( t->armed() && t->record_ds ) t->record_ds->shutdown(); } + + DMESSAGE( "All record threads stopped." ); _punched_in = false; _punch_in_frame = 0; @@ -122,6 +137,8 @@ Timeline::punch_out ( nframes_t frame ) void Timeline::stop ( void ) { + THREAD_ASSERT( UI ); + nframes_t frame = transport->frame; if ( transport->punch_enabled() ) diff --git a/timeline/src/Engine/Track.C b/timeline/src/Engine/Track.C index 49f08b0..6a62639 100644 --- a/timeline/src/Engine/Track.C +++ b/timeline/src/Engine/Track.C @@ -272,15 +272,19 @@ Track::record ( Capture *c, nframes_t frame ) /* open it again for reading in the GUI thread */ // Audio_File *af = Audio_File::from_file( c->audio_file->name() ); + /* must acquire the FLTK lock because adding a widget might interfere with drawing */ + Fl::lock(); /* must acquire a write lock because the Audio_Region constructor - * will add the region to the specified sequence */ + * will add the region to the specified sequence, which might affect playback */ timeline->wrlock(); c->region = new Audio_Region( c->audio_file, sequence(), frame ); timeline->unlock(); + Fl::unlock(); + c->region->prepare(); } @@ -305,14 +309,11 @@ Track::finalize ( Capture *c, nframes_t frame ) /* adjust region start for latency */ /* FIXME: is just looking at the first channel good enough? */ - timeline->wrlock(); - DMESSAGE( "finalizing audio file" ); - /* must finalize audio before peaks file, otherwise another thread - * might think the peaks are out of date and attempt to regenerate - * them */ c->audio_file->finalize(); + timeline->wrlock(); + c->region->finalize( frame ); nframes_t capture_offset = 0; @@ -333,6 +334,4 @@ Track::finalize ( Capture *c, nframes_t frame ) c->region->offset( capture_offset ); timeline->unlock(); - -// delete c->audio_file; } diff --git a/timeline/src/Project.C b/timeline/src/Project.C index ad3526f..92ef265 100644 --- a/timeline/src/Project.C +++ b/timeline/src/Project.C @@ -131,9 +131,7 @@ Project::write_info ( void ) void Project::undo ( void ) { - timeline->wrlock(); Loggable::undo(); - timeline->unlock(); } bool @@ -203,12 +201,8 @@ Project::close ( void ) if ( ! save() ) return false; - timeline->wrlock(); - Loggable::close(); - timeline->unlock(); - // write_info(); _is_open = false; diff --git a/timeline/src/Sequence.C b/timeline/src/Sequence.C index 6681271..aa642b4 100644 --- a/timeline/src/Sequence.C +++ b/timeline/src/Sequence.C @@ -188,14 +188,11 @@ Sequence::add ( Sequence_Widget *r ) { // Logger _log( this ); - if ( r->sequence() ) + if ( r->sequence() && r->sequence() != this ) { /* This method can be called from the Capture thread as well as the GUI thread, so we must lock FLTK before redraw */ - Fl::lock(); r->redraw(); r->sequence()->remove( r ); - Fl::unlock(); -// r->track()->redraw(); } r->sequence( this ); @@ -481,28 +478,30 @@ Sequence::handle ( int m ) Sequence_Widget::pushed( NULL ); } - Loggable::block_start(); - - timeline->wrlock(); - while ( _delete_queue.size() ) + if ( _delete_queue.size() ) { - - Sequence_Widget *t = _delete_queue.front(); - _delete_queue.pop(); - - if ( Sequence_Widget::pushed() == t ) - Sequence_Widget::pushed( NULL ); - if ( Sequence_Widget::belowmouse() == t ) + Loggable::block_start(); + + while ( _delete_queue.size() ) { - Sequence_Widget::belowmouse()->handle( FL_LEAVE ); - Sequence_Widget::belowmouse( NULL ); - } + Sequence_Widget *t = _delete_queue.front(); + _delete_queue.pop(); + + if ( Sequence_Widget::pushed() == t ) + Sequence_Widget::pushed( NULL ); + if ( Sequence_Widget::belowmouse() == t ) + { + Sequence_Widget::belowmouse()->handle( FL_LEAVE ); + Sequence_Widget::belowmouse( NULL ); + } - delete t; + timeline->wrlock(); + delete t; + timeline->unlock(); } - timeline->unlock(); Loggable::block_end(); + } if ( m == FL_PUSH ) return 1; @@ -550,6 +549,7 @@ const Sequence_Widget * Sequence::next ( nframes_t from ) const { for ( list ::const_iterator i = _widgets.begin(); i != _widgets.end(); i++ ) +// if ( (*i)->start() >= from ) if ( (*i)->start() > from ) return *i; diff --git a/timeline/src/TLE.fl b/timeline/src/TLE.fl index 16ed9f1..6efd3ed 100644 --- a/timeline/src/TLE.fl +++ b/timeline/src/TLE.fl @@ -396,7 +396,7 @@ Project::compact();} } { MenuItem {} { label Undo - callback {Project::undo();} + callback {timeline->command_undo();} xywh {5 5 40 25} shortcut 0x4007a divider } MenuItem {} { diff --git a/timeline/src/Timeline.C b/timeline/src/Timeline.C index 96b0c18..5dcf979 100644 --- a/timeline/src/Timeline.C +++ b/timeline/src/Timeline.C @@ -1253,6 +1253,8 @@ Timeline::draw_cursors ( void ) const void Timeline::draw ( void ) { + /* Any code that might affect the structures used for drawing from + * another thread must use Fl::lock()/unlock()! */ THREAD_ASSERT( UI ); int X, Y, W, H; @@ -1260,8 +1262,6 @@ Timeline::draw ( void ) int bdx = 0; int bdw = 0; - rdlock(); - X = tracks->x() + bdx + 1; Y = tracks->y(); W = tracks->w() - bdw - 1; @@ -1363,8 +1363,6 @@ done: _old_xposition = xoffset; _old_yposition = panzoomer->y_value(); - - unlock(); } void @@ -1438,6 +1436,7 @@ Timeline::redraw_playhead ( void ) /* we've passed one or more punch regions... punch in for the next, if available. */ const Sequence_Widget *w = punch_cursor_track->next( transport->frame ); + DMESSAGE( "Delayed punch in" ); if ( w && w->start() > transport->frame ) { @@ -1993,15 +1992,27 @@ Timeline::command_remove_track ( Track *track ) } void -Timeline::command_quit ( ) +Timeline::command_quit ( void ) { + timeline->wrlock(); + Project::close(); + + timeline->unlock(); command_save(); while ( Fl::first_window() ) Fl::first_window()->hide(); } +void +Timeline::command_undo ( void ) +{ + wrlock(); + Project::undo(); + unlock(); +} + bool Timeline::command_load ( const char *name, const char *display_name ) { diff --git a/timeline/src/Timeline.H b/timeline/src/Timeline.H index 1736b79..e4e2e40 100644 --- a/timeline/src/Timeline.H +++ b/timeline/src/Timeline.H @@ -257,6 +257,7 @@ public: void command_move_track_up ( Track *track ); void command_move_track_down ( Track *track ); + void command_undo ( void ); int find_track ( const Track * track ) const;