Bug 34631 – [Qt] Switching from Phonon to QtMultimedia Backend for Qt 4.7

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

[Nick Young]

This change should bring superior support for HTML5 Media elements in QtWebkit from Qt 4.7.
Hopefully we should be able to make this switch without breaking anything along the way.

Patch is forthcoming.

[Nick Young]

Created attachment 48202 [details]
Initial Patch

Note that there are build guards everywhere which prevent much of the new code from running in versions of Qt prior to 4.7.
To build the QtMultimedia compatible code, you'll have to build and link against a recent version of qt-multimedia (git://scm.dev.nokia.troll.no/qt/qt-multimedia.git).

[Nick Young]

Created attachment 48203 [details]
Updated Patch

Whoops. Fixed a memory leak. :)

[Simon Hausmann]

Comment on attachment 48203 [details]
Updated Patch


> +#include <private/qpaintervideosurface_p.h>

This is going to be an issue, as we cannot include private header files from Qt in WebKit. Is there a way to remove this dependency?

[Simon Hausmann]

In general, btw, Nicholas, your patch looks great! Now we just need to sort out the details :)

[Nick Young]

(In reply to comment #3)
> (From update of attachment 48203 [details])
> 
> > +#include <private/qpaintervideosurface_p.h>
> 
> This is going to be an issue, as we cannot include private header files from Qt
> in WebKit. Is there a way to remove this dependency?

Whilst this is a private header, it is an exported symbol. There is no technical reason why we can not link against this.

Or is this a matter of principle/convention "We WILL not link against private headers!"?

For a variety of reasons, this is not a dependency which can be easily removed.

I suppose the simplest solution would be to make this a public header - but I'll have to speak to Justin before I can say any more on this.

[Simon Hausmann]

(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 48203 [details] [details])
> > 
> > > +#include <private/qpaintervideosurface_p.h>
> > 
> > This is going to be an issue, as we cannot include private header files from Qt
> > in WebKit. Is there a way to remove this dependency?
> 
> Whilst this is a private header, it is an exported symbol. There is no
> technical reason why we can not link against this.

The technical reason is that it does not compile, because the private headers are not installed as part of "make install" in Qt.

It does compile for _our_ builds of Qt as we don't use "make install", but Qt customers use "make install" for example on Linux or Unix.

We need to be able to compile against an installed version of Qt :)

> Or is this a matter of principle/convention "We WILL not link against private
> headers!"?

It is also a matter of principle indeed, as we intend to be able to support at least two Qt versions simultaneously, therefore we cannot rely on private Qt API that is not guaranteed to be source and binary compatible.

> For a variety of reasons, this is not a dependency which can be easily removed.
> 
> I suppose the simplest solution would be to make this a public header - but
> I'll have to speak to Justin before I can say any more on this.

That would be great :)

[Nick Young]

Created attachment 48309 [details]
Updated Patch - No Private Headers

After having a discussion with the rest of the multimedia team, we decided that the correct decision at this stage is to move a copy of the private class in to WebCore.

This is a temporary solution, and we are hopeful that this class can be made public within Qt at some time in the future.
However, we are not ready to commit to binary and source compatibility for that class - particularly given prospective changes to the Qt Painting engine which could allow us to clean up the API for that class.

[Nick Young]

Created attachment 48310 [details]
Minor Modification

Very small change. See previous patch description.

[WebKit Review Bot]

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

Failed to run "['git', 'reset', '--hard', 'HEAD']" exit_code: 128
fatal: Unable to create '.git/index.lock': File exists.

If no other git process is currently running, this probably means a
git process crashed in this repository earlier. Make sure no other git
process is running and remove the file manually to continue.
fatal: Could not reset index file to revision 'HEAD'.


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

[Nick Young]

(In reply to comment #9)
> Attachment 48310 [details] did not pass style-queue:
> 
> Failed to run "['git', 'reset', '--hard', 'HEAD']" exit_code: 128
> fatal: Unable to create '.git/index.lock': File exists.
> 
> If no other git process is currently running, this probably means a
> git process crashed in this repository earlier. Make sure no other git
> process is running and remove the file manually to continue.
> fatal: Could not reset index file to revision 'HEAD'.
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

This was an issue with the ews script, apparently.
The patch passes check-webkit-style with no errors :)

[Nick Young]

Created attachment 48312 [details]
Updated Patch - Removed Dependency

Removed references to the Window control. Full screen functionality requires it - there is not full screen in this patch as it is still "experimental" (read: broken).
Otherwise, same as previous patches.

[Nick Young]

Created attachment 48460 [details]
Updated Patch - Various Improvements

I have addressed some outstanding issues with the integration.

A bug with seeking has been resolved, and a bug with resource selection has been resolved (For multi-source media, webkit will now correctly load an alternate source if this one fails).

