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

Fix destructor call in NotePlayHandleManager #3884

Merged
merged 1 commit into from Oct 16, 2017

Conversation

Projects
None yet
3 participants
@PhysSong
Member

PhysSong commented Oct 15, 2017

Fixes a regression from #3783 due to missing destructor call. It should have been added in 42e67d2, but not.
Since BufferManager::releaseBuffer() call is moved in #3783, buffer isn't freed(by MM_FREE()) in NotePlayHandle::done(). It isn't a memory leak because MemoryManager works as a garbage collector, but decreases usable memory.
This PR also makes sure PlayHandle::m_processingLock's destructor is called.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Oct 15, 2017

Member

I tested it with the 'football theme' from the original issue, #3223, and it works fine.

Member

zonkmachine commented Oct 15, 2017

I tested it with the 'football theme' from the original issue, #3223, and it works fine.

Show outdated Hide outdated src/core/NotePlayHandle.cpp
@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Oct 16, 2017

Member

Now NotePlayHandle::release() will call parent classes' destructor correctly, always once. #1088 looks really bad for now...

Member

PhysSong commented Oct 16, 2017

Now NotePlayHandle::release() will call parent classes' destructor correctly, always once. #1088 looks really bad for now...

@PhysSong PhysSong changed the title from Fix missing destructor call in NotePlayHandle to Fix destructor call in NotePlayHandleManager Oct 16, 2017

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Oct 16, 2017

Member

Side note: NotePlayHandleManager seems not safe from ABA problem.
For example, s_available[ s_availableIndex.fetchAndAddOrdered( 1 ) + 1 ] = nph; isn't an atomic operation. It consists of two atomic operations: fetch-and-add s_availableIndex and store in s_available with the index. If a context switch occurs between them and some other operations are done, it may lead to wrong results.
Though it won't make a trouble frequently(since it is a rare case), this should be fixed later if it matters.

Member

PhysSong commented Oct 16, 2017

Side note: NotePlayHandleManager seems not safe from ABA problem.
For example, s_available[ s_availableIndex.fetchAndAddOrdered( 1 ) + 1 ] = nph; isn't an atomic operation. It consists of two atomic operations: fetch-and-add s_availableIndex and store in s_available with the index. If a context switch occurs between them and some other operations are done, it may lead to wrong results.
Though it won't make a trouble frequently(since it is a rare case), this should be fixed later if it matters.

@lukas-w lukas-w merged commit 2930ef6 into LMMS:stable-1.2 Oct 16, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@PhysSong PhysSong deleted the PhysSong:nphdestruct branch Oct 16, 2017

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Oct 17, 2017

Member

Cool destructor. You can even hear it working... . ;)
(turn the volume down and loop it)
destructor.mmp.zip

Member

zonkmachine commented Oct 17, 2017

Cool destructor. You can even hear it working... . ;)
(turn the volume down and loop it)
destructor.mmp.zip

@lukas-w

This comment has been minimized.

Show comment
Hide comment
@lukas-w

lukas-w Oct 17, 2017

Member

@zonkmachine That file stops playing for me after a couple of seconds both with and without this PR merged.

Edit: Bisecting shows f23cf4e to be the commit that introduced this, so it's a regression from #3783

Member

lukas-w commented Oct 17, 2017

@zonkmachine That file stops playing for me after a couple of seconds both with and without this PR merged.

Edit: Bisecting shows f23cf4e to be the commit that introduced this, so it's a regression from #3783

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Oct 17, 2017

Member

I bisected wrongly. Loosing my favourite game. :(

Member

zonkmachine commented Oct 17, 2017

I bisected wrongly. Loosing my favourite game. :(

@lukas-w

This comment has been minimized.

Show comment
Hide comment
@lukas-w

lukas-w Oct 17, 2017

Member

Don't worry, you'll have enough chances (a.k.a. "bugs") to play this game again. :)

Member

lukas-w commented Oct 17, 2017

Don't worry, you'll have enough chances (a.k.a. "bugs") to play this game again. :)

lukas-w added a commit that referenced this pull request Oct 17, 2017

PlayHandle: Zero out buffer before processing
Fixes buffer noises when instruments don't write the whole buffer, such as
bitinvader. Related:
* #3884 (comment): #3884 (comment)
* #3883
# #3383
@lukas-w

This comment has been minimized.

Show comment
Hide comment
@lukas-w

