Bug 44043 – [Qt] Flash does not work on n900

• Component: Plug-ins
• Status: RESOLVED FIXED

[Girish Ramakrishnan]

Flash does not work on n900 with latest Qt/Webkit and qt-4.7.

[Girish Ramakrishnan]

Created attachment 64482 [details]
wmode=opaque fix

This fixes the wmode=opaque case. Flash on n900 is version 9.x

[Girish Ramakrishnan]

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.

[Girish Ramakrishnan]

BTW, the reason fennec is unaffected is because it uses https://wiki.mozilla.org/Plugins:NokiaMaemoImageSurface (which I intend to implement for Qt/WebKit)

[Girish Ramakrishnan]

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.

[Andreas Kling]

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?

[Girish Ramakrishnan]

(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.

[Girish Ramakrishnan]

wmode=opaque patch landed in 65524

[Girish Ramakrishnan]

Created attachment 64614 [details]
libgdk-x11-2.0.so.0

Look for libgdk-x11-2.0.so.0 on all platforms.

[Antonio Gomes]

Comment on attachment 64614 [details]
libgdk-x11-2.0.so.0

r=me

[Girish Ramakrishnan]

libgdk-x11-2.0.so.0 landed in r65586.

[Antonio Gomes]

hence fixed? :)

[Girish Ramakrishnan]

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.

[Kenneth Rohde Christiansen]

(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 :-)

[Girish Ramakrishnan]

Created attachment 64710 [details]
localrendering

[Girish Ramakrishnan]

Created attachment 64711 [details]
localrendering

[Girish Ramakrishnan]

localrendering landed in r65612.

[Girish Ramakrishnan]

Created attachment 64835 [details]
Inject wmode=opaque in QWebView in maemo5

Last patch for the n900.

[WebKit Review Bot]

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.

[Kenneth Rohde Christiansen]

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

[Girish Ramakrishnan]

Created attachment 64844 [details]
 Inject wmode=opaque in QWebView in maemo5

Fixed style errors and commit message

[Girish Ramakrishnan]

Inject wmode=opaque landed in r65668

[Girish Ramakrishnan]

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.

[Girish Ramakrishnan]

Reopening, since I have a couple of optimization patches. And it's easier to cherry-pick for PE1.

[Ariya Hidayat]

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.

[Girish Ramakrishnan]

Landed in r65775.

[Girish Ramakrishnan]

(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.

[Girish Ramakrishnan]

Created attachment 65022 [details]
Allow wmode=transparent in QWebView on Maemo5

r65775 adds wmode=transparent support.

[Ariya Hidayat]

> 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.

[Ariya Hidayat]

Comment on attachment 65022 [details]
Allow wmode=transparent in QWebView on Maemo5

LGTM.

[Girish Ramakrishnan]

(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?

[Ariya Hidayat]

> 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 :)

[Girish Ramakrishnan]

Thanks ariya!

Landed in r65796.

[Ademar Reis]

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