StreamRecording memory counting is wrong

[martinwork]

It doesn't account the overhead of more smaller buffers when the sample rate changes, and runs out of memory.

The extra for each buffer is something like sizeof(StreamRecording_Buffer) + sizeof(void *)

[microbit-carlos]

Thanks Martin!
Can you create a PR with the fix, or does it need further investigation?
Was this somewhat related to the increase in ADC sampling rate? Or do you know how the issue was introduced/uncovered in CODAL?

[martinwork]

I tested a simple solution with another counter to "shadow" totalBufferLength, but add on the size of the buffer object itself, and replace totalBufferLength in isFull and canPull.
https://github.com/lancaster-university/codal-core/blob/509086cc8590465041b15493ab52b56e7071c110/source/streams/StreamRecording.cpp#L116

The issue was made worse by 44000Hz sampling. With recording rates below the ADC sample rate there are more smaller buffers in the recording chain, So the 16 or so bytes each chain node occupies becomes relatively more significant, and memory runs out before the totalBufferLength reaches 50K.

With very low rates there could be many tiny byte buffers, each with the overhead, so we might consider combining buffers less than a certain size.

I'll make a PR for the simple solution.

[martinwork]

@microbit-carlos Please check the PR lancaster-university/codal-core#175

Without the PR, the example below records just over 50000 samples for each sample rate, but stops with 020 when the rate falls below 11000, like MakeCode v7. This is mostly because the example claims a block of RAM to simulate the problem!

The problem is that, with lower sample rates, there are more smaller buffers each with a memory overhead. At around 5000Hz there are over 1000 buffers, with an overhead of around 20K.

With the PR, the example copes with very small buffers but records less data.

The allowed length is increased so the 256 byte case records the same total data length. I was in two minds whether this adjustment should be in the default value macros in the header or in the constructor.

Maybe this just reveals the problem, and we ought to consider combining small blocks. Perhaps always extend the chain with a 256 byte buffer and copy smaller buffers into that?

C++ example with audio-recording extension code

#include "MicroBit.h"
#include "StreamRecording.h"

#define MICROBIT_CODAL 1

MicroBit uBit;

//================================================================
//  From
//  https://github.com/microsoft/pxt-microbit/blob/1226ac8de5640f426cbe84636cdc55669bae2c1b/libs/audio-recording/recording.cpp#L1

/*
    The MIT License (MIT)

    Copyright (c) 2022 Lancaster University

    Permission is hereby granted, free of charge, to any person obtaining a
    copy of this software and associated documentation files (the "Software"),
    to deal in the Software without restriction, including without limitation
    the rights to use, copy, modify, merge, publish, distribute, sublicense,
    and/or sell copies of the Software, and to permit persons to whom the
    Software is furnished to do so, subject to the following conditions:
    The above copyright notice and this permission notice shall be included in
    all copies or substantial portions of the Software.
    THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
    IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
    FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
    THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
    LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
    FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
    DEALINGS IN THE SOFTWARE.
*/


namespace record {

#if MICROBIT_CODAL
static StreamRecording *recording = NULL;
static SplitterChannel *splitterChannel = NULL;
static MixerChannel *channel = NULL;
#endif


void checkEnv() {
#if MICROBIT_CODAL
    if (recording == NULL) {
        int defaultSampleRate = 11000;
        MicroBitAudio::requestActivation();

        splitterChannel = uBit.audio.splitter->createChannel();
        splitterChannel->requestSampleRate( defaultSampleRate );

        recording = new StreamRecording(*splitterChannel);

        channel = uBit.audio.mixer.addChannel(*recording, defaultSampleRate);

        channel->setVolume(75.0);
        uBit.audio.mixer.setVolume(1000);
        uBit.audio.setSpeakerEnabled(true);
    }
#endif
}

/**
 * Record an audio clip
 */
//% promise
void record() {
#if MICROBIT_CODAL
    checkEnv();
    recording->recordAsync();
#else
    target_panic(PANIC_VARIANT_NOT_SUPPORTED);
#endif
}

/**
 * Play the audio clip that is saved in the buffer
 */
//%
void play() {
#if MICROBIT_CODAL
    checkEnv();
    recording->playAsync();
#else
    target_panic(PANIC_VARIANT_NOT_SUPPORTED);
#endif
}

/**
 * Stop recording
 */
//%
void stop() {
#if MICROBIT_CODAL
    checkEnv();
    recording->stop();
#else
    target_panic(PANIC_VARIANT_NOT_SUPPORTED);
#endif
}

/**
 * Clear the buffer
 */
//%
void erase() {
#if MICROBIT_CODAL
    checkEnv();
    recording->erase();
#endif
}

/**
 * Set sensitity of the microphone input
 */
//%
void setMicrophoneGain(float gain) {
#if MICROBIT_CODAL
    uBit.audio.processor->setGain(gain);
#endif
}

/**
 * Get how long the recorded audio clip is
 */
//%
int audioDuration(int sampleRate) {
#if MICROBIT_CODAL
    return recording->duration(sampleRate);
#else
    target_panic(PANIC_VARIANT_NOT_SUPPORTED);
    return MICROBIT_NOT_SUPPORTED;
#endif
}

/**
 * Get whether the playback is active
 */
//%
bool audioIsPlaying() {
#if MICROBIT_CODAL
    return recording->isPlaying();
#else
    return false;
#endif
}

/**
 * Get whether the microphone is listening
 */
//%
bool audioIsRecording() {
#if MICROBIT_CODAL
    return recording->isRecording();
#else
    return false;
#endif
}

/**
 * Get whether the board is recording or playing back
 */
//%
bool audioIsStopped() {
#if MICROBIT_CODAL
    return recording->isStopped();
#else
    return false;
#endif
}

/**
 * Change the sample rate of the splitter channel (audio input)
 */
//%
void setInputSampleRate(int sampleRate) {
#if MICROBIT_CODAL
    checkEnv();
    splitterChannel->requestSampleRate(sampleRate);
#else
    target_panic(PANIC_VARIANT_NOT_SUPPORTED);
#endif
}


/**
 * Change the sample rate of the mixer channel (audio output)
 */
//%
void setOutputSampleRate(int sampleRate) {
#if MICROBIT_CODAL
    checkEnv();
    channel->setSampleRate(sampleRate);
#else
    target_panic(PANIC_VARIANT_NOT_SUPPORTED);
#endif
}

/**
 * Set the sample rate for both input and output
*/
//%
void setBothSamples(int sampleRate) {
#if MICROBIT_CODAL
    setOutputSampleRate(sampleRate);
    splitterChannel->requestSampleRate(sampleRate);
#else
    target_panic(PANIC_VARIANT_NOT_SUPPORTED);
#endif
}

} // namespace record

