Bug 35524 – [Qt] GraphicsLayer: support plugins

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Noam Rosenthal]

This would probably involve creating a special PluginWidget and QGraphicsItem for NPAPI/Qt plugins. Right now the WebCore API for that doesn't seem very clear and the NPAPI plugins are very platform specific.

[Tor Arne Vestbø]

Please follow the QtWebKit bug reporting guidelines when reporting bugs.

See http://trac.webkit.org/wiki/QtWebKitBugs

Specifically:

  - The 'QtWebKit' component should be used for bugs/features in the public QtWebKit API layer, not to signify that the bug is specific to the Qt port of WebKit

    http://trac.webkit.org/wiki/QtWebKitBugs#Component

[Girish Ramakrishnan]

Initial version - http://gitorious.org/~girish/webkit/girishs-webkit/commits/plugins_ac_35524

[Noam Rosenthal]

> #if USE(ACCELERATED_COMPOSITING)
>     m_platformLayer->update();
> #else
>     invalidate();
> #endif

Maybe instead of duplicating that code we can just do it once in forceUpdate(), and call forceUpdate() from all the other places?

[Girish Ramakrishnan]

(In reply to comment #3)
> > #if USE(ACCELERATED_COMPOSITING)
> >     m_platformLayer->update();
> > #else
> >     invalidate();
> > #endif
> 
> Maybe instead of duplicating that code we can just do it once in forceUpdate(), and call forceUpdate() from all the other places?

Agree. 

Do you think I had it all correct conceptually?

[Girish Ramakrishnan]

What's left:
1. Check if the resizing code is correct. 
    1.a. Check if we need a Plugin type (and just media type) in the layering code.
2. Enable/Disable AC not just based on the compile-time define but also on the settings value.
3. Transparent flash
4. Check on m6.

[Noam Rosenthal]

> Do you think I had it all correct conceptually?
Yes, looks pretty clean!

[Girish Ramakrishnan]

Created attachment 70438 [details]
Refactor rendering code

[Early Warning System Bot]

Attachment 70438 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4362022

[Girish Ramakrishnan]

Created attachment 70444 [details]
Second try

Fix the build problem

[Eric Seidel (no email)]

Attachment 70444 [details] was posted by a committer and has review+, assigning to Girish Ramakrishnan for commit.

[Girish Ramakrishnan]

Patch landed in http://trac.webkit.org/changeset/69518. 

There is still work to do on this patch, please don't close the bug.

[Girish Ramakrishnan]

Created attachment 71039 [details]
AC for NPAPI plugins

[Ariya Hidayat]

Comment on attachment 71039 [details]
AC for NPAPI plugins

Looks good!

[Girish Ramakrishnan]

Created attachment 71068 [details]
use OwnPtr

per ariya's suggestion on irc

[Girish Ramakrishnan]

Created attachment 71070 [details]
remove 0 initialization

OwnPtr is already inited to 0.

[Ariya Hidayat]

Comment on attachment 71070 [details]
remove 0 initialization

LGTM. r+=me

[Girish Ramakrishnan]

Landed in http://trac.webkit.org/changeset/69981

[Ademar Reis]

Unfortunately I'm not familiar with this change... can someone please tell me if there's a hard-requirement to have this as part of qtwebkit-2.1? The release is frozen and unless there's a hard-requirement (discussed in the program meetings) or a critical bug, patches will not be cherry-picked anymore.

The same is valid for the other related bugs: Bug 47418 and bug 47571. I would like to remove both as well.

[Girish Ramakrishnan]

(In reply to comment #18)
> Unfortunately I'm not familiar with this change... can someone please tell me if there's a hard-requirement to have this as part of qtwebkit-2.1? The release is frozen and unless there's a hard-requirement (discussed in the program meetings) or a critical bug, patches will not be cherry-picked anymore.
> 
> The same is valid for the other related bugs: Bug 47418 and bug 47571. I would like to remove both as well.

This change might be required for m6. I am not 100% sure, I will get back in a day or two.

[Girish Ramakrishnan]

I have removed this from 2.1. Tests show that AC is slower than non-AC on m6. On the desktop, there appears to be no difference.

[Viatcheslav Ostapenko]

(In reply to comment #20)
> I have removed this from 2.1. Tests show that AC is slower than non-AC on m6. On the desktop, there appears to be no difference.

Hi Girish,

  How did you test this? Did you try AC with tiled backing store switched on with animated plugins? Something like this: http://goo.gl/uhgYk or this: http://goo.gl/bb202
  Looks strange, that repainting plugin on tiles is faster, than repainting only AC layer.

[Simon Hausmann]

This corresponds to http://bugreports.qt.nokia.com/browse/QTWEBKIT-94

[Girish Ramakrishnan]

(In reply to comment #21)
> (In reply to comment #20)
> > I have removed this from 2.1. Tests show that AC is slower than non-AC on m6. On the desktop, there appears to be no difference.
> 
> Hi Girish,
> 
>   How did you test this? Did you try AC with tiled backing store switched on with animated plugins? Something like this: http://goo.gl/uhgYk or this: http://goo.gl/bb202
>   Looks strange, that repainting plugin on tiles is faster, than repainting only AC layer.

IIRC, I tested with flash fps test at http://www.shinedraw.com/mathematics/flash-vs-silverlight-fps-meter-stress-test/. There is also this other flash fps file that I used to test. I don't think I can attach that swf here since I am not sure about it's license.

As for whether tiled backing store was off/on, I just used the default which I think is off. As I understand, enabling tiled backing store shouldn't affect my tests (it might make the thing slower because of the caching)

[Viatcheslav Ostapenko]

> As for whether tiled backing store was off/on, I just used the default which I 
> think is off. As I understand, enabling tiled backing store shouldn't affect 
> my tests (it might make the thing slower because of the caching)

Actually, it should affect your tests a lot!
Without tiled backing store you will not get any advantage, because on any flash update layers below flash will have to be redrawn.
If tiled backing store is switched on and animated plugin is not in layer, then it will cause redrawing of plugin (and content) on tiles. This is quite slow and can cause visual artifacts if plugin covers more than one tile.
The best case if tiled backing store is used and plugin is in AC layer. On plugin repaint only plugin layer gets repainted and content below plugin will be blit from tiles (no content redraw).
IMHO, the whole idea of AC is to removed dynamically updated/repositioned content from cached main layer and there is no use to have AC without main layer cache.