Summary: | [Qt] GraphicsLayer: support tiling | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Noam Rosenthal <noam> | ||||||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | ademar, kenneth, koivisto, laszlo.gombos, ossy, smagnuso, suresh.voruganti, webkit.review.bot, zalan | ||||||||||||||
Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 38744, 41137 | ||||||||||||||||
Attachments: |
|
Description
Noam Rosenthal
2010-05-25 15:07:15 PDT
Apparently right now QtWebkit crashes when using a QGLWidget as a viewport on a site that big, even without AC. Created attachment 57822 [details]
AC + Tiling
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.
Created attachment 57824 [details]
AC + tiling (style fix)
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?
Created attachment 57826 [details]
AC + Tiling
Created attachment 57827 [details]
AC+ tiling (~uploaded wrong file)
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?
> 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 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.
Are we getting an updated patch? :) 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. 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. No'am is this still relevant? Apparently this is causing some issues, reopening Created attachment 72770 [details]
Patch
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? > > 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 on attachment 72770 [details] Patch Committed manually, r71253 committed manually, http://trac.webkit.org/changeset/71253. Fix is required for Qtwebkit 2.1 as this is impacting Meego as said in 41137 (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. |