View Issue Details

IDProjectCategoryView StatusLast Update
0002238ardourbugspublic2020-04-19 20:13
Reportermtaht Assigned Topaul  
PrioritynormalSeveritytweakReproducibilityalways
Status closedResolutionfixed 
Product VersionSVN/2.0-ongoing 
Summary0002238: Getting rid of gettimeofday (making ardour safe for ntp)
DescriptionGettimeofday's ability to move backward in time with ntp updates has sunk a spacecraft or two, and has also caused issues with gui applications that rely on it, when you click on something effectively after you released it.

So far as I know it was eliminated from glib around 2.12. Ardour, however, still uses it in multiple places.

It's also kind of a pain to deal with, in an age when compilers can naturally deal with 64 bit math without complaint.

And - most importantly - sometime in the last year or so - jack has exposed its get_microseconds API...

The attached patch takes care of most but not all, users of gettimeofday in ardour, by having them call ARDOUR::get_microseconds() (which in turn, calls jack)
Additional InformationThe attached patch has three issues:

1) in editor_mouse.cc, some kind of overflow magic is taking place?

gint
 Editor::track_height_step_timeout ()

- if (delta.tv_sec * 1000000 + delta.tv_usec > 250000)

I think this code was supposed to be tv_usec *1000000 + tv_sec, so it's wrong in svn head right now. Fixed in this patch.

2) there doesn't appear to be an actual users for read_data_rate and write_data_rate in the (compute_io) stuff in session_butler. Why bother to calculate it at all?

3) not clear to me when or even if jack_get_microseconds() can fail

4) There are still a couple other users of gettimeofday, in particular in gtkmmext
TagsNo tags attached.

Activities

2008-05-07 12:08

 

gettimeofday.patch (9,845 bytes)   
diff --git a/gtk2_ardour/ardour_ui.cc b/gtk2_ardour/ardour_ui.cc
index 4e47466..af0da45 100644
--- a/gtk2_ardour/ardour_ui.cc
+++ b/gtk2_ardour/ardour_ui.cc
@@ -63,6 +63,8 @@
 #include <ardour/port.h>
 #include <ardour/audio_track.h>
 
+typedef uint64_t microseconds_t;
+
 #include "actions.h"
 #include "ardour_ui.h"
 #include "public_editor.h"
@@ -195,8 +197,7 @@ ARDOUR_UI::ARDOUR_UI (int *argcp, char **argvp[])
 	last_speed_displayed = -1.0f;
 	ignore_dual_punch = false;
 
-	last_configure_time.tv_sec = 0;
-	last_configure_time.tv_usec = 0;
+	last_configure_time= 0;
 
 	shuttle_grabbed = false;
 	shuttle_fract = 0.0;
@@ -205,8 +206,9 @@ ARDOUR_UI::ARDOUR_UI (int *argcp, char **argvp[])
 	shuttle_style_menu = 0;
 	shuttle_unit_menu = 0;
 
-	gettimeofday (&last_peak_grab, 0);
-	gettimeofday (&last_shuttle_request, 0);
+        // We do not have jack linked in yet so;
+        
+	last_shuttle_request = last_peak_grab = 0; //  get_microseconds();
 
 	ARDOUR::Diskstream::DiskOverrun.connect (mem_fun(*this, &ARDOUR_UI::disk_overrun_handler));
 	ARDOUR::Diskstream::DiskUnderrun.connect (mem_fun(*this, &ARDOUR_UI::disk_underrun_handler));
