Bug 39691 - [Qt] GraphicsLayer: support tiling
Summary: [Qt] GraphicsLayer: support tiling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 38744 41137
  Show dependency treegraph
 
Reported: 2010-05-25 15:07 PDT by Noam Rosenthal
Modified: 2011-01-06 10:42 PST (History)
9 users (show)

See Also:


Attachments
AC + Tiling (10.98 KB, patch)
2010-06-03 15:40 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
AC + tiling (style fix) (9.08 KB, patch)
2010-06-03 15:50 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Tiling performance issue (1.14 KB, patch)
2010-06-03 15:54 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
AC + Tiling (9.08 KB, patch)
2010-06-03 16:04 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
AC+ tiling (~uploaded wrong file) (8.81 KB, patch)
2010-06-03 16:06 PDT, Noam Rosenthal
kenneth: review-
Details | Formatted Diff | Diff
Patch (7.89 KB, patch)
2010-11-02 16:57 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2010-05-25 15:07:15 PDT
GraphicsLayers in Qt wouldn't currently work when the layers are very big. They need to be connected to the tiled backing store.
See LayoutTests/compositing/tiling.
Comment 1 Noam Rosenthal 2010-05-25 16:38:33 PDT
Apparently right now QtWebkit crashes when using a QGLWidget as a viewport on a site that big, even without AC.
Comment 2 Noam Rosenthal 2010-06-03 15:40:55 PDT
Created attachment 57822 [details]
AC + Tiling
Comment 3 WebKit Review Bot 2010-06-03 15:43:19 PDT
Attachment 57822 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:391:  Missing spaces around =  [whitespace/operators] [4]
WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:836:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Noam Rosenthal 2010-06-03 15:50:39 PDT
Created attachment 57824 [details]
AC + tiling (style fix)
Comment 5 Noam Rosenthal 2010-06-03 15:54:13 PDT
Created attachment 57825 [details]
Tiling performance issue

I've noticed this while using valgrind - we're iterating over each pixel to get the tile for it out of a map. This is a bottleneck for a large web-page.
In the patch I jump over the pixels based on the tile size but I'm not sure it's correct. Antti?
Comment 6 Noam Rosenthal 2010-06-03 16:04:46 PDT
Created attachment 57826 [details]
AC + Tiling
Comment 7 Noam Rosenthal 2010-06-03 16:06:21 PDT
Created attachment 57827 [details]
 AC+ tiling (~uploaded wrong file)
Comment 8 Kenneth Rohde Christiansen 2010-06-04 12:54:42 PDT
Comment on attachment 57827 [details]
 AC+ tiling (~uploaded wrong file)

Generally looks good to me, but it would be nice if Antti could have a look.

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:50
 +  #define GRAPHICS_LAYER_TILING_THRESHOLD 2000
We shouldnt use defines, you can use a static const. and also please do not use capitalized naming, gGraphicsLayerTilingThreshold would be fine.

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:119
 +          , public virtual TiledBackingStoreClient
Should have indentation of 4

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:841
 +      return mapFromScene(view->mapToScene(view->viewport()->visibleRegion().boundingRect()).boundingRect()).boundingRect().toAlignedRect();
Wow, lots of bounding here :-) Antti, is there no better way of doing this?
Comment 9 Noam Rosenthal 2010-06-04 13:38:51 PDT
> WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:841
>  +      return mapFromScene(view->mapToScene(view->viewport()->visibleRegion().boundingRect()).boundingRect()).boundingRect().toAlignedRect();

Actually looking at it again one boundingRect is redundant,
      return mapFromScene(
          view->mapToScene(view->viewport()->visibleRegion().boundingRect())
      ).boundingRect().toAlignedRect();
would also work.
There's no other way I found to get the visible portion of a QGraphicsItem.
Comment 10 Kenneth Rohde Christiansen 2010-06-13 07:44:23 PDT
Comment on attachment 57827 [details]
 AC+ tiling (~uploaded wrong file)

r- due to my earlier comments.

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:534
 +      // FIXME: There's currently no Qt API to know if a new region of an item is exposed, apart from overloading paint
Have a bug been filed? Would be nice to add the link here.
Comment 11 Kenneth Rohde Christiansen 2010-06-24 06:48:04 PDT
Are we getting an updated patch? :)
Comment 12 Noam Rosenthal 2010-06-24 07:07:37 PDT
Kenneth: I want anttik to look at this first to see if the content is right, especially the changes I made to tiling code itself, unless you're comfortable reviewing that code yourself.
Comment 13 zalan 2010-06-24 07:31:12 PDT
if you are going to init the backbuffer with fill, i think it would be nice to fix it properly and call fill on the non-transparent case too. 
Quoting from qpixmap class reference "Warning: This will create a QPixmap with uninitialized data. Call fill() to fill the pixmap with an appropriate color before drawing onto it with QPainter" http://doc.qt.nokia.com/4.6/qpixmap.html#QPixmap-3

I noticed this uninitialized behavior on symbian, while using a test browser with tiles on.
Comment 14 Kenneth Rohde Christiansen 2010-08-14 01:34:39 PDT
No'am is this still relevant?
Comment 15 Noam Rosenthal 2010-10-29 10:01:14 PDT
Apparently this is causing some issues, reopening
Comment 16 Noam Rosenthal 2010-11-02 16:57:45 PDT
Created attachment 72770 [details]
Patch
Comment 17 Kenneth Rohde Christiansen 2010-11-03 01:47:55 PDT
Comment on attachment 72770 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72770&action=review

LGTM

> WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:52
> +#define GRAPHICS_LAYER_TILING_THRESHOLD 2000

Maybe this needs change on a mobile device? Did you check the iphone source dump?
Comment 18 Noam Rosenthal 2010-11-03 07:18:49 PDT
> > WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:52
> > +#define GRAPHICS_LAYER_TILING_THRESHOLD 2000
> 
> Maybe this needs change on a mobile device? Did you check the iphone source dump?
It's 2000 on relevant mobile devices :)
Comment 19 Noam Rosenthal 2010-11-03 11:34:02 PDT
Comment on attachment 72770 [details]
Patch

Committed manually, r71253
Comment 20 Noam Rosenthal 2010-11-03 11:34:41 PDT
committed manually, http://trac.webkit.org/changeset/71253.
Comment 21 Suresh Voruganti 2010-11-03 11:38:12 PDT
Fix is required for Qtwebkit 2.1 as this is impacting Meego as said in 41137
Comment 22 Ademar Reis 2010-11-05 07:40:54 PDT
(In reply to comment #21)
> Fix is required for Qtwebkit 2.1 as this is impacting Meego as said in 41137

As discussed there on bug 41137#c13, this patch doesn't fix the issue, so I'm removing it as a blocker for 2.1.