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

Fix "out of buffers" crash #3783

Merged
merged 9 commits into from
Sep 26, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
2 changes: 1 addition & 1 deletion .travis/linux..install.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
PACKAGES="cmake libsndfile-dev fftw3-dev libvorbis-dev libogg-dev libmp3lame-dev
libasound2-dev libjack-dev libsdl-dev libsamplerate0-dev libstk0-dev
libfluidsynth-dev portaudio19-dev wine-dev g++-multilib libfltk1.3-dev
libgig-dev libsoundio-dev"
libgig-dev libsoundio-dev libboost-dev"

# Help with unmet dependencies
PACKAGES="$PACKAGES libjack0"
Expand Down
2 changes: 1 addition & 1 deletion .travis/osx..install.sh
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#!/usr/bin/env bash

PACKAGES="cmake pkgconfig fftw libogg libvorbis lame libsndfile libsamplerate jack sdl libgig libsoundio stk portaudio node fltk"
PACKAGES="cmake pkgconfig boost fftw libogg libvorbis lame libsndfile libsamplerate jack sdl libgig libsoundio stk portaudio node fltk"

if [ $QT5 ]; then
PACKAGES="$PACKAGES homebrew/versions/qt55"
Expand Down
4 changes: 4 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,10 @@ ELSE()
INCLUDE("${QT_USE_FILE}")
ENDIF()

# check for boost
FIND_PACKAGE(Boost 1.54.0 REQUIRED)
INCLUDE_DIRECTORIES(${Boost_INCLUDE_DIRS})

# check for libsndfile
PKG_CHECK_MODULES(SNDFILE REQUIRED sndfile>=1.0.11)
IF(NOT SNDFILE_FOUND)
Expand Down
14 changes: 0 additions & 14 deletions include/BufferManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,6 @@
#include "export.h"
#include "lmms_basics.h"

const int BM_INITIAL_BUFFERS = 512;
//const int BM_INCREMENT = 64;

class EXPORT BufferManager
{
public:
Expand All @@ -46,17 +43,6 @@ class EXPORT BufferManager
const f_cnt_t offset = 0 );
#endif
static void release( sampleFrame * buf );
static void refresh();
// static void extend( int c );

private:
static sampleFrame ** s_available;
static AtomicInt s_availableIndex;

static sampleFrame ** s_released;
static AtomicInt s_releasedIndex;
// static QReadWriteLock s_mutex;
static int s_size;
};

#endif
2 changes: 1 addition & 1 deletion include/MemoryHelper.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
class MemoryHelper {
public:

static void* alignedMalloc( int );
static void* alignedMalloc(size_t );
Copy link
Member

Choose a reason for hiding this comment

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

inconsistent spaces inside the bracket here


static void alignedFree( void* );

Expand Down
26 changes: 22 additions & 4 deletions include/MemoryManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ struct MemoryPool
{
void * m_pool;
char * m_free;
int m_chunks;
size_t m_chunks;
QMutex m_mutex;

MemoryPool() :
Expand All @@ -51,10 +51,10 @@ struct MemoryPool
m_chunks( 0 )
{}

MemoryPool( int chunks ) :
MemoryPool( size_t chunks ) :
m_chunks( chunks )
{
m_free = (char*) MemoryHelper::alignedMalloc( chunks );
m_free = reinterpret_cast<char*>(MemoryHelper::alignedMalloc( chunks ));
Copy link
Member

Choose a reason for hiding this comment

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

same inconsistency here, chunks should have spaces removed

memset( m_free, 1, chunks );
}

Expand Down Expand Up @@ -103,6 +103,24 @@ class EXPORT MemoryManager
static QMutex s_pointerMutex;
};

template<typename T>
struct MmAllocator
{
typedef T value_type;
template< class U > struct rebind { typedef MmAllocator<U> other; };

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

void deallocate(T* p, std::size_t)
{
MemoryManager::free( p );
}
};

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.

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

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

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

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

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 All @@ -124,7 +142,7 @@ static void operator delete[] ( void * ptr ) \
}

// for use in cases where overriding new/delete isn't a possibility
#define MM_ALLOC( type, count ) (type*) MemoryManager::alloc( sizeof( type ) * count )
#define MM_ALLOC( type, count ) reinterpret_cast<type*>(MemoryManager::alloc( sizeof( type ) * count ))
// and just for symmetry...
#define MM_FREE( ptr ) MemoryManager::free( ptr )

