Bug 53774 – [Qt] Crash in QGraphicsVideoItem when closing a scene in fullscreen mode.

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: WebKit Qt

[Norbert Leser]

When ACCELERATED_COMPOSITION is enabled in WebKit build for QT (as per default), a crash occurs in QGraphicsVideoItem when closing a scene in fullscreen mode.

A patch that fixes the crash, as implemented and tested by Pat (Vincent Bradley), will follow.

[Norbert Leser]

Created attachment 81230 [details]
Patch that fixes crash of bug 53774

The attached patch that was implemented and tested by Vincent Bradley contains a fix for the reported crash.

[Antonio Gomes]

Comment on attachment 81230 [details]
Patch that fixes crash of bug 53774

View in context: https://bugs.webkit.org/attachment.cgi?id=81230&action=review

> Source/WebCore/ChangeLog:9
> +        No new tests needed.

why?

[Benjamin Poulain]

Comment on attachment 81230 [details]
Patch that fixes crash of bug 53774

View in context: https://bugs.webkit.org/attachment.cgi?id=81230&action=review

> Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:655
> +bool MediaPlayerPrivateQt::isAcceleratedRenderingEnabled()
> +{
> +#if USE(ACCELERATED_COMPOSITING)
> +    MediaPlayerClient* client = m_webCorePlayer->mediaPlayerClient();
> +    bool composited = client->mediaPlayerRenderingCanBeAccelerated(m_webCorePlayer);
> +    if (composited)
> +        return true;
> +#endif
> +    return false;
> +}

Maybe I totally misunderstand what is going on here...but doesn't that class have an attribute for that? m_composited?

[Eric Seidel (no email)]

Comment on attachment 81230 [details]
Patch that fixes crash of bug 53774

r- for testing.

[Norbert Leser]

(In reply to comment #4)
> (From update of attachment 81230 [details])
> r- for testing.

I'd like to find out why new tests are required since this patch is a pure bug fix and does not add new features and functionality.

We'll be creating a new patch anyway, to account for the simplification that Benjamin suggested, but if a new test is required, what kind of tests do you have in mind?

[Benjamin Poulain]

(In reply to comment #5)
> I'd like to find out why new tests are required since this patch is a pure bug fix and does not add new features and functionality.

The policy for test coverage never excluded bugs. When you submit a bug fix, you have to create a test for it so:
1) this bug never comes back (regressions)
2) the reviewer get a better idea of what is covered by the patch

[Pat]

Created attachment 81642 [details]
Patch to check if QtMediaPlayer is in accelerated rendering mode, if it is do not add and remove QGraphicsVideoItem from QGraphicsScene - NO test case

This patch only contains the code change, no tests included.  The code change checks the member variable m_composited, which means QMediaPlayer is using Accelerated Compositing. I QMediaPlayer is using Accelerated Compositing, m_compositing=true, then do not add and remove the QGraphicsVideoItem from the QGraphicsScene when entering and exiting fullscreen. The QGraphicsVideoItem has been removed from our QGraphicsScene and is expected to still removed, but we were adding QGraphicsVideoItem back into QGraphicsScene when we exited fullscreen. The first time we entered fullscreen, the QGraphicsVideoItem has already been removed from QGraphicsScene. so removeItem() was a NOP.

[Suresh Voruganti]

Adding to Qtwebkit 2.1.1 master bug as this blocks the release

[Simon Hausmann]

Comment on attachment 81642 [details]
Patch to check if QtMediaPlayer is in accelerated rendering mode, if it is do not add and remove QGraphicsVideoItem from QGraphicsScene - NO test case

Patch looks correct to me, but you also need to provide a ChangeLog entry and mark the patch for review.

I don't think that with our current framework this is testable easily. The testing framework does not support transitioning to and from fullscreen video playback.

[Pat]

Created attachment 82789 [details]
Patch with code changes and fullscreen test case (QtTestLib test case)

In QtWebKit 2.1.x, early on we make a call to MediaPlayerPrivateQt::acceleratedRenderingStateChanged() which removes our QGraphicsVideoItem "m_videoItem" from our QGraphicsScene AND sets the flag m_composited = true. 

