Skip to content

Fix "out of buffers" crash #3783

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.

Sign up for GitHub

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

Merged
lukas-w merged 9 commits into from
Sep 26, 2017
Merged

Fix "out of buffers" crash #3783

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b6421d4
Reimplement BufferManager
lukas-w Aug 29, 2017
6b557d2
Find Boost in CMake, fix Travis
lukas-w Aug 29, 2017
c11d12f
Remove BufferManager implementation
lukas-w Aug 30, 2017
9c91079
Revert "Find Boost in CMake, fix Travis"
lukas-w Aug 30, 2017
73728d7
Fix C++03 compatibility
lukas-w Aug 30, 2017
aebbe22
Fix code formatting inconsistencies
lukas-w Sep 17, 2017
70ac7db
Fix code formatting inconsistencies
lukas-w Sep 17, 2017
d1aa39b
Fix C++03 compatibility again
lukas-w Sep 17, 2017
93e4bd7
Fix conventions
PhysSong Sep 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion include/MemoryManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ struct MmAllocator

T* allocate(std::size_t n)
{
return reinterpret_cast<T*>( MemoryManager::alloc( n ) );
return reinterpret_cast<T*>( MemoryManager::alloc( sizeof(T) * n ) );
}

void deallocate(T* p, std::size_t)
Expand All @@ -120,6 +120,7 @@ struct MmAllocator
}
};

Copy link
Member

@PhysSong PhysSong Sep 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

These parts aren't used. Are there some reasons you leave them in our codebase?
Some more inconsistencies: you mixed < foo > and <foo>, it makes the code look inconsistent.
One more thing(minor): if you are going to use this later, I think vector is not very good name. If you want to keep the name, of course you may do.

Copy link
Member Author

@lukas-w lukas-w Sep 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Are there some reasons you leave them in our codebase?

I plan on using them at a later point in time. Using an Allocator class is the standard C++ way of having custom allocators, and should be preferred over using the macros MM_ALLOC and MM_FREE.

you mixed < foo > and , it makes the code look inconsistent.

This has already been discussed by @Umcaruje and me in the comments here. C++03 doesn't allow >> for nested template arguments (it always interprets it as the stream right shift operator >>). We can't use >> here, because the headers is included by plugins that are compiled without C++11 support. See d1aa39b.

Copy link
Member Author

@lukas-w lukas-w Sep 19, 2017

Choose a reason for hiding this comment

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

if you are going to use this later, I think vector is not very good name

Well it's a vector, so why not call it vector? What would you call it?

Copy link
Member

@PhysSong PhysSong Sep 19, 2017

Choose a reason for hiding this comment

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

You're right, but you used both template<typename T> and template< class U > above. Is that intentional?

Copy link
Member Author

@lukas-w lukas-w Sep 19, 2017

Choose a reason for hiding this comment

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

Ah, no that's not intentional.

template <typename T> using MmVector = std::vector<T, MmAllocator<T>>;

#define MM_OPERATORS \
public: \
Expand Down
11 changes: 5 additions & 6 deletions include/PlayHandle.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
#include <QtCore/QList>
#include <QtCore/QMutex>

#include "MemoryManager.h"

#include "ThreadableJob.h"
#include "lmms_basics.h"

Expand Down Expand Up @@ -142,20 +144,17 @@ class PlayHandle : public ThreadableJob

void releaseBuffer();

sampleFrame * buffer()
{
return m_playHandleBuffer;
}
sampleFrame * buffer();

private:
Type m_type;
f_cnt_t m_offset;
QThread* m_affinity;
QMutex m_processingLock;
sampleFrame * m_playHandleBuffer;
sampleFrame* m_playHandleBuffer;
bool m_bufferReleased;
bool m_usesBuffer;
AudioPort * m_audioPort;

} ;


Expand Down
52 changes: 3 additions & 49 deletions src/core/BufferManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,68 +30,24 @@
#include "Mixer.h"
#include "MemoryManager.h"

#include <boost/lockfree/stack.hpp>


namespace {
using namespace boost::lockfree;

static const int BM_INITIAL_BUFFERS = 1024;
static const int BM_INCREMENT = 64;

static stack<sampleFrame*
,allocator<MmAllocator<void>>
> s_available;

static fpp_t framesPerPeriod;

void extend( size_t c )
{
s_available.reserve(c);

size_t cc = c * framesPerPeriod;

sampleFrame * b = MM_ALLOC( sampleFrame, cc );

for( size_t i = 0; i < c; ++i )
{
s_available.push(b);
b += framesPerPeriod;
}
}

}



void BufferManager::init( fpp_t framesPerPeriod )
{
::framesPerPeriod = framesPerPeriod;
extend(BM_INITIAL_BUFFERS);
}


sampleFrame * BufferManager::acquire()
{
sampleFrame* b;
while (! s_available.pop(b))
{
qWarning( "BufferManager: out of buffers" );
extend(BM_INCREMENT);
}

//qDebug( "acquired buffer: %p - index %d", b, i );
return b;
return MM_ALLOC(sampleFrame, ::framesPerPeriod);
}


