Bug 56826 – [Qt] Implement fullscreen playback for the GStreamer backend.

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Alexis Menard (darktears)]

Today when building the Qt port with the GStreamer backend (DEFINES+=USE_GSTREAMER=1) there is no fullscreen playback support.

[Alexis Menard (darktears)]

Created attachment 86457 [details]
Proposed patch.

This patch implement the support in FullScreenVideoQt alongside with the Qt Multimedia support. I though about reusing the base class QWebFullScreenVideoHandler of the platform plugin but it required to change the interface of the plugin and the show(QMediaPlayer*) was also an issue (and I found very ugly to have a plugin interface with some ifdef inside). The #if defined(WTF_USE_GSTREAMER) && WTF_USE_GSTREAMER is for moc to compile the class definition (moc does not expand the USE macro). Any feedback?

[WebKit Review Bot]

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

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

Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowQt.cpp:21:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 7 files


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

[Alexis Menard (darktears)]

Created attachment 86458 [details]
V2 with changelog

[WebKit Review Bot]

Attachment 86458 [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/WebCore/platform/graphics/gstreamer/PlatformVideoWindowQt.cpp:21:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 9 files


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

[Alexis Menard (darktears)]

Created attachment 86481 [details]
V3 with style fixes

[Benjamin Poulain]

Comment on attachment 86481 [details]
V3 with style fixes

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

r- because of a massive style problem ;)
And maybe a little because this is not tested and you don't say why :)

> Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowPrivate.h:50
> +    HTMLVideoElement *m_mediaElement;

Andreas: Style issue.

[Alexis Menard (darktears)]

Created attachment 87719 [details]
V4 with comments taken into accounts.

[Alexis Menard (darktears)]

Created attachment 87721 [details]
V5 with Changelog fixed.

[Kenneth Rohde Christiansen]

How is this related to Simon's work?

[Alexis Menard (darktears)]

(In reply to comment #9)
> How is this related to Simon's work?

It's not. Nothing to do with it. WebKit2 stuff is completely different. It won't conflict.

[Andreas Kling]

Comment on attachment 87721 [details]
V5 with Changelog fixed.

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

> Source/WebCore/ChangeLog:14
> +        No new tests because layout tests cover it. They are not yet activated
> +        but will be any time soon.

Which tests? :)

[Alexis Menard (darktears)]

(In reply to comment #11)
> (From update of attachment 87721 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87721&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        No new tests because layout tests cover it. They are not yet activated
> > +        but will be any time soon.
> 
> Which tests? :)

media/media-fullscreen-inline.html
media/media-fullscreen-not-in-document.html

But I need to investigate why they don't pass yet despite the code is here. (They also fail for the QtMM implementation).

[Andreas Kling]

Comment on attachment 87721 [details]
V5 with Changelog fixed.

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

Looks mostly good, but let's do one more iteration. :)

> Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowQt.cpp:25
>  #include "PlatformVideoWindow.h"
>  
> +#include "HTMLVideoElement.h"
>  #include "PlatformVideoWindowPrivate.h"

Includes should be in alphabetic order.

> Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowQt.cpp:53
> +void FullScreenVideoWindow::closeEvent(QCloseEvent* event)

No need for the 'event' name on the parameter.

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp:47
> +

Unnecessary empty line.

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp:69
> +    m_fullScreenWidget = reinterpret_cast<FullScreenVideoWindow* >(gstreamerGWorld->platformVideoWindow()->window());

Extra space after *.

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp:75
> +    m_fullScreenWidget->setVideoElement(m_videoElement);
> +
> +    connect(m_fullScreenWidget, SIGNAL(closed()), this, SLOT(windowClosed()));
> +
> +    m_fullScreenWidget->showFullScreen();

No need for the blank lines.

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp:90
> +    m_fullScreenWidget->setVideoElement(0);
> +
> +    m_fullScreenWidget->close();

Ditto.

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp:103
> +

Ditto.

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp:175
> +

Ditto.

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp:202
> +

Ditto.

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:23
> +#include "Platform.h"

#include <wtf/Platform.h>

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:42
> +#if defined(WTF_USE_GSTREAMER) && WTF_USE_GSTREAMER

#if USE(GSTREAMER)

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:49
> +    ~GStreamerFullScreenVideoHandler() {};

Coding style, need a space between { and }. And remove the semicolon at end of line.

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:64
> +#if defined(ENABLE_QT_MULTIMEDIA) && ENABLE_QT_MULTIMEDIA

#if ENABLE(QT_MULTIMEDIA)

[Tor Arne Vestbø]

Comment on attachment 87721 [details]
V5 with Changelog fixed.

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

>> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:42
>> +#if defined(WTF_USE_GSTREAMER) && WTF_USE_GSTREAMER
> 
> #if USE(GSTREAMER)

moc will barf on the USE() macro, that's why we need to do it like this :(

>> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:64
>> +#if defined(ENABLE_QT_MULTIMEDIA) && ENABLE_QT_MULTIMEDIA
> 
> #if ENABLE(QT_MULTIMEDIA)

ditto

[Andreas Kling]

(In reply to comment #14)
> (From update of attachment 87721 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87721&action=review
> 
> >> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:42
> >> +#if defined(WTF_USE_GSTREAMER) && WTF_USE_GSTREAMER
> > 
> > #if USE(GSTREAMER)
> 
> moc will barf on the USE() macro, that's why we need to do it like this :(
> 
> >> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:64
> >> +#if defined(ENABLE_QT_MULTIMEDIA) && ENABLE_QT_MULTIMEDIA
> > 
> > #if ENABLE(QT_MULTIMEDIA)
> 
> ditto

Right, so the ENABLE(QT_MULTIMEDIA) earlier in that file should be removed instead. :)

[Alexis Menard (darktears)]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 87721 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=87721&action=review
> > 
> > >> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:42
> > >> +#if defined(WTF_USE_GSTREAMER) && WTF_USE_GSTREAMER
> > > 
> > > #if USE(GSTREAMER)
> > 
> > moc will barf on the USE() macro, that's why we need to do it like this :(
> > 
> > >> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:64
> > >> +#if defined(ENABLE_QT_MULTIMEDIA) && ENABLE_QT_MULTIMEDIA
> > > 
> > > #if ENABLE(QT_MULTIMEDIA)
> > 
> > ditto
> 
> Right, so the ENABLE(QT_MULTIMEDIA) earlier in that file should be removed instead. :)

Well I did it on purpose. Where I used ENABLE(QT_MULTIMEDIA) it's because it's not related to moc (includes for example). So I picked readability vs consistency. But I can change :D.

[Alexis Menard (darktears)]

Comment on attachment 87721 [details]
V5 with Changelog fixed.

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

>> Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowQt.cpp:25

> 
> Includes should be in alphabetic order.

Well PlatformVideoWindow.h is the primary header it should be on the first position. Though I will remove the extra space.

[Alexis Menard (darktears)]

Created attachment 88448 [details]
V6

[Alexis Menard (darktears)]

Created attachment 88454 [details]
V7 with the changelog.

Sorry for the noise.

[Andreas Kling]

Comment on attachment 88454 [details]
V7 with the changelog.

LGTM.

[WebKit Commit Bot]

Comment on attachment 88454 [details]
V7 with the changelog.

Clearing flags on attachment: 88454

Committed r83078: <http://trac.webkit.org/changeset/83078>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.