Rework Song::processNextBuffer #5723

DomClark · 2020-10-19T22:07:54Z

This was originally an attempt to fix a VST bug reported on Discord. The issue turned out to be in Song::processNextBuffer, which was fairly scary and convoluted, so I tried to clean it up a bit. Testing and review would be particularly appreciated, as this bit of code is rather integral to playback, and is rather complex.

Original report, from @Ambi#0808 on Discord:

Error report:
VST by the name of "Church Organ 2nd" by C.Hackl consistently stops playing all notes starting at bar 8 of all scheduled midi notes, when its internal reverb is enabled.
Unsure whether this problem is with the VST itself or with the LMMS program. Testing on the latest Windows 10 update, with version 1.2.1 of LMMS.
The plugin is bridged from 32-bit to 64-bit, and it happens at any tempo.
For some reason after repeatedly enabling and disabling the internal reverb, the problem is no longer apparent.
The error also occurs when adding voices on top of each other over time, such that trying to write a CmM7 chord where a voice appears each bar, playing the last and highest note B cancels all voices.
Signs point to a poorly-coded VST, but if anyone would like to test with a more recent version of LMMS as well as other DAWs it would be appreciated.

Downloadable here: https://vst4free.com/plugin/622/

another odd part of this bug is that it seems to behave entirely differently when played in the piano roll vs the song editor

update: i've discovered that when played in the piano roll, it's not merely bar 8 of all scheduled midi notes, but simply the last bar

It turns out that the kVstTransportChanged was incorrectly set for the whole last bar in the song editor, due to faulty loop handling in Song::processNextBuffer.

The first error is here:

lmms/src/core/Song.cpp

Lines 288 to 292 in 0d89d64

    
           int ticks = m_playPos[m_playMode].getTicks() + 
        
           	( int )( currentFrame / framesPerTick ); 
        
           // did we play a whole bar? 
        
           if( ticks >= MidiTime::ticksPerBar() )

MidiTime::getTicks() returns the total number of ticks, not the number of ticks within the current bar, so this condition is true all the time after the first bar, not just at the beginning of a new bar.

This leads to problems here:

lmms/src/core/Song.cpp

Lines 316 to 318 in 0d89d64

    
           // end of played object reached? 
        
           if( m_playPos[m_playMode].getBar() + 1 
        
           				>= maxBar )

This is intended to run when m_playPos[m_playMode] is at the end of the previous bar, but ticks is at the beginning of the next, hence the + 1 correction. However, due to the previous issue, the condition is checked every tick after the first bar, and is true throughout the last bar. This leads to erroneous calls to m_vstSyncController.setPlaybackJumped(true), resulting in the observed issue.

LmmsBot · 2020-10-19T22:14:26Z

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

Linux (AppImage): lmms-1.3.0-alpha.1.15%2Bg92425b3-linux-x86_64.AppImage (build link)

Windows

Windows 32-bit: lmms-1.3.0-alpha.1.15%2Bg92425b309-mingw-win32.exe (build link)
Windows 64-bit: lmms-1.3.0-alpha.1.15%2Bg92425b309-mingw-win64.exe (build link)
Windows 32-bit: lmms-1.3.0-alpha-msvc2017-win32.exe (build link)
Windows 64-bit: lmms-1.3.0-alpha-msvc2017-win64.exe (build link)

macOS

macOS : lmms-1.3.0-alpha.1.15%2Bg92425b309-mac10.13.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://10559-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.15%2Bg92425b3-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10559?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://10555-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.15%2Bg92425b309-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10555?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://10556-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.15%2Bg92425b309-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10556?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/2fdfchvd2r28xxbq/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36389263"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/tu1jd6i67y4j3790/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36389263"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://10557-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.15%2Bg92425b309-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10557?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "e0f50e80d3c429cc1175ff22d22d1432fd41b7b1"}

src/core/Song.cpp

Hey-Holokin · 2020-10-28T05:26:54Z

Original reporter of the issue here. Can confirm this pull request fixes the issue completely. No random note stops during the last bar in the piano roll!

superpaik · 2020-11-16T11:20:10Z

Tested on 1.2.2 and found the misbehaviour (though only happend with this VST, I tested with other VST and they worked well ¿?). Reproduced with this PR and it works fine.

sziegler103 · 2020-11-16T13:16:36Z

Tested with the organ plugin on 1.2.2, 32bit Windows (so no usage of a bit bridge.) Was only able to reproduce the last bar not playing within the piano roll. Was not able to reproduce >bar 8 not playing in the song editor or piano roll.

Tested again using the 32bit MSVC build from this PR. The fix seems to have worked. Last bar in piano roll plays successfully with the organ plugin. Other plugins tested perform as expected on both 1.2.2 and this build.

Spekular · 2020-11-17T18:46:22Z

I'm hesitant to leave an approving review since I haven't thoroughly checked out the old code to see that everything except the fix is (functionally) identical, but I don't see any obviously suspicious changes either. If you want, I can probably give it a more thorough look some time. Quality wise the new code looks good, and much better than the old.

src/core/Song.cpp

IanCaio

Code LGTM!

I had to learn the logic of the method as I reviewed, and I've small doubts about some details (would you mind me asking about them in Discord for the sake of learning?), but as far as the behavior of the method goes, the only difference I noticed was the way the loop points are enforced and how the time elapsed on the processed buffer is counted (aka the bugfix itself).

Great job on making the method more organized and readable!

I like the variables renaming proposed by PhysSong: I don't think their meaning is too obscure right now, but his suggestion could improve their understanding even more! There's also that small change on the lambda function we talked about earlier, but since you already said you'll look into it I'm not leaving it as a requested change (it's also not critical, just something to keep in mind in terms of code maintenance if we at some point reach C++20).

DomClark · 2020-11-26T15:19:23Z

Merge?

zonkmachine · 2020-11-26T19:43:37Z

Tested and works fine. Merge.

On a related note, please see #3028.

lmms/src/core/Song.cpp

Lines 258 to 259 in e0f50e8

    
           // Ensure playback begins within the loop if it is enabled 
        
           if (loopEnabled) { enforceLoop(timeline->loopBegin(), timeline->loopEnd()); }

This doesn't work if the loop is activated when the song is playing and outside of the loop. The song will skip to the loop but miss the first events. Off by one thing...

DomClark · 2020-11-26T22:22:54Z

I think it's because the frame count isn't reset when looping. The loop code only sets the ticks, so if we're not in the first frame of a tick when we jump to the beginning of the loop, the first tick's events will be skipped. The other loop checking code only ever runs at the first frame of a tick, so isn't affected.

PhysSong · 2020-11-30T06:30:25Z

src/core/Song.cpp

+		const f_cnt_t framesUntilNextPeriod = framesPerPeriod - frameOffsetInPeriod;
+		const f_cnt_t framesUntilNextTick = static_cast<f_cnt_t>(std::ceil(framesPerTick - frameOffsetInTick));


frameOffsetInTick might be invalid at this point if enforceLoop() changed the play position.

Edit: It's not a regression can even be wrong, so you can ignore it at this moment.

A situation where it matters is:

Add a note pattern starting at bar 3

Add a note starting at the beginning of that pattern

Locate the loop begin point at bar 3, but don't enable it yet

Start playing song at bar 1

Now enable looping

The play head jumps to bar 3, but the note is not played

As I mentioned above, it's not a regression, but an old bug. You can ignore this.
If it's considered as a bug, I can open a new bug report.

frameOffsetInTick is correct, in that it still corresponds to the ticks of the current PlayPos. The ticks of the PlayPos should be reset to zero/fmod'd with one though (can't decide which is correct). The issue is #3028. (Zonkmachine mentioned this here: #5723 (comment).)

Oh, I see. Thanks for mentioning the relevant issue.

PhysSong · 2020-11-30T06:33:15Z

src/core/Song.cpp

 		}

-		if( ( f_cnt_t ) currentFrame == 0 )
+		if (static_cast<f_cnt_t>(frameOffsetInTick) == 0)


For the same reason, this check might go wrong. This can result in skipping some TCOs.

Edit: It's not a regression and can even be wrong, so you can ignore it at this moment.

PhysSong · 2020-11-30T07:15:18Z

src/core/Song.cpp


-			if (checkLoop || m_loopRenderRemaining > 1)
+			// Handle loop points, and inform VST plugins of the loop status
+			if (loopEnabled || m_loopRenderRemaining > 1)


Suggested change

if (loopEnabled || m_loopRenderRemaining > 1)

if (loopEnabled || (m_loopRenderRemaining > 1 && getPlayPos() >= timeline->loopBegin()))

This will be explained in the comment below.

PhysSong · 2020-11-30T07:15:42Z

src/core/Song.cpp

