Fix threading problems of QVideoSink

- change notification signals should be emitted out of the mutex
  since it causes a deadlock if the user connects directly and
  call QVideoSink getters.
- currentVideoFrame should be under the mutex since frames assignment
  is not atomic.
- minor clean up with moving to cpp file.

Pick-to: 6.5
Change-Id: I07a7e54108c100cd18dc71c88acd93a5622bd2f5
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Lars Knoll <lars@knoll.priv.no>

3 files changed, 112 insertions, 42 deletions

diff --git a/src/multimedia/platform/qplatformvideosink.cpp b/src/multimedia/platform/qplatformvideosink.cpp
index fb63cf193..268291da2 100644
--- a/src/multimedia/platform/qplatformvideosink.cpp
+++ b/src/multimedia/platform/qplatformvideosink.cpp
@@ -5,10 +5,67 @@
 
 QT_BEGIN_NAMESPACE
 
-QPlatformVideoSink::QPlatformVideoSink(QVideoSink *parent)
-    : QObject(parent),
-    sink(parent)
+QPlatformVideoSink::QPlatformVideoSink(QVideoSink *parent) : QObject(parent), m_sink(parent) { }
+
+QPlatformVideoSink::~QPlatformVideoSink() = default;
+
+QSize QPlatformVideoSink::nativeSize() const
+{
+    QMutexLocker locker(&m_mutex);
+    return m_nativeSize;
+}
+
+void QPlatformVideoSink::setNativeSize(QSize s)
+{
+    {
+        QMutexLocker locker(&m_mutex);
+        if (m_nativeSize == s)
+            return;
+        m_nativeSize = s;
+    }
+    emit m_sink->videoSizeChanged();
+}
+
+void QPlatformVideoSink::setVideoFrame(const QVideoFrame &frame)
+{
+    bool sizeChanged = false;
+
+    {
+        QMutexLocker locker(&m_mutex);
+        if (frame == m_currentVideoFrame)
+            return;
+        m_currentVideoFrame = frame;
+        m_currentVideoFrame.setSubtitleText(m_subtitleText);
+        sizeChanged = m_nativeSize != frame.size();
+        m_nativeSize = frame.size();
+    }
+
+    if (sizeChanged)
+        emit m_sink->videoSizeChanged();
+    emit m_sink->videoFrameChanged(frame);
+}
+
+QVideoFrame QPlatformVideoSink::currentVideoFrame() const
+{
+    QMutexLocker locker(&m_mutex);
+    return m_currentVideoFrame;
+}
+
+void QPlatformVideoSink::setSubtitleText(const QString &subtitleText)
+{
+    {
+        QMutexLocker locker(&m_mutex);
+        if (m_subtitleText == subtitleText)
+            return;
+        m_subtitleText = subtitleText;
+    }
+    emit m_sink->subtitleTextChanged(subtitleText);
+}
+
+QString QPlatformVideoSink::subtitleText() const
 {
+    QMutexLocker locker(&m_mutex);
+    return m_subtitleText;
 }
 
 QT_END_NAMESPACE
diff --git a/src/multimedia/platform/qplatformvideosink_p.h b/src/multimedia/platform/qplatformvideosink_p.h
index 1a99e3434..4477b28cb 100644
--- a/src/multimedia/platform/qplatformvideosink_p.h
+++ b/src/multimedia/platform/qplatformvideosink_p.h
@@ -36,6 +36,8 @@ class Q_MULTIMEDIA_EXPORT QPlatformVideoSink : public QObject
     Q_OBJECT
 
 public:
+    ~QPlatformVideoSink() override;
+
     virtual void setRhi(QRhi * /*rhi*/) {}
 
     virtual void setWinId(WId) {}
@@ -43,55 +45,31 @@ public:
     virtual void setFullScreen(bool) {}
     virtual void setAspectRatioMode(Qt::AspectRatioMode) {}
 
-    QSize nativeSize() const
-    {
-        QMutexLocker locker(&mutex);
-        return m_nativeSize;
-    }
+    QSize nativeSize() const;
 
     virtual void setBrightness(float /*brightness*/) {}
     virtual void setContrast(float /*contrast*/) {}
     virtual void setHue(float /*hue*/) {}
     virtual void setSaturation(float /*saturation*/) {}
 