//================================================================

void testRecord()
{
    uBit.serial.printf("testRecord\n");
    uBit.display.print('R');
    record::record();

    while ( record::audioIsRecording())
    {
        uBit.sleep(1000);
    }

    int nodes = 0;
    for ( StreamRecording_Buffer *node = record::recording->bufferChain; node; node = node->next)
    {
        nodes++;
        //uBit.serial.printf("node %d \r\n", node->buffer.length());
    }

    uBit.serial.printf("length %d buffer size %d nodes %d rate %d\n",
      (int) record::recording->length(), 
      (int) record::recording->bufferChain->buffer.length(), 
      (int) nodes,
      (int) record::recording->getSampleRate());

    uBit.display.print(' ');
}

void testPlay()
{
    uBit.display.print('P');
    record::play();

    while ( record::audioIsPlaying())
    {
        uBit.sleep(1000);
    }
    uBit.display.print(' ');
}

void onButtonA(MicroBitEvent e)
{
    testRecord();
}

void onButtonB(MicroBitEvent e)
{
    testPlay();
}

void forever()
{
    while (true)
    {
      uBit.sleep(1000);
    }
}


int main() {
    uBit.init();

    uBit.serial.printf("sizeof( StreamRecording_Buffer) %d\n", sizeof( StreamRecording_Buffer));
    uBit.serial.printf("sizeof( BufferData) %d\n", sizeof( BufferData));
    uBit.serial.printf("sizeof( PROCESSOR_WORD_TYPE) %d\n", sizeof( PROCESSOR_WORD_TYPE));

    // a guess at RAM taken by MakeCode
    // set where 11000 succeeds and 5500 leads to error 020
    ManagedBuffer b = ManagedBuffer( 20 * 1024);

    testRecord();
    testPlay();

    // How to find the default rate set by MicroBitAudio?
    // The rate to get unresampled 256 byte buffers
    // The ADC channel returns a float but does an integer division
    float defaultRate = 1000000 / uBit.adc.getSamplePeriod();

    for ( int divisor = 1; divisor <= 1024 && defaultRate / divisor >= 400; divisor *= 2)
    {
        record::setBothSamples( defaultRate / divisor);
        testRecord();
        testPlay();
    }

    uBit.serial.printf("Reset to default - press A or B\n");
    record::setBothSamples( defaultRate);

    //record::setBothSamples( CONFIG_MIXER_DEFAULT_CHANNEL_SAMPLERATE);
    //record::setBothSamples( 45454);
    //record::setBothSamples( 11000);
    //record::setBothSamples( 11111);
    //record::setBothSamples( 5500);
    //record::setBothSamples( 500);

    uBit.messageBus.listen( MICROBIT_ID_BUTTON_A,  MICROBIT_BUTTON_EVT_CLICK, onButtonA);
    uBit.messageBus.listen( MICROBIT_ID_BUTTON_B,  MICROBIT_BUTTON_EVT_CLICK, onButtonB);

    create_fiber( forever);

    release_fiber();
    return 0;
}

[martinwork]

Note the example code calculation

    // How to find the default rate set by MicroBitAudio?
    // The rate to get unresampled 256 byte buffers
    // The ADC channel returns a float but does an integer division
    float defaultRate = 1000000 / uBit.adc.getSamplePeriod();

With CODAL 0.2.68, I noticed that the buffers for sample rate 11000Hz were 62 bytes, not 64, and when I modified MicroBitAudio to set the default to 11000Hz, buffers were 254 bytes, not 256.

We know that the ADC can't do 11000 or 44100, and actually returns 11111 or 45454, but the puzzle was that in CODAL 0.2.63, MicroBitAudio sets the ADC period to 1e6 / 11000 and the buffers are 256 bytes, with the same example code.

The difference is that SplitterChannel::resample works differently to the 0.2.63 code, and reduces the number of bytes by roughly e.g. 11000/11111 (rounded up).
https://github.com/lancaster-university/codal-core/blob/509086cc8590465041b15493ab52b56e7071c110/source/streams/StreamSplitter.cpp#L56

This result of this extra precision also reveals that setting the sample rate to 11000Hz or 44100Hz (as opposed to leaving it undefined), causes the test sampleRate >= parent->upstream.getSampleRate() in SplitterChannel::pull() to fail (e.g. 11000 >= 11111), and the data is unnecessarily resampled, even in 0.2.63.

To get 256 byte buffers without resampling, it's necessary to request sample rate 1000000 / uBit.adc.getSamplePeriod().

[microbit-carlos]

@finneyj I'll close this as resolved in v0.2.69 via lancaster-university/codal-core#175.
If a rework is needed via the pipeline review we can track that in a different issue.