@@ -398,21 +400,15 @@ ARDOUR_UI::pop_back_splash ()
 gint
 ARDOUR_UI::configure_timeout ()
 {
-	struct timeval now;
-	struct timeval diff;
-
-	if (last_configure_time.tv_sec == 0 && last_configure_time.tv_usec == 0) {
+	if (last_configure_time == 0) {
 		/* no configure events yet */
 		return TRUE;
 	}
 
-	gettimeofday (&now, 0);
-	timersub (&now, &last_configure_time, &diff);
-
 	/* force a gap of 0.5 seconds since the last configure event
 	 */
 
-	if (diff.tv_sec == 0 && diff.tv_usec < 500000) {
+	if (get_microseconds() - last_configure_time < 500000) {
 		return TRUE;
 	} else {
 		have_configure_timeout = false;
@@ -425,7 +421,7 @@ gboolean
 ARDOUR_UI::configure_handler (GdkEventConfigure* conf)
 {
 	if (have_configure_timeout) {
-		gettimeofday (&last_configure_time, 0);
+		last_configure_time = get_microseconds();
 	} else {
 		Glib::signal_timeout().connect (mem_fun(*this, &ARDOUR_UI::configure_timeout), 100);
 		have_configure_timeout = true;
diff --git a/gtk2_ardour/ardour_ui.h b/gtk2_ardour/ardour_ui.h
index 70ce687..33e7217 100644
--- a/gtk2_ardour/ardour_ui.h
+++ b/gtk2_ardour/ardour_ui.h
@@ -58,6 +58,7 @@
 #include <gtkmm2ext/bindable_button.h>
 #include <ardour/ardour.h>
 #include <ardour/session.h>
+#include <ardour/types.h>
 
 #include "audio_clock.h"
 #include "ardour_dialog.h"
@@ -643,11 +644,11 @@ class ARDOUR_UI : public Gtkmm2ext::UI
 	void flush_trash ();
 
 	bool have_configure_timeout;
-	struct timeval last_configure_time;
+	ARDOUR::microseconds_t last_configure_time;
 	gint configure_timeout ();
 
-	struct timeval last_peak_grab;
-	struct timeval last_shuttle_request;
+	ARDOUR::microseconds_t last_peak_grab;
+	ARDOUR::microseconds_t last_shuttle_request;
 
 	bool have_disk_speed_dialog_displayed;
 	void disk_speed_dialog_gone (int ignored_response, Gtk::MessageDialog*);
diff --git a/gtk2_ardour/ardour_ui2.cc b/gtk2_ardour/ardour_ui2.cc
index 8a29e64..50fe0e2 100644
--- a/gtk2_ardour/ardour_ui2.cc
+++ b/gtk2_ardour/ardour_ui2.cc
@@ -755,17 +755,13 @@ ARDOUR_UI::set_shuttle_fract (double f)
 void
 ARDOUR_UI::use_shuttle_fract (bool force)
 {
-	struct timeval now;
-	struct timeval diff;
+	microseconds_t now = get_microseconds();
 	
 	/* do not attempt to submit a motion-driven transport speed request
 	   more than once per process cycle.
 	 */
 
-	gettimeofday (&now, 0);
-	timersub (&now, &last_shuttle_request, &diff);
-
-	if (!force && (diff.tv_usec + (diff.tv_sec * 1000000)) < engine->usecs_per_cycle()) {
+	if (!force && (last_shuttle_request - now) < engine->usecs_per_cycle()) {
 		return;
 	}
 	
diff --git a/gtk2_ardour/editor.h b/gtk2_ardour/editor.h
index fef1914..57007d0 100644
--- a/gtk2_ardour/editor.h
+++ b/gtk2_ardour/editor.h
@@ -2053,7 +2053,7 @@ public:
 	/* tracking step changes of track height */
 
 	TimeAxisView* current_stepping_trackview;
-	struct timeval last_track_height_step_timestamp;
+	ARDOUR::microseconds_t last_track_height_step_timestamp;
 	gint track_height_step_timeout();
 	sigc::connection step_timeout;
 
diff --git a/gtk2_ardour/editor_canvas_events.cc b/gtk2_ardour/editor_canvas_events.cc
index 00ea97a..4615fda 100644
--- a/gtk2_ardour/editor_canvas_events.cc
+++ b/gtk2_ardour/editor_canvas_events.cc
@@ -90,7 +90,7 @@ Editor::track_canvas_scroll (GdkEventScroll* ev)
 					return false;
 				}
 			}
-			gettimeofday (&last_track_height_step_timestamp, 0);
+			last_track_height_step_timestamp = get_microseconds();
 			current_stepping_trackview->step_height (true);
 			return true;
 		} else {
@@ -125,7 +125,7 @@ Editor::track_canvas_scroll (GdkEventScroll* ev)
 					return false;
 				}
 			}
-			gettimeofday (&last_track_height_step_timestamp, 0);
+			last_track_height_step_timestamp = get_microseconds();
 			current_stepping_trackview->step_height (false);
 			return true;
 		} else {
diff --git a/gtk2_ardour/editor_mouse.cc b/gtk2_ardour/editor_mouse.cc
index 7aaccc6..07f2ac3 100644
--- a/gtk2_ardour/editor_mouse.cc
+++ b/gtk2_ardour/editor_mouse.cc
@@ -5392,13 +5392,7 @@ Editor::mouse_brush_insert_region (RegionView* rv, nframes_t pos)
 gint
 Editor::track_height_step_timeout ()
 {
-	struct timeval now;
-	struct timeval delta;
-	
-	gettimeofday (&now, 0);
-	timersub (&now, &last_track_height_step_timestamp, &delta);
-	
-	if (delta.tv_sec * 1000000 + delta.tv_usec > 250000) { /* milliseconds */
+	if (get_microseconds() - last_track_height_step_timestamp < 250000) {
 		current_stepping_trackview = 0;
 		return false;
 	}
diff --git a/libs/ardour/ardour/ardour.h b/libs/ardour/ardour/ardour.h
index 22e79e1..cab4601 100644
--- a/libs/ardour/ardour/ardour.h
+++ b/libs/ardour/ardour/ardour.h
@@ -32,6 +32,8 @@
 #include <ardour/configuration.h>
 #include <ardour/types.h>
 
+// #include <jack/jack.h> need this to inline jack_get_microseconds
+
 namespace MIDI {
 	class MachineControl;
 	class Port;
@@ -63,7 +65,12 @@ namespace ARDOUR {
 	const layer_t max_layer = UCHAR_MAX;
 
 	microseconds_t get_microseconds ();
-
+/*	{
+        JACK has exported this functionality for a long time now 
+	but inlining this causes problems
+        return (microseconds_t) jack_get_time();
+	}
+*/
 	Change new_change ();
 
 	extern Change StartChanged;
diff --git a/libs/ardour/ardour/session.h b/libs/ardour/ardour/session.h
index ea89ab8..e5750e8 100644
--- a/libs/ardour/ardour/session.h
+++ b/libs/ardour/ardour/session.h
@@ -889,7 +889,7 @@ class Session : public PBD::StatefulDestructible
 	void reset_playback_load_min ();
 	void reset_capture_load_min ();
 	
-	float read_data_rate () const;
+	float read_data_rate () const; // in usec
 	float write_data_rate () const;
 
 	/* ranges */
diff --git a/libs/ardour/globals.cc b/libs/ardour/globals.cc
index a5bcedb..9eab179 100644
--- a/libs/ardour/globals.cc
+++ b/libs/ardour/globals.cc
@@ -371,15 +371,11 @@ ARDOUR::cleanup ()
 	return 0;
 }
 
-
 microseconds_t
 ARDOUR::get_microseconds ()
 {
-	/* XXX need JACK to export its functionality */
-
-	struct timeval now;
-	gettimeofday (&now, 0);
-	return now.tv_sec * 1000000ULL + now.tv_usec;
+	// JACK has exported this functionality for a long time now 
+	return (microseconds_t) jack_get_time();
 }
 
 ARDOUR::Change
diff --git a/libs/ardour/session_butler.cc b/libs/ardour/session_butler.cc
index 2ac06e5..92ebc50 100644
--- a/libs/ardour/session_butler.cc
+++ b/libs/ardour/session_butler.cc
@@ -166,7 +166,8 @@ Session::butler_thread_work ()
 	uint32_t err = 0;
 	int32_t bytes;
 	bool compute_io;
-	struct timeval begin, end;
+	microseconds_t begin, end;
+
 	struct pollfd pfd[1];
 	bool disk_work_outstanding = false;
 	DiskstreamList::iterator i;
@@ -243,7 +244,7 @@ Session::butler_thread_work ()
 		bytes = 0;
 		compute_io = true;
 
-		gettimeofday (&begin, 0);
+		begin = get_microseconds();
 
 		boost::shared_ptr<DiskstreamList> dsl = diskstreams.reader ();
 
@@ -290,17 +291,16 @@ Session::butler_thread_work ()
 		}
 
 		if (compute_io) {
-			gettimeofday (&end, 0);
-			
-			double b = begin.tv_sec  + (begin.tv_usec/1000000.0);
-			double e = end.tv_sec + (end.tv_usec / 1000000.0);
-			
-			_read_data_rate = bytes / (e - b);
+			end = get_microseconds(); 
+			if(end-begin > 0) {
+			_read_data_rate = (float) bytes / (float) (end - begin);
+			} else { _read_data_rate = 0; // infinity better
+			 }
 		}
 
 		bytes = 0;
 		compute_io = true;
-		gettimeofday (&begin, 0);
+		begin = get_microseconds();
 
 		for (i = dsl->begin(); !transport_work_requested() && butler_should_run && i != dsl->end(); ++i) {
 			// cerr << "write behind for " << (*i)->name () << endl;
@@ -344,12 +344,13 @@ Session::butler_thread_work ()
 		}
 
 		if (compute_io) {
-			gettimeofday (&end, 0);
-			
-			double b = begin.tv_sec  + (begin.tv_usec/1000000.0);
-			double e = end.tv_sec + (end.tv_usec / 1000000.0);
-			
-			_write_data_rate = bytes / (e - b);
+			// there are no apparent users for this calculation?
+			end = get_microseconds();
+			if(end-begin > 0) {
+			_write_data_rate = (float) bytes / (float) (end - begin);
+			} else {
+			_write_data_rate = 0; // Well, infinity would be better
+			}
 		}
 		
 		if (!disk_work_outstanding) {
@@ -416,7 +417,7 @@ Session::read_data_rate () const
 	/* disk i/o in excess of 10000MB/sec indicate the buffer cache
 	   in action. ignore it.
 	*/
-	return _read_data_rate > 10485760000.0f ? 0.0f : _read_data_rate;
+	return _read_data_rate > 10485.7600000f ? 0.0f : _read_data_rate;
 }
 
 float
@@ -425,7 +426,7 @@ Session::write_data_rate () const
 	/* disk i/o in excess of 10000MB/sec indicate the buffer cache
 	   in action. ignore it.
 	*/
-	return _write_data_rate > 10485760000.0f ? 0.0f : _write_data_rate;
+	return _write_data_rate > 10485.7600000f ? 0.0f : _write_data_rate;
 }
 
 uint32_t
gettimeofday.patch (9,845 bytes)   

paul

2008-05-07 12:41

administrator   ~0004918

your guess in (1) is wrong. we're converting to usecs all around, so its tv_sec * 1000000. but yes, there is a potential overflow problem.

mtaht

2008-05-07 12:53

developer   ~0004919

Well, the patch eliminates that (wrong, precoffee) guess anyway, correctly.... 64 bit math is so much easier to deal with...

mtaht

2008-05-07 13:21

developer   ~0004920

Or do I have the sense of the test inverted?

paul

2008-05-15 13:52

administrator   ~0004935

patch applied and about to be committed.

jack's "computation" can fail if the kernel doesn't support CLOCK_MONOTONIC and the user didn't elect to use the HPET timer, but otherwise, it will always be monotonic.

system

2020-04-19 20:13

developer   ~0021722

Issue has been closed automatically, by Trigger Close Plugin.
Feel free to re-open with additional information if you think the issue is not resolved.

Issue History

Date Modified Username Field Change
2008-05-07 12:08 mtaht New Issue
2008-05-07 12:08 mtaht File Added: gettimeofday.patch
2008-05-07 12:41 paul Note Added: 0004918
2008-05-07 12:53 mtaht Note Added: 0004919
2008-05-07 13:21 mtaht Note Added: 0004920
2008-05-15 13:52 paul Status new => resolved
2008-05-15 13:52 paul Resolution open => fixed
2008-05-15 13:52 paul Assigned To => paul
2008-05-15 13:52 paul Note Added: 0004935
2020-04-19 20:13 system Note Added: 0021722
2020-04-19 20:13 system Status resolved => closed