Looks great, thanks for the clarification! Go ahead and commit it... :) Albert
On Fri, Oct 16, 2009 at 4:13 PM, mad jester <[email protected]> wrote: > 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------ > ------------------------------------------------------------------------------ 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
