Better support for Nyquist stereo plugins

[werame]

Is your feature request related to a problem? Please describe.

Right now if you write a stereo generator e.g. in Nyquist you're faced with the following usability issue: if you have no track selected a mono will be inserted, and your stereo plugin will fail.

Describe the solution you'd like

The easiest thing would be for a plug-in to declare how many channels it wants to generate, like say, at leas support some syntax like

;type generate stereo

Update: actually, that's not the best of ideas of how to solve this, because it burdens the user with more boilerplate. There's no real need to pre-declare the (mono/stereo) type returned, since the Nyquist expresion itself (1) allows that and (2) it's usually a delayed-evaluation "thunk" SOUND object, so we can determine the mono/stereo type and insert the right type of track after the first xleval returns, but before pulling audio data (with nyx_get_audio).

Obsolete (first) implementation idea for the ";type generate stereo" approach. See next post for the "auto" one, which uses the Nyquist type. This section might still be worth reading for the analysis on the inflexible way in which the auto-generated track insertion is currently done in the base Effect class. Even for the "auto" approach that I ultimately implemented, this issue of base class generation code has some impact; I had to add two new fields to the base Effect class and one to the Nyquist subclass to pass around the info relating to that auto-generated track and its CopyInputTracks counterpart, plus its deferred deletion imposed by the TrackList iterator semantics.

Additional context

Although I first thought this would be an easy hack to add to Nyquist.cpp, and I even wrote the parser part, it turns out not to be the easy at all because the decision what/whether to insert is done in the massive, monolithic method of the Effect base namely DoEffect.

And there's no single object in Audacity that represents a stereo track, WaveTrack is mono. Inserting a stereo track via the menu does this little song and dance:

   auto left = tracks.Add( trackFactory.NewWaveTrack( defaultFormat, rate ) );
   left->SetSelected(true);

   auto right = tracks.Add( trackFactory.NewWaveTrack( defaultFormat, rate ) );
   right->SetSelected(true);

   tracks.MakeMultiChannelTrack(*left, 2, true);

   ProjectHistory::Get( project )
      .PushState(XO("Created new stereo audio track"), XO("New Track"));

   TrackFocus::Get(project).Set(left);
   left->EnsureVisible();

But DoEffect assumes newTrack is one object, not a pair, including in cleanup. So it will require fairly substantial rewrite to handle "atomic" insertion and cleanup of more than one mono track, which is how a stereo track is resented internally.

So I'll leave this to people who designed this class affair to sort it out how to implement this relatively trivial feature (from the user's perspective, that is).

What should probably happen in a less monolithic interface is to have some additional methods like:

• prepareToDoEffect, delegated to subclasses, like the NyquistEffect, which allocates tracks
  based, e.g. in the Nyquist case on what it parsed.
• cleanupFailedEffect. Could be just in the base Effect class, if the Audacity transaction
  system, which I don't understand well at the moment, can do this by itself, assuming the
  subclasses can put enough info in the transaction so a generic undo code is possible. If not, then
  cleanupFailedEffect should also be delegated to sub-classes.

The general idea is that DoEffect needs more hooks that are overridden in subclasses, instead of
trying to decide everything by itself in terms of what project resources effects (of any and all subclasses) are going to need.

A somewhat simpler approach wold be to make newTrack a collection in DoEffect, but
I have a feeling that just piles up bad design.

[werame]

Or scratch that simple idea of predeclaring return type as too inflexible as well. Come to think of it, Audacity is capable of inserting the right resource in one case: when a process plugin returns a label track. So, instead of preinserting any track at all for generate, it should simply do it after the plugin returns a Lisp object, which isn't a data track yet, just a generator for one, aka Nyquist SOUND object--that's how Nyquist works.

So the logic for type generate should probably rather be:

1. run the Nyquist code, see what it returns.
2. If the number of channels in the SOUND object matches the selected track, just insert in there.
3. If not, pop up a dialog telling the user about the mismatch and asking if they'd rather have the result inserted in a new track... which can be now generated with the right number of channels.

That fact that 3 happens after a SOUND object is setup also probably means that no cleanup is needed, but I'm not entirely sure. If a Ny SOUND has closures (e.g. for seq or trigger), they can error later, so the whole generation process can be ultimately unsuccessful. It's a bit unclear what should happen in that case with the track inserted and possibly with partially returned data in it.

As a special case of 3:

3b. If there's no track selected (including when the project has zero tracks), it could skip the dialog box from 3 and just create the track. Again, if this is done post-running the Ny code, it knows the type returned, so number of channels, in case of audio.

What this would mean is that NyquistEffect would only need a simple override of the generic Effect code via a flag-type virtual dispatch from the base Effect class that would merely make it not pre-insert anything for Nyquist plugins, i.e. just skip over those 3 lines. Something like wrapping those in an extra condition if (!subclassHandlesResources() && ...).

[SteveDaulton]

1. run the Nyquist code, see what it returns.

Audacity does not know if the plug-in is a generate type until it parses the plug-in header. If the plug-in is type ;process or ;analyze, then the track selection must exist before the Nyquist code runs because Nyquist needs to be able to access the track audio data.

3. If not, pop up a dialog telling the user about the mismatch and asking if they'd rather have the result inserted in a new track...

If, for example, the plug-in returns several hours of stereo audio, where is that data stored while waiting for the user to decide which track to write it to? We certainly wouldn't want it to be held in RAM.

