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

[Alexandre Janniaux]

Hi,

Thank you for review,

On Fri, Mar 20, 2020 at 02:57:01PM +0100, Pierre Lamot wrote:
> 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)

Yes, you're right, I'll fix this in one commit.

> > +    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

The file has both kind of check. To be honest, I stopped using the
implicit conversion to boolean the day there was confusion in overloaded
function, and now follow Hugo's convention. If you prefer I can change it
the boolean operator but it might not bring anything unless a convention
is chosen for the file.


Regards,
--
Alexandre Janniaux
Videolabs

> 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;