[vlc-devel] [PATCH 1/2] Add a 'Play and stop once' feature to the Qt interface

David Sferruzza david.sferruzza at gmail.com
Tue Aug 19 17:21:12 CEST 2014


Hi!

Could you give me some help/feedback, please? (see my previous email)
I'd love to be able to finish this feature!

Have a nice day!


2014-07-24 11:49 GMT+02:00 David Sferruzza <david.sferruzza at gmail.com>:

> Hi!
>
> I just saw your answer (didn't receive it by email... weird):
> https://mailman.videolan.org/pipermail/vlc-devel/2014-July/098928.html
> I understand the race condition issue.
>
> So the problem is in playlist/thread.c: when the NextItem() function is
> called, the value of the state can change between the var_GetBool() and the
> var_SetBool().
> In term of user experience, this would result in the following situation:
> if the user uncheck "Stop after current track" at the exact moment
> NextItem() is executing, VLC could ignore it and stop anyway.
> If I'm right (am I?) this doesn't feel a really bad behavior as it would
> happen very rarely and wouldn't be really annoying for the user.
>
> Anyway, I tried to fix it, but I got stuck.
> If I use var_GetAndSet(), the only i_action value I can use when
> manipulating a boolean would be *VLC_VAR_BOOL_TOGGLE*, according to
> https://www.videolan.org/developers/vlc/doc/doxygen/html/group__var__GetAndSet.html
> .
> But that's not the behavior I want (?).
> I want something like: if (var is false) then do nothing ; but if (var is
> true) then set it to false immediately and do some actions.
> Does this make sense?
>
> Also, I can't find any reference (in the Doxygen doc, and in the codebase)
> to the var_ExchangeBool() function you mentioned.
> Could you give me a hint on that?
>
> Apart from that, is the rest of my patch acceptable?
>
> Have a nice day!
>
>
> 2014-07-16 18:18 GMT+02:00 David Sferruzza <david.sferruzza at gmail.com>:
>
> Hi!
>>
>> Thanks for your advice!
>> I started over: there are 2 diffs replacing this one at the end of this
>> email.
>> (If necessary, I will start a new thread when the patch is nice enough.)
>>
>> I put the logic into the playlist.
>> But I didn't get what you meant by "That's a bit racy. I would add an
>> exchange operation to var_GetAndSet(). ...then you could do: else if(
>> var_ExchangeBool( p_playlist, "play-and-stop-once", false ) )"
>> I didn't find what var_ExchangeBool was supposed to do (I searched here:
>> https://wiki.videolan.org/Hacker_Guide/Variables/ and here:
>> https://www.videolan.org/developers/vlc/doc/doxygen/html/group__variables.html
>> and also in the codebase itself).
>>
>> The following diffs seem to work.
>> What do you think?
>>
>> Playlist:
>>
>> diff --git a/src/playlist/control.c b/src/playlist/control.c
>> index e1be944..3ac7ac8 100644
>> --- a/src/playlist/control.c
>> +++ b/src/playlist/control.c
>> @@ -84,6 +84,7 @@ static int PlaylistVAControl( playlist_t * p_playlist,
>> int i_query, va_list args
>>          pl_priv(p_playlist)->request.i_status = PLAYLIST_STOPPED;
>>          pl_priv(p_playlist)->request.b_request = true;
>>          pl_priv(p_playlist)->request.p_item = NULL;
>>
>> +        var_SetBool( p_playlist, "play-and-stop-once", false );
>>          break;
>>
>>      // Node can be null, it will keep the same. Use with care ...
>> diff --git a/src/playlist/engine.c b/src/playlist/engine.c
>> index ebb46dc..77ea295 100644
>> --- a/src/playlist/engine.c
>> +++ b/src/playlist/engine.c
>> @@ -275,6 +275,9 @@ playlist_t *playlist_Create( vlc_object_t *p_parent )
>>      pl_priv(p_playlist)->request.b_request = false;
>>      pl_priv(p_playlist)->status.i_status = PLAYLIST_STOPPED;
>>
>> +    var_Create( p_playlist, "play-and-stop-once", VLC_VAR_BOOL );
>>
>> +    var_SetBool( p_playlist, "play-and-stop-once", false );
>> +
>>      if(b_ml)
>>          playlist_MLLoad( p_playlist );
>>
>> diff --git a/src/playlist/thread.c b/src/playlist/thread.c
>> index a6ecbdb..4f2efb6 100644
>>
>> --- a/src/playlist/thread.c
>> +++ b/src/playlist/thread.c
>> @@ -359,6 +359,7 @@ static playlist_item_t *NextItem( playlist_t
>> *p_playlist )
>>          bool b_loop = var_GetBool( p_playlist, "loop" );
>>          bool b_repeat = var_GetBool( p_playlist, "repeat" );
>>          bool b_playstop = var_InheritBool( p_playlist, "play-and-stop" );
>> +        bool b_playstoponce = var_GetBool( p_playlist,
>> "play-and-stop-once" );
>>
>>
>>          /* Repeat and play/stop */
>>          if( b_repeat && get_current_status_item( p_playlist ) )
>> @@ -371,6 +372,12 @@ static playlist_item_t *NextItem( playlist_t
>> *p_playlist )
>>              msg_Dbg( p_playlist,"stopping (play and stop)" );
>>              return NULL;
>>          }
>> +        else if( b_playstoponce )
>> +        {
>> +            msg_Dbg( p_playlist, "stopping (play and stop once)" );
>> +            var_SetBool( p_playlist, "play-and-stop-once", false );
>>  +            return NULL;
>> +        }
>>
>>          /* */
>>          if( get_current_status_item( p_playlist ) )
>>
>> Qt:
>>
>> diff --git a/modules/gui/qt4/input_manager.cpp
>> b/modules/gui/qt4/input_manager.cpp
>> index f72b3b3..5c3995d 100644
>> --- a/modules/gui/qt4/input_manager.cpp
>> +++ b/modules/gui/qt4/input_manager.cpp
>> @@ -1205,6 +1205,16 @@ bool MainInputManager::getPlayExitState()
>>
>>      return var_InheritBool( THEPL, "play-and-exit" );
>>  }
>>
>> +void MainInputManager::activatePlayStopOnce( bool b_exit )
>> +{
>> +    var_SetBool( THEPL, "play-and-stop-once", b_exit );
>>
>> +}
>> +
>> +bool MainInputManager::getPlayStopOnceState()
>> +{
>> +    return var_InheritBool( THEPL, "play-and-stop-once" );
>> +}
>> +
>>  bool MainInputManager::hasEmptyPlaylist()
>>  {
>>      playlist_Lock( THEPL );
>> diff --git a/modules/gui/qt4/input_manager.hpp
>> b/modules/gui/qt4/input_manager.hpp
>> index dac3bc3..6932a77 100644
>> --- a/modules/gui/qt4/input_manager.hpp
>> +++ b/modules/gui/qt4/input_manager.hpp
>> @@ -268,6 +268,7 @@ public:
>>      audio_output_t *getAout();
>>
>>      bool getPlayExitState();
>> +    bool getPlayStopOnceState();
>>      bool hasEmptyPlaylist();
>>
>>      void requestVoutUpdate() { return im->UpdateVout(); }
>> @@ -298,6 +299,7 @@ public slots:
>>      void prev();
>>      void prevOrReset();
>>      void activatePlayQuit( bool );
>> +    void activatePlayStopOnce( bool );
>>
>>      void loopRepeatLoopStatus();
>>
>> diff --git a/modules/gui/qt4/menus.cpp b/modules/gui/qt4/menus.cpp
>> index a29333b..584cc6e 100644
>> --- a/modules/gui/qt4/menus.cpp
>> +++ b/modules/gui/qt4/menus.cpp
>> @@ -826,6 +826,15 @@ void VLCMenuBar::PopupMenuPlaylistEntries( QMenu
>> *menu,
>>          action->setEnabled( false );
>>      action->setData( ACTION_DELETE_ON_REBUILD );
>>
>> +    /* Stop after current track */
>> +    action = addMIMStaticEntry( p_intf, menu, qtr( "Stop &after current
>> track" ),
>> +            "", SLOT( activatePlayStopOnce( bool ) ), true );
>> +    action->setCheckable( true );
>> +    action->setChecked( THEMIM->getPlayStopOnceState() );
>> +    if( !p_input )
>> +        action->setEnabled( false );
>> +    action->setData( ACTION_DELETE_ON_REBUILD );
>> +
>>      /* Next / Previous */
>>      bool bPlaylistEmpty = THEMIM->hasEmptyPlaylist();
>>      action = addMIMStaticEntry( p_intf, menu, qtr( "Pre&vious" ),
>>
>>
>> 2014-07-15 10:36 GMT+02:00 Rémi Denis-Courmont <remi at remlab.net>:
>>
>> Le 2014-07-15 11:08, David Sferruzza a écrit :
>>>
>>>  This is a patch for #10732
>>>> Work document: https://gist.github.com/dsferruzza/8e9079a329975f3579e9
>>>> ---
>>>>  modules/gui/qt4/input_manager.cpp | 12 ++++++++++++
>>>>  modules/gui/qt4/input_manager.hpp |  2 ++
>>>>  modules/gui/qt4/menus.cpp         |  9 +++++++++
>>>>  src/libvlc-module.c               |  6 ++++++
>>>>  src/playlist/thread.c             |  7 +++++++
>>>>  5 files changed, 36 insertions(+)
>>>>
>>>> diff --git a/modules/gui/qt4/input_manager.cpp
>>>> b/modules/gui/qt4/input_manager.cpp
>>>> index f72b3b3..d69233a 100644
>>>> --- a/modules/gui/qt4/input_manager.cpp
>>>> +++ b/modules/gui/qt4/input_manager.cpp
>>>> @@ -1097,6 +1097,7 @@ void MainInputManager::customEvent( QEvent *event
>>>> )
>>>>  void MainInputManager::stop()
>>>>  {
>>>>     playlist_Stop( THEPL );
>>>> +   var_SetBool( THEPL, "play-and-stop-once", false );
>>>>
>>>
>>> IMHO, this belongs in the playlist state machine, not the UI.
>>>
>>>
>>>   }
>>>>
>>>>  void MainInputManager::next()
>>>> @@ -1205,6 +1206,17 @@ bool MainInputManager::getPlayExitState()
>>>>      return var_InheritBool( THEPL, "play-and-exit" );
>>>>  }
>>>>
>>>> +void MainInputManager::activatePlayStopOnce( bool b_exit )
>>>> +{
>>>> +    var_Create( THEPL, "play-and-stop-once", VLC_VAR_BOOL );
>>>>
>>>
>>> Ditto for var_Create().
>>>
>>>
>>>  +    var_SetBool( THEPL, "play-and-stop-once", b_exit );
>>>> +}
>>>> +
>>>> +bool MainInputManager::getPlayStopOnceState()
>>>> +{
>>>> +    return var_InheritBool( THEPL, "play-and-stop-once" );
>>>> +}
>>>> +
>>>>  bool MainInputManager::hasEmptyPlaylist()
>>>>  {
>>>>      playlist_Lock( THEPL );
>>>> diff --git a/modules/gui/qt4/input_manager.hpp
>>>> b/modules/gui/qt4/input_manager.hpp
>>>> index dac3bc3..6932a77 100644
>>>> --- a/modules/gui/qt4/input_manager.hpp
>>>> +++ b/modules/gui/qt4/input_manager.hpp
>>>> @@ -268,6 +268,7 @@ public:
>>>>      audio_output_t *getAout();
>>>>
>>>>      bool getPlayExitState();
>>>> +    bool getPlayStopOnceState();
>>>>      bool hasEmptyPlaylist();
>>>>
>>>>      void requestVoutUpdate() { return im->UpdateVout(); }
>>>> @@ -298,6 +299,7 @@ public slots:
>>>>      void prev();
>>>>      void prevOrReset();
>>>>      void activatePlayQuit( bool );
>>>> +    void activatePlayStopOnce( bool );
>>>>
>>>>      void loopRepeatLoopStatus();
>>>>
>>>> diff --git a/modules/gui/qt4/menus.cpp b/modules/gui/qt4/menus.cpp
>>>> index a29333b..584cc6e 100644
>>>> --- a/modules/gui/qt4/menus.cpp
>>>> +++ b/modules/gui/qt4/menus.cpp
>>>> @@ -826,6 +826,15 @@ void VLCMenuBar::PopupMenuPlaylistEntries( QMenu
>>>> *menu,
>>>>          action->setEnabled( false );
>>>>      action->setData( ACTION_DELETE_ON_REBUILD );
>>>>
>>>> +    /* Stop after current track */
>>>> +    action = addMIMStaticEntry( p_intf, menu, qtr( "Stop &after
>>>> current track" ),
>>>> +            "", SLOT( activatePlayStopOnce( bool ) ), true );
>>>> +    action->setCheckable( true );
>>>> +    action->setChecked( THEMIM->getPlayStopOnceState() );
>>>> +    if( !p_input )
>>>> +        action->setEnabled( false );
>>>> +    action->setData( ACTION_DELETE_ON_REBUILD );
>>>> +
>>>>      /* Next / Previous */
>>>>      bool bPlaylistEmpty = THEMIM->hasEmptyPlaylist();
>>>>      action = addMIMStaticEntry( p_intf, menu, qtr( "Pre&vious" ),
>>>> diff --git a/src/libvlc-module.c b/src/libvlc-module.c
>>>> index f0ee030..429732e 100644
>>>> --- a/src/libvlc-module.c
>>>> +++ b/src/libvlc-module.c
>>>>
>>>
>>> Qt and core code should be patched separately.
>>>
>>>
>>>  @@ -1136,6 +1136,10 @@ static const char *const ppsz_prefres[] = {
>>>>  #define PAS_LONGTEXT N_( \
>>>>      "Stop the playlist after each played playlist item." )
>>>>
>>>> +#define PASO_TEXT N_("Play and stop once")
>>>> +#define PASO_LONGTEXT N_( \
>>>> +    "Stop the playlist after the next played playlist item." )
>>>> +
>>>>  #define PAE_TEXT N_("Play and exit")
>>>>  #define PAE_LONGTEXT N_( \
>>>>      "Exit if there are no more items in the playlist." )
>>>> @@ -2628,6 +2632,8 @@ vlc_module_begin ()
>>>>      add_string( "bookmark10", NULL,
>>>>                BOOKMARK10_TEXT, BOOKMARK_LONGTEXT, false )
>>>>
>>>> +    add_bool( "play-and-stop-once", 0, PASO_TEXT, PASO_LONGTEXT, false
>>>> )
>>>>
>>>
>>> This should not be needed at all, since this is not a persistent
>>> parameter.
>>>
>>>
>>>  +
>>>>  #define HELP_TEXT \
>>>>      N_("print help for VLC (can be combined with --advanced and " \
>>>>         "--help-verbose)")
>>>> diff --git a/src/playlist/thread.c b/src/playlist/thread.c
>>>> index a6ecbdb..dedf3b0 100644
>>>> --- a/src/playlist/thread.c
>>>> +++ b/src/playlist/thread.c
>>>> @@ -359,6 +359,7 @@ static playlist_item_t *NextItem( playlist_t
>>>> *p_playlist )
>>>>          bool b_loop = var_GetBool( p_playlist, "loop" );
>>>>          bool b_repeat = var_GetBool( p_playlist, "repeat" );
>>>>          bool b_playstop = var_InheritBool( p_playlist, "play-and-stop"
>>>> );
>>>> +        bool b_playstoponce = var_InheritBool( p_playlist,
>>>> "play-and-stop-once" );
>>>>
>>>>          /* Repeat and play/stop */
>>>>          if( b_repeat && get_current_status_item( p_playlist ) )
>>>> @@ -371,6 +372,12 @@ static playlist_item_t *NextItem( playlist_t
>>>> *p_playlist )
>>>>              msg_Dbg( p_playlist,"stopping (play and stop)" );
>>>>              return NULL;
>>>>          }
>>>> +        else if( b_playstoponce )
>>>> +        {
>>>> +            msg_Dbg( p_playlist, "stopping (play and stop once)" );
>>>> +            var_SetBool( p_playlist, "play-and-stop-once", false );
>>>>
>>>
>>> That's a bit racy. I would add an exchange operation to var_GetAndSet().
>>>
>>>  +            return NULL;
>>>> +        }
>>>>
>>>
>>> ...then you could do:
>>>
>>> +        else if( var_ExchangeBool( p_playlist, "play-and-stop-once",
>>> false ) )
>>>
>>> +        {
>>> +            msg_Dbg( p_playlist, "stopping (play and stop once)" );
>>> +            return NULL;
>>> +        }
>>>
>>>
>>>>          /* */
>>>>          if( get_current_status_item( p_playlist ) )
>>>>
>>>
>>> --
>>> Rémi Denis-Courmont
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20140819/6d7ffe29/attachment.html>


More information about the vlc-devel mailing list