Bug 44043 - [Qt] Flash does not work on n900
Summary: [Qt] Flash does not work on n900
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Girish Ramakrishnan
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2010-08-15 22:51 PDT by Girish Ramakrishnan
Modified: 2010-08-25 07:02 PDT (History)
7 users (show)

See Also:


Attachments
wmode=opaque fix (1.71 KB, patch)
2010-08-16 03:11 PDT, Girish Ramakrishnan
kenneth: review+
Details | Formatted Diff | Diff
libgdk-x11-2.0.so.0 (1.55 KB, patch)
2010-08-16 11:25 PDT, Girish Ramakrishnan
no flags Details | Formatted Diff | Diff
libgdk-x11-2.0.so.0 (1.24 KB, patch)
2010-08-17 11:53 PDT, Girish Ramakrishnan
tonikitoo: review+
Details | Formatted Diff | Diff
localrendering (10.11 KB, application/octet-stream)
2010-08-18 08:00 PDT, Girish Ramakrishnan
no flags Details
localrendering (10.11 KB, patch)
2010-08-18 08:01 PDT, Girish Ramakrishnan
kenneth: review+
Details | Formatted Diff | Diff
Inject wmode=opaque in QWebView in maemo5 (2.05 KB, patch)
2010-08-19 05:53 PDT, Girish Ramakrishnan
no flags Details | Formatted Diff | Diff
Inject wmode=opaque in QWebView in maemo5 (1.96 KB, patch)
2010-08-19 07:13 PDT, Girish Ramakrishnan
kenneth: review+
Details | Formatted Diff | Diff
Direct local rendering (5.02 KB, patch)
2010-08-20 14:33 PDT, Girish Ramakrishnan
ariya.hidayat: review+
Details | Formatted Diff | Diff
Allow wmode=transparent in QWebView on Maemo5 (3.06 KB, patch)
2010-08-20 23:27 PDT, Girish Ramakrishnan
ariya.hidayat: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Girish Ramakrishnan 2010-08-15 22:51:01 PDT
Flash does not work on n900 with latest Qt/Webkit and qt-4.7.
Comment 1 Girish Ramakrishnan 2010-08-16 03:11:02 PDT
Created attachment 64482 [details]
wmode=opaque fix

This fixes the wmode=opaque case. Flash on n900 is version 9.x
Comment 2 Girish Ramakrishnan 2010-08-16 07:33:29 PDT
XEmbed - It appears the Maemo5/Flash doesn't support embedded mode. I would like to be proven wrong.

Transparent mode - Maemo5/Flash is broken and it thinks that the XPixmap that we pass it is a shared memory image!

X Error: BadMatch (invalid parameter attributes) 8
  Extension:    137 (MIT-SHM)
  Minor opcode: 4 (X_ShmGetImage)
  Resource id:  0x3a0001f

0x3a0001f is actually the X Pixmap handle that we pass to Flash. It renders ok with some artifacts. We are loading the wrong gdk library on the n900, I will add a patch for this.
Comment 3 Girish Ramakrishnan 2010-08-16 07:34:59 PDT
BTW, the reason fennec is unaffected is because it uses https://wiki.mozilla.org/Plugins:NokiaMaemoImageSurface (which I intend to implement for Qt/WebKit)
Comment 4 Girish Ramakrishnan 2010-08-16 11:25:38 PDT
Created attachment 64504 [details]
libgdk-x11-2.0.so.0

On maemo5, libgdk-x11-2.0.so is missing. Look for libgdk-x11-2.0.so.0 instead.
Comment 5 Andreas Kling 2010-08-16 17:47:55 PDT
Comment on attachment 64504 [details]
libgdk-x11-2.0.so.0

>WebCore/plugins/qt/PluginViewQt.cpp:665
> +  #ifdef Q_WS_MAEMO_5
Any reason we can't use "libgdk-x11-2.0.so.0" for all platforms?
Comment 6 Girish Ramakrishnan 2010-08-16 22:40:11 PDT
(In reply to comment #5)
> (From update of attachment 64504 [details])
> >WebCore/plugins/qt/PluginViewQt.cpp:665
> > +  #ifdef Q_WS_MAEMO_5
> Any reason we can't use "libgdk-x11-2.0.so.0" for all platforms?

Nope, I think that's a good idea. Flash in fact links to libgdk-x11-2.0.so.0 so it is probably more correct to look for that instead of libgdk-x11-2.0.so.
Comment 7 Girish Ramakrishnan 2010-08-17 11:40:55 PDT
wmode=opaque patch landed in 65524
Comment 8 Girish Ramakrishnan 2010-08-17 11:53:24 PDT
Created attachment 64614 [details]
libgdk-x11-2.0.so.0

Look for libgdk-x11-2.0.so.0 on all platforms.
Comment 9 Antonio Gomes 2010-08-17 12:09:01 PDT
Comment on attachment 64614 [details]
libgdk-x11-2.0.so.0

r=me
Comment 10 Girish Ramakrishnan 2010-08-17 20:54:43 PDT
libgdk-x11-2.0.so.0 landed in r65586.
Comment 11 Antonio Gomes 2010-08-17 21:35:46 PDT
hence fixed? :)
Comment 12 Girish Ramakrishnan 2010-08-18 02:04:18 PDT
Initial version of local rendering is ready - https://wiki.mozilla.org/Plugins:NokiaMaemoImageSurface.

The current implementation is simple - Create a QImage. Give QImage for the plugin to render.

I would like to commit the fix as a series of patches. The first patch will contain a working implementation of the spec. Everything else that follows will just be optimization fixes.

Currently, everything works but there are a few details that I need to double check with the Nokia folks before this is ready for commit. The code is at http://gitorious.org/~girish/webkit/girishs-webkit/commits/maemo5_local_rendering_44043.

The patch is made specifically for Maemo5 but it's not ifdef'ed with Q_WS_MAEMO_5 intentionally. It just makes the whole code a mess. If you object to this, please let me know.

My TODO list:
1. Get the image format correct.
    * Transparent (32-bit), Opaque (16-bit) - Check if Flash can render 32-bit in Opaque mode and 16-bit in transparent mode.
2. Transparency
    * Is it expecting premultiplied ARGB?
    * If I clear the image with Qt::transparent, the plugin seems to be painting it all black.
3. Test in raster and opengl
    * Works OK. 63% CPU with raster for X.org and Qt/WebKit combined.
4. Test if fallback is correct to pixmap mode.
    * This code path only works for opaque mode (see comment 2). Disable it or leave it broken?
5. Should we be using shared memory instead of QImage?
    * When using the X11 native backend, creating the image using MIT-SHM has the benefit that we can use XShmPutImage.
6. Should we do direct rendering to backing store?
    * With raster graphicssystem, maybe we can render straight into the backing store? This would give the _best_ performance. AFAIK, Qt will always create a 16-bit surface. So, need to check if transparent Flash can be drawn on 16-bit.
7. Force to windowless mode even for QWebView
    * See comment 2. XEmbed seems broken. Only windowless mode works. Maybe we can put the hack we use for QGraphicsWebView for QWebView also.
Comment 13 Kenneth Rohde Christiansen 2010-08-18 02:29:08 PDT
(In reply to comment #12)
> Initial version of local rendering is ready - https://wiki.mozilla.org/Plugins:NokiaMaemoImageSurface.
> 
> The current implementation is simple - Create a QImage. Give QImage for the plugin to render.
> 
> I would like to commit the fix as a series of patches. The first patch will contain a working implementation of the spec. Everything else that follows will just be optimization fixes.

I'm fine with that :-) Please attach patches.

> The patch is made specifically for Maemo5 but it's not ifdef'ed with Q_WS_MAEMO_5 intentionally. It just makes the whole code a mess. If you object to this, please let me know.

Also fine with me :-)
Comment 14 Girish Ramakrishnan 2010-08-18 08:00:14 PDT
Created attachment 64710 [details]
localrendering
Comment 15 Girish Ramakrishnan 2010-08-18 08:01:27 PDT
Created attachment 64711 [details]
localrendering
Comment 16 Girish Ramakrishnan 2010-08-18 09:53:22 PDT
localrendering landed in r65612.
Comment 17 Girish Ramakrishnan 2010-08-19 05:53:38 PDT
Created attachment 64835 [details]
Inject wmode=opaque in QWebView in maemo5

Last patch for the n900.
Comment 18 WebKit Review Bot 2010-08-19 05:55:55 PDT
Attachment 64835 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1466:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Kenneth Rohde Christiansen 2010-08-19 05:57:20 PDT
Comment on attachment 64835 [details]
Inject wmode=opaque in QWebView in maemo5

1465 #if defined(MOZ_PLATFORM_MAEMO) && (MOZ_PLATFORM_MAEMO == 5)
 1466                 true ||
 1467 #endif
 1468                 !client || !qobject_cast<QWidget*>(client->pluginParent())) {

Wouldnt this be better as

true) {
#else
!client || !qobject_cast<QWidget*>(client->pluginParent())) {
#endif
Comment 20 Girish Ramakrishnan 2010-08-19 07:13:33 PDT
Created attachment 64844 [details]
 Inject wmode=opaque in QWebView in maemo5

Fixed style errors and commit message
Comment 21 Girish Ramakrishnan 2010-08-19 07:47:53 PDT
Inject wmode=opaque landed in r65668
Comment 22 Girish Ramakrishnan 2010-08-20 14:33:07 PDT
Created attachment 64994 [details]
Direct local rendering

When using the raster graphicssystem (which is the default Qt graphicssystem on maemo5), allow flash to draw directly to the raster surface.
Comment 23 Girish Ramakrishnan 2010-08-20 14:33:59 PDT
Reopening, since I have a couple of optimization patches. And it's easier to cherry-pick for PE1.
Comment 24 Ariya Hidayat 2010-08-20 16:22:42 PDT
Comment on attachment 64994 [details]
Direct local rendering

WebCore/plugins/qt/PluginViewQt.cpp:200
 +              imagePainter.fillRect(exposedRect, Qt::white);
Please use m_image.fill(Qt::white) instead.


Otherwise LGTM, re=me.
Comment 25 Girish Ramakrishnan 2010-08-20 22:15:22 PDT
Landed in r65775.
Comment 26 Girish Ramakrishnan 2010-08-20 22:17:46 PDT
(In reply to comment #24)
> (From update of attachment 64994 [details])
> WebCore/plugins/qt/PluginViewQt.cpp:200
>  +              imagePainter.fillRect(exposedRect, Qt::white);
> Please use m_image.fill(Qt::white) instead.
> 

Ariya, I cannot use QImage::fill() to clear a portion of the QImage, right? Or are you suggesting something like:

if (exposedRect == m_image.rect()) {
     m_image.fill();
} else {
    // create QPainter and fillRect
}

I went ahead and committed it anyway, that code was not introduced in this patch.
Comment 27 Girish Ramakrishnan 2010-08-20 23:27:53 PDT
Created attachment 65022 [details]
Allow wmode=transparent in QWebView on Maemo5

r65775 adds wmode=transparent support.
Comment 28 Ariya Hidayat 2010-08-21 15:50:09 PDT
> if (exposedRect == m_image.rect()) {
>      m_image.fill();
> } else {
>     // create QPainter and fillRect
> }

Yes, unless you think exposedRect == m_image.rect() is not a common case.

Also, when filing with QPainter, when you know the destination image should be opaque, use source composition mode.
Comment 29 Ariya Hidayat 2010-08-21 15:51:17 PDT
Comment on attachment 65022 [details]
Allow wmode=transparent in QWebView on Maemo5

LGTM.
Comment 30 Girish Ramakrishnan 2010-08-22 11:40:10 PDT
(In reply to comment #28)
> > if (exposedRect == m_image.rect()) {
> >      m_image.fill();
> > } else {
> >     // create QPainter and fillRect
> > }
> 
> Yes, unless you think exposedRect == m_image.rect() is not a common case.
> 

Yes, it's not the common case.

> Also, when filing with QPainter, when you know the destination image should be opaque, use source composition mode.

AFAIK, Source has the same effect as SourceOver (default) when the source pixel is opaque (Qt::white in this case). Is there some implementation detail I should know about? Also, m_image is always 16-bit, so does setting composition mode Source matter here?
Comment 31 Ariya Hidayat 2010-08-22 13:49:36 PDT
> AFAIK, Source has the same effect as SourceOver (default) when the source pixel is opaque
> (Qt::white in this case). Is there some implementation detail I should know about? 

I reread the patch again and I realize now I was wrong. You're right, leave it as it is :)
Comment 32 Girish Ramakrishnan 2010-08-23 01:10:49 PDT
Thanks ariya!

Landed in r65796.
Comment 33 Ademar Reis 2010-08-25 06:55:49 PDT
Revision r65524 cherry-picked into qtwebkit-2.1 with commit 8ba974ee9f11c0dceef3cd1905ccfde3e00d4396
Revision r65586 cherry-picked into qtwebkit-2.1 with commit 62187e66f76f3fd914a124b0a83392a087e6b9e5
Revision r65612 cherry-picked into qtwebkit-2.1 with commit 2f4975cb871858c834a0b1d14f5ded437f0d7758
Revision r65668 cherry-picked into qtwebkit-2.1 with commit c18140525797adc8da944aa57615b2364a48a4aa
Revision r65775 cherry-picked into qtwebkit-2.1 with commit a5841cad362505f9df286e2cd17af624ed2ec666
Revision r65796 cherry-picked into qtwebkit-2.1 with commit 2908706c089e66b5fcd1e0f004c5ade32efadb78