Fix mapping of QVideoFrame in write mode

After mapping of QVideoFrame with a write mode, we get this:
- the hw frame in QFFmpegVideoBuffer is not valid anymore
- the cached QImage is not valid anymore

The implemented approach might be revisited if we decide to
implement copy-on-modify idiom for QVideoFrame instead of
buffer sharing.

Fixes: QTBUG-123131
Pick-to: 6.5
Change-Id: I1c1662c5a4ac6f5bffa2cea8e36c4fbf60c20f64
Reviewed-by: Jøger Hansegård <joger.hansegard@qt.io>
Reviewed-by: Artem Dyomin <artem.dyomin@qt.io>
(cherry picked from commit 537fae96a9fabec2203460e786ff62075cf43bb8)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
(cherry picked from commit fab367331f0f781be8ab0af62dfd8226013e5a3a)

4 files changed, 102 insertions, 7 deletions

diff --git a/src/multimedia/video/qvideoframe.cpp b/src/multimedia/video/qvideoframe.cpp
index 348baf023..7f93882bb 100644
--- a/src/multimedia/video/qvideoframe.cpp
+++ b/src/multimedia/video/qvideoframe.cpp
@@ -427,6 +427,15 @@ bool QVideoFrame::map(QVideoFrame::MapMode mode)
     }
 
     d->mappedCount++;
+
+    // unlock mapMutex to avoid potential deadlock imageMutex <--> mapMutex
+    lock.unlock();
+
+    if ((mode & QVideoFrame::WriteOnly) != 0) {
+        QMutexLocker lock(&d->imageMutex);
+        d->image = {};
+    }
+
     return true;
 }
 
@@ -648,10 +657,12 @@ QImage QVideoFrame::toImage() const
     if (!isValid())
         return {};
 
-    std::call_once(d->imageOnceFlag, [this]() {
+    QMutexLocker lock(&d->imageMutex);
+
+    if (d->image.isNull()) {
         const bool mirrorY = surfaceFormat().scanLineDirection() != QVideoFrameFormat::TopToBottom;
         d->image = qImageFromVideoFrame(*this, QtVideo::Rotation(rotationAngle()), mirrored(), mirrorY);
-    });
+    }
 
     return d->image;
 }
diff --git a/src/multimedia/video/qvideoframe_p.h b/src/multimedia/video/qvideoframe_p.h
index 4bb0619c9..6eb8ddf51 100644
--- a/src/multimedia/video/qvideoframe_p.h
+++ b/src/multimedia/video/qvideoframe_p.h
@@ -21,7 +21,6 @@
 #include "private/qtvideo_p.h"
 
 #include <qmutex.h>
-#include <mutex> // std::once
 
 QT_BEGIN_NAMESPACE
 
@@ -53,7 +52,7 @@ public:
     QtVideo::Rotation rotation = QtVideo::Rotation::None;
     bool mirrored = false;
     QImage image;
-    std::once_flag imageOnceFlag;
+    QMutex imageMutex;
 
 private:
     Q_DISABLE_COPY(QVideoFramePrivate)
diff --git a/src/plugins/multimedia/ffmpeg/qffmpegvideobuffer.cpp b/src/plugins/multimedia/ffmpeg/qffmpegvideobuffer.cpp
index b7fca2c53..98c074312 100644
--- a/src/plugins/multimedia/ffmpeg/qffmpegvideobuffer.cpp
+++ b/src/plugins/multimedia/ffmpeg/qffmpegvideobuffer.cpp
@@ -5,6 +5,7 @@
 #include "private/qvideotexturehelper_p.h"
 #include "private/qmultimediautils_p.h"
 #include "qffmpeghwaccel_p.h"
