Samples count is always zero in write callback for Ogg FLAC

[njlarsson]

The documentation of FLAC__StreamEncoderWriteCallback says “Unlike when writing to native FLAC, when writing to Ogg FLAC the write callback will be called twice when writing each audio frame; once for the page header, and once for the page body. When writing the page header, the samples argument to the write callback will be 0.” Unfortunately, it appears that the samples argument is always 0 with Ogg FLAC.

The code below demonstrates the problem, encoding a buffer of 100 zero samples with native and Ogg FLAC. It prints (where both should be 100):

Total samples output without ogg: 100
Total samples output with ogg: 0

I tested it with the same results with FLAC 1.4.3 installed via Homebrew in macOS, and with 1.3.3 on Ubuntu (1.3.3-2ubuntu0.2, the version that standard package management on Ubuntu 22.04.3 LTS supplies for package libflac-dev).

This breaks the use of Ogg FLAC in my application, since I need to determine the number of samples actually output in order not to flood the recipient of the stream. (The number of output samples is sometimes much smaller than the number of processed samples for highly compressible audio.)

#include <stdlib.h>
#include <stdio.h>
#include "FLAC/stream_encoder.h"

long outsamples;

static FLAC__StreamEncoderWriteStatus
write_cb(const FLAC__StreamEncoder *encoder,
         const FLAC__byte buffer[],
         size_t bytes,
         uint32_t samples,
         uint32_t current_frame,
         void *handle)
{
    outsamples += samples;
    return FLAC__STREAM_ENCODER_WRITE_STATUS_OK;
}

int main(int argc, char *argv[]) {
    const int samples = 100;
    FLAC__int32 **buffer;
    buffer = malloc(2 * sizeof(FLAC__int32 *));
    if (buffer == NULL) {
        fprintf(stderr, "Failed to allocate buffer");
        exit(1);
    }
    for (int c = 0; c < 2; c++) {
        if ((buffer[c] = malloc(samples * sizeof **buffer)) == NULL) {
            fprintf(stderr, "Failed to allocate buffer");
            exit(1);
        }
        for (int i = 0; i < samples; i++) {
            buffer[c][i] = 0;
        }
    }

    for (int ogg = 0; ogg < 2; ogg++) {
        FLAC__StreamEncoder *encoder;
        if ((encoder = FLAC__stream_encoder_new()) == NULL) {
            fprintf(stderr, "Failed to create FLAC encoder");
            exit(1);
        }
        if (!(FLAC__stream_encoder_set_streamable_subset(encoder, true) &&
              FLAC__stream_encoder_set_compression_level(encoder, 5) &&
              FLAC__stream_encoder_set_channels(encoder, 2) &&
              FLAC__stream_encoder_set_bits_per_sample(encoder, 16) &&
              FLAC__stream_encoder_set_sample_rate(encoder, 44100))) {
            fprintf(stderr, "Failed to set encoder parameters\n");
            exit(1);
        }
        
        FLAC__StreamEncoderInitStatus status = ogg ?
            FLAC__stream_encoder_init_ogg_stream(encoder, NULL, write_cb, NULL, NULL, NULL, NULL) :
            FLAC__stream_encoder_init_stream(encoder, write_cb, NULL, NULL, NULL, NULL);
        if (status != FLAC__STREAM_ENCODER_INIT_STATUS_OK) {
            fprintf(stderr, "Failed to initialize encoder: %s\n", FLAC__StreamEncoderInitStatusString[status]);
            exit(1);
        }

        outsamples = 0;
        if (!FLAC__stream_encoder_process(encoder, (const FLAC__int32 *const *) buffer, samples)) {
            fprintf(stderr, "Process failed: %s\n", FLAC__StreamEncoderStateString[FLAC__stream_encoder_get_state(encoder)]);
            exit(1);
        }
        if (!FLAC__stream_encoder_finish(encoder)) {
            fprintf(stderr, "Finish failed: %s\n", FLAC__StreamEncoderStateString[FLAC__stream_encoder_get_state(encoder)]);
            exit(1);
        }
        fprintf(stdout, "Total samples output %s ogg: %ld\n", ogg ? "with" : "without", outsamples);
        FLAC__stream_encoder_delete(encoder);
    }
    
    return 0;
}

[njlarsson]

I got no reaction to this for eight months, so I took a look at it myself now. The problem isn’t difficult to find, indeed it’s stated in a comment in on line 195 of ogg_encoder_aspect.c:

/*@@@ can't figure out a way to pass a useful number for 'samples' to the write_callback, so we'll just pass 0 */

This is exactly the problem: just passing zero is not what we should do according to the documentation. What to do about it is less obvious, unfortunately. The way I understand it, ogg_stream_flush which is used here (from libogg) does not provide the information that would be needed to know the exact number of samples involved per write_callback call. There are several options for what to do, but to be as non-intrusive as possible I suggest simply passing the full number of samples in the first page-body callback in the while loop, to let the callee know how many they are, and then zero in any subsequent call in the while loop. This will match the intention of FLAC__StreamEncoderWriteCallback most of the time I think, and in any case it’s more useful than the current behavior. It should not break any existing working applications, since they are bound to ignore the samples parameter anyway in this case. (Nothing useful could be done with it since it’s always zero.) In my own application it would have worked without me even noticing that the behavior wasn’t exactly according to the original specification. But the exact behavior should be documented of course.

I’m submitting a pull request with this change, including a corresponding documentation correction+update for stream_encoder.h.