Update two NotePlayHandle methods to ignore child NotePlayHandles

[Fastigium]

The methods NotePlayHandle::index and NotePlayHandle::nphsOfInstrumentTrack
had not yet been brought up-to-date with the new system of attaching child
NotePlayHandles directly to the mixer. This caused strange glitches when
arpeggio was used in sort mode.

[Fastigium]

This fixes #2589 for me. Testing welcome!

[zonkmachine]

This fixes the issue when I try it with one note but not more than one. It also fixes an issue I've had with stuck notes on a pull request I just postponed to 1.3 ... but not for single stream instruments... ( #2130 ( that PR isn't up to date however ) ).

[Fastigium]

@zonkmachine Thanks for testing! Can you elaborate on how it doesn't fix the issue with more than one note? How does it behave, and how do you expect it to behave? What steps do you take to get unexpected behavior?

[zonkmachine]

Easiest to show. I expect this short track to repeat in the exact same way. It skips notes randomly in the second part.

arpeggio.mmp.zip

[Fastigium]

It skips notes randomly in the second part

Confirmed, and it doesn't do that in 1.1.3. Hm, I foresee more bug hunting in my future 😁

[Fastigium]

@zonkmachine Well, how do you know? If I cherry-pick* this pull request onto a821187 (the commit you found bisecting #2589), the second part doesn't skip any notes! Can you confirm that? If yes, it means that some change after a821187 messed things up again. In that case, would you feel like doing another bisect run?

*: Resolving the conflict with git checkout --ours include/NotePlayHandle.h

[zonkmachine]

If I cherry-pick* this pull request onto a821187 (the commit you found bisecting #2589), the second part doesn't skip any notes! Can you confirm that?

Just want to make sure we're on the same code here. I'm getting compile errors with a821187 and it's basic stuff like:

/lmms/src/gui/SetupDialog.cpp:35:25: fatal error: SetupDialog.h: No such file or directory
 #include "SetupDialog.h"

[Fastigium]

The exact commands I used to get a skip-free a821187:

git checkout a821187
git cherry-pick -n eec7f634a866a145149c37e0bbf3ae960fbd298b
git checkout --ours include/NotePlayHandle.h

Note that this will only work if you have Fastigium@eec7f63 locally. If you don't, you should be able to fetch it using git fetch https://github.com/Fastigium/lmms.git

[zonkmachine]

No go over here...

Note that this will only work if you have Fastigium/lmms@eec7f63

I tested arpeggiator-sort-fix and it's still on my git. I can git checkout Fastigium@eec7f63 And there it is, but I can't merge it onto a821187.

$ git checkout a821187
HEAD is now at a821187... Fix arpeggio to work better...
$ git cherry-pick -n eec7f634a866a145149c37e0bbf3ae960fbd298b
error: could not apply eec7f63... Update two NotePlayHandle methods to ignore child NotePlayHandles
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
zonkmachine@zonkmachine:~/builds/lmms/lmms$ 

I went stone age and even made a diff that wouldn't apply either.

[zonkmachine]

It's woodoo. Now it's working and I can confirm the fix at a821187 .
I'm testing this thoroughly now and will go ahead with a bisect but it may take quite a while.

Edit: Sorry, that fix didn't work.

[zonkmachine]

@zonkmachine Well, how do you know? If I cherry-pick* this pull request onto a821187 (the commit you found bisecting #2589), the second part doesn't skip any notes! Can you confirm that?

@Fastigium No, sorry. I can no longer confirm this. If there was a fix in there it's not clearly repeatable.
I'll give it another go and see if there was anything wrong on my side. Well, considering... ^ ^^

[Fastigium]

@zonkmachine Thanks so much for trying! Let me look into what you've been doing...

And there it is, but I can't merge it onto a821187.

That's actually expected behavior you're getting there. That's what the git checkout --ours include/NotePlayHandle.h is for.

If there was a fix in there it's not clearly repeatable.

Well, you got me there. It skips notes here at a821187 with my commit applied, too. Just very seldom, so I thought at first that it didn't skip them at all. Hm. This needs more investigation. Do the missing notes get added to the mixer at all? I'll see if I can get some investigation time in today. Thanks again!

[Fastigium]

Check, I found the culprit. And this LB302 note skipping was there in 1.1.3 as well! The problem is caused by this code in InstrumentPlayHandle.h. It was added after the mixer was made multi-threaded because it could no longer be guaranteed that all of an instrument's NotePlayHandles would be played before its InstrumentPlayHandle. Note that only a few instruments (including LB302) use this dual construction of both NotePlayHandles and an InstrumentPlayHandle.

At any rate, what goes wrong is that some NotePlayHandles are processed twice simultaneously in different threads, causing all kinds of mayhem with their current frame counters. Frankly I'm much surprised at how usable LB302 still is with this bug going on. Which is probably the reason that it has survived this long 😋.

However, fixing this isn't a trivial matter. I managed to quick-fix it by preventing InstrumentPlayHandle::play() from processing the NotePlayHandles and just letting it busy-wait until they're all done, but that's a waste of time and CPU cycles. To solve this correctly, either (1) the mixer should implement some kind of dependency system to prevent a PlayHandle from being processed until certain specific other PlayHandles have been processed, or (2) a ThreadableJob should be able to cancel its processing and be put back in the job pool to be tried again later. Option (1) sounds like a lot of work and extra business logic just to solve what is almost an edge case, since only a few instruments need this, but (2) sounds like a dirty hack. Almost as dirty as making InstrumentPlayHandle::play() busy-wait 😁.

So, what to do... This seems complicated enough to me to get its own issue. Besides, the regression reported in #2589 is already fixed by this pull request; the problem we're dealing with now is there in stable-1.1 as well. So I propose to merge this pull request if DeRobyJ is happy (I'll fix him up with a Win64 build so he can test) and create a new issue to discuss how this new bug is best solved. What do you think, @zonkmachine?

[Fastigium]

Oh, by the way, should you want to test the busy-wait fix, just comment out this line: https://github.com/LMMS/lmms/blob/master/include/InstrumentPlayHandle.h#L65

[tresf]

So I propose to merge this pull request if DeRobyJ is happy

👍

[zonkmachine]

At any rate, what goes wrong is that some NotePlayHandles are processed twice simultaneously in different threads, causing all kinds of mayhem with their current frame counters.

#2565 ?

So I propose to merge this pull request if DeRobyJ is happy

👍