[vlc-devel] [PATCH] demux: adaptive: fix inverted logic and don't do wait in a loop

[Rémi Denis-Courmont]

Le keskiviikkona 26. heinäkuuta 2017, 23.08.21 EEST Zhao Zhili a écrit :
> On Wed, Jul 26, 2017 at 10:38 PM, Rémi Denis-Courmont <remi at remlab.net>
> 
> wrote:
> > Le keskiviikkona 26. heinäkuuta 2017, 19.43.34 EEST Zhao Zhili a écrit :
> > > ---
> > > 
> > >  modules/demux/adaptive/PlaylistManager.cpp | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/modules/demux/adaptive/PlaylistManager.cpp
> > > b/modules/demux/adaptive/PlaylistManager.cpp
> > > index f560d0f..f489a18f 100644
> > > --- a/modules/demux/adaptive/PlaylistManager.cpp
> > > +++ b/modules/demux/adaptive/PlaylistManager.cpp
> > > @@ -664,8 +664,7 @@ void PlaylistManager::Run()
> > > 
> > >              vlc_mutex_unlock(&demux.lock);
> > >              
> > >              mutex_cleanup_push(&lock);
> > > 
> > > -            while(vlc_cond_timedwait(&waitcond, &lock, i_deadline) == 0
> > > -                 && i_deadline < mdate());
> > > +            vlc_cond_timedwait(&waitcond, &lock, i_deadline);
> > > 
> > >              vlc_cleanup_pop();
> > >          
> > >          }
> > >      
> > >      }
> > 
> > The existing code is an obtuse and inefficient emulation of mwait(). It
> > does
> > not make much sense. But the patch is also wrong due to spurious wake-ups.
> 
> I think it's more similar to ControlPop() than mwait().

No. The second part of the loop predicate is a no-op since 
vlc_cond_timedwait() will only return non-zero after the deadline is reached. 
So the code is equivalent to:

    mutex_cleanup_push(&lock);

    while (vlc_cond_timedwait(&waitcond, &lock, i_deadline) == 0);

    vlc_cleanup_pop();

The predicate for the condition variable is always false. So that is obviously 
equivalent to a simple sleep:

    mutex_cleanup_push(&lock);

    vlc_mutex_unlock(&lock);
    vlc_cleanup_push(vlc_mutex_lock, &lock);
    mwait(i_deadline);
    vlc_cleanup_pop();
    vlc_mutex_lock(&lock);

    vlc_cleanup_pop();

The cleanup handlers obviously cancel out one another, so the code can 
trivially be further simplified:

    vlc_mutex_unlock(&lock);
    mwait(i_deadline);
    vlc_mutex_lock(&lock);


-- 
雷米‧德尼-库尔蒙
https://www.remlab.net/