[vlc-devel] Reworking OpenSLES audio output

[Rémi Denis-Courmont]

   Hello,

On Tuesday 03 May 2011, Hugo Beauzee-Luyssen wrote:
> Hi,
> 
> I'm currently working on the OpenSLES audio output, initially created
> by Asmadeus, in order to have a native aout module for android.
> This is a (almost) working version, but I'm pretty sure there's a lot
> to fix, so I'll gladly hear your remarks !

You don't need to preprend opensles to error messages. The core already takes 
care of that thanks to the module name.

Using a callback to push audio samples exposes the same bug that the 
PulseAudio output in VLC <= 1.1 has. If the callback is not called for 
whatever reason, audio buffers will stack up until memory is exhausted.

And it maximizes the buffer size, thus reducing the risk of underruns.

    // we want 16bit signed data little endian.
    p_aout->output.output.i_format              = VLC_CODEC_S16L;

Always little-endian seems odd.

    p_aout->output.i_nb_samples                 = A52_FRAME_NB;

Why the size of an S/PDIF frame??

    p_aout->output.output.i_rate                = 44100;

Where did you get that value? Some mobile audio chips run at 48kHz.

    p_aout->output.output.i_physical_channels   = AOUT_CHAN_LEFT | 
AOUT_CHAN_RIGHT;

And the number of channels?

    vlc_value_t val, text;
    val.i_int = AOUT_VAR_STEREO;
    text.psz_string = _("Stereo");
    var_Change( p_aout, "audio-device", VLC_VAR_ADDCHOICE, &val, &text );
    var_Set( p_aout, "audio-device", val );

The variable is totally useless if there is only one choice.

    //Better let OpenSLES ask for a buffer
    if ( p_sys->buffer_enqueud != 0 )
        return ;
    if ( p_sys->next_date < mdate() )
        p_sys->next_date = mdate();

mdate() is at least one system call and cannot be optimized. You shouldn't 
call it multiple times. In any case, this computation of the next date cannot 
be correct (unless your hardware is *infinitely* fast).

And you can't use next_date in the callback without synchronization.

    aout_buffer_t *p_buffer = aout_FifoPop( p_aout, &p_aout->output.fifo );
    if ( p_buffer == NULL )
        return ;

Either you assume that one pf_play invocation = one buffer. Then this should be 
an assertion. Or you don't assume anything, and you should loop until the FIFO 
is emptied.

    SLresult    result = (*p_sys->playerBufferQueue)->Enqueue(p_sys-
>playerBufferQueue, p_buffer->p_buffer, p_buffer->i_buffer);
    if ( result != SL_RESULT_SUCCESS )
    {
        if (result == SL_RESULT_BUFFER_INSUFFICIENT)
            msg_Err( p_aout, "write error: buffer insufficient" );
        else
            msg_Err( p_aout, "write error (%m)" );

errno? really?

    }
    aout_BufferFree( p_buffer );
    p_sys->buffer_enqueud = 1;

It's spelt "enqueued".

    if ( bufq != p_sys->playerBufferQueue )
        return ; //If this gets trigger, universe might implode.

Universe implosion is implemented by assert(), and abort(), not return.

-- 
Rémi Denis-Courmont
http://www.remlab.info