[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