[vlc-devel] Reworking OpenSLES audio output

[Hugo Beauzee-Luyssen]

On Tue, May 3, 2011 at 10:15 PM, Dominique Martinet
<asmadeus at codewreck.org> wrote:
> Hi,
>
> First, thanks for sharing your work :)
>
Thanks for commiting it in the first place !

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

I will post a patch tomorrow, as I'm not on the same computer I wrote
the "patch" on. However, since I changed a bit of the coding style, it
might be hard to read.
As soon as the file gets accepted, I'm willing to reformat it in
multiple patches !

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

> 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...
>
Will try that tomorrow.

>
>> >    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 )
>
Will do.
As far as I remember VideoLAN's coding style it should be ok, but I
guess it won't hurt me to read them again and to double check.
As stated earlier, sending the file seems like the easiest way of
getting feedbacks, but I'll provide multiple patches.

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

Will do, I don't know why I didn't put this declaration in the same place...

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

Same spirit as A52_NB_SAMPLES, it worked fine (and better than 10 in
my tests), but I don't like that kind of magic numbers.
This is clearly a draft, in order to get feedbacks before further work.

>
>> -    // 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.
>
Yep I removed it at a point and forget to check for errors. To be
honest, the original file was performing the check :)

>
>> -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?
>
No particular reasons, and no the compiler doesn't complain if we
receive a p_aout directly

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

Thanks !

> Regards,
> Dominique Martinet | Asmadeus

Regards,

-- 
Hugo Beauzée-Luyssen