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.
Writing a patch for this.
Created attachment 104657 [details] Patch
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.
Created attachment 104664 [details] Patch
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.
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.
Created attachment 104806 [details] Patch
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).
Girish, any input on my patch?
(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.
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.
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
(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.
Created attachment 113167 [details] Patch Fixed patch errors
(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.
(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?
(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().
(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?
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.
(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!
=== 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.