+#include "qloggingcategory.h"
 
 extern "C" {
 #include <libavutil/pixdesc.h>
@@ -12,6 +13,8 @@ extern "C" {
 #include <libavutil/mastering_display_metadata.h>
 }
 
+QT_BEGIN_NAMESPACE
+
 static bool isFrameFlipped(const AVFrame& frame) {
     for (int i = 0; i < AV_NUM_DATA_POINTERS && frame.data[i]; ++i) {
         if (frame.linesize[i] < 0)
@@ -21,7 +24,7 @@ static bool isFrameFlipped(const AVFrame& frame) {
     return false;
 }
 
-QT_BEGIN_NAMESPACE
+static Q_LOGGING_CATEGORY(qLcFFmpegVideoBuffer, "qt.multimedia.ffmpeg.videobuffer");
 
 QFFmpegVideoBuffer::QFFmpegVideoBuffer(AVFrameUPtr frame, AVRational pixelAspectRatio)
     : QAbstractVideoBuffer(QVideoFrame::NoHandle),
@@ -196,6 +199,19 @@ QAbstractVideoBuffer::MapData QFFmpegVideoBuffer::map(QVideoFrame::MapMode mode)
         mapData.bytesPerLine[i] = m_swFrame->linesize[i];
         mapData.size[i] = mapData.bytesPerLine[i]*desc->heightForPlane(m_swFrame->height, i);
     }
+
+    if ((mode & QVideoFrame::WriteOnly) != 0 && m_hwFrame) {
+        m_type = QVideoFrame::NoHandle;
+        m_hwFrame.reset();
+        if (m_textures) {
+            qCDebug(qLcFFmpegVideoBuffer)
+                    << "Mapping of FFmpeg video buffer with write mode when "
+                       "textures have been created. Visual artifacts might "
+                       "happen if the frame is still in the rendering pipeline";
+            m_textures.reset();
+        }
+    }
+
     return mapData;
 }
 
diff --git a/tests/auto/integration/qvideoframebackend/tst_qvideoframebackend.cpp b/tests/auto/integration/qvideoframebackend/tst_qvideoframebackend.cpp
index 51940e6b4..60b55f0a1 100644
--- a/tests/auto/integration/qvideoframebackend/tst_qvideoframebackend.cpp
+++ b/tests/auto/integration/qvideoframebackend/tst_qvideoframebackend.cpp
@@ -8,6 +8,7 @@
 
 #include "mediafileselector.h"
 #include "testvideosink.h"
+#include "private/qvideotexturehelper_p.h"
 
 QT_USE_NAMESPACE
 
@@ -26,6 +27,9 @@ private slots:
     void toImage_retainsThePreviousMappedState_data();
     void toImage_retainsThePreviousMappedState();
 
+    void toImage_rendersUpdatedFrame_afterMappingInWriteModeAndModifying_data();
+    void toImage_rendersUpdatedFrame_afterMappingInWriteModeAndModifying();
+
 private:
     QVideoFrame createDefaultFrame() const;
 
@@ -123,18 +127,83 @@ void tst_QVideoFrameBackend::toImage_retainsThePreviousMappedState()
     QFETCH(const QVideoFrame::MapMode, initialMapMode);
     const bool initiallyMapped = initialMapMode != QVideoFrame::NotMapped;
 
-    auto frame = std::invoke(frameCreator, this);
+    QVideoFrame frame = std::invoke(frameCreator, this);
     QVERIFY(frame.isValid());
 
     frame.map(initialMapMode);
     QCOMPARE(frame.mapMode(), initialMapMode);
 
-    auto image = frame.toImage();
+    QImage image = frame.toImage();
     QVERIFY(!image.isNull());
 
     QCOMPARE(frame.mapMode(), initialMapMode);
     QCOMPARE(frame.isMapped(), initiallyMapped);
 }
 
+void tst_QVideoFrameBackend::toImage_rendersUpdatedFrame_afterMappingInWriteModeAndModifying_data()
+{
+    QTest::addColumn<FrameCreator>("frameCreator");
+    QTest::addColumn<QVideoFrame::MapMode>("mapMode");
+
+    // clang-format off
+    QTest::addRow("defaulFrame.writeOnly") << &tst_QVideoFrameBackend::createDefaultFrame
+                                           << QVideoFrame::WriteOnly;
+    QTest::addRow("defaulFrame.readWrite") << &tst_QVideoFrameBackend::createDefaultFrame
+                                           << QVideoFrame::ReadWrite;
+
+    addMediaPlayerFrameTestData([]()
+    {
+        QTest::addRow("mediaPlayerFrame.writeOnly")
+                << &tst_QVideoFrameBackend::createMediaPlayerFrame
+                << QVideoFrame::WriteOnly;
+        QTest::addRow("mediaPlayerFrame.readWrite")
+                << &tst_QVideoFrameBackend::createMediaPlayerFrame
+                << QVideoFrame::ReadWrite;
+    });
+    // clang-format on
+}
+
+void tst_QVideoFrameBackend::toImage_rendersUpdatedFrame_afterMappingInWriteModeAndModifying()
+{
+    QFETCH(const FrameCreator, frameCreator);
+    QFETCH(const QVideoFrame::MapMode, mapMode);
+
+    // Arrange
+
+    QVideoFrame frame = std::invoke(frameCreator, this);
+    QVERIFY(frame.isValid());
+
+    QImage originalImage = frame.toImage();
+    QVERIFY(!originalImage.isNull());
+
+    // Act: map the frame in write mode and change the top level pixel
+    frame.map(mapMode);
+    QVERIFY(frame.isWritable());
+
+    QCOMPARE_NE(frame.pixelFormat(), QVideoFrameFormat::Format_Invalid);
+
+    const QVideoTextureHelper::TextureDescription *textureDescription =
+            QVideoTextureHelper::textureDescription(frame.pixelFormat());
+    QVERIFY(textureDescription);
+
+    uchar *firstPlaneBits = frame.bits(0);
+    QVERIFY(firstPlaneBits);
+
+    for (int i = 0; i < textureDescription->strideFactor; ++i)
+        firstPlaneBits[i] = ~firstPlaneBits[i];
+
+    frame.unmap();
+
+    // get an image from modified frame
+    QImage modifiedImage = frame.toImage();
+
+    // Assert
+
+    QVERIFY(!frame.isMapped());
+    QCOMPARE_NE(originalImage.pixel(0, 0), modifiedImage.pixel(0, 0));
+    QCOMPARE(originalImage.pixel(1, 0), modifiedImage.pixel(1, 0));
+    QCOMPARE(originalImage.pixel(1, 1), modifiedImage.pixel(1, 1));
+}
+
 QTEST_MAIN(tst_QVideoFrameBackend)
 #include "tst_qvideoframebackend.moc"