[vlc-devel] Reworking OpenSLES audio output

[Dominique Martinet]

Hi,

First, thanks for sharing your work :)

Let me first explain some things about "what was here before"

Hugo Beauzee-Luyssen wrote on Tue, May 03, 2011 :
> 2011/5/3 Rémi Denis-Courmont <remi at remlab.net>:
> > On Tuesday 03 May 2011, Hugo Beauzee-Luyssen wrote:
> >    // 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.

You might have wanted to submit a diff instead of the whole file, but
since that helps getting comment to what I did I won't complain.
The 16 bit little endian and the 44.1kHz you asked where they come from
are defined in format_pcm, we tell opensles that we are giving it this
format, so that might explain why it is what works best: 
SLDataFormat_PCM format_pcm = {
    SL_DATAFORMAT_PCM,    2,    SL_SAMPLINGRATE_44_1, 
    SL_PCMSAMPLEFORMAT_FIXED_16,    16,
    SL_SPEAKER_FRONT_RIGHT | SL_SPEAKER_FRONT_LEFT,
    SL_BYTEORDER_LITTLEENDIAN};

Now I'm sure it would be better to probe the device and use "native"
values here, but at least we are guaranteed that it will probably work
on any device.
44_1 could here be any of the following: 8, 11_025, 12, 16, 22_05, 24,
32, 44_1, 48, 64, 88_2, 96, 192. I took the same value as the one my
test file was sampled in to avoid yet another conversion

As for the A52_FRAME_NB, it is something I copied from another aout
(apparently the oss one), I didn't have the slightest clue of what would
sound good so I put something that looked like it could possibly make
sense, even though I suppose it might have been better to just leave it
empty now I'm reading oss.c again...


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

More stuff I didn't try to understand; more than having only one choice,
it's also not created/destroyed nor handled if it gets changed anyway...

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

Oh, that one might be from file.c :P I suppose printing result as an int
would be a better idea.



A few comments of my own now,
some are questions to whoever knows vlc more than me as much as to you.
Appologies for not wrapping the mail at 80 for diff parts too :p

Well, you didn't run a diff so you probably didn't notice, but most of
the diff is lines with just spaces added around brakes and
parenthesis, indentation padding, etc... Doesn't mean I was right with
my conventions, just that it's hard to tell the real changes quickly.
If it ever gets commited, it might be better to split it into two
patches (and find out if there is something about that here:
http://wiki.videolan.org/Code_Conventions )

> + static void Clear( aout_sys_t *p_sys )

Might want to declare it next to the other functions's?
It is not strictly needed as you define it first, but it sounds safer.

> -    SLDataLocator_AndroidSimpleBufferQueue loc_bufq = {SL_DATALOCATOR_ANDROIDSIMPLEBUFFERQUEUE, 10};
> +    SLDataLocator_AndroidSimpleBufferQueue loc_bufq = {
> +        SL_DATALOCATOR_ANDROIDSIMPLEBUFFERQUEUE,
> +        5
> +    };

My turn to ask where that 5 comes from?

> -    // register callback on the buffer queue 
> -    result = (*p_sys->playerBufferQueue)->RegisterCallback(p_sys->playerBufferQueue, PlayCallback, p_aout);
> - [blabla complain if result isn't SUCCES]
> +    (*p_sys->playerBufferQueue)->RegisterCallback( p_sys->playerBufferQueue, &BufferCallback, p_aout ); 

Can't RegisterCallback throw errors too? The sample "native-audio.c"
from the ndk does an assert on that one too.


> -static void PlayCallback(SLAndroidSimpleBufferQueueItf bq, aout_instance_t *p_aout)
> +static void BufferCallback( SLAndroidSimpleBufferQueueItf bufq, void *opaque )
> +    aout_instance_t *p_aout = (aout_instance_t*)opaque;

Is there any reason to have it typed to void in the function?
(i.e. does the call to RegisterCallback() complain if it's not)
Even if there is.. Why not call the variable with a name so we
understand what it is anyway?


Anyway, I didn't try yet but there are definitely some improvement, so
thank you :)

Regards,
Dominique Martinet | Asmadeus