Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am writing a networked audio application that sends the audio in 320 byte chunks, later I need to provide the player a bigger amount of bytes to play so I try to merge 2 or more chunks into one buffer and return it, well the code works but I hear some small audio artifacts, any ideas if the code is correct?

-(NSData*) getRawBufferWithSize:(int) requestedBufferSize
{
    if(audio.isBuffering && audio.bufferTimer>[[NSDate date] timeIntervalSince1970] && !AUDIO_BYPASS_NET)
    {
        return nil;
    }
    audio.isBuffering = NO;
    int readyBufferSize = 0;
    int bytesPending = requestedBufferSize;

    if(returnBufferSize<requestedBufferSize)
    {
        returnBuffer = (char*)realloc(returnBuffer, requestedBufferSize);
        returnBufferSize = requestedBufferSize;
    }

    while (readyBufferSize<requestedBufferSize)
    {
        NSData* newBuffer;
        @synchronized(playbackRawBuffer)
        {
            if([playbackRawBuffer count] != 0)
            {
                newBuffer = [[playbackRawBuffer objectAtIndex:0] retain];
                [playbackRawBuffer removeObjectAtIndex:0];
            }
            else
            {
                break;
            }
        }        
        char* bytes = (char*)[newBuffer bytes];

        int readableBytes = newBuffer.length;
        int amountOfBytesToRead = bytesPending;
        int amountOfBytesToSaveForLater = 0;

        if(readableBytes<amountOfBytesToRead)//if we can read less than we want
        {
            amountOfBytesToRead = readableBytes;
        }
        else if(readableBytes>amountOfBytesToRead)//if we can read more bytes than we want (we save the remaining bytes)
        {
            amountOfBytesToSaveForLater = readableBytes - amountOfBytesToRead;
        }

        bytesPending -= amountOfBytesToRead;
        memcpy((char*)returnBuffer+readyBufferSize, bytes, amountOfBytesToRead);
        [newBuffer release];

        readyBufferSize+=amountOfBytesToRead;

        if(amountOfBytesToSaveForLater>0)
        {//save these bytes back in the buffers array
            if(tBufSize<amountOfBytesToSaveForLater)
            {
                tBuf = (char*)realloc(tBuf, amountOfBytesToSaveForLater);
                tBufSize = amountOfBytesToSaveForLater;
            }
            memcpy(tBuf, &(bytes[amountOfBytesToRead]), amountOfBytesToSaveForLater);
            @synchronized(playbackRawBuffer)
            {
                [playbackRawBuffer insertObject:[NSData dataWithBytes:tBuf length:amountOfBytesToSaveForLater] atIndex:0];
            }
            break;
        }
    }

    if(readyBufferSize>0)
    {
        return [NSData dataWithBytes:returnBuffer length:readyBufferSize];
    }
    else
    {
        audio.isBuffering = YES;
        audio.bufferTimer = [[NSDate date] timeIntervalSince1970]+0.1;
        return nil;
    }
}
share|improve this question

2 Answers 2

This is the wrong way to sue realloc:

returnBuffer = (char*)realloc(returnBuffer, requestedBufferSize);

If it fails you have just leaked the old returnBuffer.
The way realloc should be used is:

char* tmp = (char*)realloc(returnBuffer, requestedBufferSize);
if (tmp != NULL)
{
    returnBuffer = tmp;
}
else { /* Deal with realloc() failure */ }

But why are you dealing with this. I would use a std::vector<char> it deals with all the memory management issues for you (and your post id tagged C++).

TO be honest I have trouble following the rest of your code.

share|improve this answer
    
Well in this case the requested buffer size is from 600 to 700 bytes of data, if realloc cannot manage to extend it to this size then I think I have more serious problems than a memory leak :) –  Anton Banchev Sep 28 '12 at 7:12
    
@AntonBanchev: That's not really the point. You are writing code that is known bad pattern. I reject people from an interview for making such obvious mistakes. –  Loki Astari Sep 28 '12 at 19:04
    
+1 for “use std::vector”, although maybe for this particular problem, std::deque would work even better. –  microtherion Jul 24 '13 at 23:00

I'm not sure what your sample rate is, but two 320-byte blocks put together is likely to be such a short time interval that merging them into 640 entirely wrong bytes would just sound like a quick "artifact."

What evidence do you have that this merge works correctly at all? It's hard to interpret on it's own, and if all you've got to go on currently is an audible artifact, there's a possibility that it is just 640 bytes of junknoise.

Suggestion: run the method on it's own with known input, expected output, and validate that it does merge at all.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.