10

Here I am trying to write some code for a continuously recording audio system. I am then attempting to record the audio for a certain amount of time when a certain amplitude threshold is broken.

#include <stdio.h>
#include <stdlib.h>
#include <math.h>
#include <string.h>
#include <time.h>
#include <portaudio.h>
#include <sndfile.h>

#define FRAMES_PER_BUFFER (1024)
#define SAMPLE_SIZE (4)

typedef struct
{
    uint16_t formatType;
    uint16_t numberOfChannels;
    uint32_t sampleRate;
    float* recordedSamples;
} AudioData;

AudioData initAudioData(uint32_t sampleRate, uint16_t channels, int type)
{
    AudioData data;
    data.formatType = type;
    data.numberOfChannels = channels;
    data.sampleRate = sampleRate;
    return data;
}

float avg(float *data)
{
    int elems = sizeof(data) / sizeof(data[0]);
    float sum = 0;
    for (int i = 0; i < elems; i++)
    {
        sum += fabs(*(data + i));
    }
    return (float) sum / elems;
}

int main(void)
{
    AudioData data = initAudioData(44100, 2, paFloat32);
    PaStream *stream = NULL;
    PaError err = paNoError;
    int size = FRAMES_PER_BUFFER * data.numberOfChannels * SAMPLE_SIZE;
    float *sampleBlock = malloc(size);
    float *recordedSamples = NULL;
    time_t talking = 0;
    time_t silence = 0;

    if((err = Pa_Initialize())) goto done;
    PaStreamParameters inputParameters =
    {
        .device = Pa_GetDefaultInputDevice(),
        .channelCount = data.numberOfChannels,
        .sampleFormat = data.formatType,
        .suggestedLatency = Pa_GetDeviceInfo(Pa_GetDefaultInputDevice())->defaultHighInputLatency,
        .hostApiSpecificStreamInfo = NULL
    };
    if((err = Pa_OpenStream(&stream, &inputParameters, NULL, data.sampleRate, FRAMES_PER_BUFFER, paClipOff, NULL, NULL))) goto done;
    if((err = Pa_StartStream(stream))) goto done;
    for(int i = 0;;)
    {
        err = Pa_ReadStream(stream, sampleBlock, FRAMES_PER_BUFFER);
        if(avg(sampleBlock) > 0.000550) // talking
        {
            printf("You're talking! %d\n", i);
            i++;
            time(&talking);
            recordedSamples = realloc(recordedSamples, size * i);
            if (recordedSamples) memcpy(recordedSamples + ((i - 1) * size), sampleBlock, size); // problem here writing to memory at i = 16?
            else free(recordedSamples);
        }
        else //silence
        {
            double test = difftime(time(&silence), talking);
            printf("Time diff: %g\n", test);
            if (test >= 1.5)
            {
                // TODO: finish code processing audio snippet
                talking = 0;
                free(recordedSamples); // problem freeing memory?
            }
        }
    }

done:
    free(sampleBlock);
    Pa_Terminate();
    return err;
}

However, the code is being somewhat finicky. Sometimes when I run my program in Xcode, I get the following output:

Time diff: 1.4218e+09
You're talking! 0
You're talking! 1
You're talking! 2
You're talking! 3
You're talking! 4
You're talking! 5
You're talking! 6
You're talking! 7
You're talking! 8
You're talking! 9
You're talking! 10
You're talking! 11
You're talking! 12
You're talking! 13
You're talking! 14
You're talking! 15
(lldb)

With Xcode pointing to this line being the problem:

if (recordedSamples) memcpy(recordedSamples + ((i - 1) * size), sampleBlock, size); // problem here writing to memory at i = 16?

Other times I run the code, I get this error:

Time diff: 1.4218e+09
You're talking! 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 0
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 1
Time diff: 2
Time diff: 1.4218e+09
CTestEnvironment(55085,0x7fff7938e300) malloc: *** error for object 0x10081ea00: pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug

Both errors are somewhat confusing me... any suggestions?

