Bug 66650 – [Qt] Flash in a QGraphicsWebView using OpenGL is slow

• Component: Plug-ins
• Status: RESOLVED INVALID

[Christophe Oosterlynck]

When showing a flash using a QGraphicsWebView in a QGraphicsScene that is rendered by a QGraphicsView using a QGLWidget, is slower than when not using a QGlWidget as viewport for the QGraphicsView.

The problem is in WebCore/plugins/qt/PluginViewQt.cpp. The plugin is being drawn on a X11Pixmap when the paint() method is called. In my case the supplied painter uses the OpenGL drawing engine. Calling the drawpixmap() method of this painting engine while using the X11Pixmap as the source pixmap will cause the X11Pixmap to be converted to an image by calling the QPixmap::toImage() method for every update of the plugin.

Letting the plugin draw on a normal QPixmap instead of a X11Pixmap when the painter is using the OpenGL engine will avoid this costly conversion for every update.

[Christophe Oosterlynck]

Writing a patch for this.

[Christophe Oosterlynck]

Created attachment 104657 [details]
Patch

[WebKit Review Bot]

Attachment 104657 [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/plugins/PluginView.h:426:  The parameter name "painter" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 3 files


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

[Christophe Oosterlynck]

Created attachment 104664 [details]
Patch

[Noam Rosenthal]

Comment on attachment 104664 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        Plugins are drawn on QPixmap instead of X11Pixmap when OpenGL backend is used.
> +        https://bugs.webkit.org/show_bug.cgi?id=66650

Please expand about what the patch actually does.

> Source/WebCore/plugins/qt/PluginViewQt.cpp:167
> +            // Create QPixmap (to be used when paintengine is OpenGL)
> +            m_pixmap = QPixmap(m_windowRect.width(), m_windowRect.height());
> +            // Create X11 pixmap
>              if (m_drawable)
>                  XFreePixmap(QX11Info::display(), m_drawable);

So, if we're on Linux we keep two pixmaps alive all the time? Sounds memory inefficient.

> Source/WebCore/plugins/qt/PluginViewQt.cpp:288
> +    bool bUseX11Pixmap = true;

No Hungarian prefixes please :)

> Source/WebCore/plugins/qt/PluginViewQt.cpp:299
> +        qtDrawable = QPixmap::fromX11Pixmap(m_drawable, QPixmap::ExplicitlyShared);

This is cheap, but not free. Doing it for every draw is not efficient.

[Christophe Oosterlynck]

Comment on attachment 104664 [details]
Patch

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

>> Source/WebCore/plugins/qt/PluginViewQt.cpp:167
>>                  XFreePixmap(QX11Info::display(), m_drawable);
> 
> So, if we're on Linux we keep two pixmaps alive all the time? Sounds memory inefficient.

True. I could work with pointers to pixmap and only initialize the pixmap needed. The problem is, that there is no way to know at this point if the painter that is going to be passed to the paint() method is using OpenGL or not.

In the paint method I could also keep the check and create the QPixmap on every paint call (and maybe keep a pointer to it as a member variable => only create the QPixmap on the first draw). The same would have to be done for the m_drawable handle (call XCreatePixmap in the paint() method the first time paint() gets called).

>> Source/WebCore/plugins/qt/PluginViewQt.cpp:299
>> +        qtDrawable = QPixmap::fromX11Pixmap(m_drawable, QPixmap::ExplicitlyShared);
> 
> This is cheap, but not free. Doing it for every draw is not efficient.

This is not new. See the original @ line 284. This is just the original code necessary to work with an X11Pixmap.

[Christophe Oosterlynck]

Created attachment 104806 [details]
Patch

[Noam Rosenthal]

Comment on attachment 104806 [details]
Patch

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

Girish, could you look at this patch?

> Source/WebCore/ChangeLog:8
> +        Plugins are now drawn on a QPixmap instead of on a X11 Pixmap
> +        when the OpenGL backend is used. The X11 Pixmap used to be transformed
> +        to a QImage on every paint when the painter used the OpenGL engine.

This still doesn't describe what your patch changes.

> Source/WebCore/plugins/qt/PluginViewQt.cpp:284
> +    // Check whether we should use a QPixmap or X11 Pixmap to draw the plugin on

. at end of sentence.

> Source/WebCore/plugins/qt/PluginViewQt.cpp:285
> +    bool UseX11Pixmap = true;

useX11Pixmap (start with lowercase).

[Christophe Oosterlynck]

Girish,

any input on my patch?

[Girish Ramakrishnan]