There has also been a concern raised in regards to the fact that the dimensions of the media controls are specified in pixels, rather than a DPI independent measurement.
Would be interested to hear thoughts on that issue.

[Simon Hausmann]

Comment on attachment 48460 [details]
Updated Patch - Various Improvements

Nicholas, I'm not so familiar with this code myself, I think Tor Arne will be a better reviewer. But there are a few things I can give feedback on:

> -# Disable HTML5 media compilation if phonon is unavailable
> -!contains(DEFINES, ENABLE_VIDEO=1) {
> -    !contains(QT_CONFIG, phonon) {
> -        DEFINES -= ENABLE_VIDEO=1
> -        DEFINES += ENABLE_VIDEO=0
> +# QtMultimedia since 4.7
> +greaterThan(QT_MINOR_VERSION, 6) {
> +    # Disable HTML5 media compilation if qtmultimedia is unavailable
> +    !contains(DEFINES, ENABLE_VIDEO=1) {
> +        !contains(QT_CONFIG, multimedia) {
> +            DEFINES -= ENABLE_VIDEO=1
> +            DEFINES += ENABLE_VIDEO=0
> +        }
> +    }
> +} else {
> +    # Disable HTML5 media compilation if phonon is unavailable
> +    !contains(DEFINES, ENABLE_VIDEO=1) {
> +        !contains(QT_CONFIG, phonon) {
> +            DEFINES -= ENABLE_VIDEO=1
> +            DEFINES += ENABLE_VIDEO=0
> +        }
>      }
>  }

Just a thought, but perhaps the logic of the above checks would be
simpler if reversed, i.e. _enable_ video only if we have either phonon
available or 4.7 and multimedia.

> +void MediaPlayerPrivate::load(const String& url)
[...]
> +    const QUrl rUrl(url);

I hope that conversion is safe. An alternate approach, slower but safer would be something
along the lines of

const QUrl rUrl(KURL(ParsedURLString, url));

> +
> +            // Set the refferer
> +            request.setRawHeader("Refferer", document->documentURI().utf8().data());

That looks like a typo to me, shouldn't it be Referrer with only one f? :)

That said, there's another rule that's important to obey for referrers: When an https site requests a non-http resources, the referrer should not be set. See QNetworkReplyHandler.cpp.

Perhaps there's a way to re-use some more code from the networking layer here, just a thought :)

> +MediaPlayerPrivateDefault::MediaPlayerPrivateDefault(MediaPlayer *player)

Coding style nitpick: The * position should be to the left.

> +    : MediaPlayerPrivate(player)
> +    , m_videoSurface(new QPainterVideoSurface(m_mediaPlayer))
> +{
> +    if (m_mediaPlayer) {

Is it really possible that m_mediaPlayer is null?

> +void MediaPlayerPrivateDefault::paint(GraphicsContext* context, const IntRect& rect)
> +{
> +    if (context->paintingDisabled())
> +        return;
> +
> +    if (!m_isVisible)
> +        return;
> +
> +    // Grab the painter
> +    QPainter* painter = context->platformContext();
> +
> +    if (!m_isActive) {
> +        // Attempt GL Setup
> +#if !defined(QT_NO_OPENGL) && !defined(QT_OPENGL_ES_1_CL) && !defined(QT_OPENGL_ES_1)
> +        if ((painter->paintEngine()->type() == QPaintEngine::OpenGL
> +             || painter->paintEngine()->type() == QPaintEngine::OpenGL2)) {
> +            // Set GL Context
> +            m_videoSurface->setGLContext(const_cast<QGLContext *>(QGLContext::currentContext()));

Style nitpick: No space before * here.


> +
> +class QVideoSurfacePainter {
> +public:

If this class is here only temporarily, then it would be best to add a comment here
explaining that it'll be removed in the future. That would also help to understand why
the coding style is different.


> +
> +class QVideoSurfacePainter;
> +class Q_AUTOTEST_EXPORT QPainterVideoSurface : public QAbstractVideoSurface {

There shouldn't be a Q_AUTOTEST_EXPORT macro here, not when building inside WebKit.


> +bool RenderThemeQt::paintMediaCurrentTime(RenderObject* o, const RenderObject::PaintInfo& paintInfo, const IntRect& r)
> +{
> +    StylePainter p(this, paintInfo);
> +    if (!p.isValid())
> +        return true;
> +
> +    if (o->style()) {
> +        // Set the text color depending on platform
> +        QPalette pal = QApplication::palette();

I think it would be better to use setPaletteFromPageClientIfExists instead of the application global palette.


> +    static const QColor highlightText = QApplication::palette().brush(QPalette::Active, QPalette::HighlightedText).color();
> +    static const QColor scaleColor(highlightText.red(), highlightText.green(), highlightText.blue(), mediaControlsBaselineOpacity() * 255);

I'd prefer if these weren't global variables but taken from the page client's palette instead.


> +    #if QT_VERSION >= 0x040700
> +    if (MediaPlayer* player = mediaElement->player()) {
> +        // Get the buffered parts of the media
> +        PassRefPtr<TimeRanges> buffered = player->buffered();
> +        if (buffered->length() > 0 && player->duration() < std::numeric_limits<float>::infinity()) {
> +            // Set the transform and brush
> +            WorldMatrixTransformer transformer(p.painter, o, r);
> +            p.painter->setBrush(getMediaControlForegroundColor());
> +
> +            // Paint each buffered section
> +            ExceptionCode ex;
> +            for (int i = 0; i < buffered->length(); i++) {
> +                float startX = (buffered->start(i, ex) / player->duration()) * 100;
> +                float width = ((buffered->end(i, ex) / player->duration()) * 100) - startX;
> +                p.painter->drawRect(startX, 37, width, 26);
> +            }
> +        }
> +    }
> +    #endif

Usually preprocessor statements begin at the beginning of the line :)


How does the new RenderTheme media controls part affect the existing Phonon backend?

[Nick Young]

(In reply to comment #13)
> Nicholas, I'm not so familiar with this code myself, I think Tor Arne will be a
> better reviewer. But there are a few things I can give feedback on:

Your partial review is still much appreciated :)

> Just a thought, but perhaps the logic of the above checks would be
> simpler if reversed, i.e. _enable_ video only if we have either phonon
> available or 4.7 and multimedia.

Yep. Good Point.

> I hope that conversion is safe. An alternate approach, slower but safer would
> be something
> along the lines of
> 
> const QUrl rUrl(KURL(ParsedURLString, url));

On this, I will need more convincing. My question would be this: where is it said that KURL is better at parsing than QUrl?

In either case, the url parameter passed to this method has already been parsed by KURL and checked by security policies, before being converted back to a String. See HTMLMediaElement::loadResource().

Aside: the Media Player interface should probably specify a KURL parameter for this load method, rather than a String.

Bottom line: I don't see how this conversion could ever be unsafe.
Perhaps removing the implicit conversion would be sufficient:

const QUrl rUrl(QString(url));

> That looks like a typo to me, shouldn't it be Referrer with only one f? :)

It should be "Referer". Neither of us was right ;)

> That said, there's another rule that's important to obey for referrers: When an
> https site requests a non-http resources, the referrer should not be set. See
> QNetworkReplyHandler.cpp.
> 
> Perhaps there's a way to re-use some more code from the networking layer here,
> just a thought :)

I had a look at the file you specified, as well as some of the related networking code. Looks like I could replace this entire block of code
with:

    // Construct the media content with a network request if the resource is http[s]
    if (scheme == "http" || scheme == "https") {
        const QNetworkRequest request = ResourceRequest(rurl).toNetworkRequest();
        m_mediaPlayer->setMedia(QMediaContent(request));
    } else {
        // Otherwise, just use the URL
        m_mediaPlayer->setMedia(QMediaContent(rUrl));
    }

Would that work?
I certainly appreciate your guidance here.


> Coding style nitpick: The * position should be to the left.

Fixed :)

> Is it really possible that m_mediaPlayer is null?

Yeah, this check is probably unnecessary.
Technically, if "new QMediaPlayer" fails, this could happen - but new rarely fails, even when out of memory.

I've removed the check in both of the places where it appears :)

> Style nitpick: No space before * here.

Yep.
 
> If this class is here only temporarily, then it would be best to add a comment
> here
> explaining that it'll be removed in the future. That would also help to
> understand why
> the coding style is different.

Done.

> There shouldn't be a Q_AUTOTEST_EXPORT macro here, not when building inside
> WebKit.

Whoops. I missed that when moving that class in to Webkit. Removed.

> I think it would be better to use setPaletteFromPageClientIfExists instead of
> the application global palette.

Sure.

> I'd prefer if these weren't global variables but taken from the page client's
> palette instead.

Yep.

> Usually preprocessor statements begin at the beginning of the line :)

Indeed they do. Fixed.

> How does the new RenderTheme media controls part affect the existing Phonon
> backend?

The phonon backend gains a current time display and a functional volume control. The controls have been tested for both backends :)

That part in RenderThemeQt which has a version guard is to account for the fact that the phonon backend neither implements the buffered() method nor the maxTimeSeekable() method. If you would prefer, I could try and implement one of those methods on the phonon backend so that the media controls are on a truly equal footing.

[Ariya Hidayat]

> To build the QtMultimedia compatible code, you'll have to build and link
> against a recent version of qt-multimedia
> (git://scm.dev.nokia.troll.no/qt/qt-multimedia.git).

For all non-Trolls like us, can you point out the public repository for this?

[Nick Young]

(In reply to comment #15)
> > To build the QtMultimedia compatible code, you'll have to build and link
> > against a recent version of qt-multimedia
> > (git://scm.dev.nokia.troll.no/qt/qt-multimedia.git).
> 
> For all non-Trolls like us, can you point out the public repository for this?

Mmm, not exactly.
What you can do is look at the multimedia sections of Qt Mobility, which is essentially equal to QtMultimedia (with the exception that QtMultimedia does not have the QtMobility namespace).

Relevant files can be seen here: http://qt.gitorious.org/qt-mobility/qt-mobility/trees/master/src/multimedia

Note that this public repo is not 100% up to date.

It should be possible to successfully link against QtMobility, provided you add a "using namespace QtMobility;". You will also need to build qtmobility, then add in some additions to WebCore.pro

My build against mobility has these lines:

    INCLUDEPATH += \
        $$PWD/../../../../../../qtmobility/qtm-multimedia/src/global \
        $$PWD/../../../../../../qtmobility/qtm-multimedia/src/multimedia
        
    LIBS += \
        -L$$PWD/../../../../../../../build/qtmobility/qtm-multimedia/lib \
        -lQtMedia

[Nick Young]

Created attachment 48548 [details]
Updated Patch - Addressed issues from review

The ResourceRequest path didn't work out. It seems headers and cookies are not added to the network request until later..?

[Ariya Hidayat]

> It should be possible to successfully link against QtMobility, provided you add
> a "using namespace QtMobility;". You will also need to build qtmobility, then
> add in some additions to WebCore.pro

Then should this patch be modified to use QtMobility instead?

I mean, what's the point of upstreaming big change which can't be built and tested by anyone else who do not have access to QtMultimedia? I might miss something obvious here...

[Nick Young]

(In reply to comment #18)
> Then should this patch be modified to use QtMobility instead?
> 
> I mean, what's the point of upstreaming big change which can't be built and
> tested by anyone else who do not have access to QtMultimedia? I might miss
> something obvious here...

There is a very good reason why we have version guards everywhere in this patch.
As the bug title implies, this patch is targeted at Qt4.7, at which stage all of the relevant APIs should be available within Qt.

I appreciate where you are coming from - I recognized from the start that this would be an interesting balancing act.

Hopefully at some stage in the near future, the new playback APIs should make it in to Qt master. Some time after that, it should be synched in to the public Gitorious repo. This is the best I can do for non Trolls.

[Eric Carlson]

(In reply to comment #14)
> Aside: the Media Player interface should probably specify a KURL parameter for
> this load method, rather than a String.
> 
True enough. Patches are *always* welcome ;-)

[Tor Arne Vestbø]

Tested on Mac OS 10.6 with qt-multimedia dc7a0b2842a5866b6248e56d41db0af05f7efbc2:

http://chaos.troll.no/~tavestbo/webkit/mediaelement/ seems to work fine, although sometimes the video view does not seem to be initialized (no resize, and no video blitting, only sound).

http://jilion.com/sublime/video does not work, no video or sound out of the box, but after clicking around the seek bar i managed to get sound and play/pause working, but no video.

http://www.youtube.com/watch?v=H6UsFe6X3cc (after visiting youtube.com/html5 and joining the beta), does not seem to load.

http://vimeo.com/8736190 (after clicking "Switch to HTML5 player"), does not seem to load. Stdout:

Building Pixel Buffer visual context 
Could not create visual context (PixelBuffer) 
QT7MovieRenderer: failed to create visual context 

If i switch HD to off loading and playback with sound but no video seems to work.

Neither of these problems are showstoppers though, as they didn't work with the old Phonon backend either, and this new backend has a lot more potential than the old one.

[Nick Young]

(In reply to comment #21)
> Tested on Mac OS 10.6 with qt-multimedia
> dc7a0b2842a5866b6248e56d41db0af05f7efbc2:

Hey there Tor Arne, thanks for taking the time to look this patch over.
I will say in advance, that I'm working on an updated patch - It should be much smaller and elegant, it won't have that QPainterVideoSurface dependency that we've added, and it will keep platform specific optimisations in QtMultimedia instead of Webkit.

> http://chaos.troll.no/~tavestbo/webkit/mediaelement/ seems to work fine,
> although sometimes the video view does not seem to be initialized (no resize,
> and no video blitting, only sound).

Connection is timing out. I'll try it again later.

> http://jilion.com/sublime/video does not work, no video or sound out of the
> box, but after clicking around the seek bar i managed to get sound and
> play/pause working, but no video.

Doesn't work for me either. I'll investigate.

> http://www.youtube.com/watch?v=H6UsFe6X3cc (after visiting youtube.com/html5
> and joining the beta), does not seem to load.

I've spent quite some time trying to get youtube/html5 to work. I am yet to have success, and I am extremely suspicious that youtube is serving only to specific user agents.

> http://vimeo.com/8736190 (after clicking "Switch to HTML5 player"), does not
> seem to load. Stdout:
> 
> Building Pixel Buffer visual context 
> Could not create visual context (PixelBuffer) 
> QT7MovieRenderer: failed to create visual context 
> 
> If i switch HD to off loading and playback with sound but no video seems to
> work.

Seems like you may have stumbled over an issue without our QuickTime backend. I'll try and investigate. Vimeo has been a difficult one to test, as the flash plugin I have on my workstation has a propensity for segfaults when browsing Vimeo.

[Eric Carlson]

(In reply to comment #22)
> 
> > http://www.youtube.com/watch?v=H6UsFe6X3cc (after visiting youtube.com/html5
> > and joining the beta), does not seem to load.
> 
> I've spent quite some time trying to get youtube/html5 to work. I am yet to
> have success, and I am extremely suspicious that youtube is serving only to
> specific user agents.
> 
Check to make sure you are sending all of the cookies for the movie url. Both the Quicktime/Windows and GTK based media engines had this problem.

[Tor Arne Vestbø]

Comment on attachment 48548 [details]
Updated Patch - Addressed issues from review

A few initial comments:

> Index: WebCore/WebCore.pro
> ===================================================================
> --- WebCore/WebCore.pro	(revision 54641)
> +++ WebCore/WebCore.pro	(working copy)
> @@ -131,12 +131,20 @@ mameo5|symbian|embedded {
>  
>  include($$PWD/../JavaScriptCore/JavaScriptCore.pri)
>  
> -# Disable HTML5 media compilation if phonon is unavailable
> -!contains(DEFINES, ENABLE_VIDEO=1) {
> -    !contains(QT_CONFIG, phonon) {
> -        DEFINES -= ENABLE_VIDEO=1
> -        DEFINES += ENABLE_VIDEO=0
> -    }
> +
> +# HTML5 Media Support
> +# We require phonon for versions of Qt < 4.7
> +# We require QtMultimedia for versions of Qt >= 4.7
> +DEFINES -= ENABLE_VIDEO=1
> +DEFINES += ENABLE_VIDEO=0

Please keep the !contains(DEFINES, ENABLE_VIDEO=1) logic, as it allow you to pass --[no-]video to build-webkit to override the detection

>  audio::-webkit-media-controls-seek-back-button, video::-webkit-media-controls-seek-back-button {
> -    /* Since MediaControlElements are always created with a renderer we have to hide
> -       the controls we don't use, so they don't mess up activation and event handling */
> -    left: 0px;
> -    top: 0px;
> -    width: 0px;
> -    height: 0px;
> -
>      display: none;
>  }
>  
>  audio::-webkit-media-controls-seek-forward-button, video::-webkit-media-controls-seek-forward-button {
> -    /* Since MediaControlElements are always created with a renderer we have to hide
> -       the controls we don't use, so they don't mess up activation and event handling */
> -    left: 0px;
> -    top: 0px;
> -    width: 0px;
> -    height: 0px;
> -
>      display: none;
>  }
>  
>  audio::-webkit-media-controls-fullscreen-button, video::-webkit-media-controls-fullscreen-button {
> -    /* Since MediaControlElements are always created with a renderer we have to hide
> -       the controls we don't use, so they don't mess up activation and event handling */
> -    left: 0px;
> -    top: 0px;
> -    width: 0px;
> -    height: 0px;
> -
>      display: none;
>  }

The sizes-hack was used due to display: none not working. Have you verified that this should work now? I remember seeing clicks in the areas of the seek-button being picked up and causing the video to pause, even though the element was set to display: none.

> Index: WebCore/platform/graphics/MediaPlayer.cpp
> ===================================================================
> --- WebCore/platform/graphics/MediaPlayer.cpp	(revision 54641)
> +++ WebCore/platform/graphics/MediaPlayer.cpp	(working copy)
> -#include "MediaPlayerPrivatePhonon.h"
> +    #if QT_VERSION < 0x040700
> +    #include "MediaPlayerPrivatePhonon.h"
> +    #else
> +    #include "MediaPlayerPrivateQt.h"
> +    #endif
>  #elif PLATFORM(CHROMIUM)

No indentation please.

> Index: WebCore/platform/qt/RenderThemeQt.cpp
> ===================================================================
> --- WebCore/platform/qt/RenderThemeQt.cpp	(revision 54641)
> +++ WebCore/platform/qt/RenderThemeQt.cpp	(working copy)


> +double RenderThemeQt::mediaControlsBaselineOpacity() const
> +{
> +    return 0.4;
> +}
> +
>  void RenderThemeQt::paintMediaBackground(QPainter* painter, const IntRect& r) const
>  {
>      painter->setPen(Qt::NoPen);
> -    static QColor transparentBlack(0, 0, 0, 100);
> +    static const QColor shadow = QApplication::palette().brush(QPalette::Active, QPalette::Shadow).color();
> +    static QColor transparentBlack(shadow.red(), shadow.green(), shadow.blue(), mediaControlsBaselineOpacity() * 255);

This leaves me with a transparent light gray overlay instead of black transparent one, probably due to the shadow color on mac. Dunno if this was intentional (you didn't like the black i chose? :)

Anyways, the variable transparentBlack should probably be renamed transparentShadow to match the logical change.

Also, we don't normally use const for variables like this.

Will continue review when new updated patch is up.

[Tor Arne Vestbø]

(In reply to comment #22)
> (In reply to comment #21)
> > Tested on Mac OS 10.6 with qt-multimedia
> > dc7a0b2842a5866b6248e56d41db0af05f7efbc2:
> 
> Hey there Tor Arne, thanks for taking the time to look this patch over.
> I will say in advance, that I'm working on an updated patch - It should be much
> smaller and elegant, it won't have that QPainterVideoSurface dependency that
> we've added, and it will keep platform specific optimisations in QtMultimedia
> instead of Webkit.

Rock! Can't wait to get this landed :)

> > http://chaos.troll.no/~tavestbo/webkit/mediaelement/ seems to work fine,
> > although sometimes the video view does not seem to be initialized (no resize,
> > and no video blitting, only sound).
> 
> Connection is timing out. I'll try it again later.

Hmm, should be available from the outside. Try anarki internally.

> > http://jilion.com/sublime/video does not work, no video or sound out of the
> > box, but after clicking around the seek bar i managed to get sound and
> > play/pause working, but no video.
> 
> Doesn't work for me either. I'll investigate.

Cool, but as I said, these didn't work before either, so we can do that sort of bugfixing as follow-up patches too if they appear to require more than trivial work.

> I've spent quite some time trying to get youtube/html5 to work. I am yet to
> have success, and I am extremely suspicious that youtube is serving only to
> specific user agents.

Hmm, possibly, see Eric's comment about cookies. 

We should probably have a menu in the QtLauncher for changing the user agent like Safari in dev-mode has, would be useful for testing.

> > http://vimeo.com/8736190 (after clicking "Switch to HTML5 player"), does not
> > seem to load. Stdout:
> > 
> > Building Pixel Buffer visual context 
> > Could not create visual context (PixelBuffer) 
> > QT7MovieRenderer: failed to create visual context 
> > 
> > If i switch HD to off loading and playback with sound but no video seems to
> > work.
> 
> Seems like you may have stumbled over an issue without our QuickTime backend.
> I'll try and investigate. Vimeo has been a difficult one to test, as the flash
> plugin I have on my workstation has a propensity for segfaults when browsing
> Vimeo.

Ah, you can try hacking QtLauncher to disable plugins using QWebSettings while you investigate (should probably also be an option ;)

Thanks for looking into these issues!

[Nick Young]

Created attachment 49059 [details]
Updated Patch - Simplified and Improved

Ok. Here's the updated patch that I promised.
It removes the QPainterVideoSurface dependency, as well as dramatically simplifying the task of painting video frames.

The method for painting is very similar to the previous patch, the difference is it is deferred to a different class - QGraphicsVideoItem - which is in QtMultimedia. We use this item even when we don't have a QGraphicsView. When we do have a QGraphicsView, we can easily support Accelerated Compositing, I've been chatting with Noam about this on IRC, so we should hopefully have another patch not too far in the future.

Also in this patch is a bunch of bugfixes.
Including:
- More Robust Seeking
- Support for the recently added "supportsMuting()" API.
- Video Size changes are propagated to WebKit
- Different path for pulling the QNAM during load(), which actually works _before_ our renderer is created (which is when load() is usually called).

We now have support for:

- Sublime Video HTML5 Player
> http://jilion.com/sublime/video
(Note that on OSX, QuickTime requires the entire movie to be buffered before it will play. This seems to be specific to some HD movies, including the one used on this page.) 

- Vimeo HTML5
(I've seen working on OSX. Am yet to test on other backends, but I believe this should work.)

We are still working on support for youtube/html5. Cookies are being sent, but our requests still result in 403 Forbidden.
Investigation is ongoing.

After realising that using the application pallete didn't work so well for the controls on Mac, I went back to the hard coded colours approach.

Note that some of the issues previously mentioned were related to our QuickTime backend on OSX.
You will need to pull a new version of qt-multimedia.

In fact, I recommend pulling qt-mulutimedia-team, which is our staging repo that we're currently in the process of integrating in to Qt.

[Nick Young]

(In reply to comment #24)
> The sizes-hack was used due to display: none not working. Have you verified
> that this should work now? I remember seeing clicks in the areas of the
> seek-button being picked up and causing the video to pause, even though the
> element was set to display: none.

I have not seen any evidence that this is still an issue.

[Nick Young]

Created attachment 49060 [details]
Updated Patch..

Malformed Patch. Let's try that again ;)

[Tor Arne Vestbø]

(In reply to comment #26)
> Ok. Here's the updated patch that I promised.
> It removes the QPainterVideoSurface dependency, as well as dramatically
> simplifying the task of painting video frames.

Nice!

> We now have support for:
> 
> - Sublime Video HTML5 Player
> > http://jilion.com/sublime/video
> (Note that on OSX, QuickTime requires the entire movie to be buffered before it
> will play. This seems to be specific to some HD movies, including the one used
> on this page.) 

Tested with qt-multimedia-team repo 7e7cbe2152efe47972e11875f008ddb8f0cc78db

Can confirm this works now, but I get a spinning beach ball on OS X while the movie loads. In chrome the loading is almost instentanious, and in Safari the load is slower and seems to match the time we spend, but the UI is responsive and the seek bar is updated with the load progress. Dunno what could be the cause, just reporting what I'm seeing :)

The seek bare also flickers a lot when you hover over it while the video is playing. See screenshot. 

Also, when I then move from the sublime-player to http://vimeo.com/8736190, the video sound keeps playing in the background, so something is not unloaded as it should it seems.

Anyways, we can investigate these bugs as follow-up patches.

> - Vimeo HTML5
> (I've seen working on OSX. Am yet to test on other backends, but I believe this
> should work.)

Confirmed

> We are still working on support for youtube/html5. Cookies are being sent, but
> our requests still result in 403 Forbidden.
> Investigation is ongoing.

Cool

> After realising that using the application pallete didn't work so well for the
> controls on Mac, I went back to the hard coded colours approach.

Looks good now.

[Tor Arne Vestbø]

Comment on attachment 49060 [details]
Updated Patch..

LGTM, a few nitpicks below. I also had to change QtMedia to QtMultimedia a few places to make it compile against latest qt-multimedia-team's master, so r- for now, but should be able to land this soon.


> +!contains(DEFINES, ENABLE_VIDEO=1):!contains(DEFINES, ENABLE_VIDEO=0) {

You can use the short-form !contains(DEFINES, ENABLE_VIDEO=.)


> +bool MediaPlayerPrivate::hasVideo() const
> +{
> +    // return m_mediaPlayer->isVideoAvailable();
> +    return true;
> +}

Is this not implemented yet in QtMultimedia?

> +    QLatin1String blKey("bytes-loaded");

Coding-style: bytesLoadedKey

> +    p.painter->setPen(mediaElement->muted() ? Qt::darkGray : Qt::lightGray);

Minor visual style nitpick: Is this to contrast the dark red? The gray outline does not match the other controls, such as the play buttons etc. I'd say skip this for now, and we can revisit the styling of the whole default control style if neccecary.

> +bool RenderThemeQt::paintMediaVolumeSliderTrack(RenderObject *o, const RenderObject::PaintInfo &paintInfo, const IntRect &r)
> +    // Draw the outer rectangle
> +    p.painter->setPen(Qt::lightGray);

Same with this, I'd rather we keep the no-pen look for now :)

[Eric Carlson]

(In reply to comment #26)
> - Sublime Video HTML5 Player
> > http://jilion.com/sublime/video
> (Note that on OSX, QuickTime requires the entire movie to be buffered before it
> will play. This seems to be specific to some HD movies, including the one used
> on this page.) 
> 
That is a symptom of a movie that was not "flattened", saving so the 'moov' atom (the movie table of contents) is at the beginning of the file. Looking at the sublime mp4 file that is indeed the case:

    'ftyp'  size: 28  offset: 0   - File Type
    'free'  size: 132  offset: 28   - Free Space
    'mdat'  size: 29887735  offset: 160   - Movie Data
    'moov'  size: 69206  offset: 29887895   - Movie

This means that if the client doesn't support reading the file with byte range requests, as is the case with "classic" QuickTime, it has to read 29887895 bytes before it gets to the tables it needs to understand the media data.

[Nick Young]

(In reply to comment #30)
> LGTM, a few nitpicks below. I also had to change QtMedia to QtMultimedia a few
> places to make it compile against latest qt-multimedia-team's master, so r- for
> now, but should be able to land this soon.

A namespace change was made between me posting that patch and you pulling the repo ;) Nevermind, fixed now.

> You can use the short-form !contains(DEFINES, ENABLE_VIDEO=.)

Sure.
 
> Is this not implemented yet in QtMultimedia?

At one stage it was causing some problems, because the gstreamer backend only returns true to this after you've begun playback. However, I've done a quick test and there does not seem to be any problems now.

> Coding-style: bytesLoadedKey

Done.

> Minor visual style nitpick: Is this to contrast the dark red? The gray outline
> does not match the other controls, such as the play buttons etc. I'd say skip
> this for now, and we can revisit the styling of the whole default control style
> if neccecary.
> 
> Same with this, I'd rather we keep the no-pen look for now :)

OK, that seems reasonable. I've removed those, and changed the volume control slightly so it's consistent with the no-pen style.

Patch Soon :)

[Nick Young]

Created attachment 49180 [details]
Patch - Round 10

10th time lucky? ;-)

[Tor Arne Vestbø]

Comment on attachment 49180 [details]
Patch - Round 10

We have a winner!

[Tor Arne Vestbø]

Landed in r55079

[Eric Carlson]

(In reply to comment #34)
> (From update of attachment 49180 [details])
> We have a winner!

A few late comments:


155     // Grab the client media element
156     HTMLMediaElement* element = static_cast<HTMLMediaElement*>(m_player->mediaPlayerClient());
 ... 
197     // Set the current volume and mute status
198     // We get these from the element, rather than the player, in case we have
199     // transitioned from a media engine which doesn't support muting, to a media
200     // engine which does.
201     m_mediaPlayer->setMuted(element->muted());
202     m_mediaPlayer->setVolume(static_cast<int>(element->volume() * 100.0));

I cry foul - no fair casting the "client" pointer to the element so you can use it! If there is a problem here you should file a bug and fix it so all ports benefit from the change. Is there any reason that MediaPlayer needs to cache these values?


315 unsigned MediaPlayerPrivate::bytesLoaded() const
316 {
317     unsigned percentage = m_mediaPlayer->bufferStatus();
318
319     if (percentage == 100) {
320         if (m_networkState != MediaPlayer::Idle) {
321             m_networkState = MediaPlayer::Idle;
322             m_player->networkStateChanged();
323         }
324         if (m_readyState != MediaPlayer::HaveEnoughData) {
325             m_readyState = MediaPlayer::HaveEnoughData;
326             m_player->readyStateChanged();
327         }
328     }
329

Is it possible for bufferStatus() to return 100 when mediaStatus() does not also return QMediaPlayer::BufferedMedia or QMediaPlayer::EndOfMedia? If so, why? If not, why do this check here when the condition will be detected in updateStates()?


34 class MediaPlayerPrivate : public QObject, public MediaPlayerPrivateInterface {
35
36     Q_OBJECT
37
38 public:
39     static MediaPlayerPrivateInterface* create(MediaPlayer* player);
40     ~MediaPlayerPrivate();
41
42     static void registerMediaEngine(MediaEngineRegistrar);
43     static void getSupportedTypes(HashSet<String>&);
44     static MediaPlayer::SupportsType supportsType(const String&, const String&);
45     static bool isAvailable() { return true; }
46

Only registerMediaEngine should be public, everything else can be private.

[Nick Young]

> I cry foul - no fair casting the "client" pointer to the element so you can use
> it! If there is a problem here you should file a bug and fix it so all ports
> benefit from the change. Is there any reason that MediaPlayer needs to cache
> these values?

To be fair, the caching isn't really the problem. There's a problem related to supportsMuting() which crops up when you move from a NullMediaPlayerPrivate (which doesn't support muting) to an actual MediaPlayerPrivate (which does).

It was easier to fix here. But if I get a chance, I'll file a bug and submit a patch.

> Is it possible for bufferStatus() to return 100 when mediaStatus() does not
> also return QMediaPlayer::BufferedMedia or QMediaPlayer::EndOfMedia? If so,
> why? If not, why do this check here when the condition will be detected in
> updateStates()?

This is here because of an inconsistency with our backends. Fair to say, I should probably patch the backend and remove this ;)

> Only registerMediaEngine should be public, everything else can be private.

I suppose. I don't really see a problem with it how it is unless it introduces some sort of security issue..

[Nick Young]

> To be fair, the caching isn't really the problem. There's a problem related to
> supportsMuting() which crops up when you move from a NullMediaPlayerPrivate
> (which doesn't support muting) to an actual MediaPlayerPrivate (which does).
> 
> It was easier to fix here. But if I get a chance, I'll file a bug and submit a
> patch.

Bug 35327
After some investigation, the problem wasn't exactly how I described it here, but somewhat similar.

[Nick Young]

Eric's other concerns have been (mostly) addressed in Bug 35328.