Bug 39691 – [Qt] GraphicsLayer: support tiling

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Noam Rosenthal]

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.

[Noam Rosenthal]

Apparently right now QtWebkit crashes when using a QGLWidget as a viewport on a site that big, even without AC.

[Noam Rosenthal]

Created attachment 57822 [details]
AC + Tiling

[WebKit Review Bot]

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.

[Noam Rosenthal]

Created attachment 57824 [details]
AC + tiling (style fix)

[Noam Rosenthal]

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?

[Noam Rosenthal]

Created attachment 57826 [details]
AC + Tiling

[Noam Rosenthal]

Created attachment 57827 [details]
 AC+ tiling (~uploaded wrong file)

[Kenneth Rohde Christiansen]

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?

[Noam Rosenthal]

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

[Kenneth Rohde Christiansen]

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.

[Kenneth Rohde Christiansen]

Are we getting an updated patch? :)

[Noam Rosenthal]

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.

[zalan]

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.

[Kenneth Rohde Christiansen]

No'am is this still relevant?

[Noam Rosenthal]

Apparently this is causing some issues, reopening

[Noam Rosenthal]

Created attachment 72770 [details]
Patch

[Kenneth Rohde Christiansen]

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?

[Noam Rosenthal]

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

[Noam Rosenthal]

Comment on attachment 72770 [details]
Patch

Committed manually, r71253

[Noam Rosenthal]

committed manually, http://trac.webkit.org/changeset/71253.

[Suresh Voruganti]

Fix is required for Qtwebkit 2.1 as this is impacting Meego as said in 41137

[Ademar Reis]

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