void BufferManager::clear( sampleFrame * ab, const f_cnt_t frames,
const f_cnt_t offset )
void BufferManager::clear(sampleFrame *ab, const f_cnt_t frames, const f_cnt_t offset)
{
memset( ab + offset, 0, sizeof( *ab ) * frames );
}


#ifndef LMMS_DISABLE_SURROUND
void BufferManager::clear( surroundSampleFrame * ab, const f_cnt_t frames,
const f_cnt_t offset )
Expand All @@ -103,8 +59,6 @@ void BufferManager::clear( surroundSampleFrame * ab, const f_cnt_t frames,

void BufferManager::release( sampleFrame * buf )
{
if (buf == nullptr) return;
s_available.push(buf);
//qDebug( "released buffer: %p - index %d", buf, i );
MM_FREE(buf);
Copy link
Member

@Umcaruje Umcaruje Sep 17, 2017

Choose a reason for hiding this comment

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

I'm a nitpick, but space missing here too. Thanks for fixing the other lines.

}

31 changes: 20 additions & 11 deletions src/core/PlayHandle.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,23 @@
*
*/

#include "PlayHandle.h"
#include "BufferManager.h"
#include "Engine.h"
#include "Mixer.h"
#include "PlayHandle.h"
Copy link
Member

@PhysSong PhysSong Sep 19, 2017

Choose a reason for hiding this comment

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

Why this line have been moved from above? It should stay above #include "BufferManager.h" according to our convention, and that way seems more consistent for me.

Copy link
Member Author

@lukas-w lukas-w Sep 19, 2017

Choose a reason for hiding this comment

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

No reason, probably an unintentional change.


#include <QtCore/QThread>
#include <QDebug>

#include <iterator>

PlayHandle::PlayHandle( const Type type, f_cnt_t offset ) :
m_type( type ),
m_offset( offset ),
m_affinity( QThread::currentThread() ),
m_playHandleBuffer( NULL ),
m_usesBuffer( true )
PlayHandle::PlayHandle(const Type type, f_cnt_t offset) :
m_type(type),
m_offset(offset),
m_affinity(QThread::currentThread()),
m_playHandleBuffer(BufferManager::acquire()),
m_bufferReleased(true),
m_usesBuffer(true)
{
}

Expand All @@ -48,8 +53,8 @@ void PlayHandle::doProcessing()
{
if( m_usesBuffer )
{
if( ! m_playHandleBuffer ) m_playHandleBuffer = BufferManager::acquire();
play( m_playHandleBuffer );
m_bufferReleased = false;
play( buffer() );
}
else
{
Expand All @@ -60,6 +65,10 @@ void PlayHandle::doProcessing()

void PlayHandle::releaseBuffer()
{
BufferManager::release( m_playHandleBuffer );
Copy link
Member

@PhysSong PhysSong Oct 15, 2017

Choose a reason for hiding this comment

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

@lukas-w Why did you move this to destructor?

Copy link
Member Author

@lukas-w lukas-w Oct 15, 2017

Choose a reason for hiding this comment

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

@PhysSong Because I moved buffer allocation to the constructor. The buffer's lifetime matches the PlayHandle's lifetime now, instead of being allocated everytime doProcessing is called.

m_playHandleBuffer = NULL;
m_bufferReleased = true;
}

sampleFrame* PlayHandle::buffer()
{
return m_bufferReleased ? nullptr : reinterpret_cast<sampleFrame*>(m_playHandleBuffer);
};
8 changes: 3 additions & 5 deletions src/core/audio/AudioPort.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ AudioPort::AudioPort( const QString & _name, bool _has_effect_chain,
FloatModel * volumeModel, FloatModel * panningModel,
BoolModel * mutedModel ) :
m_bufferUsage( false ),
m_portBuffer( NULL ),
m_portBuffer( BufferManager::acquire() ),
m_extOutputEnabled( false ),
m_nextFxChannel( 0 ),
m_name( "unnamed port" ),
Expand All @@ -57,6 +57,7 @@ AudioPort::~AudioPort()
setExtOutputEnabled( false );
Engine::mixer()->removeAudioPort( this );
delete m_effects;
BufferManager::release( m_portBuffer );
}


Expand Down Expand Up @@ -110,8 +111,7 @@ void AudioPort::doProcessing()

const fpp_t fpp = Engine::mixer()->framesPerPeriod();

// get a buffer for processing and clear it
m_portBuffer = BufferManager::acquire();
// clear the buffer
BufferManager::clear( m_portBuffer, fpp );

//qDebug( "Playhandles: %d", m_playHandles.size() );
Expand Down Expand Up @@ -225,8 +225,6 @@ void AudioPort::doProcessing()
// TODO: improve the flow here - convert to pull model
m_bufferUsage = false;
}

BufferManager::release( m_portBuffer ); // release buffer, we don't need it anymore
}


Expand Down