-				// beginning of the range
-				if( m_playPos[m_playMode] >= tl->loopEnd() )
+				// Loop if necessary, and decrement the remaining loops if we did
+				if (enforceLoop(timeline->loopBegin(), timeline->loopEnd())


This adds m_playPos[m_playMode] < timeline->loopBegin() condition for jumping back which is undesired in case m_loopRenderRemaining > 1, and make the area before the loop point be skipped or play wrongly. A relevant bug was reported on Discord.
Also, the startCycle() call should be checked by m_playPos[m_playMode] >= timeline->loopBegin(), though it was already wrong before this PR.
The proposed fix is in the comment above.

Fix looks good to me and seems to work properly; PR open at #5814.

Rework Song::processNextBuffer

6877575

DomClark added needs code review A functional code review is currently required for this PR needs testing This pull request needs more testing labels Oct 19, 2020

M374LX reviewed Oct 19, 2020

View reviewed changes

src/core/Song.cpp Show resolved Hide resolved

DomClark removed the needs testing This pull request needs more testing label Nov 16, 2020

PhysSong reviewed Nov 18, 2020

View reviewed changes

src/core/Song.cpp Outdated Show resolved Hide resolved

src/core/Song.cpp Outdated Show resolved Hide resolved

IanCaio approved these changes Nov 18, 2020

View reviewed changes

Apply suggestions from review

e0f50e8

Veratil approved these changes Nov 26, 2020

View reviewed changes

DomClark removed the needs code review A functional code review is currently required for this PR label Nov 26, 2020

DomClark merged commit 246b822 into LMMS:master Nov 27, 2020

DomClark deleted the song-processnextbuffer-rework branch November 27, 2020 13:46

PhysSong reviewed Nov 30, 2020

View reviewed changes

DomClark mentioned this pull request Nov 30, 2020

Fix export when rendering looped section multiple times #5814

Merged

IanCaio pushed a commit to IanCaio/lmms that referenced this pull request Mar 28, 2021

Rework Song::processNextBuffer (LMMS#5723)

d31117e

devnexen pushed a commit to devnexen/lmms that referenced this pull request Apr 10, 2021

Rework Song::processNextBuffer (LMMS#5723)

ac3315f

sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022

Rework Song::processNextBuffer (LMMS#5723)

fd934a9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework Song::processNextBuffer #5723

Rework Song::processNextBuffer #5723

DomClark commented Oct 19, 2020

LmmsBot commented Oct 19, 2020 •

edited

Loading

Linux

Windows

macOS

Hey-Holokin commented Oct 28, 2020

superpaik commented Nov 16, 2020

sziegler103 commented Nov 16, 2020

Spekular commented Nov 17, 2020

IanCaio left a comment

DomClark commented Nov 26, 2020

zonkmachine commented Nov 26, 2020

DomClark commented Nov 26, 2020

PhysSong Nov 30, 2020 •

edited

Loading

PhysSong Nov 30, 2020 •

edited

Loading

DomClark Nov 30, 2020

PhysSong Nov 30, 2020

PhysSong Nov 30, 2020 •

edited

Loading

PhysSong Nov 30, 2020

PhysSong Nov 30, 2020

DomClark Nov 30, 2020

	int ticks = m_playPos[m_playMode].getTicks() +
	( int )( currentFrame / framesPerTick );

	// did we play a whole bar?
	if( ticks >= MidiTime::ticksPerBar() )

	// end of played object reached?
	if( m_playPos[m_playMode].getBar() + 1
	>= maxBar )

		const f_cnt_t framesUntilNextPeriod = framesPerPeriod - frameOffsetInPeriod;
		const f_cnt_t framesUntilNextTick = static_cast<f_cnt_t>(std::ceil(framesPerTick - frameOffsetInTick));

	if (loopEnabled \|\| m_loopRenderRemaining > 1)
	if (loopEnabled \|\| (m_loopRenderRemaining > 1 && getPlayPos() >= timeline->loopBegin()))

Rework Song::processNextBuffer #5723

Rework Song::processNextBuffer #5723

Conversation

DomClark commented Oct 19, 2020

LmmsBot commented Oct 19, 2020 • edited Loading

Linux

Windows

macOS

Hey-Holokin commented Oct 28, 2020

superpaik commented Nov 16, 2020

sziegler103 commented Nov 16, 2020

Spekular commented Nov 17, 2020

IanCaio left a comment

Choose a reason for hiding this comment

DomClark commented Nov 26, 2020

zonkmachine commented Nov 26, 2020

DomClark commented Nov 26, 2020

PhysSong Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

PhysSong Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

DomClark Nov 30, 2020

Choose a reason for hiding this comment

PhysSong Nov 30, 2020

Choose a reason for hiding this comment

PhysSong Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

PhysSong Nov 30, 2020

Choose a reason for hiding this comment

PhysSong Nov 30, 2020

Choose a reason for hiding this comment

DomClark Nov 30, 2020

Choose a reason for hiding this comment

LmmsBot commented Oct 19, 2020 •

edited

Loading

PhysSong Nov 30, 2020 •

edited

Loading

PhysSong Nov 30, 2020 •

edited

Loading

PhysSong Nov 30, 2020 •

edited

Loading