Keep UI responsive while loading media

The mediaplayer caused the UI thread to become unresponsive while
loading media. This was particularly noticeable when loading network
streams, especially when connection could not be established.

The cause of the issue was that loading the file or connecting to
network was done on the UI thread. If the network resource was
unavailable, the UI thread would hang for the duration of the socket
connect timeout (5 seconds).

This patch fixes the issue by offloading media loading to a worker
thread. This is thread safe because MediaDataHolder::create is a static
function without any shared data. Only MediaDataHolder::create is
required to run on a separate thread.

Once the media is loaded, the media player is notified, and playback can
start. An extra state variable is introduced to be able to keep track of
play/pause/stop events that arrive while media is loaded in the
background.

Cancellation is not yet implemented. The result is that app shutdown and
loading another URL while the first is being processed will still cause
UI thread hang for a duration determined by the network timeout (5
seconds).

Task-number: QTBUG-116779
Pick-to: 6.5 6.6
Change-Id: I76c8d08c58d2f7795b3e3828a070b7695336978a
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Artem Dyomin <artem.dyomin@qt.io>

8 files changed, 122 insertions, 58 deletions

diff --git a/src/plugins/multimedia/ffmpeg/playbackengine/qffmpegmediadataholder.cpp b/src/plugins/multimedia/ffmpeg/playbackengine/qffmpegmediadataholder.cpp
index 8a467a06a..62830a4a5 100644
--- a/src/plugins/multimedia/ffmpeg/playbackengine/qffmpegmediadataholder.cpp
+++ b/src/plugins/multimedia/ffmpeg/playbackengine/qffmpegmediadataholder.cpp
@@ -206,14 +206,15 @@ QMaybe<AVFormatContextUPtr, MediaDataHolder::ContextError> loadMedia(const QUrl
 #endif
     return AVFormatContextUPtr{ context };
 }
-}
+} // namespace
 
-QMaybe<MediaDataHolder, MediaDataHolder::ContextError> MediaDataHolder::create(const QUrl &url,
-                                                                               QIODevice *stream)
+MediaDataHolder::Maybe MediaDataHolder::create(const QUrl &url, QIODevice *stream)
 {
     QMaybe context = loadMedia(url, stream);
-    if (context)
-        return MediaDataHolder{ std::move(context.value()) };
+    if (context) {
+        // MediaDataHolder is wrapped in a shared pointer to interop with signal/slot mechanism
+        return QSharedPointer<MediaDataHolder>{ new MediaDataHolder{ std::move(context.value()) } };
+    }
     return context.error();
 }
 
diff --git a/src/plugins/multimedia/ffmpeg/playbackengine/qffmpegmediadataholder_p.h b/src/plugins/multimedia/ffmpeg/playbackengine/qffmpegmediadataholder_p.h
index c35b1ee74..b8d12ae67 100644
--- a/src/plugins/multimedia/ffmpeg/playbackengine/qffmpegmediadataholder_p.h
+++ b/src/plugins/multimedia/ffmpeg/playbackengine/qffmpegmediadataholder_p.h
@@ -69,7 +69,8 @@ public:
 
     int currentStreamIndex(QPlatformMediaPlayer::TrackType trackType) const;
 
-    static QMaybe<MediaDataHolder, ContextError> create(const QUrl &url, QIODevice *stream);
+    using Maybe = QMaybe<QSharedPointer<MediaDataHolder>, ContextError>;
+    static Maybe create(const QUrl &url, QIODevice *stream);
 
     bool setActiveTrack(QPlatformMediaPlayer::TrackType type, int streamNumber);
 
diff --git a/src/plugins/multimedia/ffmpeg/qffmpegaudiodecoder.cpp b/src/plugins/multimedia/ffmpeg/qffmpegaudiodecoder.cpp
index c40826383..49bf151b0 100644
--- a/src/plugins/multimedia/ffmpeg/qffmpegaudiodecoder.cpp
+++ b/src/plugins/multimedia/ffmpeg/qffmpegaudiodecoder.cpp
@@ -137,7 +137,15 @@ void QFFmpegAudioDecoder::start()
     connect(m_decoder.get(), &AudioDecoder::newAudioBuffer, this,
             &QFFmpegAudioDecoder::newAudioBuffer);
 
-    m_decoder->setMedia(m_url, m_sourceDevice);
+    QFFmpeg::MediaDataHolder::Maybe media = QFFmpeg::MediaDataHolder::create(m_url, m_sourceDevice);
+
+    if (media)
+        m_decoder->setMedia(std::move(*media.value()));
+    else {
+        auto [code, description] = media.error();
+        errorSignal(code, description);
+    }
+
     if (!checkNoError())
         return;
 