[werame]

, where is that data stored while waiting for the user to decide which track to write it to?

The trick is that the user is prompted right after Nyquist returns the generator, before data is actually pulled/generated. If you noticed, any error messages happen like that, before actual data is pulled/generated.

---

Well I tried this b58d771
which essentially does AddToOutputTracks(outputTrack[i]) in case of overflow (I'll deal with niceties of converting to a stereo track later) While it doesn't crash, insofar I have some issue with Audacity going in a loop with the "too many channels" message. No idea why right now.

To test it: generate a mono track, the insert something stereo like (vector *track* *track*).

I'll debug it tomorrow. A bit too tired right now. Well, the code I wrote seems ok in the debugger, gets two copies and add the to the project, nyx doesn't choke on that. But then it's this top level loop

audacity/src/effects/nyquist/Nyquist.cpp

Line 838 in b58d771

for (;

that somehow thinks it's not done. I'm not sure what it's testing for, to make it happy.

[werame]

My hunch is that what happens is that those (pRange) iterators are not mutation-safe. So when I add tracks to the project as a result of the plug-in run, those iterators try to go over the newly added tracks and apply the effect again to them, for some reason.

I don't understand though why that problem doesn't occur when the fx adds a label track though. I'll have to make several debug runs to see how those iterators behave in all these cases... Ok, the iterator only goes over WaveTracks, so that probably explains why it's safe to add label but not wave tracks:

   Optional<TrackIterRange<WaveTrack>> pRange;

[werame]

Ha, ha. I made it work. The trick was to not select the resulting tracks. Even if the iterator goes over them, nyx won't be called unless the tracks are selected. A bit obvious in hindsight.

That was made by running (vector (mult 0.4 *track*) (mult 0.8 *track*)) over a mono track. It auto-dumped the "excess" return stuff, meaning both tracks to new ones.

And it works for generate too. The example below is a bit confusing because I used nearly the same time in the previous selection as the standard 1.0 second generate

The latter two tracks were added by not selecting any range but having the pointer in the 1st track and doing:

;type generator
(vector (hzosc 100) (hzosc 800))

So, it's a good hack. The only somewhat strage bit is that after creating those last two tracks, Audacity selected a 1.0 seconds range in the original track, which is what usually happens when you generate something and it doesn't error with "too many channels returned".

My hack sill needs some polish to auto-create stereo tracks, instead of pairs of mono. But it also opens the possibility to e.g. return 5 channels from Nyquist and have those be inserted ok in Audacity, although presently there are some limitations with hardcoded 2-channel buffer/track allocators in Nyquist.cpp, which were not my doing but I exploited them in the sense that it was almost always possible to return stereo.

---

The way it actually breaks out of that big loop is none of the output tracks are selected is interestingly enough through the iterator test itself. I'm not sure if those are undocumented semantics... because Track.h doesn't seem to suggest it should skip over unselected ones, but maybe I missed something in the 1,700 lines of code that cover various ways of iterating over tracks... with lots of nested template calls.

[werame]

Also, if you call stereo generator from a completely empty project, it work basically now, as it results in something like this

The empty track is a bit of a nuisase, which I plan to remove (as I outlined a couple of posts earlier), but the best part of this experiment that if you hit Ctrl+Z to undo, all three of those tracks go away in one hit, and I didn't have to do anything special for that to happen. The SQLite3 transaction just does its rollback magic!

[werame]

I ran into another roadblock while trying to make that array into a stereo track. Because

      if (numChansBoosted > 1) {
         auto pProject = (AudacityProject*) FindProject(); // de-const for MakeMultiChannelTrack
         auto &tracks = TrackList::Get(*pProject); // hopefully pRange is ok with this (turns out it's not!)
         auto left = outputTrack[0].get();
         tracks.MakeMultiChannelTrack(*left, numChansBoosted, true); // INCONSISTENCY_EXCEPTION!
      }

i.e. it seems MakeMultiChannelTrack is unhappy being called while the top-level loop holds an iterator over the tracks.

Hmm, so ironically this doesn't work because in MakeMultiChannelTrack

if (list.get() == this)

has the lhs with 3 elements/tracks, because it sees modified one, but the rhs actually is a snapshot before current transction began, so it only sees one track. And that makes it throw the INCONSISTENCY_EXCEPTION.

[werame]

Ok, I solved that little conundrum too. There was a way to get the current/dirty track list. And MakeMultiChannelTrack is perfectly happy to operate on that.

Now there's a one-shot stereo result returned:

Also, Ctrl+Z undoes it atomically.

[werame]

Now I got stuck at the final step in case of a stereo gen ran on an empty project: removing the empty mono "stub" track.

It's quite possible I don't understand how to use the API(s) to remove tracks... but it shouldn't be this !#$%#$ hard.

#2450

---

I got the final bit working. Less flexibly than the insertion. Due to iterator invalidation nastiness... which popped up a pretty hard to understand error

I had to delay deletion... which means Nyquist would not be generally be able to do this easily, i.e. it's only for a special case. But for the simple stereo gen affair, or even the Nyquist prompt track insertion (which can happen at any position, not just empty project), it work ok to delay-delete that specific track that was inserted.

I'm gonna make one or two pull requests with this, after I clean the code a bit. I'm not sure if I should make a PR for the part that allows overflow addition separately from the deletion part... which is bit less elegant, so might need more discussion(s).