[vlc-devel] [PATCH] opensles: protect buffer array with a spinlock

[Rafaël Carré]

Le 2012-01-21 07:40, Rafaël Carré a écrit :
> Append buffer to array before playing it back.
> 
> The callback might fire after Enqueue() returned, but before we append
> the buffer.
> 
> We could lock the whole call to Enqueue but it is simpler to just
> increment the array index after the function succeeded.
> If it failed, callback will not be called anyway.
> ---
>  modules/audio_output/opensles_android.c |   17 +++++++++++++++--
>  1 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/modules/audio_output/opensles_android.c b/modules/audio_output/opensles_android.c
> index 906b6bb..0afb6cb 100644
> --- a/modules/audio_output/opensles_android.c
> +++ b/modules/audio_output/opensles_android.c
> @@ -64,6 +64,7 @@ struct aout_sys_t
>      SLInterfaceID                 * SL_IID_VOLUME;
>      SLInterfaceID                 * SL_IID_PLAY;
>      void                          * p_so_handle;
> +    vlc_spinlock_t                  lock;
>  };
>  
>  typedef SLresult (*slCreateEngine_t)(
> @@ -249,6 +250,8 @@ static int Open( vlc_object_t *p_this )
>                                                   SL_PLAYSTATE_PLAYING );
>      CHECK_OPENSL_ERROR( result, "Failed to switch to playing state" );
>  
> +    vlc_spin_init(&p_sys->lock);
> +
>      // we want 16bit signed data little endian.
>      p_aout->format.i_format              = VLC_CODEC_S16L;
>      p_aout->format.i_physical_channels   = AOUT_CHAN_LEFT | AOUT_CHAN_RIGHT;
> @@ -278,6 +281,9 @@ static void Close( vlc_object_t *p_this )
>      //Flush remaining buffers if any.
>      if( p_sys->playerBufferQueue != NULL )
>          (*p_sys->playerBufferQueue)->Clear( p_sys->playerBufferQueue );
> +
> +    vlc_spin_destroy(&p_sys->lock);
> +
>      Clear( p_sys );
>  }
>  
> @@ -289,6 +295,9 @@ static void Play( audio_output_t *p_aout, block_t *p_buffer )
>      aout_sys_t *p_sys = p_aout->sys;
>      int tries = 5;
>  
> +    vlc_spin_lock(&p_sys->lock);
> +    p_sys->p_buffer_array[p_sys->i_toappend_buffer] = p_buffer;
> +    vlc_spin_unlock(&p_sys->lock);
>      for (;;)
>      {
>          SLresult result = (*p_sys->playerBufferQueue)->Enqueue(
> @@ -298,7 +307,6 @@ static void Play( audio_output_t *p_aout, block_t *p_buffer )
>          switch (result)
>          {
>          case SL_RESULT_SUCCESS:
> -            p_sys->p_buffer_array[p_sys->i_toappend_buffer] = p_buffer;
>              if( ++p_sys->i_toappend_buffer == BUFF_QUEUE )
>                  p_sys->i_toappend_buffer = 0;
>              return;
> @@ -324,10 +332,15 @@ static void Play( audio_output_t *p_aout, block_t *p_buffer )
>  static void PlayedCallback (SLAndroidSimpleBufferQueueItf caller, void *pContext )
>  {
>      aout_sys_t *p_sys = pContext;
> +    block_t *p_buffer;
>  
>      assert (caller == p_sys->playerBufferQueue);
>  
> -    aout_BufferFree( p_sys->p_buffer_array[p_sys->i_toclean_buffer] );
> +    vlc_spin_lock(&p_sys->lock);
> +    p_buffer =  p_sys->p_buffer_array[p_sys->i_toclean_buffer];
> +    vlc_spin_unlock(&p_sys->lock);
> +
> +    aout_BufferFree( p_buffer );
>      if( ++p_sys->i_toclean_buffer == BUFF_QUEUE )
>          p_sys->i_toclean_buffer = 0;
>  }

I still see problems with this patch, e.g.

in logcat:

W/libOpenSLES(13045): Leaving BufferQueue::Enqueue
(SL_RESULT_BUFFER_INSUFFICIENT)
E/vlc     (13045): buffer insufficient
W/libOpenSLES(13045): Leaving BufferQueue::Enqueue
(SL_RESULT_PARAMETER_INVALID)
W/vlc     (13045): Error 2, dropping buffer

in gdb:

Program received signal SIGSEGV, Segmentation fault.
(gdb) frame 0
#0  0x00000000 in ?? ()
(gdb) frame 1
#1  0x463e2cb8 in block_Release (p_block=0x42bec8) at
../../../extras/package/android/../../../include/vlc_block.h:161
161	    p_block->pf_release( p_block );
(gdb) print *p_block
$2 = {p_next = 0x458880, p_buffer = 0x458880 "", i_buffer = 0, i_flags =
0, i_nb_samples = 716142024, i_pts = 47244640259, i_dts = 1101004800,
  i_length = 4740038608952164352, pf_release = 0x1}
(gdb) frame 2
#2  0x463e3f30 in Play (p_aout=0x3eae78, p_buffer=0x42bec8) at
../../../extras/package/android/../../../include/vlc_block.h:234
234	        p_list = p_list->p_next;

amem works fine though