(In reply to comment #9)
> Girish,
> 
> any input on my patch?

I had a cursory look the other day and the patched looked ok. I am travelling atm, so give me a day or two for a complete review.

[Girish Ramakrishnan]

Christophe,
Thanks for the patch. It looks good in general, Can you fix the style issues that Noam has mentioned and we can make it upstream?

It might be a good idea to have two separate functions paintUsingXPixmap and paintUsingQPixmap. Since if'ery is really complicating things. Thoughts? 

OT: I will also propose on the mailing list to remove all the maemo5 support. It's just needlessly complicating the code.

[Christophe Oosterlynck]

Created attachment 113163 [details]
Patch

Optimization only applies to the case when painting with OpenGL engine (not when painting with OpenGL 2 engine) + minor style fixes

[Noam Rosenthal]

(In reply to comment #12)
> Created an attachment (id=113163) [details]
> Patch
> 
> Optimization only applies to the case when painting with OpenGL engine (not when painting with OpenGL 2 engine) + minor style fixes

Please r? if this is up for review.

[Christophe Oosterlynck]

Created attachment 113167 [details]
Patch

Fixed patch errors

[Christophe Oosterlynck]

(In reply to comment #13)
> (In reply to comment #12)
> > Created an attachment (id=113163) [details] [details]
> > Patch
> > 
> > Optimization only applies to the case when painting with OpenGL engine (not when painting with OpenGL 2 engine) + minor style fixes
> 
> Please r? if this is up for review.

Done. But I still don't know what you mean with "This still doesn't describe what your patch changes." on my Changelog entry. Could you be more specific on what is expected to be written in the Changelog?
Right now it is still: one line that summarizes the fact that my patch is an optimization fix and a couple of lines which describe the difference with what used to happen.

[Simon Hausmann]

(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Created an attachment (id=113163) [details] [details] [details]
> > > Patch
> > > 
> > > Optimization only applies to the case when painting with OpenGL engine (not when painting with OpenGL 2 engine) + minor style fixes
> > 
> > Please r? if this is up for review.
> 
> Done. But I still don't know what you mean with "This still doesn't describe what your patch changes." on my Changelog entry. Could you be more specific on what is expected to be written in the Changelog?
> Right now it is still: one line that summarizes the fact that my patch is an optimization fix and a couple of lines which describe the difference with what used to happen.

For example add a [Qt] prefix and shorten it a bit maybe.

One thing I haven't quite gotten yet is what in you case the difference is between a QPixmap and an X11 pixmap? Are you using the GL graphics system?

In other words: QPainter::drawPixmap should end up using TextureFromPixmap when QPainter is backed by the OpenGL paint-engine and the QPixmap is an X11 pixmap. Why exactly isn't that the case for you?

[Christophe Oosterlynck]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #13)
> > > (In reply to comment #12)
> > > > Created an attachment (id=113163) [details] [details] [details] [details]
> > > > Patch
> > > > 
> > > > Optimization only applies to the case when painting with OpenGL engine (not when painting with OpenGL 2 engine) + minor style fixes
> > > 
> > > Please r? if this is up for review.
> > 
> > Done. But I still don't know what you mean with "This still doesn't describe what your patch changes." on my Changelog entry. Could you be more specific on what is expected to be written in the Changelog?
> > Right now it is still: one line that summarizes the fact that my patch is an optimization fix and a couple of lines which describe the difference with what used to happen.
> 
> For example add a [Qt] prefix and shorten it a bit maybe.
> 
> One thing I haven't quite gotten yet is what in you case the difference is between a QPixmap and an X11 pixmap? Are you using the GL graphics system?
> 
> In other words: QPainter::drawPixmap should end up using TextureFromPixmap when QPainter is backed by the OpenGL paint-engine and the QPixmap is an X11 pixmap. Why exactly isn't that the case for you?

qt_gl_preferredTextureTarget() returns GL_TEXTURE_RECTANGLE_NV instead of GL_TEXTURE_2D in my case (when not using the OpenGl 2 engine but the OpenGl 1 engine). The QPainter::drawPixmap() will call QGLContextPrivate::bindTexture(const QPixmap &pixmap, GLenum target, ...), which will try to use texture_from_pixmap. But texture_from_pixmap will only be attempted if "target == GL_TEXTURE_2D", which isn't the case since target will be GL_TEXTURE_RECTANGLE_NV because of a previous call to qt_gl_preferredTextureTarget().

[Simon Hausmann]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (In reply to comment #13)
> > > > (In reply to comment #12)
> > > > > Created an attachment (id=113163) [details] [details] [details] [details] [details]
> > > > > Patch
> > > > > 
> > > > > Optimization only applies to the case when painting with OpenGL engine (not when painting with OpenGL 2 engine) + minor style fixes
> > > > 
> > > > Please r? if this is up for review.
> > > 
> > > Done. But I still don't know what you mean with "This still doesn't describe what your patch changes." on my Changelog entry. Could you be more specific on what is expected to be written in the Changelog?
> > > Right now it is still: one line that summarizes the fact that my patch is an optimization fix and a couple of lines which describe the difference with what used to happen.
> > 
> > For example add a [Qt] prefix and shorten it a bit maybe.
> > 
> > One thing I haven't quite gotten yet is what in you case the difference is between a QPixmap and an X11 pixmap? Are you using the GL graphics system?
> > 
> > In other words: QPainter::drawPixmap should end up using TextureFromPixmap when QPainter is backed by the OpenGL paint-engine and the QPixmap is an X11 pixmap. Why exactly isn't that the case for you?
> 
> qt_gl_preferredTextureTarget() returns GL_TEXTURE_RECTANGLE_NV instead of GL_TEXTURE_2D in my case (when not using the OpenGl 2 engine but the OpenGl 1 engine). The QPainter::drawPixmap() will call QGLContextPrivate::bindTexture(const QPixmap &pixmap, GLenum target, ...), which will try to use texture_from_pixmap. But texture_from_pixmap will only be attempted if "target == GL_TEXTURE_2D", which isn't the case since target will be GL_TEXTURE_RECTANGLE_NV because of a previous call to qt_gl_preferredTextureTarget().

Aha!

So isn't this patch actually a workaround for a bug that's really in the Qt OpenGL module? Shouldn't it still be using TFP?

[Simon Hausmann]

Comment on attachment 113167 [details]
Patch

I'm sorry, I'm giving this an r-. At the very least because the ChangeLog doesn't make much sense
by saying that "Plugins are now drawn on a QPixmap instead of on a X11 Pixmap when the OpenGL backend is used".

X11 windowless plugins can only be rendered onto X11 pixmaps (technically it's a general purpose X11 drawable, but a pixmap
by all means). The following line is responsible for passing the information _where_ the plugin should paint to:

    exposeEvent.drawable = qtDrawable.handle();

That means qtDrawable must be a QPixmap that is backed by an X11 pixmap.

This is the case with the current code and unless you're using the OpenGL graphics system, a regular QPixmap instance allocated
with Qt compiled for X11 will _also_ be backed by an x11 pixmap. So the statement "...draw onto QPixmap instead of on an X11 Pixmap" doesn't make sense because QPixmap is _also_ backed by an X11 pixmap, returned by handle().

If however the OpenGL graphics system is used, then QPixmaps will not be backed by X11 pixmaps and QPixmap::handle() will return 0. A 0
drawable is certainly not going to make the plugin paint.


So either I am missing something (in which case please elaborate in the ChangeLog) or the patch will not have any effect and certainly break plugin rendering when the OpenGL graphics system is used, because QPixmap::handle() will return 0 unless the QPixmap was constructed using QPixmap::fromX11Pixmap.

[Christophe Oosterlynck]

(In reply to comment #19)
> (From update of attachment 113167 [details])
> I'm sorry, I'm giving this an r-. At the very least because the ChangeLog doesn't make much sense
> by saying that "Plugins are now drawn on a QPixmap instead of on a X11 Pixmap when the OpenGL backend is used".
> 
> X11 windowless plugins can only be rendered onto X11 pixmaps (technically it's a general purpose X11 drawable, but a pixmap
> by all means). The following line is responsible for passing the information _where_ the plugin should paint to:
> 
>     exposeEvent.drawable = qtDrawable.handle();
> 
> That means qtDrawable must be a QPixmap that is backed by an X11 pixmap.
> 
> This is the case with the current code and unless you're using the OpenGL graphics system, a regular QPixmap instance allocated
> with Qt compiled for X11 will _also_ be backed by an x11 pixmap. So the statement "...draw onto QPixmap instead of on an X11 Pixmap" doesn't make sense because QPixmap is _also_ backed by an X11 pixmap, returned by handle().
> 
> If however the OpenGL graphics system is used, then QPixmaps will not be backed by X11 pixmaps and QPixmap::handle() will return 0. A 0
> drawable is certainly not going to make the plugin paint.
> 
> 
> So either I am missing something (in which case please elaborate in the ChangeLog) or the patch will not have any effect and certainly break plugin rendering when the OpenGL graphics system is used, because QPixmap::handle() will return 0 unless the QPixmap was constructed using QPixmap::fromX11Pixmap.

Hmm, it does make sense what you're saying and indeed makes my patch questionable. Although I'm pretty sure I saw performance increase with my patch when using OpenGl 1 (backed up by profiling) and the plugin (flash) was still visible.

But it seems that I am following the wrong path to solve my issue. I don't have much time these days to work on my issue anymore, but I'll try to look into it in the near future and provide a better patch (or maybe get it fixed in Qt if the problem lies not within webkit) and be back here with an update.

Thanks for the input!

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.