Bug 35524

Summary: [Qt] GraphicsLayer: support plugins
Product: WebKit Reporter: Noam Rosenthal <noam>
Component: Plug-insAssignee: Girish Ramakrishnan <girish>
Status: RESOLVED FIXED    
Severity: Enhancement CC: ademar, christian.webkit, eric, girish, hausmann, ostap73, vestbo, webkit-ews, zuh
Priority: P3 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 47418, 47485, 47571    
Bug Blocks: 57418    
Attachments:
Description Flags
Refactor rendering code
none
Second try
kenneth: review+
AC for NPAPI plugins
ariya.hidayat: review+
use OwnPtr
none
remove 0 initialization ariya.hidayat: review+

Description Noam Rosenthal 2010-03-01 04:45:29 PST
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.
Comment 1 Tor Arne Vestbø 2010-03-05 09:39:50 PST
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
Comment 2 Girish Ramakrishnan 2010-10-08 05:21:26 PDT
Initial version - http://gitorious.org/~girish/webkit/girishs-webkit/commits/plugins_ac_35524
Comment 3 Noam Rosenthal 2010-10-08 06:18:57 PDT
> #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?
Comment 4 Girish Ramakrishnan 2010-10-08 07:03:41 PDT
(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?
Comment 5 Girish Ramakrishnan 2010-10-08 07:07:02 PDT
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.
Comment 6 Noam Rosenthal 2010-10-08 07:12:51 PDT
> Do you think I had it all correct conceptually?
Yes, looks pretty clean!
Comment 7 Girish Ramakrishnan 2010-10-11 09:01:10 PDT
Created attachment 70438 [details]
Refactor rendering code
Comment 8 Early Warning System Bot 2010-10-11 09:59:50 PDT
Attachment 70438 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4362022
Comment 9 Girish Ramakrishnan 2010-10-11 10:20:35 PDT
Created attachment 70444 [details]
Second try

Fix the build problem
Comment 10 Eric Seidel (no email) 2010-10-13 12:28:10 PDT
Attachment 70444 [details] was posted by a committer and has review+, assigning to Girish Ramakrishnan for commit.
Comment 11 Girish Ramakrishnan 2010-10-13 21:05:05 PDT
Patch landed in http://trac.webkit.org/changeset/69518. 

There is still work to do on this patch, please don't close the bug.
Comment 12 Girish Ramakrishnan 2010-10-18 08:35:31 PDT
Created attachment 71039 [details]
AC for NPAPI plugins
Comment 13 Ariya Hidayat 2010-10-18 10:41:31 PDT
Comment on attachment 71039 [details]
AC for NPAPI plugins

Looks good!
Comment 14 Girish Ramakrishnan 2010-10-18 11:57:06 PDT
Created attachment 71068 [details]
use OwnPtr

per ariya's suggestion on irc
Comment 15 Girish Ramakrishnan 2010-10-18 12:09:02 PDT
Created attachment 71070 [details]
remove 0 initialization

OwnPtr is already inited to 0.
Comment 16 Ariya Hidayat 2010-10-18 12:10:03 PDT
Comment on attachment 71070 [details]
remove 0 initialization

LGTM. r+=me
Comment 17 Girish Ramakrishnan 2010-10-18 12:14:55 PDT
Landed in http://trac.webkit.org/changeset/69981
Comment 18 Ademar Reis 2010-10-19 10:27:58 PDT
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.
Comment 19 Girish Ramakrishnan 2010-10-19 12:16:05 PDT
(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.
Comment 20 Girish Ramakrishnan 2010-10-21 23:10:28 PDT
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.
Comment 21 Viatcheslav Ostapenko 2011-03-01 18:58:19 PST
(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.
Comment 22 Simon Hausmann 2011-03-02 06:01:19 PST
This corresponds to http://bugreports.qt.nokia.com/browse/QTWEBKIT-94
Comment 23 Girish Ramakrishnan 2011-03-02 23:35:23 PST
(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)
Comment 24 Viatcheslav Ostapenko 2011-03-03 07:16:30 PST
> 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.