WAVE to FLAC encoder in C

Question

This is a .wav to .flac encoder that I wrote a little while ago. The only method that is really called is encode(), which takes in a .wav file, converts it to FLAC, and stores it in the .flac file that is taken in as a parameter.

I would prefer suggestions on how to improve the method, decrease run-time, or cut down on the length of the code. Any other suggestions are acceptable though.

---

The header file (encode.h):

#include <FLAC/stream_encoder.h>

int encode(const char *wavfile, const char *flacfile);
static void progress_callback(const FLAC__StreamEncoder *encoder, FLAC__uint64 bytes_written, FLAC__uint64 samples_written, unsigned frames_written, unsigned total_frames_estimate, void *client_data);

The encoding program:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "encode.h"

#define READSIZE 1020

static unsigned totalSamples = 0; /* can use a 32-bit number due to WAVE size limitations */
static FLAC__byte buffer[READSIZE/*samples*/ * 2/*bytes_per_sample*/ * 2/*channels*/]; /* we read the WAVE data into here */
static FLAC__int32 pcm[READSIZE/*samples*/ * 2/*channels*/];

int encode(const char *wavfile, const char *flacfile)
{
    FLAC__bool ok = true;
    FLAC__StreamEncoder *encoder = 0;
    FLAC__StreamEncoderInitStatus initStatus;
    FILE *fin;
    unsigned sampleRate = 0;
    unsigned channels = 0;
    unsigned bps = 0;

    if((fin = fopen(wavfile, "rb")) == NULL)
    {
        fprintf(stderr, "ERROR: opening %s for output\n", wavfile);
        return 1;
    }

    /* read wav header and validate it */
    if (fread(buffer, 1, 44, fin) != 44 || memcmp(buffer, "RIFF", 4) || memcmp(buffer + 8, "WAVEfmt \020\000\000\000\001\000\002\000", 16) ||memcmp(buffer + 32, "\004\000\020\000data", 8))
    {
        fprintf(stderr, "ERROR: invalid/unsupported WAVE file, only 16bps stereo WAVE in canonical form allowed\n");
        fclose(fin);
        return 1;
    }
    sampleRate = ((((((unsigned)buffer[27] << 8) | buffer[26]) << 8) | buffer[25]) << 8) | buffer[24];
    channels = 2;
    bps = 16;
    totalSamples = (((((((unsigned)buffer[43] << 8) | buffer[42]) << 8) | buffer[41]) << 8) | buffer[40]) / 4;

    /* allocate the encoder */
    if((encoder = FLAC__stream_encoder_new()) == NULL) 
    {
        fprintf(stderr, "ERROR: allocating encoder\n");
        fclose(fin);
        return 1;
    }

    ok &= FLAC__stream_encoder_set_verify(encoder, true);
    ok &= FLAC__stream_encoder_set_compression_level(encoder, 5);
    ok &= FLAC__stream_encoder_set_channels(encoder, channels);
    ok &= FLAC__stream_encoder_set_bits_per_sample(encoder, bps);
    ok &= FLAC__stream_encoder_set_sample_rate(encoder, sampleRate);
    ok &= FLAC__stream_encoder_set_total_samples_estimate(encoder, totalSamples);

    /* initialize encoder */
    if(ok)
    {
        initStatus = FLAC__stream_encoder_init_file(encoder, flacfile, progress_callback);
        if(initStatus != FLAC__STREAM_ENCODER_INIT_STATUS_OK)
        {
            fprintf(stderr, "ERROR: initializing encoder: %s\n", FLAC__StreamEncoderInitStatusString[initStatus]);
            ok = false;
        }
    }

    /* read blocks of samples from WAVE file and feed to encoder */
    if(ok)
    {
        fprintf(stdout, "Encoding: ");
        size_t left = (size_t)totalSamples;
        while(ok && left) 
        {
            size_t need = (left>READSIZE ? (size_t)READSIZE : (size_t)left);
            if (fread(buffer, channels * (bps / 8), need, fin) != need) 
            {
                fprintf(stderr, "ERROR: reading from WAVE file\n");
                ok = false;
            }
            else 
            {
                /* convert the packed little-endian 16-bit PCM samples from WAVE into an interleaved FLAC__int32 buffer for libFLAC */
                size_t i;
                for(i = 0; i < need*channels; i++)
                {
                    /* inefficient but simple and works on big- or little-endian machines */
                    pcm[i] = (FLAC__int32)(((FLAC__int16)(FLAC__int8)buffer[2 * i + 1] << 8) | (FLAC__int16)buffer[2 * i]);
                }
                /* feed samples to encoder */
                ok = FLAC__stream_encoder_process_interleaved(encoder, pcm, need);
            }
            left -= need;
        }
    }

    ok &= FLAC__stream_encoder_finish(encoder);

    fprintf(stdout, "%s\n", ok ? "Succeeded" : "FAILED");
    if (!ok) fprintf(stderr, "   State: %s\n", FLAC__StreamEncoderStateString[FLAC__stream_encoder_get_state(encoder)]);

    FLAC__stream_encoder_delete(encoder);
    fclose(fin);

    return 0;
}