diff --git a/src/plugins/multimedia/ffmpeg/qffmpegmediaplayer.cpp b/src/plugins/multimedia/ffmpeg/qffmpegmediaplayer.cpp
index 171f082d5..ca3b66fee 100644
--- a/src/plugins/multimedia/ffmpeg/qffmpegmediaplayer.cpp
+++ b/src/plugins/multimedia/ffmpeg/qffmpegmediaplayer.cpp
@@ -10,6 +10,7 @@
 #include <qiodevice.h>
 #include <qvideosink.h>
 #include <qtimer.h>
+#include <QtConcurrent/QtConcurrent>
 
 #include <qloggingcategory.h>
 
@@ -25,7 +26,10 @@ QFFmpegMediaPlayer::QFFmpegMediaPlayer(QMediaPlayer *player)
     connect(&m_positionUpdateTimer, &QTimer::timeout, this, &QFFmpegMediaPlayer::updatePosition);
 }
 
-QFFmpegMediaPlayer::~QFFmpegMediaPlayer() = default;
+QFFmpegMediaPlayer::~QFFmpegMediaPlayer()
+{
+    m_loadMedia.waitForFinished(); // TODO: Add cancellation
+};
 
 qint64 QFFmpegMediaPlayer::duration() const
 {
@@ -34,6 +38,9 @@ qint64 QFFmpegMediaPlayer::duration() const
 
 void QFFmpegMediaPlayer::setPosition(qint64 position)
 {
+    if (mediaStatus() == QMediaPlayer::LoadingMedia)
+        return;
+
     if (m_playbackEngine) {
         m_playbackEngine->seek(position * 1000);
         updatePosition();
@@ -104,28 +111,57 @@ const QIODevice *QFFmpegMediaPlayer::mediaStream() const
     return m_device;
 }
 
+void QFFmpegMediaPlayer::handleIncorrectMedia(QMediaPlayer::MediaStatus status)
+{
+    seekableChanged(false);
+    audioAvailableChanged(false);
+    videoAvailableChanged(false);
+    metaDataChanged();
+    mediaStatusChanged(status);
+    m_playbackEngine = nullptr;
+};
+
 void QFFmpegMediaPlayer::setMedia(const QUrl &media, QIODevice *stream)
 {
+    // Wait for previous unfinished load attempts. TODO: add cancellation
+    m_loadMedia.waitForFinished();
+
     m_url = media;
     m_device = stream;
     m_playbackEngine = nullptr;
 
-    positionChanged(0);
-
-    auto handleIncorrectMedia = [this](QMediaPlayer::MediaStatus status) {
-        seekableChanged(false);
-        audioAvailableChanged(false);
-        videoAvailableChanged(false);
-        metaDataChanged();
-        mediaStatusChanged(status);
-    };
-
     if (media.isEmpty() && !stream) {
         handleIncorrectMedia(QMediaPlayer::NoMedia);
         return;
     }
 
     mediaStatusChanged(QMediaPlayer::LoadingMedia);
+
+    m_requestedStatus = QMediaPlayer::StoppedState;
+
+    // Load media asynchronously to keep GUI thread responsive while loading media
+    m_loadMedia = QtConcurrent::run(
+            [this](const QUrl &media, QIODevice *stream) {
+                // On worker thread
+                const MediaDataHolder::Maybe mediaHolder = MediaDataHolder::create(media, stream);
+
+                // Transition back to calling thread using invokeMethod because
+                // QFuture continuations back on calling thread may deadlock (QTBUG-117918)
+                QMetaObject::invokeMethod(this, [this, mediaHolder] {
+                    setMediaAsync(mediaHolder);
+                });
+            }, media, stream);
+}
+
+void QFFmpegMediaPlayer::setMediaAsync(QFFmpeg::MediaDataHolder::Maybe mediaDataHolder)
+{
+    if (!mediaDataHolder) {
+        const auto [code, description] = mediaDataHolder.error();
+        error(code, description);
+        handleIncorrectMedia(QMediaPlayer::MediaStatus::InvalidMedia);
+        return;
+    }
+
     m_playbackEngine = std::make_unique<PlaybackEngine>();
 
     connect(m_playbackEngine.get(), &PlaybackEngine::endOfStream, this,
@@ -135,11 +171,7 @@ void QFFmpegMediaPlayer::setMedia(const QUrl &media, QIODevice *stream)
     connect(m_playbackEngine.get(), &PlaybackEngine::loopChanged, this,
             &QFFmpegMediaPlayer::onLoopChanged);
 
-    if (!m_playbackEngine->setMedia(media, stream)) {
-        m_playbackEngine.reset();
-        handleIncorrectMedia(QMediaPlayer::InvalidMedia);
-        return;
-    }
+    m_playbackEngine->setMedia(std::move(*mediaDataHolder.value()));
 
     m_playbackEngine->setAudioSink(m_audioOutput);
     m_playbackEngine->setVideoSink(m_videoSink);
@@ -156,12 +188,23 @@ void QFFmpegMediaPlayer::setMedia(const QUrl &media, QIODevice *stream)
     videoAvailableChanged(
             !m_playbackEngine->streamInfo(QPlatformMediaPlayer::VideoStream).isEmpty());
 
-    // TODO: get rid of the delayed update
-    QMetaObject::invokeMethod(this, "delayedLoadedStatus", Qt::QueuedConnection);
+    mediaStatusChanged(QMediaPlayer::LoadedMedia);
+
+    if (m_requestedStatus != QMediaPlayer::StoppedState) {
+        if (m_requestedStatus == QMediaPlayer::PlayingState)
+            play();
+        else if (m_requestedStatus == QMediaPlayer::PausedState)
+            pause();
+    }
 }
 
 void QFFmpegMediaPlayer::play()
 {
+    if (mediaStatus() == QMediaPlayer::LoadingMedia) {
+        m_requestedStatus = QMediaPlayer::PlayingState;
+        return;
+    }
+
     if (!m_playbackEngine)
         return;
 
@@ -183,8 +226,14 @@ void QFFmpegMediaPlayer::runPlayback()
 
 void QFFmpegMediaPlayer::pause()
 {
+    if (mediaStatus() == QMediaPlayer::LoadingMedia) {
+        m_requestedStatus = QMediaPlayer::PausedState;
+        return;
+    }
+
     if (!m_playbackEngine)
         return;
+
     if (mediaStatus() == QMediaPlayer::EndOfMedia && state() == QMediaPlayer::StoppedState) {
         m_playbackEngine->seek(0);
         positionChanged(0);
@@ -197,8 +246,14 @@ void QFFmpegMediaPlayer::pause()
 
 void QFFmpegMediaPlayer::stop()
 {
+    if (mediaStatus() == QMediaPlayer::LoadingMedia) {
+        m_requestedStatus = QMediaPlayer::StoppedState;
+        return;
+    }
+
     if (!m_playbackEngine)
         return;
+
     m_playbackEngine->stop();
     m_positionUpdateTimer.stop();
     positionChanged(0);
diff --git a/src/plugins/multimedia/ffmpeg/qffmpegmediaplayer_p.h b/src/plugins/multimedia/ffmpeg/qffmpegmediaplayer_p.h
index b330948f6..69945ae43 100644
--- a/src/plugins/multimedia/ffmpeg/qffmpegmediaplayer_p.h
+++ b/src/plugins/multimedia/ffmpeg/qffmpegmediaplayer_p.h
@@ -19,13 +19,16 @@
 #include <qmediametadata.h>
 #include <qtimer.h>
 #include <qpointer.h>
+#include <qfuture.h>
 #include "qffmpeg_p.h"
+#include "playbackengine/qffmpegmediadataholder_p.h"
 
 QT_BEGIN_NAMESPACE
 
 namespace QFFmpeg {
 class PlaybackEngine;
 }
+
 class QPlatformAudioOutput;
 
 class QFFmpegMediaPlayer : public QObject, public QPlatformMediaPlayer
@@ -67,13 +70,10 @@ public:
     void setActiveTrack(TrackType, int streamNumber) override;
     void setLoops(int loops) override;
 
-    Q_INVOKABLE void delayedLoadedStatus() {
-        if (mediaStatus() == QMediaPlayer::LoadingMedia)
-            mediaStatusChanged(QMediaPlayer::LoadedMedia);
-    }
-
 private:
     void runPlayback();
+    void handleIncorrectMedia(QMediaPlayer::MediaStatus status);
+    void setMediaAsync(QFFmpeg::MediaDataHolder::Maybe mediaDataHolder);
 
 private slots:
     void updatePosition();
@@ -86,6 +86,7 @@ private slots:
 
 private:
     QTimer m_positionUpdateTimer;
+    QMediaPlayer::PlaybackState m_requestedStatus = QMediaPlayer::StoppedState;
 
     using PlaybackEngine = QFFmpeg::PlaybackEngine;
 
@@ -96,6 +97,7 @@ private:
     QUrl m_url;
     QPointer<QIODevice> m_device;
     float m_playbackRate = 1.;
+    QFuture<void> m_loadMedia;
 };
 
 QT_END_NAMESPACE
diff --git a/src/plugins/multimedia/ffmpeg/qffmpegplaybackengine.cpp b/src/plugins/multimedia/ffmpeg/qffmpegplaybackengine.cpp
index 91d444d9b..964125bff 100644
--- a/src/plugins/multimedia/ffmpeg/qffmpegplaybackengine.cpp
+++ b/src/plugins/multimedia/ffmpeg/qffmpegplaybackengine.cpp
@@ -457,30 +457,14 @@ void PlaybackEngine::deleteFreeThreads() {
         thr->wait();
 }
 
-bool PlaybackEngine::setMedia(const QUrl &media, QIODevice *stream)
+void PlaybackEngine::setMedia(MediaDataHolder media)
 {
-    stop();
-
-    // we should wait for objects deleting instead
-    // optimized solution might be implemented in the future.
-    deleteFreeThreads();
-
-    m_codecs = {};
-    m_media = {};
-
-    QMaybe mediaHolder = MediaDataHolder::create(media, stream);
-
-    if (mediaHolder) {
-        m_media = std::move(mediaHolder.value());
-    } else {
-        const auto [code, description] = mediaHolder.error();
-        emit errorOccured(code, description);
-        return false;
-    }
+    Q_ASSERT(!m_media.avContext()); // Playback engine does not support reloading media
+    Q_ASSERT(m_state == QMediaPlayer::StoppedState);
+    Q_ASSERT(m_threads.empty());
 
+    m_media = std::move(media);
     updateVideoSinkSize();
-
-    return true;
 }
 
 void PlaybackEngine::setVideoSink(QVideoSink *sink)
diff --git a/src/plugins/multimedia/ffmpeg/qffmpegplaybackengine_p.h b/src/plugins/multimedia/ffmpeg/qffmpegplaybackengine_p.h
index 1a8f10619..1317e7715 100644
--- a/src/plugins/multimedia/ffmpeg/qffmpegplaybackengine_p.h
+++ b/src/plugins/multimedia/ffmpeg/qffmpegplaybackengine_p.h
@@ -38,7 +38,7 @@
  * OBJECTS WEAK CONNECTIVITY
  *
  * - The objects know nothing about others and about PlaybackEngine.
- *   For any interractions the objects use slots/signals.
+ *   For any interactions the objects use slots/signals.
  *
  * - PlaybackEngine knows the objects object and is able to create/delete them and
  *   call their public methods.
@@ -73,7 +73,7 @@ public:
 
     ~PlaybackEngine() override;
 
-    bool setMedia(const QUrl &media, QIODevice *stream);
+    void setMedia(MediaDataHolder media);
 
     void setVideoSink(QVideoSink *sink);
 
diff --git a/tests/auto/integration/qmediaplayerbackend/tst_qmediaplayerbackend.cpp b/tests/auto/integration/qmediaplayerbackend/tst_qmediaplayerbackend.cpp
index cc5fff0ab..2e79e55c2 100644
--- a/tests/auto/integration/qmediaplayerbackend/tst_qmediaplayerbackend.cpp
+++ b/tests/auto/integration/qmediaplayerbackend/tst_qmediaplayerbackend.cpp
@@ -54,7 +54,7 @@ private slots:
     void setSource_emitsError_whenCalledWithInvalidFile();
     void setSource_emitsMediaStatusChange_whenCalledWithInvalidFile();
     void setSource_doesNotEmitPlaybackStateChange_whenCalledWithInvalidFile();
-    void setSouce_setsSourceMediaStatusAndError_whenCalledWithInvalidFile();
+    void setSource_setsSourceMediaStatusAndError_whenCalledWithInvalidFile();
     void pause_doesNotChangePlayerState_whenInvalidFileLoaded();
     void play_resetsErrorState_whenCalledWithInvalidFile();
     void loadInvalidMediaWhilePlayingAndRestore();
@@ -311,7 +311,7 @@ void tst_QMediaPlayerBackend::setSource_doesNotEmitPlaybackStateChange_whenCalle
     QVERIFY(m_fixture->playbackStateChanged.empty());
 }
 
-void tst_QMediaPlayerBackend::setSouce_setsSourceMediaStatusAndError_whenCalledWithInvalidFile()
+void tst_QMediaPlayerBackend::setSource_setsSourceMediaStatusAndError_whenCalledWithInvalidFile()
 {
     if (!isWavSupported())
         QSKIP("Sound format is not supported");
@@ -483,7 +483,7 @@ void tst_QMediaPlayerBackend::loadMediaWhilePlaying()
 
     m_fixture->player.setSource(m_localVideoFile3ColorsWithSound);
     m_fixture->player.play();
-    QCOMPARE(m_fixture->player.playbackState(), QMediaPlayer::PlayingState);
+    QTRY_COMPARE_EQ(m_fixture->player.playbackState(), QMediaPlayer::PlayingState);
     QVERIFY(m_fixture->surface.waitForFrame().isValid());
     QVERIFY(m_fixture->player.hasAudio());
     QVERIFY(m_fixture->player.hasVideo());
@@ -491,6 +491,9 @@ void tst_QMediaPlayerBackend::loadMediaWhilePlaying()
     m_fixture->clearSpies();
 
     m_fixture->player.setSource(m_localWavFile2);
+
+    QTRY_COMPARE_EQ(m_fixture->player.mediaStatus(), QMediaPlayer::MediaStatus::LoadedMedia);
+
     QCOMPARE(m_fixture->player.source(), m_localWavFile2);
     QCOMPARE(m_fixture->player.playbackState(), QMediaPlayer::StoppedState);
     QCOMPARE(m_fixture->playbackStateChanged.size(), 1);
@@ -502,6 +505,9 @@ void tst_QMediaPlayerBackend::loadMediaWhilePlaying()
     m_fixture->player.play();
 
     m_fixture->player.setSource(m_localVideoFile2);
+
+    QTRY_COMPARE_EQ(m_fixture->player.mediaStatus(), QMediaPlayer::MediaStatus::LoadedMedia);
+
     QCOMPARE(m_fixture->player.playbackState(), QMediaPlayer::StoppedState);
     QVERIFY(m_fixture->player.hasVideo());
     QVERIFY(!m_fixture->player.hasAudio());
@@ -545,7 +551,7 @@ void tst_QMediaPlayerBackend::playPauseStop()
 
     m_fixture->player.play();
 
-    QCOMPARE(m_fixture->player.playbackState(), QMediaPlayer::PlayingState);
+    QTRY_COMPARE_EQ(m_fixture->player.playbackState(), QMediaPlayer::PlayingState);
 
     QTRY_COMPARE(m_fixture->player.mediaStatus(), QMediaPlayer::BufferedMedia);
 
@@ -1755,6 +1761,8 @@ void tst_QMediaPlayerBackend::durationDetectionIssues()
     player.setAudioOutput(&output);
     player.setSource(videoWithDurationIssues);
 
+    QTRY_COMPARE_EQ(player.mediaStatus(), QMediaPlayer::LoadedMedia);
+
     // Duration event received
     QCOMPARE(durationSpy.size(), 1);
     QCOMPARE(durationSpy.front().front(), expectedDuration);
@@ -2087,7 +2095,7 @@ void tst_QMediaPlayerBackend::lazyLoadVideo()
     QQuickItem *videoPlayer = qobject_cast<QQuickItem *>(loader->findChild<QQuickItem *>("videoPlayer"));
     QVERIFY(videoPlayer);
 
-    QCOMPARE(QQmlProperty::read(videoPlayer, "playbackState").value<QMediaPlayer::PlaybackState>(), QMediaPlayer::PlayingState);
+    QTRY_COMPARE_EQ(QQmlProperty::read(videoPlayer, "playbackState").value<QMediaPlayer::PlaybackState>(), QMediaPlayer::PlayingState);
     QCOMPARE(QQmlProperty::read(videoPlayer, "error").value<QMediaPlayer::Error>(), QMediaPlayer::NoError);
 
     QVideoSink *videoSink = QQmlProperty::read(videoPlayer, "videoSink").value<QVideoSink *>();
@@ -2118,6 +2126,8 @@ void tst_QMediaPlayerBackend::videoSinkSignals()
 
     player.setSource(m_localVideoFile2);
 
+    QTRY_COMPARE(player.mediaStatus(), QMediaPlayer::MediaStatus::LoadedMedia);
+
     sink.platformVideoSink()->setNativeSize({}); // reset size to be able to check the size update
 
     connect(&sink, &QVideoSink::videoFrameChanged, this, [&](const QVideoFrame &frame) {
@@ -2168,6 +2178,9 @@ void tst_QMediaPlayerBackend::setMedia_setsVideoSinkSize_beforePlaying()
     QCOMPARE(sink1.videoSize(), QSize());
 
     player.setSource(m_localVideoFile3ColorsWithSound);
+
+    QTRY_COMPARE(player.mediaStatus(), QMediaPlayer::MediaStatus::LoadedMedia);
+
     QCOMPARE(sink1.videoSize(), QSize(684, 384));
 
     player.setVideoOutput(&sink2);