Ardour
TempoMap init and superclock_ticks_per_second
This relates to random comments/questions I made on IRC. I suggest taking the first changeset now - a slight improvement. The rest - especially the "big" TempoMap change is mainly a RFC. I think my change shows "something", but most certainly not the right solution. Diving into code and debugging has given many random thoughts and hypothesises. I have tried to straighten them out, but would like some feedback.
This hack solves my problem with playback of trivial sessions created with 7.0pre, before https://github.com/Ardour/ardour/commit/a803dd0df87ad75b5449afca2e7617f39fd28f10 changed superclock_ticks_per_second from 508032000 to 56448000. I saw two sides of the problem: the UI would show 1080 bpm (exactly the ratio between the two), and playback would fail reporting problems with time going backwards.
The global TempoMap is initialized early with Tempo 120. The problem turned out to be related to how constructing a Tempo uses superclock_ticks_per_second to compute _superclocks_per_note_type. This global Tempo survives loading a session that sets a different superclock_ticks_per_second and invalidates _superclocks_per_note_type.
While the early 7.0pre versions with different superclock_ticks_per_second are "unsupported", it still expose a real problem with the reading superclock_ticks_per_second from session files - that is something that is intended to work.
(BTW: While I see the benefit of using superclock internally, I'm not convinced of the benefits of using it in the storage format. It seems to me like it would be cleaner to keep it as samples or beat fractions. The superclock rate seems like a runtime implementation detail to allow samples and beats to co-exist.)
Both superclock_ticks_per_second and samplerate are very basic ... with superclock even more basic than samplerate. Both are in session XML, as 'Session.sample-rate' and 'Session>TempoMap.superclocks-per-second'. Both are decided when creating a new session, and cannot be changed later. Conceptually (but pointless, except for testing), superclock_ticks_per_second could be exposed in the UI when creating projects.
It seems like it would be nice if set_superclock_ticks_per_second were invoked pretty much the same place as set_sample_rate. They both control global variables through superclock.h . But it doesn't seem like there is a clear boundary where the sample rate is set.
(It would perhaps be better if none of these global values had a default value - that would make it explicit if a code path doesn't set it. A default value of 0 would also make it easy to assert if it had been set.)
Taking a step back and going in another direction:
The global TempoMap is initialized early, in Temporal::init . The first actual use is in AudioEngine::process_callback , introduced in https://github.com/Ardour/ardour/commit/22b50c1716bda71e8f97471e21ebcf6bde8bc57d to handle tempo map changes synchronously. It doesn't seem like there currently is a good way to see if TempoMap is initialized at all, and skip if it isn't. If there was, TempoMap::init could be moved to a point after reading / defaulting superclock_ticks_per_second ... and before needing the TempoMap to render BMP in ARDOUR_UI::set_session .
But the ideal place for TempoMap::init when loading sessions is tricky. TempoMap::set_state has an inherent chicken-and-egg problem: It needs the TempoMap instance before it can read the right superclocks-per-second and invoke set_superclock_ticks_per_second so TempoMap::init can do the right thing, without accessing superclock_ticks_per_second before it has been set. We can thus not use TempoMap::init as it was.
Back to the solution chosen and proposed here:
The best solution thus seems to make sure TempoMap::init doesn't create default values, but allow them to be set later - as defaults, or reading from a session.
I guess it could work to accept early access to superclock_ticks_per_second and keep the (incorrect) default entries in TempoMap, but let session load override them. But overriding doesn't seem to work. And accessing superclock before it has been set and creating (unnecessary) Tempo/Meter entries seems unnecessarily unfortunate and fragile.
The new two-phase creation of TempoMap, with the empty constructor leaving it in a somewhat invalid state, is unfortunate. It would perhaps be better to not have to create TempoMap early.
When loading a session, we don't have to populate TempoMap first. But when creating a new session we have to find the right place to do it. gtk2_ardour/ardour_ui_session.cc seems too high level, but I haven't found a good lower level place to do it. The Session itself doesn't seem to know the difference.
2cfc013b made some changes in the area of this PR. That seems to have solved the problem of using sessions with a non-default superclock rate.
It doesn't solve the problem with using the superclock rate before it has been set. DEBUG_EARLY_SCTS_USE is thus still conceptually flawed. How should that be fixed?
Note: Right now, if a new session is created after loading a session with non-default superclock rate, the new session will inherit the old "odd" superclock. 9964f20c introduced clearing of the TempoMap. With the current architecture, I guess it should reset the superclock rate too?
(set_sample_rate could seem to have similar issues (but is hard to grep for ...)
Closing this - it has served as proff of concept and is no longer relevant.