lukas-w Oct 17, 2017

Member

@zonkmachine I did some debugging and the error seems to be within bitinvader not zeroing the buffer, so I added a line in PlayHandle do just that and guard against instruments that don't (#3883 comes to mind). Can you confirm 6fc4577 fixes it?

Member

lukas-w commented Oct 17, 2017

@zonkmachine I did some debugging and the error seems to be within bitinvader not zeroing the buffer, so I added a line in PlayHandle do just that and guard against instruments that don't (#3883 comes to mind). Can you confirm 6fc4577 fixes it?

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Oct 17, 2017

Member

It's definitely an improvement. There is no longer a thunderclap coming out of the reverb. I still get a NaN and the noise cuts out though when moving the tempo slider while looping.

I ran it with the fpe debug and got the backtrace below. It reminds me of a comment here which also had a problem file with a bitinvader in it.

Program received signal SIGFPE, Arithmetic exception.
[Switching to Thread 0x7fffdddde700 (LWP 19879)]
0x000000000052499f in InstrumentSoundShaping::processAudioBuffer (this=0x7fffeab2aca0, buffer=0x7fffeab2f5e0, frames=256, n=0x7fffeab218e8)
    at /home/zonkmachine/builds/lmms/lmms/src/core/InstrumentSoundShaping.cpp:206
206					float new_cut_val = EnvelopeAndLfoParameters::expKnobVal( cutBuffer[frame] ) *
(gdb) bt
#0  0x000000000052499f in InstrumentSoundShaping::processAudioBuffer (this=0x7fffeab2aca0, buffer=0x7fffeab2f5e0, frames=256, n=0x7fffeab218e8)
    at /home/zonkmachine/builds/lmms/lmms/src/core/InstrumentSoundShaping.cpp:206
#1  0x000000000062b3ae in InstrumentTrack::processAudioBuffer (this=0x7fffeab29720, buf=0x7fffeab2f5e0, frames=256, n=0x7fffeab218e8)
    at /home/zonkmachine/builds/lmms/lmms/src/tracks/InstrumentTrack.cpp:203
#2  0x00007fffdc3b3704 in bitInvader::playNote (this=0x7fffeab2c5a0, _n=0x7fffeab218e8, _working_buffer=0x7fffeab2f5e0)
    at /home/zonkmachine/builds/lmms/lmms/plugins/bit_invader/bit_invader.cpp:301
#3  0x000000000062c41e in InstrumentTrack::playNote (this=0x7fffeab29720, n=0x7fffeab218e8, workingBuffer=0x7fffeab2f5e0)
    at /home/zonkmachine/builds/lmms/lmms/src/tracks/InstrumentTrack.cpp:483

(gdb) bt full
#0  0x000000000052499f in InstrumentSoundShaping::processAudioBuffer (this=0x7fffeab2aca0, buffer=0x7fffeab2f5e0, frames=256, n=0x7fffeab218e8)
    at /home/zonkmachine/builds/lmms/lmms/src/core/InstrumentSoundShaping.cpp:206
        new_cut_val = 6025
        fcv = 25
        cutBuffer = 0x7fffddddd6c0
        resBuffer = 0x7fffddddd2b0
        old_filter_cut = 6025
        old_filter_res = 0
        frv = 0,790000021
        envTotalFrames = 32316
        envReleaseBegin = -370
#1  0x000000000062b3ae in InstrumentTrack::processAudioBuffer (this=0x7fffeab29720, buf=0x7fffeab2f5e0, frames=256, n=0x7fffeab218e8)
    at /home/zonkmachine/builds/lmms/lmms/src/tracks/InstrumentTrack.cpp:203
        offset = 0
        vol = 2,35106045e-38
        pan = 0 '\000'
        vv = {vol = {-1,07653206e+26, 4,59163468e-41}}
        DefaultVolumeRatio = 0,00999999978
#2  0x00007fffdc3b3704 in bitInvader::playNote (this=0x7fffeab2c5a0, _n=0x7fffeab218e8, _working_buffer=0x7fffeab2f5e0)
    at /home/zonkmachine/builds/lmms/lmms/plugins/bit_invader/bit_invader.cpp:301
        frames = 256
        offset = 0
        ps = 0x7fffeab2fde0
Member

zonkmachine commented Oct 17, 2017

It's definitely an improvement. There is no longer a thunderclap coming out of the reverb. I still get a NaN and the noise cuts out though when moving the tempo slider while looping.

I ran it with the fpe debug and got the backtrace below. It reminds me of a comment here which also had a problem file with a bitinvader in it.

Program received signal SIGFPE, Arithmetic exception.
[Switching to Thread 0x7fffdddde700 (LWP 19879)]
0x000000000052499f in InstrumentSoundShaping::processAudioBuffer (this=0x7fffeab2aca0, buffer=0x7fffeab2f5e0, frames=256, n=0x7fffeab218e8)
    at /home/zonkmachine/builds/lmms/lmms/src/core/InstrumentSoundShaping.cpp:206
206					float new_cut_val = EnvelopeAndLfoParameters::expKnobVal( cutBuffer[frame] ) *
(gdb) bt
#0  0x000000000052499f in InstrumentSoundShaping::processAudioBuffer (this=0x7fffeab2aca0, buffer=0x7fffeab2f5e0, frames=256, n=0x7fffeab218e8)
    at /home/zonkmachine/builds/lmms/lmms/src/core/InstrumentSoundShaping.cpp:206
#1  0x000000000062b3ae in InstrumentTrack::processAudioBuffer (this=0x7fffeab29720, buf=0x7fffeab2f5e0, frames=256, n=0x7fffeab218e8)
    at /home/zonkmachine/builds/lmms/lmms/src/tracks/InstrumentTrack.cpp:203
#2  0x00007fffdc3b3704 in bitInvader::playNote (this=0x7fffeab2c5a0, _n=0x7fffeab218e8, _working_buffer=0x7fffeab2f5e0)
    at /home/zonkmachine/builds/lmms/lmms/plugins/bit_invader/bit_invader.cpp:301
#3  0x000000000062c41e in InstrumentTrack::playNote (this=0x7fffeab29720, n=0x7fffeab218e8, workingBuffer=0x7fffeab2f5e0)
    at /home/zonkmachine/builds/lmms/lmms/src/tracks/InstrumentTrack.cpp:483

(gdb) bt full
#0  0x000000000052499f in InstrumentSoundShaping::processAudioBuffer (this=0x7fffeab2aca0, buffer=0x7fffeab2f5e0, frames=256, n=0x7fffeab218e8)
    at /home/zonkmachine/builds/lmms/lmms/src/core/InstrumentSoundShaping.cpp:206
        new_cut_val = 6025
        fcv = 25
        cutBuffer = 0x7fffddddd6c0
        resBuffer = 0x7fffddddd2b0
        old_filter_cut = 6025
        old_filter_res = 0
        frv = 0,790000021
        envTotalFrames = 32316
        envReleaseBegin = -370
#1  0x000000000062b3ae in InstrumentTrack::processAudioBuffer (this=0x7fffeab29720, buf=0x7fffeab2f5e0, frames=256, n=0x7fffeab218e8)
    at /home/zonkmachine/builds/lmms/lmms/src/tracks/InstrumentTrack.cpp:203
        offset = 0
        vol = 2,35106045e-38
        pan = 0 '\000'
        vv = {vol = {-1,07653206e+26, 4,59163468e-41}}
        DefaultVolumeRatio = 0,00999999978
#2  0x00007fffdc3b3704 in bitInvader::playNote (this=0x7fffeab2c5a0, _n=0x7fffeab218e8, _working_buffer=0x7fffeab2f5e0)
    at /home/zonkmachine/builds/lmms/lmms/plugins/bit_invader/bit_invader.cpp:301
        frames = 256
        offset = 0
        ps = 0x7fffeab2fde0
@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Oct 17, 2017

Member

@zonkmachine It looks like #3775.

Member

PhysSong commented Oct 17, 2017

@zonkmachine It looks like #3775.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Oct 17, 2017

Member

Yes, that looks like it.

Member

zonkmachine commented Oct 17, 2017

Yes, that looks like it.

@lukas-w

This comment has been minimized.

Show comment
Hide comment
@lukas-w

lukas-w Oct 18, 2017

Member

@zonkmachine So I guess it's not related to this PR or #3783?

Member

lukas-w commented Oct 18, 2017

@zonkmachine So I guess it's not related to this PR or #3783?

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Oct 18, 2017

Member

Yes, job done here.

Member

zonkmachine commented Oct 18, 2017

Yes, job done here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment