Bug 31183

Summary: [Qt][Mac] flash plugin doesn't work when using QGraphicsWebView.
Product: WebKit Reporter: Yongjun Zhang <yongjun.zhang>
Component: Plug-insAssignee: Yongjun Zhang <yongjun.zhang>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, girish, hausmann, kenneth, laszlo.gombos, vestbo, zecke
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 31794    
Attachments:
Description Flags
first attempt to fix this issue - use a QPixmap to paint plugin content.
kenneth: review-
Implement plugin rendering
none
Implement plugin rendering (2)
girish: review-, girish: commit-queue-
Implement plugin rendering (3)
girish: review-
Implement Plugin Rendering (4)
none
Implement Plugin Rendering (5) hausmann: review+

Description Yongjun Zhang 2009-11-05 13:35:36 PST
Flash plugin doesn't show any content in QGVLauncher, which uses QGraphcisWebView.  Audio still works; the page shows a blank placeholder in where the plugin suppose to be displayed.
Comment 1 Yongjun Zhang 2009-11-05 13:42:18 PST
The reason is CGContextRef obtained from QGraphicsView is null in function widget->macCGHandle(), if widget is of class type QGraphicsView.

Although it seems to be a Qt bug, Simon suggested to make a dedicated QPixmap and let the plugin paint on the pixmap, just like how windowless plugin is implemented.
Comment 2 Yongjun Zhang 2009-11-05 13:57:15 PST
Created attachment 42593 [details]
first attempt to fix this issue - use a QPixmap to paint plugin content.
Comment 3 Holger Freyther 2009-11-07 00:32:18 PST
You should initialize the contextRef from the constructor in the initializer list...
Comment 4 Kenneth Rohde Christiansen 2009-11-09 06:26:26 PST
Also,

+#if PLATFORM(QT)
+    CGContextRelease(m_contextRef);
+#endif

Here you dont check if m_contextRef is non-0, which you do elsewhere. If you need to do that, you should do it both places, or not do it at all.