-    QVideoSink *videoSink() { return sink; }
-
-    void setNativeSize(QSize s) {
-        QMutexLocker locker(&mutex);
-        if (m_nativeSize == s)
-            return;
-        m_nativeSize = s;
-        emit sink->videoSizeChanged();
-    }
-    virtual void setVideoFrame(const QVideoFrame &frame) {
-        setNativeSize(frame.size());
-        if (frame == m_currentVideoFrame)
-            return;
-        m_currentVideoFrame = frame;
-        m_currentVideoFrame.setSubtitleText(subtitleText());
-        emit sink->videoFrameChanged(m_currentVideoFrame);
-    }
-    QVideoFrame currentVideoFrame() const { return m_currentVideoFrame; }
-
-    void setSubtitleText(const QString &subtitleText)
-    {
-        QMutexLocker locker(&mutex);
-        if (m_subtitleText == subtitleText)
-            return;
-        m_subtitleText = subtitleText;
-        emit sink->subtitleTextChanged(subtitleText);
-    }
-    QString subtitleText() const
-    {
-        QMutexLocker locker(&mutex);
-        return m_subtitleText;
-    }
+    QVideoSink *videoSink() { return m_sink; }
+
+    void setNativeSize(QSize s);
+
+    virtual void setVideoFrame(const QVideoFrame &frame);
+
+    QVideoFrame currentVideoFrame() const;
+
+    void setSubtitleText(const QString &subtitleText);
+
+    QString subtitleText() const;
 
 protected:
     explicit QPlatformVideoSink(QVideoSink *parent);
-    QVideoSink *sink = nullptr;
-    mutable QMutex mutex;
+
 private:
+    QVideoSink *m_sink = nullptr;
+    mutable QMutex m_mutex;
     QSize m_nativeSize;
     QString m_subtitleText;
     QVideoFrame m_currentVideoFrame;
diff --git a/tests/auto/integration/qmediaplayerbackend/tst_qmediaplayerbackend.cpp b/tests/auto/integration/qmediaplayerbackend/tst_qmediaplayerbackend.cpp
index a19ff44a8..a3e005929 100644
--- a/tests/auto/integration/qmediaplayerbackend/tst_qmediaplayerbackend.cpp
+++ b/tests/auto/integration/qmediaplayerbackend/tst_qmediaplayerbackend.cpp
@@ -78,6 +78,7 @@ private slots:
     void seekOnLoops();
     void changeLoopsOnTheFly();
     void lazyLoadVideo();
+    void videoSinkSignals();
 
 private:
     QUrl selectVideoFile(const QStringList& mediaCandidates);
@@ -1883,6 +1884,40 @@ void tst_QMediaPlayerBackend::lazyLoadVideo()
     QVERIFY(frame.isValid());
 }
 
+void tst_QMediaPlayerBackend::videoSinkSignals()
+{
+    // TODO: come up with custom frames source,
+    // create the test target tst_QVideoSinkBackend,
+    // and move the test there
+
+    if (localVideoFile2.isEmpty())
+        QSKIP("Video format is not supported");
+
+    QVideoSink sink;
+    QMediaPlayer player;
+    player.setVideoSink(&sink);
+
+    std::atomic<int> videoFrameCounter = 0;
+    std::atomic<int> videoSizeCounter = 0;
+
+    connect(&sink, &QVideoSink::videoFrameChanged, [&](const QVideoFrame &frame) {
+        QCOMPARE(sink.videoFrame(), frame);
+        QCOMPARE(sink.videoSize(), frame.size());
+        ++videoFrameCounter;
+    });
+
+    connect(&sink, &QVideoSink::videoSizeChanged, [&]() {
+        QCOMPARE(sink.videoSize(), sink.videoFrame().size());
+        ++videoSizeCounter;
+    });
+
+    player.setSource(localVideoFile2);
+    player.play();
+
+    QTRY_COMPARE_GE(videoFrameCounter, 2);
+    QCOMPARE(videoSizeCounter, 1);
+}
+
 QTEST_MAIN(tst_QMediaPlayerBackend)
 #include "tst_qmediaplayerbackend.moc"