[vlc-devel] Reworking OpenSLES audio output

[Hugo Beauzee-Luyssen]

2011/5/3 Rémi Denis-Courmont <remi at remlab.net>:
>   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.
>
Fixed.

> 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.
>
Yep I was kinda affraid of that... would having some kind of
"available buffers" counter that gets incremented by the OpenSLES
callback, and decremented by the aout thread at each queued buffer
would be enough ?
I'd say having a waid condition in the Play() function is a *really*
bad idea, I guess you will confirm.


>    // 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??

I don't have an answer to that. It was the value in the existing code,
and it happens to be one of the best working one. Though it feels
strange indeed.

>
>    p_aout->output.output.i_rate                = 44100;
>
> Where did you get that value? Some mobile audio chips run at 48kHz.
>

I'll work on probing the device once the callback stuff gets fixed.

>    p_aout->output.output.i_physical_channels   = AOUT_CHAN_LEFT |
> AOUT_CHAN_RIGHT;
>
> And the number of channels?

Do you mean output.i_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.
>

I agree, I just didn't commented the code out, I'll work on that part
when the device can be probed.

>    //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.
>

Flushing the fifo to the OpenSLES buffer queue seems like a bad idea,
as it can be full quite quickly, that why the callback must be used.
I though Play could be called even with an empty fifo, I'll use an assert.

>    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?
>
Already present in the code, will gladely remove it.

>    }

In case the buffer wasn't pushed to the OpenSL queue, is there
something that should be done with the buffer ? Or is it lost ?

>    aout_BufferFree( p_buffer );
>    p_sys->buffer_enqueud = 1;
>
> It's spelt "enqueued".
It does indeed.

>
>    if ( bufq != p_sys->playerBufferQueue )
>        return ; //If this gets trigger, universe might implode.
>
> Universe implosion is implemented by assert(), and abort(), not return.
Well if this happens I'm pretty sure the universe will explode by
itself, but I'll use an assert.

-- 
Hugo Beauzée-Luyssen