Also, this patch seems to clutter the code a bit. Maybe try to refactor it into some methods with descriptive names.
Comment 5 Girish Ramakrishnan 2009-11-12 01:36:57 PST
Is the patch supposed to work? Crashes for me after applying on Qt 4.6 with cocoa build.
Comment 6 Yongjun Zhang 2009-11-13 07:53:35 PST
(In reply to comment #5)
> Is the patch supposed to work? Crashes for me after applying on Qt 4.6 with
> cocoa build.

It works for me.  could you paste the crash call stack?
Comment 7 Girish Ramakrishnan 2009-11-15 12:54:06 PST
(In reply to comment #6)
> (In reply to comment #5)
> > Is the patch supposed to work? Crashes for me after applying on Qt 4.6 with
> > cocoa build.
> 
> It works for me.  could you paste the crash call stack?

Ok, the crash happens only in 32-bit cocoa builds. Your patch works fine in carbon build. Are plugins in 32-bit cocoa supported? It looks like things like NP_NO_CARBON assume that cocoa is 64-bit only. I haven't looked into the problem deeply.
Comment 8 Girish Ramakrishnan 2009-11-15 13:07:42 PST
Wondeful patch!

Comments:
1. I don't agree with using the QPixmap even when we have a platformWidget(). With this patch, QWebView will triple buffer. We should use QPixmap only when we don't have a view.
2. The pixmap must be resized in updatePlugin(). This is the call back we get when the frame rect changes. Also, you must call initialize with a fill().
3. In transparent mode, the pixmap is not cleared with transparency. You will see that the ball leaves trailing marks in http://www.communitymx.com/content/source/E5141/wmodetrans.htm.
4. npWindow.type must be NPWindowTypeDrawable
5. Doesn't work there is no QWebPageClient. For example, taking screenshot of a QWebPage.
Comment 9 Girish Ramakrishnan 2009-11-15 13:09:39 PST
(In reply to comment #1)
> The reason is CGContextRef obtained from QGraphicsView is null in function
> widget->macCGHandle(), if widget is of class type QGraphicsView.
> 

This is not a Qt bug. QGraphicsView is a scroll area, you can only get the handle for the viewport() when painting.
Comment 10 Girish Ramakrishnan 2009-11-15 13:25:06 PST
Created attachment 43251 [details]
Implement plugin rendering

Here's my take. I was working on this simultaneously for a customer, I have merged my patch and  Yongjun Zhang's patch.

The idea is that when we don't have a QWebView as the pluginParent, we set the platformPluginWidget to 0. IOW, have a null platformPluginWidget() indicates to the code that we have to draw to a pixmap. I also prefer that plugins draw directly to the view's contextref when we have a native handle available.

A really ugly hack I had to do was to create a fake window to please Flash. Chrome does this too - http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/plugins/webplugin_delegate_impl_mac.mm?view=markup&pathrev=25433 (grep for fake).

What's missing: mouse and (keyboard? - i can test this only after i can give it focus :D) events don't work, printing doesn't work.

Can someone explain to me how mouse events are sent? Am I reading it right that we send the global pos(!). If so, having to make it work with QGraphicsView presents quite a challenge.
Comment 11 Girish Ramakrishnan 2009-11-15 13:53:09 PST
Created attachment 43252 [details]
Implement plugin rendering (2)

Only coding style fixes (the if statement had a brace though it was only one line)
Comment 12 Kenneth Rohde Christiansen 2009-11-15 14:14:20 PST
We seem to be getting a lot of Qt specific code in PluginViewMac. Is that really where they belong? or is it time to make a PluginViewMacQt ? Just wondering.

-        if (QWidget* widget = client->ownerWidget()) {
+        if (QWidget* widget = qobject_cast<QWidget*>(client->pluginParent()))

is ownerWidget not needed anymore? or only for linux?
Comment 13 Girish Ramakrishnan 2009-11-15 21:04:45 PST
(In reply to comment #12)
> We seem to be getting a lot of Qt specific code in PluginViewMac. Is that
> really where they belong? or is it time to make a PluginViewMacQt ? Just
> wondering.
> 

Right now it's doing ok. But yes, a few more Qt specific changes and we need to start thinking of the above.

> -        if (QWidget* widget = client->ownerWidget()) {
> +        if (QWidget* widget = qobject_cast<QWidget*>(client->pluginParent()))
> 
> is ownerWidget not needed anymore? or only for linux?

We still need it to position popups. But yes, ideally, the NPAPI should be modified so that we don't need to have such a thing as ownerWidget().
Comment 14 Girish Ramakrishnan 2009-11-16 00:36:43 PST
There's a problem with relying on platformPluginWidget(). For cases like,
m_page->mainFrame()->render(&pixmap) and the page is associated with a QWebView the plugin ends drawing to the view instead of the pixmap because we use the context of the platformPluginWidget(). Solution is to use yongjun's initial approach of redirecting painting of plugin to a pixmap in all cases.

Please reject this patch. I will post a new one.
Comment 15 Girish Ramakrishnan 2009-11-16 00:37:55 PST
Comment on attachment 43252 [details]
Implement plugin rendering (2)

Rejecting my own patch :)
Comment 16 Girish Ramakrishnan 2009-11-16 02:05:57 PST
Created attachment 43280 [details]
Implement plugin rendering (3)
Comment 17 Girish Ramakrishnan 2009-11-16 02:07:18 PST
Attached patch fixes:
1. Printing
2. Plugins use offscreen rendering in all cases.

Only events remains to be fixed.
Comment 18 Girish Ramakrishnan 2009-11-16 06:00:57 PST
Sigh, I should have checked before r?. There is a rather sever performance regression with QWebView. When playing flash, the cpu usage is 90%. Without using the pixmap, it is like 40-60%. I am not too happy about this. Should we just go back to my old patch?

To make my patch work with printing, we have to pass the correct CGContextRef based on the paintDevice (we have to get this from the qpainter)
Comment 19 Girish Ramakrishnan 2009-11-17 04:28:58 PST
Comment on attachment 43280 [details]
Implement plugin rendering (3)

Had a discussion with Tor Arne on irc:
1. Decided not to use off screen pixmap in all cases since performance regression not acceptable.
2. Make the contextref code x-platform. Only the QPixmap code will be ifdef'ed Qt.

Also, I have managed to get events working. But I will make that a separate patch.

To get printing and rendering to image to work, we have to probe painter->device(). I don't plan to implement this bit atm since it's not needed for my customer.
Comment 20 Tor Arne Vestbø 2009-11-17 05:04:27 PST
Comment on attachment 43280 [details]
Implement plugin rendering (3)

r- as per the earlier comment about doing this in a x-port way and abstracting the QPixmap stuff away in helper functions

> +#if defined(XP_MACOSX)
> +#include <QPixmap>
> +#endif

This needs to be wrapped in a PLATFORM(QT) btw
Comment 21 Girish Ramakrishnan 2009-11-17 08:00:20 PST
It was really bugging me as to _why_ we painting to pixmap is slow. I found the
root cause for the performance problem. 

+    IntRect r(rect->left, rect->top, rect->right + rect->left, rect->bottom +
rect->top);
+    invalidateRect(r);

Spot the bug :-) 

Fixing that makes QtLauncher and QGVLauncher have pretty much the same
performance.
Comment 22 Simon Hausmann 2009-11-17 08:05:38 PST
(In reply to comment #21)
> It was really bugging me as to _why_ we painting to pixmap is slow. I found the
> root cause for the performance problem. 
> 
> +    IntRect r(rect->left, rect->top, rect->right + rect->left, rect->bottom +
> rect->top);
> +    invalidateRect(r);
> 
> Spot the bug :-) 
> 
> Fixing that makes QtLauncher and QGVLauncher have pretty much the same
> performance.

Fantastic :)
Comment 23 Girish Ramakrishnan 2009-11-18 01:26:52 PST
Created attachment 43414 [details]
Implement Plugin Rendering (4)

Ok, I imagined events working correctly. Mouse clicks work but not mouse over. I am still figuring out how to fix it. In the meantime, can we get the rendering part in?
Comment 24 Girish Ramakrishnan 2009-11-18 02:18:14 PST
There is a missing ifdef around this. Any other comments?

+            m_pixmap = QPixmap(m_windowRect.size());
+            m_pixmap.fill(Qt::transparent);
+            m_contextRef = qt_mac_cg_context(&m_pixmap);
Comment 25 Girish Ramakrishnan 2009-11-19 07:48:20 PST
Created attachment 43502 [details]
Implement Plugin Rendering (5)

Latest and greatest patch with the ifdef changes in my previous comment.

Mouse move just does not get processed by Flash. Incredibly frustrating stuff... I would still like to get this patch in since rendering works great.
Comment 26 Kenneth Rohde Christiansen 2009-11-19 09:08:08 PST
(In reply to comment #25)
> Created an attachment (id=43502) [details]
> Implement Plugin Rendering (5)
> 
> Latest and greatest patch with the ifdef changes in my previous comment.
> 
> Mouse move just does not get processed by Flash. Incredibly frustrating
> stuff... I would still like to get this patch in since rendering works great.

Let me try to ping Tor Arne
Comment 27 Simon Hausmann 2009-11-20 01:25:11 PST
Comment on attachment 43502 [details]
Implement Plugin Rendering (5)

This is a great first step
Comment 28 Girish Ramakrishnan 2009-11-20 03:22:39 PST
Landed in r51234.
Comment 29 Eric Seidel (no email) 2009-11-21 14:18:39 PST
Closing based on comment above.
Comment 30 Girish Ramakrishnan 2009-11-22 21:09:53 PST
Opened 31794 for the events (I cannot change the status of this bug)
Comment 31 Simon Hausmann 2009-11-25 01:16:26 PST
*** Bug 29446 has been marked as a duplicate of this bug. ***