[vlc-devel] [PATCH] qt: player_controller: check item before restore playback position

[Pierre Lamot]

Hi,

Shouldn't you apply the same principle for other player accessor ?
vlc_player_ChangeRate, vlc_player_SetPosition, vlc_player_JumpTime, 
etc...

IMHO, this is either all or nothing (but still ensure not restoring the 
position of a null intput in vlc_player_RestorePlaybackPos)


On 2020-03-20 13:50, Alexandre Janniaux wrote:
> The restoration request is emitted from the UI, and the player event
> signalling that the input item changed is emitted from a player thread
> and asynchronously processed later by the UI thread. It means that the
> restoration request could happen although there was no input item to
> restore anymore.
> 
> Store the input item currently acknowledged by the UI thread and check
> it against the current item being played by the player when the
> restoration is requested by the UI. If they don't match, avoid
> processing the restoration request.
> 
> Note that another solution would have been to use the item currently
> acknowledged by the UI and restore it by rolling back the previous
> playlist state to ensure the media is current. As it is far more 
> complex
> to implement and provide zero benefit in normal playback, where the
> media is likely to be restored at the beginning of the playback, this
> other implementation was chosen.
> 
> Fixes #24347
> ---
>  modules/gui/qt/player/player_controller.cpp   | 17 +++++++++++++++++
>  modules/gui/qt/player/player_controller_p.hpp |  5 +++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/modules/gui/qt/player/player_controller.cpp
> b/modules/gui/qt/player/player_controller.cpp
> index 724232062b..eccbcd586d 100644
> --- a/modules/gui/qt/player/player_controller.cpp
> +++ b/modules/gui/qt/player/player_controller.cpp
> @@ -241,6 +241,8 @@ static  void
> on_player_current_media_changed(vlc_player_t *, input_item_t *new_m
>      {
>          that->callAsync([that] () {
>              emit that->q_func()->inputChanged(false);
> +            vlc_player_locker lock{ that->m_player };
> +            that->m_currentItem.reset(nullptr);
>          });
>          return;
>      }
> @@ -252,6 +254,11 @@ static  void
> on_player_current_media_changed(vlc_player_t *, input_item_t *new_m
>          that->UpdateArt( newMediaPtr.get() );
>          that->UpdateMeta( newMediaPtr.get() );
> 
> +        {
> +            vlc_player_locker lock{ that->m_player };
> +            that->m_currentItem = std::move(newMediaPtr);
> +        }
> +
>          that->m_canRestorePlayback = false;
>          emit q->playbackRestoreQueried();
> 
> @@ -1521,6 +1528,16 @@ void PlayerController::restorePlaybackPos()
>  {
>      Q_D(PlayerController);
>      vlc_player_locker lock{ d->m_player };
> +
> +    /* The media can change before the UI gets updated and thus a
> +     * restorePlayback request can be submitted on the wrong media or 
> worse,
> +     * no media at all. Here d->m_currentItem is locked by the player 
> lock. */
> +    if (d->m_currentItem.get() == nullptr ||



nit, vlc_shared_data_ptr_type has a bool operator, maybe

> if (!d->m_currentItem ||

would be more concise


> +        d->m_currentItem.get() != vlc_player_GetCurrentMedia( 
> d->m_player ))
> +    {
> +        return;
> +    }
> +
>      vlc_player_RestorePlaybackPos( d->m_player );
>  }
> 
> diff --git a/modules/gui/qt/player/player_controller_p.hpp
> b/modules/gui/qt/player/player_controller_p.hpp
> index c3f3dd4ea4..1f8a6020cc 100644
> --- a/modules/gui/qt/player/player_controller_p.hpp
> +++ b/modules/gui/qt/player/player_controller_p.hpp
> @@ -82,6 +82,11 @@ public:
>      float           m_position = 0.f;
>      VLCTick      m_length= 0;
> 
> +    using InputItemPtr = vlc_shared_data_ptr_type(input_item_t,
> +                                                  input_item_Hold,
> +                                                  input_item_Release);
> +
> +    InputItemPtr    m_currentItem;
>      bool            m_canRestorePlayback = false;
> 
>      int             m_capabilities = 0;