void progress_callback(const FLAC__StreamEncoder *encoder, FLAC__uint64 bytes_written, FLAC__uint64 samples_written, unsigned frames_written, unsigned total_frames_estimate, void *client_data)
{
    (void)encoder, (void)client_data;

    fprintf(stderr, "Wrote %llu bytes, %llu/%u samples, %u/%u frames\n", bytes_written, samples_written, totalSamples, frames_written, total_frames_estimate);
}

Answer 1

These two statement could be implemented by the same subroutine:

sampleRate = ((((((unsigned)buffer[27] << 8) | buffer[26]) << 8) | buffer[25]) << 8) | buffer[24];
totalSamples = (((((((unsigned)buffer[43] << 8) | buffer[42]) << 8) | buffer[41]) << 8) | buffer[40]) / 4;

For example:

unsigned read4bytes(FLAC__byte* ptr)
{
    return ((((((unsigned)*(ptr+3) << 8) | *(ptr+2)) << 8) | *(ptr+1)) << 8) | *(ptr);
}

and

sampleRate = read4bytes(buffer + 24);
totalSamples = read4bytes(buffer + 40);

You're using ok to skip processing until you call fclose at the end. Instead all that could be a subroutine ...

fin = fopen(wavfile, "rb");
rc = encodeFile(fin, flacfile);
fclose(fin);

... and then instead of having ok in encodeFile, you can return when (as soon as) you detect an error.

You currently don't fprintf a specific error message if a function call such as FLAC__stream_encoder_set_verify fails.

I don't know why READSIZE value is 1020.

channels and bps could be set to 2 and 16 at the start of the function; or they could be const or macros like READFILE is.

---

For the second subroutine, I had something like the following in mind (untested code ahead).

• By eliminating the ok variable I eliminate if statements in the body of the code.
• As soon as encodeFile detects an error, it prints an error and immediately returns.
• The calling routine which opened the file is responsible for closing the file; there's now only one fclose(fin) statement.

I did something similar (i.e. make a subroutine) to wrap the lifetime of the allocated encoder object.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "encode.h"

#define READSIZE 1020

static unsigned totalSamples = 0; /* can use a 32-bit number due to WAVE size limitations */
static FLAC__byte buffer[READSIZE/*samples*/ * 2/*bytes_per_sample*/ * 2/*channels*/]; /* we read the WAVE data into here */
static FLAC__int32 pcm[READSIZE/*samples*/ * 2/*channels*/];

