Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up after render in ProjectRenderer destructor #3680

Merged
merged 1 commit into from
Jul 7, 2017

Conversation

zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Jul 3, 2017

We need to wait with calling Mixer::restoreAudioDevice() and
Mixer::changeQuality() after render until all threads have stopped.
Moving these calls to ProjectRenderer::~ProjectRenderer() ensures
all render theads are done.

Fixes #3555

Edit: This works just fine...

@zonkmachine
Copy link
Member Author

zonkmachine commented Jul 3, 2017

multitapechocrash.mmp.zip
rendercrash.mmp.zip
./lmms --render ~/lmms/rendercrash.mmp -o test.wav
Here is a short test project with lots of reverb and delay which are the only units I've seen these issues with (so far...). Under lmms-1.2.0-RC3 this project will crash something like one time out of four. With the fix I've seen no crashes so far. Tested both from the command line and gui.

@musikBear
Copy link

When you have a win-exe, notch me and i will test if needed.

@zonkmachine
Copy link
Member Author

When you have a win-exe, notch me and i will test if needed.

That's much appreciated. RC4 should be just around the corner... ;-)

@zonkmachine
Copy link
Member Author

@musikBear Can you test the projects above and confirm if they crash on render or not on RC3?
multitapechocrash.mmp won't complete one single render on my setup.

@liushuyu
Copy link
Member

liushuyu commented Jul 6, 2017

Please… target the master and back port to stable, otherwise the monster in #3607 will keep growing…

@musikBear
Copy link

@zonkmachine I must have overlooked a blue-dot, sorry
I have just downloaded the projects, and i will test asap today

@musikBear
Copy link

i tested the projects on win32 xp3 RC3
I can play and export multitapechocrash.mmp Playback with no crash can be done in both song-editor, and from BBE. Export in default settings.

But noticed a steange thing. All instruments are both solo and 'enabled'
soloandplayback
Is that also the case on your side?

Project rendercrash also crashes for me with default render-settings for wave, but the wave-file was exported, and in full length!
I also exported in ogg, with default settings, and that export did not crash lmms, and the exported ogg-file was fine.

@zonkmachine
Copy link
Member Author

Thanks for testing this!

I can play and export multitapechocrash.mmp Playback with no crash can be done in both song-editor, and from BBE. Export in default settings.

I should have mentioned that it doesn't happen every time. Please try it some more with this project and see if you can break it. 8)

But noticed a steange thing. All instruments are both solo and 'enabled'

I'm the bug on that one. Working fast and cloning tracks... Boom!

Wave or ogg shouldn't matter. Just export 20 times each project and see how often they break.

but the wave-file was exported, and in full length!

Yes, the crash happens at the end of export. I imagine that you could end up with both broken exports and finished working files but I haven't tested that fully.

@zonkmachine
Copy link
Member Author

but the wave-file was exported, and in full length!

Yes, the crash happens at the end of export. I imagine that you could end up with both broken exports and finished working files but I haven't tested that fully.

I've exported ogg and wav files now and I always end up with a successful output file.

I'm going to merge this tomorrow. Testing/Review is most welcome.

We need to wait with calling Mixer::restoreAudioDevice() and
Mixer::changeQuality() after render until all threads have stopped.
Moving these calls to ProjectRenderer::~ProjectRenderer() ensures
all render theads are done.
@zonkmachine zonkmachine merged commit 020f165 into LMMS:stable-1.2 Jul 7, 2017
@zonkmachine zonkmachine deleted the rendercrash branch July 7, 2017 19:45
@musikBear
Copy link

Just export 20 times each project and see how often they break.

I will do that today

@musikBear
Copy link

@zonkmachine
Rendercrash failed 1/20 exports, but the export was fine
multitapechocrash did not fail in 20 renders. The file was fine.

@zonkmachine
Copy link
Member Author

Interesting. rendercrash.mmp failed for me about the same, but multitapechocrash.mmp I haven't managed to render without a crash even once.

@musikBear
Copy link

@zonkmachine that is odd
Im on win32 are you on a 64?
Otherwise my settings are:
generalsettings
-And SDL with no 'addOns'
Cant think of anything else that could be of importance, except the actual sub-version of my RC3:
rc3

-surely there is no version shrew-ups.. or

@zonkmachine
Copy link
Member Author

There are too many variables that differ here so I no point in comparing setups. In your case you have a weaker computer than I do and your OS is much older, win XP. This issue is related to multiple threads and multi threading is being done differently by Qt on different platforms so we could be trouble shooting this until the next millennium. The code has already been committed to stable-1.2 and will be in RC4 and then you can test it again to see if the crash (hopefully) is gone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants