[vlc-devel] [PATCH 1/2] Add a 'Play and stop once' feature to the Qt interface
David Sferruzza
david.sferruzza at gmail.com
Thu Jul 24 11:49:43 CEST 2014
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/20140724/108d39cd/attachment.html>
More information about the vlc-devel
mailing list