int encodeFile2(FILE *fin, FLAC__StreamEncoder *encoder, const char *flacfile)
{
    unsigned sampleRate = ((((((unsigned)buffer[27] << 8) | buffer[26]) << 8) | buffer[25]) << 8) | buffer[24];
    unsigned channels = 2;
    unsigned bps = 16;
    totalSamples = (((((((unsigned)buffer[43] << 8) | buffer[42]) << 8) | buffer[41]) << 8) | buffer[40]) / 4;

    if (!(FLAC__stream_encoder_set_verify(encoder, true)
        && FLAC__stream_encoder_set_compression_level(encoder, 5)
        && FLAC__stream_encoder_set_channels(encoder, channels)
        && FLAC__stream_encoder_set_bits_per_sample(encoder, bps)
        && FLAC__stream_encoder_set_sample_rate(encoder, sampleRate)
        && FLAC__stream_encoder_set_total_samples_estimate(encoder, totalSamples)))
    {
        return fprintf(stderr, "ERROR: setting encoder properties\n");
    }

    /* initialize encoder */
    FLAC__StreamEncoderInitStatus initStatus = FLAC__stream_encoder_init_file(encoder, flacfile, progress_callback);
    if(initStatus != FLAC__STREAM_ENCODER_INIT_STATUS_OK)
    {
        return fprintf(stderr, "ERROR: initializing encoder: %s\n", FLAC__StreamEncoderInitStatusString[initStatus]);
    }

    /* read blocks of samples from WAVE file and feed to encoder */
    fprintf(stdout, "Encoding: ");
    size_t left = (size_t)totalSamples;
    while(left) 
    {
        size_t need = (left>READSIZE ? (size_t)READSIZE : (size_t)left);
        if (fread(buffer, channels * (bps / 8), need, fin) != need) 
        {
            return fprintf(stderr, "ERROR: reading from WAVE file\n");
        }
        /* convert the packed little-endian 16-bit PCM samples from WAVE into an interleaved FLAC__int32 buffer for libFLAC */
        size_t i;
        for(i = 0; i < need*channels; i++)
        {
            /* inefficient but simple and works on big- or little-endian machines */
            pcm[i] = (FLAC__int32)(((FLAC__int16)(FLAC__int8)buffer[2 * i + 1] << 8) | (FLAC__int16)buffer[2 * i]);
        }
        /* feed samples to encoder */
        if (FLAC__stream_encoder_process_interleaved(encoder, pcm, need))
        {
            return fprintf(stderr, "ERROR: processing WAVE file: %s\n", FLAC__StreamEncoderStateString[FLAC__stream_encoder_get_state(encoder)]);
        }
        left -= need;
    }

    if (!FLAC__stream_encoder_finish(encoder))
    {
        return fprintf(stderr, "ERROR: finishing encoder: %s\n", FLAC__StreamEncoderStateString[FLAC__stream_encoder_get_state(encoder)]);
    }

    fprintf(stdout, "Succeeded");
    return 0;
}
int encodeFile1(FILE *fin, const char *flacfile)
{
    /* read wav header and validate it */
    /* before allocating the encoder */
    if (fread(buffer, 1, 44, fin) != 44 || memcmp(buffer, "RIFF", 4) || memcmp(buffer + 8, "WAVEfmt \020\000\000\000\001\000\002\000", 16) ||memcmp(buffer + 32, "\004\000\020\000data", 8))
    {
        return fprintf(stderr, "ERROR: invalid/unsupported WAVE file, only 16bps stereo WAVE in canonical form allowed\n");
    }

    /* allocate the encoder */
    FLAC__StreamEncoder *encoder;
    if((encoder = FLAC__stream_encoder_new()) == NULL) 
    {
        return fprintf(stderr, "ERROR: allocating encoder\n");
    }

    /* use the encoder */
    rc = encodeFile2(fin, encoder, flacfile);

    /* deallocate the encoder */
    FLAC__stream_encoder_delete(encoder);
    return rc;
}
int encode(const char *wavfile, const char *flacfile)
{
    FILE *fin;
    int rc;
    if((fin = fopen(wavfile, "rb")) == NULL)
    {
        fprintf(stderr, "ERROR: opening %s for output\n", wavfile);
        return 1;
    }
    rc = encodeFile1(fin, flacfile);
    fclose(fin);
    return (rc) ? 1 : 0;
}

I also changed to location where local variables are defined: instead of defining them at the top of the function, they're now not defined until they're initialized (which you seemed to be doing already, though inconsistently, in your OP; the ability to do so is newer-style C).

---

The above changes are stylistic:

• Shorter and simpler
• Easier to verify (by inspection) that allocated resources are closed/freed.

As for performance, the first rule is to use run-time profiler (don't try to guess what's wrong, and don't bother to performance-optimize code which isn't a performance bottle-neck).

Much of the work is perhaps done by the library you're using, which implies there may be little point in optimizing your code.

From looking at the source code, performance hints could include:

1. Can you use a different encoder library?
2. Is fread the most performant file I/O function?
3. What's the effect of different READSIZE values

Apart from the file-reading I'd guess that this is your only important work:

        /* inefficient but simple and works on big- or little-endian machines */
        pcm[i] = (FLAC__int32)(((FLAC__int16)(FLAC__int8)buffer[2 * i + 1] << 8) | (FLAC__int16)buffer[2 * i]);

Search the 'net for different implementations of that; for example, it might be a no-op on some CPUs; some CPUs might have dedicated opcodes for this purpose (bswap).

Depending on how/whether your compiler optimizes, there might be more efficient source code: instead of combining bytes into an integer and storing the integer to the target buffer, copy bytes from the source buffer into corresponding bytes of the target buffer; using run-time if or compile-time #if to choose the right (machine-specific) implementation of corresponding bytes. An additional complication however is the sign-extension (converting signed int16 to signed int32).

I'd suspect that it's irrelevant though: that the FLAC library has much more work to than that, and so that its the performance of the library which dominates the performance of the whole operation.