[vlc-devel] [PATCH] Erase regularily the already played directsound buffer outside of the play callback

[Denis Charmet]

Le jeudi 17 avril 2014 à 01:52:37, Rémi Denis-Courmont a écrit :
> By definition, you cannot underrun if you loop...
> You should not loop, though that has other issues.
> 
Seeing how broken the API is, not looping will end up causing audio
gaps.

> > It may look overkill but this properly fix #11145 once and for all.
> 
> I don't see the point in using DirectSound in VLC. I don't know why it was
> introduced when it was, and I don't see how, for a media player, it would
> be any better than WaveOut.
> 
I prefer waveout too but apparently DirectSound has better integration than
WaveOut.

> > [...]
> >  static HRESULT Stop( aout_stream_sys_t *p_sys )
> >  {
> > +    vlc_mutex_lock( &p_sys->lock );
> > +    if( p_sys->b_running )
> > +    {
> > +        p_sys->b_running = false;
> > +        vlc_mutex_unlock( &p_sys->lock );
> > +        vlc_join( p_sys->eraser_thread, NULL );
> > +        vlc_mutex_lock( &p_sys->lock );
> 
> Why?
> 
> > +    }
> 
> else
>
I wasn't too proud of this solution but now I'm ashamed...

> > +    vlc_mutex_unlock( &p_sys->lock );
> >      if( p_sys->p_notify != NULL )
> >      {
> >          IDirectSoundNotify_Release(p_sys->p_notify );
> > @@ -729,7 +726,20 @@ static HRESULT Start( vlc_object_t *obj,
> > aout_stream_sys_t *sys,
> >          }
> >      }
> >  
> > +    vlc_mutex_lock(&sys->lock);
> 
> Why that?
> 
I'd say stupidity but for ego reasons I'll answer sleep deprivation...
> > +    sys->b_running = true;
> > +    int ret = vlc_clone(&sys->eraser_thread, PlayedDataEraser, (void*)
> > obj,
> > +                        VLC_THREAD_PRIORITY_LOW);
> > +    if( unlikely( ret ) )
> > +    {
> > +        if( ret != ENOMEM )
> > +            msg_Err( obj, "Couldn't start eraser thread" );
> > +        sys->b_running = false;
> > +        goto error;
> 
> Dead lock.
> 
> > +    }
> >      sys->i_write = 0;
> > +    vlc_mutex_unlock( &sys->lock );
> > +
> >      return DS_OK;
> >  
> >  error:
> > @@ -1028,6 +1038,8 @@ static int Open(vlc_object_t *obj)
> >      aout_DeviceReport(aout, dev);
> >      free(dev);
> >  
> > +    vlc_mutex_init(&sys->s.lock);
> > +
> >      return VLC_SUCCESS;
> >  }
> >  
> > @@ -1035,8 +1047,76 @@ static void Close(vlc_object_t *obj)
> >  {
> >      audio_output_t *aout = (audio_output_t *)obj;
> >      aout_sys_t *sys = aout->sys;
> > +    vlc_mutex_destroy(&sys->s.lock);
> >  
> >      var_Destroy(aout, "directx-audio-device");
> >      FreeLibrary(sys->hdsound_dll); /* free DSOUND.DLL */
> >      free(sys);
> >  }
> > +
> > +static void * PlayedDataEraser( void * data )
> > +{
> > +    const audio_output_t *aout = (audio_output_t *) data;
> > +    aout_stream_sys_t *p_sys = &aout->sys->s;
> > +    void *p_write_position, *p_wrap_around;
> > +    unsigned long l_bytes1, l_bytes2;
> > +    DWORD i_read, status;
> > +    mtime_t toerase;
> > +    HRESULT dsresult;
> > +
> > +    vlc_mutex_lock( &p_sys->lock );
> > +    while( p_sys->b_running )
> 
> You can either use VLC thread cancellation in which case the boolean is
> useless, or the Win32 thread API in which case the invalid thread handle
> value renders the boolean is useless.
> 
> Either way, the boolean seems useless.
> 
Bad habit of dealing with broken libc where cancellation fails badly but
fair enough.

> > +    {
> > +        dsresult = IDirectSoundBuffer_GetStatus( p_sys->p_dsbuffer,
> > &status );
> > +        if( dsresult != DS_OK ||
> > +            ( status & DSBSTATUS_PLAYING ) != DSBSTATUS_PLAYING )
> > +            goto wait;
> > +
> > +        toerase = 0;
> > +
> > +        dsresult = IDirectSoundBuffer_GetCurrentPosition(
> > p_sys->p_dsbuffer,
> > +                                                          &i_read, NULL
> );
> > +        if( dsresult == DS_OK )
> > +        {
> > +            int64_t max = (int64_t) i_read - (int64_t) p_sys->i_write;
> > +            if( max <= 0 )
> > +                max += DS_BUF_SIZE;
> > +            toerase = max;
> > +        }
> > +
> > +        dsresult = IDirectSoundBuffer_Lock( p_sys->p_dsbuffer,
> > +                                            p_sys->i_write,
> > +                                            toerase,
> > +                                            &p_write_position,
> > +                                            &l_bytes1,
> > +                                            &p_wrap_around,
> > +                                            &l_bytes2,
> > +                                            0 );
> > +        if( dsresult == DSERR_BUFFERLOST )
> > +        {
> > +            IDirectSoundBuffer_Restore( p_sys->p_dsbuffer );
> > +            dsresult = IDirectSoundBuffer_Lock( p_sys->p_dsbuffer,
> > +                                                p_sys->i_write,
> > +                                                toerase,
> > +                                                &p_write_position,
> > +                                                &l_bytes1,
> > +                                                &p_wrap_around,
> > +                                                &l_bytes2,
> > +                                                0 );
> > +        }
> > +        if( dsresult != DS_OK )
> > +            goto wait;
> > +
> > +        memset( p_write_position, 0, l_bytes1 );
> > +        memset( p_wrap_around, 0, l_bytes2 );
> > +
> > +        IDirectSoundBuffer_Unlock( p_sys->p_dsbuffer, p_write_position,
> > l_bytes1,
> > +                                   p_wrap_around, l_bytes2 );
> > +wait:
> > +        vlc_mutex_unlock(&p_sys->lock);
> > +        msleep(20000);
> 
> This is horrible. I would not allow such thing on a platform I cared for.
> 
While I completely agree, I couldn't come up with a better solution,
unless sleeping the time returned by a TimeGet(). If you have a better
idea I'll be glad to implement it. (And throwing away directsound isn't
an acceptable solution for people who care about windows).

Regards,

-- 
Denis Charmet - TypX
Le mauvais esprit est un art de vivre