syb0rg
  • 8,057
  • 9
  • 41
  • 81
  • you're not allocating `recordedSamples` but trying to reallocate or free it – Iłya Bursov Jan 21 '15 at 01:48
  • @Lashane Please read the `realloc` documentation – syb0rg Jan 21 '15 at 02:08
  • this line: 'int elems = sizeof(data) / sizeof(data[0]);' has the problem that 'sizeof(data)' will be the size of a pointer, rather than the size of a float. suggest: int elems = sizeof(float) / sizeof(data[0]); – user3629249 Jan 21 '15 at 02:47
  • 2
    always check the returned value from malloc and realloc to assure the operation was successful. On realloc, save the returned value into a temp variable, check for null, and if not null only then copy the temp variable into the pointer that is actually being used. Otherwise, if realloc fails, then the pointer to the original allocated memory will be lost – user3629249 Jan 21 '15 at 02:53
  • Your average function isn't working properly - it will only return the absolute value of the first element. `sizeof(data)` is returning the size of a pointer, which (I think on your platform) is the same as the size of a float. That means `elems` will always be one. You can't inspect `data` in any way to get the length - you'll need to pass it in as a parameter to your average function. – Katie Jan 23 '15 at 20:36
  • Also, RMS is a more common way to measure this. Basically, you calculate the standard deviation instead of the mean. That lets you know if the signal is varying from the mean, and eliminates problems caused by DC offset. – Katie Jan 23 '15 at 20:40
  • And this is hopefully my last comment - you really don't need to be using dynamic memory for this problem. Just declare one block of memory that is the size you need, then overwrite it each time through the loop. It looks like you free it up each time through the loop anyways. I'm also not sure what you're trying to do with `if(recordedSamples) ... else free(recordedSamples)` - you're only going to end up in that else if `recordedsamples` is null, in which case you shouldn't be freeing it. – Katie Jan 23 '15 at 20:47
  • @Katie I don't see how I can't use dynamic memory with this project, maybe I need to see things better from your perspective or maybe I need to clear some things up. Perhaps we can do that in this [chat room](http://chat.stackexchange.com/rooms/8595/the-2nd-monitor). – syb0rg Jan 23 '15 at 22:25
  • Not related to the problem, but `free()` in the `else` branch after the `realloc()` is rather pointless. The branch is only taken if `recordedSamples == NULL`, which means there is nothing that needs to be freed. – sth Jan 24 '15 at 16:17

3 Answers3

6

You are writing out of bounds of the allocated buffer:

recordedSamples = realloc(recordedSamples, size * i);
memcpy(recordedSamples + ((i - 1) * size), sampleBlock, size);

realloc() allocates a certain number of bytes, here size * i. The resulting pointer is stored in recordedSamples, which has type float*.

The memcpy() then tries to write data to recordedSamples + ((i - 1) * size. Pointer arithmetic is used to determine the location that should be written to. Since recordedSamples is of type float*, recordedSample + X points to a offset of X float values (not X bytes).

In other words, recordedSamples + ((i - 1) * size points to the memory location ((i - 1) * size * sizeof(float) bytes after recordedSamples. This is usually not inside the allocated buffer, since floats are larger than a single byte.

To fix this, the big question is if size is supposed to be a number of bytes or a number of floats. This depends on the API functions you are using, I haven't looked into it in detail.

If it's a number of floats, then you have to adjust the calls to basic memory management functions like malloc, realloc and memcpy, because the all operate on bytes. To instead of malloc(size) you would call malloc(size * sizeof(float)).

If size is indeed a number of bytes, then it would be more logical to make recordedSamples a char*, or at least cast it before doing pointer arithmetic with byte offsets, like memcpy((char*)recordedSamples + ...).

sth
  • 222,467
  • 53
  • 283
  • 367
3

These types of bugs are difficult to recreate due to platform differences, so it's hard for me to know precisely what is happening here, but I will point out some issues with your code that could potentially have something to do with it.

I have noticed some problems with your use of free().

Note that free(ptr) does not change the value of ptr, so your latter error can be caused by the following sequence of calls:

free(recordSamples);
free(recordSamples);

This may be happening because you could be entering the test >= 1.5 condition twice, and hence a double free. Solving this problem should be as simple is adding:

recordSamples = NULL;

whenever you call free. That way, the pointer is NULL when you call free a second time and you will not get an error.

Another potential issue with this very same case is that a pointer that has been freed and then passed into realloc will create undefined behavior. It could happily return an invalid pointer without throwing any errors, depending on the implementation. If that's the case, it makes sense that a memcpy would fail, although admittedly I am not sure whether this is actually happening in your first case. It is possible that some of your output isn't flushed before the error is thrown, so we may not be getting a full picture of what is being called before the errors. A debugger would be useful for that.

My recommendataion is to make sure that recordSamples is always set to NULL after a free (looks like there are only two in your code) and see if that resolves the issues. If there are still problems, I recommend using a tool like valgrind to get more details on why these memory issues are occuring.

Valgrind works by replacing the system's malloc and free with its own that has extensive metadata tracking. It can often report precisely why something like this might fail.

http://valgrind.org/

Dougvj
  • 6,409
  • 2
  • 23
  • 18
0
// Note: I do not have the portaudio.h and sndfile.h so could not compile/test


#include <stdio.h>
#include <stdlib.h>
#include <math.h>
#include <string.h>
#include <time.h>

#include "portaudio.h"
#include "sndfile.h"

#define FRAMES_PER_BUFFER  (1024)
#define SAMPLE_SIZE        (4)
#define NUMBER_OF_CHANNELS (2)
#define SAMPLE_RATE        (44100)
#define SIZE               (FRAMES_PER_BUFFER * NUMBER_OF_CHANNELS * SAMPLE_SIZE)

static const int size = SIZE;

struct AudioData_t
{
    uint16_t formatType;
    uint16_t numberOfChannels;
    uint32_t sampleRate;
    float* recordedSamples;
};


void initAudioData(
    struct AudioData_t *pData,
    uint32_t sampleRate,
    uint16_t channels,
    int type)
{
    (*pData).formatType = type;
    (*pData).numberOfChannels = channels;
    (*pData).sampleRate = sampleRate;
} // end function: initAudioData


float averageAudioLevel(float *data)
{
    int elems = size / sizeof(float); // <--
    float sum = 0;

    for (int i = 0; i < elems; i++)
    {
        sum += fabs(*(data + i));
    }
    return  sum / elems; // sum is float so result is float
} // end function: averageAudioLevel


int main(void)
{
    struct AudioData_t data;
    initAudioData(&data, SAMPLE_RATE, NUMBER_OF_CHANNELS, paFloat32);

    PaStream *stream = NULL;
    PaError err = paNoError;


    float *sampleBlock = NULL;

    if(NULL == (sampleBlock = malloc(size) ) )
    { // then, malloc failed
        perror( "malloc failed" );
        exit( EXIT_FAILURE );
    }

    // implied else, malloc successful

    float *recordedSamples = NULL;

    time_t talking = 0;
    time_t silence = 0;

    if( 0 == (err = Pa_Initialize()))
    { // then init successful

        PaStreamParameters inputParameters =
        {
            .device = Pa_GetDefaultInputDevice(),
            .channelCount = data.numberOfChannels,
            .sampleFormat = data.formatType,
            .suggestedLatency = Pa_GetDeviceInfo(Pa_GetDefaultInputDevice())->defaultHighInputLatency,
            .hostApiSpecificStreamInfo = NULL
        };

        // if err >0, exit
        if( 0 == (err = Pa_OpenStream(&stream, &inputParameters, NULL, SAMPLE_RATE, FRAMES_PER_BUFFER, paClipOff, NULL, NULL)))
        { // then success
            if( 0 == (err = Pa_StartStream(stream)) )
            { // then success

                int i = 0;
                while(1) // this loop never exits
                {
                    talking = 0;
                    if( 0 == (err = Pa_ReadStream(stream, sampleBlock, FRAMES_PER_BUFFER) ) )
                    {
                        if(averageAudioLevel(sampleBlock) > 0.000550) // talking
                        {
                            printf("You're talking! %d\n", i);

                            i++; // counting usable audio samples

                            if( !talking ) {talking = time(NULL);} // only do once per audio sample

                            // increase allocation for another audio sample (starts at 0 allocated)
                            float *temp;
                            if( NULL == (temp = realloc(recordedSamples, size * i ) )
                            { // then realloc failed
                                perror( ""realloc failed" ");
                                free( sampleBlock );
                                free( recordedSamples );
                                exit( EXIT_FAILURE );
                            }

                            // implied else, realloc successful

                            // update the actual allocated memory pointer
                            recordedSamples = temp;

                            // save the new sample into array of samples
                            memcpy(recordedSamples + ((i - 1) * size), sampleBlock, size);}
                        } // end if
                    }

                    else //silence
                    {
                        if( 0 < i )
                        { // then some samples to evaluate

                            double elapsedTime = difftime(time(NULL), talking);
                            printf("Time diff: %g\n", elapsedTime);

                            if (elapsedTime >= 1.5)
                            {
                                // TODO: finish code processing audio snippet

                                // reset time indicators so do not process silence unless proceed by audio sound
                                talking = 0;

                                // reset audio sample counter
                                i = 0;

                                // dispose of recorded samples
                                free( recordedSamples );

                                // prep for next recording
                                recordedSamples = NULL;
                            } // end if
                        } // end if
                    } // end if
                } //  end forever loop
            } // end if
        } // end if
    } // end if


    free( sampleBlock );
    free( recordedSamples );
    Pa_Terminate();
    return err;
} // end function: main
user3629249
  • 16,402
  • 1
  • 16
  • 17
  • This isn't causing the problems specified in the question. Also, the compiler utilizes `return` value optimization... – syb0rg Jan 21 '15 at 03:27
  • Your edit still contains the problems I described in my question, as well as some syntax errors. – syb0rg Feb 04 '15 at 18:14