When we enter fullscreen, a call to MediaPlayerPrivateQt::removeVideoItem() is made and we remove our QGraphicsVideoItem "m_videoItem" from our QGraphicsScene, it is already removed - so this is a NOP. When we exit fullscreen, a call to MediaPlayerPrivateQt::restoreVideoItem() is made and we add our QGraphicsVideoItem "m_videoItem" to our QGraphicsScene. This results in a crash inside of QGraphicsLayerQt, because it expects to own the QGraphicsVideoItem. The code changes check if we are in accelerated composite rendering (m_composited=true) before removing m_videoItem from our graphicsScene in removeVideoItem(), or adding m_videoItem to our graphicsScene in restoreVideoItem().

QtWebKit 2.1.x notes:
- MediaPlayerPrivateQt::acceleratedRenderingStateChanged(), removes m_videoItem from graphicsScene AND sets m_composited=true.
- The call MediaPlayerPrivateQt::acceleratedRenderingStateChanged() is guarded by #if USE(ACCELERATED_COMPOSITING) and by default enabled on QtWebKit 2.1.x, so it is used in QtWebKit 2.1.x.
- ACCELERATED_COMPOSITING can be disabled by setting QWebSettings.setAttribute(QWebSettings::AcceleratedCompositingEnabled, false), which Symbian browser does - therefore not seen yet.

WebKit.org trunk notes:
- MediaPlayerPrivateQt::acceleratedRenderingStateChanged(), new TextureMapperVideoLayerQt(m_videoItem), does NOT remove m_videoItem from graphicsScene, AND sets m_composited=true.
- The call MediaPlayerPrivateQt::acceleratedRenderingStateChanged() is guarded by #if USE(ACCELERATED_COMPOSITING) && USE(TEXTURE_MAPPER), therefore this code is not executed and m_composited is not set. Which makes the code change a safe NOP.

[Pat]

Created attachment 82794 [details]
Patch with code, changelog and no tests

Patch with code changes and changelog. No test included.
Testing is complex and may not be needed, since fullscreen is entering and exiting is may or may not be supported and if supported - any user testing will find a crash on fullscreen exit (it can't hide)

[Pat]

Created attachment 82796 [details]
Patch with code, changelog and QtTestLib test included

Test included for QtWebKit 2.1.x and consideration for WebKit.org trunk

[WebKit Review Bot]

Attachment 82796 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:300:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:300:  Extra space before )  [whitespace/parens] [2]
Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:305:  Missing space after ,  [whitespace/comma] [3]
Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:322:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 5 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pat]

Created attachment 82836 [details]
Patch to fix header order as determined by check-webkit-style

This patch is needed to fix existing style issue, so the patch for the fix can be applied.

[Early Warning System Bot]

Attachment 82836 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7918807

[Pat]

Created attachment 82844 [details]
Patch to fix header order as determined by check-webkit-style and add include path in tests.pri for required config.h file

[Pat]

Created attachment 82853 [details]
Patch to fix crash on fullscreen exit. Qt tests included. This patch depends on attachment 82844 [details] (style changes patch) from this bug report to land in webkit trunk.

Will set to r?, once 82844 lands.

[Tor Arne Vestbø]

(In reply to comment #14)
> Created an attachment (id=82836) [details]
> Patch to fix header order as determined by check-webkit-style
> 
> This patch is needed to fix existing style issue, so the patch for the fix can be applied.

If check-webkit-style complains about issues in your patch, fix those style issues in your patch, not in a pre-patch to only fix the style parts.

[Pat]

Created attachment 83159 [details]
Patch to fix crash on fullscreen exit. Qt tests and style changes (reorder header files) included.

[WebKit Review Bot]

Attachment 83159 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/qt/ChangeLog:4:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pat]

Created attachment 83160 [details]
Patch to fix crash on fullscreen exit. Qt tests and style changes (reorder header files) included.

[Tor Arne Vestbø]

Comment on attachment 83160 [details]
Patch to fix crash on fullscreen exit. Qt tests and style changes (reorder header files) included.

View in context: https://bugs.webkit.org/attachment.cgi?id=83160&action=review

> Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:291
> +    // Put the video file onto the file system, so media player can load it
> +    QString newVideoPath = QDir::tempPath();
> +    QFile videoFile(":/resources/html5-video-short.mp4");
> +    QVERIFY(videoFile.exists());
> +    if (videoFile.exists()) {
> +        if (newVideoPath.endsWith(QDir::toNativeSeparators("/")))
> +            newVideoPath += "html5-video-short.mp4";
> +        else
> +            newVideoPath += QDir::toNativeSeparators("/html5-video-short.mp4");
> +
> +        QFile newVideoFile(newVideoPath);
> +        if (!newVideoFile.exists())
> +            videoFile.copy(newVideoPath);
> +        QVERIFY(newVideoFile.exists());
> +        newVideoPath.prepend(QDir::toNativeSeparators("file://"));

Can't we use the approach used in the QWebView test of TESTS_SOURCE_DIR?

> Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:299
> +    QString html = ("<html><body>"
> +                    "<video src='" + newVideoPath + "' width='480' height='480' controls='true'>"
> +                    "Your browser does not support HTML5 video."
> +                    "</video>"
> +                    "</body></html>"
> +                    );

Use QString.arg(), and you can simplify this, no need for fallback contents, etc. controls is enough, no need for controls=true.

> Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:314
> +    // Wait a few seconds to let the video load
> +    QTimer::singleShot(3000, this, SLOT(waitHere()));
> +    waitForSignal(this, SIGNAL(doneWaiting()));

waitForSignal(view, SIGNAL(loadFinished(bool)), 3000);

> Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:319
> +        QVariant fsSupported = videoElement.evaluateJavaScript("this.webkitSupportsFullscreen");

fullScreenSupported, don't abbreviate variables like this

> Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:323
> +        qWarning("Entering fullscreen, clicked on video element");

Why is that a warning?

> Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:324
> +        webView->fireMouseClick(QPointF(240.0, 240.0));

What's this hitting? The play button, the fullscreen button?  I don't see anywhere that we set the forceFullSceenVideo flag, so presumably a play() would not enter fullscreen. Would it perhaps be better if we could set the flag and then just call play() on the video element?

> Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:328
> +        // Wait few seconds to let the video play a bit, then exit fullscreen
> +        QTimer::singleShot(2000, this, SLOT(waitHere()));
> +        waitForSignal(this, SIGNAL(doneWaiting()));

Would it work to have your test html register with the enteredFullScreen DOM event and just exit straight away there?

> Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:342
> +        QVariant isFullscreen = videoElement.evaluateJavaScript("this.webkitDisplayingFullscreen");
> +        QVERIFY(isFullscreen.isValid() && isFullscreen.Bool);
> +
> +        qWarning("Exiting fullscreen");
> +        videoElement.evaluateJavaScript("this.webkitExitFullscreen()");
> +
> +        // Wait few seconds to let the fullscreen tear down, crash usually happens 30-60 seconds after fullscreen exit
> +        QTimer::singleShot(60000, this, SLOT(waitHere()));
> +        waitForSignal(this, SIGNAL(doneWaiting()));

Can you find a better way to do this without all these timers?

> Source/WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.qrc:4
> +    <file>resources/html5-video-short.mp4</file>

Do we need this, as a qrc, or can we re-use some of the videos that are part of the regular DRT tests?

[Tor Arne Vestbø]

Comment on attachment 83160 [details]
Patch to fix crash on fullscreen exit. Qt tests and style changes (reorder header files) included.

Actually, this whole test would be better as a regular layout-test, with DRT plumbing to enable forceFullScreenVideo, or might even be covered by existing layout-tests for fullscreen

[Pat]

The code changes needed for this fix are only applicable to QtWebKit 2.1.x.
A patch will follow that needs to be applied to QtWebKit 2.1.x only.
This crash is not reproducible in WebKit trunk.

[Mahesh Kulkarni]

Created attachment 83325 [details]
patch

Disable ACCELERATED_COMPOSITING only for video use case on maemo/symbian platforms when PlatformPlugin supports FullScreen playing.
There are couple of issues with AC enabled with combination of PlatformPlugin fullscreen player, which are already fixed in trunk code with TEXTURE_MAPPING changes and this hack is needed only for QtWebkit 2.1.x.

[Ademar Reis]

(In reply to comment #25)
> Created an attachment (id=83325) [details]
> patch
> 
> Disable ACCELERATED_COMPOSITING only for video use case on maemo/symbian platforms when PlatformPlugin supports FullScreen playing.
> There are couple of issues with AC enabled with combination of PlatformPlugin fullscreen player, which are already fixed in trunk code with TEXTURE_MAPPING changes and this hack is needed only for QtWebkit 2.1.x.

Added to qtwebkit-2.1: df4f04daae4b9db8217a5388295cfb1fa10cfde8

Per comment 24, this doesn't affect trunk and there will be no patch submited, so I'm closing it as worksforme (because bugzilla is related to trunk). Please reopen if I'm missing something.