Code reviewing help needed. (audacious) (C)

Question

static void do_write (void * data, gint samples)
{
    void * allocated = NULL;

    samples = flow_execute (get_postproc_flow (), 0, & data, sizeof (gfloat) *
     samples, FMT_FLOAT, effect_rate, effect_channels) / sizeof (gfloat);

    if (data != allocated)
    {
        g_free (allocated);
        allocated = NULL;
    }

    apply_software_volume (data, output_channels, samples / output_channels);

    if (output_format != FMT_FLOAT)
    {
        void * new = g_malloc (FMT_SIZEOF (output_format) * samples);

        audio_to_int (data, new, output_format, samples);

        data = new;
        g_free (allocated);
        allocated = new;
    }

    if (output_format == FMT_S16_NE)
    {
        samples = flow_execute (get_legacy_flow (), 0, & data, 2 * samples,
         output_format, output_rate, output_channels) / 2;

        if (data != allocated)
        {
            g_free (allocated);
            allocated = NULL;
        }
    }
    if (COP->buffer_free == NULL)
        COP->write_audio (data, FMT_SIZEOF (output_format) * samples);
    else
    {
        while (1)
        {
            gint ready = COP->buffer_free () / FMT_SIZEOF (output_format);

            ready = MIN (ready, samples);
            COP->write_audio (data, FMT_SIZEOF (output_format) * ready);
            data = (char *) data + FMT_SIZEOF (output_format) * ready;
            samples -= ready;

            if (samples == 0)
                break;

            g_usleep (50000);
        }
    }

    g_free (allocated);
}

//This function is where COP->write_audio point to.

static void output_write_audio (void * data, gint size)
{
    gint samples = size / FMT_SIZEOF (decoder_format);
    void * allocated = NULL;

    LOCK;
    frames_written += samples / decoder_channels;
    UNLOCK;

    if (decoder_format != FMT_FLOAT)
    {
        gfloat * new = g_malloc (sizeof (gfloat) * samples);

        audio_from_int (data, decoder_format, new, samples);

        data = new;
        g_free (allocated);
        allocated = new;
    }

    apply_replay_gain (data, samples);
    vis_runner_pass_audio (frames_written * 1000 / decoder_rate, data, samples,
     decoder_channels);
    new_effect_process ((gfloat * *) & data, & samples);

    if (data != allocated)
    {
        g_free (allocated);
        allocated = NULL;
    }

    do_write (data, samples);
    g_free (allocated);
}

Q1: void * allocated = NULL; ... if (data != allocated) I'm confused.. what is the purpose of this? i didn't see allocated is changed

Q2. as you can see CP->write_audio will call do_write which will call CP->write_audio back. when will this end?

Answer 1

In output_write_audio() you have the variable new which is set to

gfloat * new = g_malloc (sizeof (gfloat) * samples);

and then:

allocated = new;

So allocated is set once in a while to a an allocated array of float samples. It seems to me though, allocated will freed once in a while even though there will be a NULL value in it. If you can run the code, you can check this by inserting a wrapper function around g_free() which can trace calls to it and tell if it called with a NULL argument.

Answer 2

A1 : Whole if can be removed, because the allocate is NULL, and that check is true only when data is NULL.

Answer 3

Q1: void * allocated = NULL; ... if (data != allocated) I'm confused.. what is the purpose of this? i didn't see allocated is changed

The address of data is passed as an argument to flow_execute. So that procedure can make data a NULL pointer, which would make the test false. Likewise, data might be NULL on entry.

The use of allocated seems a bit strange, but my intuition is that the two times this occurs, it is to track whether flow_execute changes the way data is stored, which will result in allocated no longer being equal to data. Then postprocessing can be performed. A guess.

Q2. as you can see CP->write_audio will call do_write which will call CP->write_audio back. when will this end?

It looks strange. Another guess: are you sure that COP->write_audio is immutably bound to output_write_audio? Two reasons for this thought: it seems odd to have the same write handler for both the buffered and unbuffered case, and COP->buffer_free is called before ->write_audio in both cases, so it could mutate COP depending on what needs to be done.

Failing that, a longjmp is possible: with all the g_unsleep and new_effect_process in the code, it doesn't seem impossible, but it would be an ugly way of going about things. If we're talking dirty code, one of those procedure-looking things could be a macro in disguise...

do_write will break from the while-true loop depending on some complex condition that is satisfied if (i) COP->buffer_free is non-null (it "normally and (ii) involving the output of COP->buffer_free ()