Expand Down
11 changes: 5 additions & 6 deletions include/PlayHandle.h
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
80 changes: 8 additions & 72 deletions src/core/BufferManager.cpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/*
* BufferManager.cpp - A buffer caching/memory management system
*
* Copyright (c) 2017 Lukas W <lukaswhl/at/gmail.com>
* Copyright (c) 2014 Vesa Kivimäki <contact/dot/diizy/at/nbl/dot/fi>
* Copyright (c) 2006-2014 Tobias Doerffel <tobydox/at/users.sourceforge.net>
*
Expand All @@ -25,56 +26,28 @@

#include "BufferManager.h"

#include "Engine.h"
#include "Mixer.h"
#include "MemoryManager.h"

sampleFrame ** BufferManager::s_available;
AtomicInt BufferManager::s_availableIndex = 0;
sampleFrame ** BufferManager::s_released;
AtomicInt BufferManager::s_releasedIndex = 0;
//QReadWriteLock BufferManager::s_mutex;
int BufferManager::s_size;

static fpp_t framesPerPeriod;

void BufferManager::init( fpp_t framesPerPeriod )
{
s_available = MM_ALLOC( sampleFrame*, BM_INITIAL_BUFFERS );
s_released = MM_ALLOC( sampleFrame*, BM_INITIAL_BUFFERS );

int c = framesPerPeriod * BM_INITIAL_BUFFERS;
sampleFrame * b = MM_ALLOC( sampleFrame, c );

for( int i = 0; i < BM_INITIAL_BUFFERS; ++i )
{
s_available[ i ] = b;
b += framesPerPeriod;
}
s_availableIndex = BM_INITIAL_BUFFERS - 1;
s_size = BM_INITIAL_BUFFERS;
::framesPerPeriod = framesPerPeriod;
}


sampleFrame * BufferManager::acquire()
{
if( s_availableIndex < 0 )
{
qFatal( "BufferManager: out of buffers" );
}

int i = s_availableIndex.fetchAndAddOrdered( -1 );
sampleFrame * b = s_available[ i ];

//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 @@ -86,43 +59,6 @@ void BufferManager::clear( surroundSampleFrame * ab, const f_cnt_t frames,

void BufferManager::release( sampleFrame * buf )
{
if (buf == nullptr) return;
int i = s_releasedIndex.fetchAndAddOrdered( 1 );
s_released[ i ] = buf;
//qDebug( "released buffer: %p - index %d", buf, i );
MM_FREE(buf);
Copy link
Member

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.

}


void BufferManager::refresh() // non-threadsafe, hence it's called periodically from mixer at a time when no other threads can interfere
{
if( s_releasedIndex == 0 ) return;
//qDebug( "refresh: %d buffers", int( s_releasedIndex ) );

int j = s_availableIndex;
for( int i = 0; i < s_releasedIndex; ++i )
{
++j;
s_available[ j ] = s_released[ i ];
}
s_availableIndex = j;
s_releasedIndex = 0;
}


/* // non-extensible for now
void BufferManager::extend( int c )
{
s_size += c;
sampleFrame ** tmp = MM_ALLOC( sampleFrame*, s_size );
MM_FREE( s_available );
s_available = tmp;

int cc = c * Engine::mixer()->framesPerPeriod();
sampleFrame * b = MM_ALLOC( sampleFrame, cc );

for( int i = 0; i < c; ++i )
{
s_available[ s_availableIndex.fetchAndAddOrdered( 1 ) + 1 ] = b;
b += Engine::mixer()->framesPerPeriod();
}
}*/
2 changes: 1 addition & 1 deletion src/core/MemoryHelper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
* Allocate a number of bytes and return them.
* @param byteNum is the number of bytes
*/
void* MemoryHelper::alignedMalloc( int byteNum )
void* MemoryHelper::alignedMalloc( size_t byteNum )
{
char *ptr, *ptr2, *aligned_ptr;
int align_mask = ALIGN_SIZE - 1;
Expand Down
3 changes: 0 additions & 3 deletions src/core/Mixer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -482,9 +482,6 @@ const surroundSampleFrame * Mixer::renderNextBuffer()
Controller::triggerFrameCounter();
AutomatableModel::incrementPeriodCounter();

// refresh buffer pool
BufferManager::refresh();

s_renderingThread = false;

m_profiler.finishPeriod( processingSampleRate(), m_framesPerPeriod );
Expand Down
31 changes: 20 additions & 11 deletions src/core/PlayHandle.cpp
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

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

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

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

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
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