View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0002238 | ardour | bugs | public | 2008-05-07 12:08 | 2020-04-19 20:13 |
| Reporter | mtaht | Assigned To | paul | ||
| Priority | normal | Severity | tweak | Reproducibility | always |
| Status | closed | Resolution | fixed | ||
| Product Version | SVN/2.0-ongoing | ||||
| Summary | 0002238: Getting rid of gettimeofday (making ardour safe for ntp) | ||||
| Description | Gettimeofday'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 Information | The 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 | ||||
| Tags | No tags attached. | ||||
|
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
|
|
|
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. |
|
|
Well, the patch eliminates that (wrong, precoffee) guess anyway, correctly.... 64 bit math is so much easier to deal with... |
|
|
Or do I have the sense of the test inverted? |
|
|
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. |
|
|
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. |
| 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 |