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

Ignore release frames for single-streamed instruments #3900

Merged
merged 4 commits into from
Oct 23, 2017

Conversation

PhysSong
Copy link
Member

Fixes #3899 by letting InstrumentSoundShaping::releaseFrames() check Instrument::IsSingleStreamed flag.
There is one further option, disabling volume envelope when a single-streamed instrument is dropped(or even loaded). However, this change seems enough.

@PhysSong PhysSong requested a review from zonkmachine October 22, 2017 14:13
@zonkmachine
Copy link
Member

zonkmachine commented Oct 22, 2017

Getting there. The issue is still present for cutoff and resonance.

@PhysSong
Copy link
Member Author

True. There was a mistake and now fixed.

If m_instrumentTrack->instrument() is NULL, there's no reason to return non-zero value in InstrumentSoundShaping::releaseFrames(). And such situation shouldn't occur anymore.
@PhysSong
Copy link
Member Author

@zonkmachine Now everything is done. Could you re-test it now?

@zonkmachine
Copy link
Member

Now release doesn't seem to work.

@PhysSong
Copy link
Member Author

Yes. I put ! by mistake... Fixed by 6ad5b0b.

Copy link
Member

@zonkmachine zonkmachine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing: 👍

@zonkmachine
Copy link
Member

Just got a crash around where you're coding here but on my 'automation' branch (which doesn't touch anything there, so basically stable-1.2).

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x0000000000524ef0 in InstrumentSoundShaping::releaseFrames (this=0x100007bd60)
    at /home/zonkmachine/builds/lmms/lmms/src/core/InstrumentSoundShaping.cpp:307
307			? m_instrumentTrack->instrument()->desiredReleaseFrames()
(gdb) bt
#0  0x0000000000524ef0 in InstrumentSoundShaping::releaseFrames (this=0x100007bd60)
    at /home/zonkmachine/builds/lmms/lmms/src/core/InstrumentSoundShaping.cpp:307
#1  0x000000000053d229 in NotePlayHandle::actualReleaseFramesToDo (this=0x10000302f8) at /home/zonkmachine/builds/lmms/lmms/src/core/NotePlayHandle.cpp:400
#2  0x000000000053ce08 in NotePlayHandle::framesLeft (this=0x10000302f8) at /home/zonkmachine/builds/lmms/lmms/src/core/NotePlayHandle.cpp:321
#6  0x000000000051c04b in MixerWorkerThread::addJob (_job=0x10000302f8) at /home/zonkmachine/builds/lmms/lmms/include/MixerWorkerThread.h:87
#7  0x0000000000536d5a in MixerWorkerThread::fillJobQueue<QList<PlayHandle*> > (_vec=..., _opMode=MixerWorkerThread::JobQueue::Static)
    at /home/zonkmachine/builds/lmms/lmms/include/MixerWorkerThread.h:99
#8  0x0000000000533cbe in Mixer::renderNextBuffer (this=0x10337d0) at /home/zonkmachine/builds/lmms/lmms/src/core/Mixer.cpp:439
#9  0x0000000000535827 in Mixer::fifoWriter::run (this=0x14cba40) at /home/zonkmachine/builds/lmms/lmms/src/core/Mixer.cpp:1103
#10 0x00007f6ec092332f in QThreadPrivate::start (arg=0x14cba40) at thread/qthread_unix.cpp:349
#11 0x00007f6ec1c98184 in start_thread (arg=0x7f6e777fe700) at pthread_create.c:312
#12 0x00007f6ebe025ffd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111

@PhysSong
Copy link
Member Author

What were you doing when it crashed?

@zonkmachine
Copy link
Member

I think I pressed either of the Open existing project and Open Recenty opened projects buttons while looping a track.

@PhysSong
Copy link
Member Author

I think it's a separate issue and very rare one. It is "dereferencing a deallocated pointer" problem. There are several approaches to fix it. I guess it could be fixed easily if needed.

@zonkmachine
Copy link
Member

OK. Open a separate issue for it?
Merge #3900?

@PhysSong
Copy link
Member Author

I'll merge it after about 10 hours.

Open a separate issue for it?

Well, I don't think that's a major bug. Anyway, you may do so if needed.

@zonkmachine
Copy link
Member

No separate issue. If you search the site you get this discussion anyway so it's already documented.

@PhysSong
Copy link
Member Author

If you search the site you get this discussion anyway so it's already documented.

Where can I find that?

@zonkmachine
Copy link
Member

I mean when you search for a part of the backtrace. Like this.

@PhysSong
Copy link
Member Author

Though I think the ctash is not really relevant to this PR, fixing that isn't harmful. I suggest fixing it later, but it can be done now if needed.

@zonkmachine
Copy link
Member

You call the shots here. Fixing later is fine by me.

@PhysSong PhysSong merged commit de20d76 into LMMS:stable-1.2 Oct 23, 2017
@PhysSong PhysSong deleted the singlestream-noenv branch October 23, 2017 15:21
@zonkmachine
Copy link
Member

Got another crash like the one in #3900 (comment) when poking the arpeggiator release time in #4355.

I was just listening to a four beat groove when it took a dive. By judging from the backtrace it crashed when looping back to the beginning.

last_metro_pos = {<MidiTime> = {m_ticks = -1, static s_ticksPerTact = 192}, m_timeLine = 0x0, m_currentFrame = 0}

Full backtrace in gist here.

@PhysSong
Copy link
Member Author

@zonkmachine Do you remember which instrument was playing? Or do you know steps to reproduce?

@zonkmachine
Copy link
Member

No steps to reproduce. It's just one kick and an arpeggiated lb302 running over 4 beats. I'll try and see if I can make it crash again. I'll leave it up and running until something happens. Just one instrument so there'll be no doubt on the matter.

@zonkmachine
Copy link
Member

8 hours and not a glitch. I think I need a plan for testing this on a larger scale.

sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
Let InstrumentSoundShaping::releaseFrames() ignore release frames for single-streamed instruments. And make it return 0 if m_instrumentTrack->instrument() is NULL.
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.

2 participants