Here are some comments on your comments, as well as a new patch.
> - encodermp3.cpp:88 - look in encoderBuffer().
> I see lots of bufferIn/OutGrow() calls, but nothing that's deleting
> these buffers or ever shrinking them again. Is this a giant memory
> leak? It looks like you're growing your buffers every single time
> encodeBuffer() is called....
The buffers only grow when they are too small to accommodate 'size'.
I took the liberty of just calling the function instead of doing the
check each time because that way the code looks simpler and all the
buffer logic is left to the growth functions.
> - encodeBuffer() again. I see a couple of fun lines of code like:
> outsize = (int)((1.25 * size + 7200) + 1);
> and
> m_bufferOutSize = (int)((1.25 * 20000 + 7200) + 1); in initStream()
>
> 1) Can you please comment these lines of code? What are these
> constants in there? (You commented the 20000 in your patch but not the
> 7200 or the 1.25.)
>
> 2) Why do we need growing buffers at all instead of just static buffers?
We'd need to know how large the buffer enginesidechain uses, and
when it changes. This method is more future proof. This code should
not allocate or grow the buffers more than once or twice.
> 3) In bufferIn/OutGrow(), you're using sizeof(float), but you should
> be using sizeof(CSAMPLE) for consistency.
FIXED
> Other stuff from your patch:
> - delete encoder;
> + if ( m_pEncoder )
> + delete m_pEncoder;
> +
> ----> In C++, "delete" does nothing if the pointer is null, so it is
> unnecessary to add that if statement. (Google it if you don't believe
> me.)
FIXED
> Lastly, to help us integrate Shoutcast stuff into the GUI, you may
> want to add some Qt signals to EngineShoutcast like connected(),
> disconnected(), etc.
I plan on leaving that for when features_looping is already merged.
The less code I commit to trunk the better, I think.
PS: thread tried out the previous patch and it worked perfectly. No
crashes at all in his entire set.
--
-----BEGIN GEEK CODE BLOCK-----
Version: 3.1
GCS d--@>+ s(+):(-) a- C++(++++)$ ULC+++(++++) P+ L+++ E-() W++ N o? K
w-- O? !M V PS+(+++) PE(-) Y+ !PGP !t !5 X(+) !R tv+ b++ DI+ D+ G e>++
h r y+
------END GEEK CODE BLOCK------
=== modified file 'mixxx/src/encodermp3.cpp'
--- mixxx/src/encodermp3.cpp 2009-02-18 04:28:21 +0000
+++ mixxx/src/encodermp3.cpp 2009-10-16 22:28:42 +0000
@@ -32,12 +32,26 @@
if (engine) pEngine = engine;
metaDataTitle = metaDataArtist = "";
m_pConfig = _config;
+
+ // Set the buffers to NULL values. The First buffer growth allocates them.
+
+ m_bufferOutSize = 0;
+ m_bufferOut = NULL;
+
+ m_bufferInSize = 0;
+ m_bufferIn[0] = (float *)NULL;
+ m_bufferIn[1] = (float *)NULL;
}
// Destructor
EncoderMp3::~EncoderMp3()
{
flushStream();
+
+ if (m_bufferOut) free(m_bufferOut);
+ if (m_bufferIn[0]) free(m_bufferIn[0]);
+ if (m_bufferIn[1]) free(m_bufferIn[1]);
+
lame_close(m_lameFlags);
}
@@ -47,14 +61,22 @@
int EncoderMp3::bufferOutGrow(int size)
{
- if ( m_bufferOutSize >= size )
+ // This calculation is based on example LAME code and factors in the worst
+ // case scenarios for MP3 encoding. Times the size by 1.25 to allow for
+ // overhead in the encoder. LAME recommends 7200 bytes for headers, so we
+ // add that too.
+
+ int osize = (int)((1.25 * size + 7200));
+
+
+ if ( m_bufferOutSize >= osize )
return 0;
- m_bufferOut = (unsigned char *)realloc(m_bufferOut, size);
+ m_bufferOut = (unsigned char *)realloc(m_bufferOut, osize);
if ( m_bufferOut == NULL )
return -1;
- m_bufferOutSize = size;
+ m_bufferOutSize = osize;
return 0;
}
@@ -67,8 +89,9 @@
if ( m_bufferInSize >= size )
return 0;
- m_bufferIn[0] = (float *)realloc(m_bufferIn[0], size * sizeof(float));
- m_bufferIn[1] = (float *)realloc(m_bufferIn[1], size * sizeof(float));
+ m_bufferIn[0] = (float *)realloc(m_bufferIn[0], size * sizeof(CSAMPLE));
+ m_bufferIn[1] = (float *)realloc(m_bufferIn[1], size * sizeof(CSAMPLE));
+
if ((m_bufferIn[0] == NULL) || (m_bufferIn[1] == NULL))
return -1;
@@ -92,9 +115,7 @@
int i;
- outsize = (int)((1.25 * size + 7200) + 1);
bufferOutGrow(outsize);
-
bufferInGrow(size);
// Deinterleave samples
@@ -114,11 +135,10 @@
void EncoderMp3::initStream()
{
- m_bufferOutSize = (int)((1.25 * 20000 + 7200) + 1);
- m_bufferOut = (unsigned char *)malloc(m_bufferOutSize);
+ // We Assume a buffer of 20000 samples as the first default.
- m_bufferIn[0] = (float *)malloc(m_bufferOutSize * sizeof(float));
- m_bufferIn[1] = (float *)malloc(m_bufferOutSize * sizeof(float));
+ bufferOutGrow(20000);
+ bufferInGrow(20000);
return;
}
=== modified file 'mixxx/src/engine/engineshoutcast.cpp'
--- mixxx/src/engine/engineshoutcast.cpp 2009-10-15 00:39:09 +0000
+++ mixxx/src/engine/engineshoutcast.cpp 2009-10-16 21:40:28 +0000
@@ -43,15 +43,13 @@
m_iShoutStatus = 0;
m_pConfig = _config;
m_pUpdateShoutcastFromPrefs = new ControlObjectThreadMain(ControlObject::getControl(ConfigKey(SHOUTCAST_PREF_KEY, "update_from_prefs")));
+ m_pEncoder = NULL;
+
m_pCrossfader = new ControlObjectThread(ControlObject::getControl(ConfigKey("[Master]","crossfader")));
m_pVolume1 = new ControlObjectThread(ControlObject::getControl(ConfigKey("[Channel1]","volume")));
m_pVolume2 = new ControlObjectThread(ControlObject::getControl(ConfigKey("[Channel2]","volume")));
- QByteArray baBitrate = m_pConfig->getValueString(ConfigKey(SHOUTCAST_PREF_KEY,"bitrate")).toLatin1();
- QByteArray baFormat = m_pConfig->getValueString(ConfigKey(SHOUTCAST_PREF_KEY,"format")).toLatin1();
- int len;
-
// Initialize libshout
shout_init();
@@ -77,40 +75,7 @@
if ( !serverConnect())
return;
-
qDebug("********SERVERCONNECTED********");
-
-
- if (( len = baBitrate.indexOf(' ')) != -1) {
- baBitrate.resize(len);
- }
-
- // Initialize encoder
- if ( ! qstrcmp(baFormat, "MP3")) {
-#ifdef __SHOUTCAST_LAME__
- encoder = new EncoderMp3(m_pConfig, this);
-#else
- qDebug() << "*** Missing MP3 Encoder Support";
- return;
-#endif // __SHOUTCAST_LAME__
- }
- else if ( ! qstrcmp(baFormat, "Ogg Vorbis")) {
-#ifdef __SHOUTCAST_VORBIS__
- encoder = new EncoderVorbis(m_pConfig, this);
-#else
- qDebug() << "*** Missing OGG Vorbis Encoder Support";
- return;
-#endif // __SHOUTCAST_VORBIS__
- }
- else {
- qDebug() << "**** Unknown Encoder Format";
- return;
- }
-
-
- if (encoder->initEncoder(baBitrate.toInt()) < 0) {
- qDebug() << "**** Vorbis init failed";
- }
}
/*
@@ -118,7 +83,7 @@
*/
EngineShoutcast::~EngineShoutcast()
{
- delete encoder;
+ delete m_pEncoder;
delete m_pUpdateShoutcastFromPrefs;
delete m_pCrossfader;
delete m_pVolume1;
@@ -160,16 +125,46 @@
int protocol;
+ if ( ! qstricmp(baServerType.data(), "Icecast 2")) {
+ protocol = SHOUT_PROTOCOL_HTTP;
+ } else if ( ! qstricmp(baServerType.data(), "Shoutcast")) {
+ protocol = SHOUT_PROTOCOL_ICY;
+ } else if ( ! qstricmp(baServerType.data(), "Icecast 1")) {
+ protocol = SHOUT_PROTOCOL_XAUDIOCAST;
+ } else {
+ qDebug() << "Error: unknown server protocol:" << baServerType.data();
+ return;
+ }
+
+
+ if ( !qstrcmp(baFormat.data(), "MP3")) {
+ format = SHOUT_FORMAT_MP3;
+ }
+ else if ( !qstrcmp(baFormat.data(), "Ogg Vorbis")) {
+ format = SHOUT_FORMAT_OGG;
+ }
+ else {
+ qDebug() << "Error: unknown format:" << baFormat.data();
+ return;
+ }
+
+
+ if (( protocol == SHOUT_PROTOCOL_ICY ) && ( format != SHOUT_FORMAT_MP3)) {
+ qDebug() << "Error: libshout only supports Shoutcast With MP3 format";
+ return;
+ }
+
+
+ if ( shout_set_protocol(m_pShout, protocol) != SHOUTERR_SUCCESS) {
+ qDebug() << "Error setting protocol: " << shout_get_error(m_pShout);
+ return;
+ }
+
if (shout_set_host(m_pShout, baHost.data()) != SHOUTERR_SUCCESS) {
qDebug() << "Error setting hostname:" << shout_get_error(m_pShout);
return;
}
-
- if (shout_set_protocol(m_pShout, SHOUT_PROTOCOL_HTTP) != SHOUTERR_SUCCESS) {
- qDebug() << "Error setting protocol:" << shout_get_error(m_pShout);
- return;
- }
-
+
if (shout_set_port(m_pShout, baPort.toUInt()) != SHOUTERR_SUCCESS) {
qDebug() << "Error setting port:" << shout_get_error(m_pShout);
return;
@@ -190,51 +185,59 @@
}
- if ( !qstrcmp(baFormat.data(), "MP3")) {
- format = SHOUT_FORMAT_MP3;
- }
- else if ( !qstrcmp(baFormat.data(), "Ogg Vorbis")) {
- format = SHOUT_FORMAT_OGG;
- }
- else {
- qDebug() << "Error: unknown format:" << baFormat.data();
- return;
- }
-
+ if ((len = baBitrate.indexOf(' ')) != -1) {
+ baBitrate.resize(len);
+ }
+
+ const char * sbrate = shout_get_audio_info(m_pShout, SHOUT_AI_BITRATE);
+ int brate = (sbrate != NULL ? atoi(sbrate) : 0);
+
+
+ if ((shout_get_format(m_pShout) != format) || (brate != baBitrate.toInt())) {
+
+ if ( m_pEncoder )
+ delete m_pEncoder;
+
+ switch(format)
+ {
+ case SHOUT_FORMAT_MP3:
+#ifdef __SHOUTCAST_LAME__
+ m_pEncoder = new EncoderMp3(m_pConfig, this);
+ break;
+#else
+ qDebug() << "*** Missing MP3 Encoder Support";
+ return;
+#endif // __SHOUTCAST_LAME__
+ case SHOUT_FORMAT_OGG:
+#ifdef __SHOUTCAST_VORBIS__
+ m_pEncoder = new EncoderVorbis(m_pConfig, this);
+ break;
+#else
+ qDebug() << "*** Missing OGG Vorbis Encoder Support";
+ return;
+#endif // __SHOUTCAST_VORBIS__
+ default:
+ qDebug() << "*** UKNOWN FORMAT:" << format;
+ return;
+ }
+
+ if (m_pEncoder->initEncoder(baBitrate.toInt()) < 0) {
+ qDebug() << "**** Encoder init failed";
+ return;
+ }
+ }
+
+
if (shout_set_format(m_pShout, format) != SHOUTERR_SUCCESS) {
qDebug() << "Error setting format:" << shout_get_error(m_pShout);
return;
}
-
- if ((len = baBitrate.indexOf(' ')) != -1) {
- baBitrate.resize(len);
- }
-
if (shout_set_audio_info(m_pShout, SHOUT_AI_BITRATE, baBitrate.data()) != SHOUTERR_SUCCESS) {
qDebug() << "Error setting bitrate:" << shout_get_error(m_pShout);
return;
}
- if ( ! qstricmp(baServerType.data(), "Icecast 2")) {
- protocol = SHOUT_PROTOCOL_HTTP;
- } else if ( ! qstricmp(baServerType.data(), "Shoutcast")) {
- protocol = SHOUT_PROTOCOL_ICY;
- } else if ( ! qstricmp(baServerType.data(), "Icecast 1")) {
- protocol = SHOUT_PROTOCOL_XAUDIOCAST;
- } else {
- qDebug() << "Error: unknown server protocol:" << baServerType.data();
- return;
- }
-
- if (( protocol == SHOUT_PROTOCOL_ICY ) && ( format != SHOUT_FORMAT_MP3)) {
- qDebug() << "Error: libshout only supports Shoutcast With MP3 format";
- }
-
- if ( shout_set_protocol(m_pShout, protocol) != SHOUTERR_SUCCESS) {
- qDebug() << "Error setting protocol: " << shout_get_error(m_pShout);
- return;
- }
}
@@ -268,14 +271,14 @@
break;
m_iShoutFailures++;
- sleep(30);
+ sleep(5);
}
m_iShoutFailures = 0;
while (m_iShoutStatus == SHOUTERR_BUSY) {
- qDebug() << "Connection pending. Sleeping...";
+ qDebug() << "Connection pending. Sleeping... " << m_iShoutStatus;
sleep(1);
m_iShoutStatus = shout_get_connected(m_pShout);
}
@@ -283,6 +286,9 @@
qDebug() << "***********Connected to Shoutcast server...";
return true;
}
+ else {
+ qDebug() << "Error connecting to Shoutcast server:" << shout_get_error(m_pShout);
+ }
return false;
}
@@ -342,7 +348,7 @@
if (m_iShoutStatus != SHOUTERR_CONNECTED)
return;
- if (iBufferSize > 0) encoder->encodeBuffer(pOut, iBufferSize);
+ if (iBufferSize > 0) m_pEncoder->encodeBuffer(pOut, iBufferSize);
if (metaDataHasChanged())
updateMetaData();
=== modified file 'mixxx/src/engine/engineshoutcast.h'
--- mixxx/src/engine/engineshoutcast.h 2009-07-10 22:30:31 +0000
+++ mixxx/src/engine/engineshoutcast.h 2009-10-15 20:43:42 +0000
@@ -63,7 +63,7 @@
long m_iShoutFailures;
ConfigObject<ConfigValue> *m_pConfig;
ControlObject* recReady;
- Encoder *encoder;
+ Encoder *m_pEncoder;
ControlObjectThreadMain* m_pUpdateShoutcastFromPrefs;
// void (*writeFn)(unsigned char *, unsigned char *, int, int);
ControlObjectThread* m_pCrossfader;
------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
_______________________________________________
Mixxx-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/mixxx-devel
