|
Description
Viatcheslav Ostapenko
2011-02-06 17:09:51 PST
Created attachment 81434 [details]
Modified anomaly demo browser with QWebView replaced with QGraphicsWebView, tiling enabled and scrolling direction reporting to tiling backing store.
Created attachment 81437 [details]
Webkit tiling rework proposal.
Not a real patch. Just preview version.
Patch summary:
1. Adds API to report scrolling direction to backing store.
2. Tile distance is biased towards scrolling direction.
3. Update rect is also biased towards scrolling direction.
4. Reuse of tiles outside of update rect.
5. Tile creation timer removed. Tiles are created or reused on demand.
6. Tile update timeout is introduced. It tile painting lasts longer than update timeout then tile update continues on next time fire or paint.
7. Missing tiles have priority in painting.
8. Viewport tiles fully updated if view is not scrolled, but during scrolling tile update timeout has priority.
9. Tiles update also called from paint (with recursive calling protection) in order to reduce flicker.
10. Fixed bug with update area calculation. Instead of width * ratio it was calculated like width + width * (ratio - 1) * 2 = width * (2 * ratio - 1) resulting in very big backing store area.
That looks good. I think you should use this bug as a meta bug, and create 10 subtask with little patches. Some changes seem obvious, other will need to be discussed (API...). Adding Carol in CC for (9), he investigated delayed layout issues and might have comments. Slava,
In double TiledBackingStore::scrollBiasedTileDistance(const IntPoint& viewportCenter, const IntRect& tileRect) (line 510)
It looks like it should be something like:
if (m_scrollDelta.y() != 0 && yOffset != 0) {
(In reply to comment #3) > That looks good. I think you should use this bug as a meta bug, and create 10 subtask with little patches. I doubt it is good idea. Changes depend on each other and I don't see how to separate them creating some meaningful patches. > Some changes seem obvious, other will need to be discussed (API...). Yes. I just added some setProperty API on webpage to be able to test it. I think it would be good to add also "paint timeout" API. Somebody might want to change it. From my experiments, one tile painting time should be at least twice less than paint timeout. At the same time paint timeout shouldn't more than 2/3 of desired frame length (1000ms/frame rate). > Adding Carol in CC for (9), he investigated delayed layout issues and might have comments. Yes, I kept his issue in mind. All update area calculations are done after layout. Created attachment 96333 [details]
Patch 1: Add API to notify tiling about scrolling direction. Bias tile distance against scrolling direction.
Created attachment 96334 [details]
Patch 2: Remove tile creation timer and do tile creation together with tile updates.
Created attachment 96335 [details]
Patch 3: Add tile reuse.
Created attachment 96336 [details]
Patch 4: Prioritize tile paiting. Viewport area is updated 1st and missing tiles are updated before dirty.
Attachment 96333 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/TiledBackingStore.h:69: The parameter name "delta" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/platform/graphics/TiledBackingStore.cpp:203: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/TiledBackingStore.cpp:212: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Source/WebKit/qt/Api/qwebpage.cpp:1230: Missing space before ( in if( [whitespace/parens] [5]
Total errors found: 4 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 96337 [details]
Patch 5: Add tile update timeout to break tile painting cycle to avoid blocking.
Created attachment 96338 [details]
Patch 6: Do some tiles updates directly from paint to reduce flicker.
Created attachment 96339 [details]
Patch 7: Fixes repainting problems if scale != 1 .
Created attachment 96344 [details]
Patch 1: Add API to notify tiling about scrolling direction. Bias tile distance against scrolling direction.
Comment on attachment 96339 [details]
Patch 7: Fixes repainting problems if scale != 1 .
Any way to make tests for this? Why wasnt this a problem before? or was it?
Comment on attachment 96335 [details] Patch 3: Add tile reuse. View in context: https://bugs.webkit.org/attachment.cgi?id=96335&action=review > Source/WebCore/ChangeLog:9 > + Part3 of tiles painting redesign. > + Add tile reuse. You don't explain the reasoning, why this is a good idea, any cons? > Source/WebCore/platform/graphics/TiledBackingStore.cpp:-238 > - // Remove tiles that extend outside the current contents rect. Why remove the explanation of what overhanging tiles are... I think it is quite confusing. > Source/WebCore/platform/graphics/TiledBackingStore.cpp:245 > + getOverhangingTiles(keepRect, visibleRect, overhangingTiles); getTilesExtendingOutsideContentsRect ? or similar. You use visibleRect here and in the implementation you use viewport.... confusing > Source/WebCore/platform/graphics/TiledBackingStore.cpp:286 > + // all tiles in place, can drop remainting overhanging tiles Please use proper sentences for comments. Start with capital and end with dot. > Source/WebCore/platform/graphics/TiledBackingStore.cpp:394 > + RefPtr<Tile> ret = tileAt(oldCoordinate); Please use something more descriptive than ret > Source/WebCore/platform/graphics/TiledBackingStore.cpp:398 > + setTile(newCoordinate, ret); > + return ret; duplicated code > Source/WebCore/platform/graphics/TiledBackingStore.cpp:402 > + setTile(newCoordinate, ret); > + return ret; same code here > Source/WebCore/platform/graphics/TiledBackingStore.cpp:410 > + return (int)((va->distance() - vb->distance()) * 1000); why not static_cast<int>() ? > Source/WebCore/platform/graphics/TiledBackingStore.cpp:413 > +void TiledBackingStore::getOverhangingTiles(const IntRect& rect, const IntRect& viewport, CoordinateDistanceVector& overhangingTiles) Why not return CoordinateDistanceVector& ? would make the api a lot more clear > Source/WebCore/platform/graphics/TiledBackingStore.h:100 > + CoordinateDistancePair(const Tile::Coordinate& coordinate, double distance) : > + m_coordinate(coordinate), m_distance(distance) {} This is not valid style > Source/WebCore/platform/graphics/TiledBackingStore.h:106 > + Tile::Coordinate m_coordinate; > + double m_distance; > + }; Remove the spacing between Coordinate and m_coordinate etc Comment on attachment 96344 [details] Patch 1: Add API to notify tiling about scrolling direction. Bias tile distance against scrolling direction. View in context: https://bugs.webkit.org/attachment.cgi?id=96344&action=review Jocelyn did something similar for webkit2, you need to sync up. > Source/WebCore/platform/graphics/TiledBackingStore.cpp:197 > + // viewport covering tile should be closer Not a proper sentence > Source/WebCore/platform/graphics/TiledBackingStore.cpp:210 > + if (m_scrollDelta.x() && xOffset) { > + double xBiasAlpha = (m_keepAreaMultiplier.width() - m_coverAreaMultiplier.width()) / > + m_coverAreaMultiplier.width() / 2 + 1; > + if ((xOffset > 0 && m_scrollDelta.x() > 0) || (xOffset < 0 && m_scrollDelta.x() < 0)) > + xBias = 1.f / xBiasAlpha; > + else > + xBias = xBiasAlpha; > + } I would almost make a method for calculating this... > Source/WebCore/platform/graphics/TiledBackingStore.h:68 > - void setKeepAndCoverAreaMultipliers(const FloatSize& keepMultiplier, const FloatSize& coverMultiplier); > + void setKeepAndCoverAreaMultipliers(const FloatSize& keepMultiplier, const FloatSize& coverMultiplier); what was changed here? > Source/WebCore/platform/graphics/TiledBackingStore.h:69 > + void setScrollDelta(const IntPoint&); We normally use IntSize for scrolling etc. Check the rest of webkit Why not scrollBy(const IntSize&) ? > Source/WebKit/qt/Api/qwebpage.cpp:1215 > + } else if (event->propertyName() == "_q_TiledBackingStoreScrollDelta") { Why this private stuff? > Source/WebKit/qt/ChangeLog:10 > + [Qt] Very jerky scrolling because of blocking in tiled backing store. > + https://bugs.webkit.org/show_bug.cgi?id=53894 > + > + Part1 of tiles painting redesing. > + Add API to notify tiling about scrolling direction. Bias tile distance against > + scrolling direction. The description doesnt have so much to do with the bug... why not create another bug report Comment on attachment 96336 [details] Patch 4: Prioritize tile paiting. Viewport area is updated 1st and missing tiles are updated before dirty. View in context: https://bugs.webkit.org/attachment.cgi?id=96336&action=review > Source/WebCore/platform/graphics/TiledBackingStore.cpp:430 > + if (tile) { > + if(tile->isDirty()) { > + dirtyTiles.append(CoordinateDistancePair(currentCoordinate, currentTileDistance)); > + } > + } else { > + missingTiles.append(CoordinateDistancePair(currentCoordinate, currentTileDistance)); > + } You are wasting reviewers time, why didnt you even run the check-webkit-style script? Comment on attachment 96338 [details] Patch 6: Do some tiles updates directly from paint to reduce flicker. View in context: https://bugs.webkit.org/attachment.cgi?id=96338&action=review > Source/WebCore/ChangeLog:9 > + Do some tiles updates directly from paint to reduce flicker. Why does that help... needs better change log. > Source/WebCore/page/Frame.cpp:966 > -void Frame::tiledBackingStorePaintBegin() > +bool Frame::tiledBackingStorePaintBegin() Very bad api, I have no clue what the returned bool could mean > Source/WebCore/page/Frame.cpp:971 > + if(m_view->isInLayout()) > + return false; yes again you submit a patch with wrong coding style... > Source/WebCore/platform/graphics/TiledBackingStore.cpp:45 > + , m_inUpdate(false) Could be more descriptive (In reply to comment #16) > (From update of attachment 96335 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96335&action=review > > > Source/WebCore/ChangeLog:9 > > + Part3 of tiles painting redesign. > > + Add tile reuse. > > You don't explain the reasoning, why this is a good idea, any cons? Reallocating pixmap takes time. Even with sorting 50 element array (10 times bigger than usual size) reuse function is much faster. Creating one tile with the associated pixmap cause at least 5 allocations. I want to avoid it here. > > Source/WebCore/platform/graphics/TiledBackingStore.cpp:-238 > > - // Remove tiles that extend outside the current contents rect. > > Why remove the explanation of what overhanging tiles are... I think it is quite confusing. In Antti's implementation tile rect is clipped by content rect, so if content size changes (javascript or loading) tiles on a border need to be dropped in recreated. In addition to extra cpu usage that causes some flicker (not very bad). I want to avoid this. Stored in tile rects are not clipped by content and have full tile area. In any case tile painting is clipped by painter clipping (coming from QWebFrame::renderFromTiledBackingStore for example). > > Source/WebCore/platform/graphics/TiledBackingStore.cpp:245 > > + getOverhangingTiles(keepRect, visibleRect, overhangingTiles); > > getTilesExtendingOutsideContentsRect ? or similar. I picked up "overhanging" from previous code. ExtendingOutside might mean "partially outside" and in my case tile rects shouldn't be intersecting keepRect. Also, they are outside keep rect. What about getTilesOutsizeKeepRect ? This should describe exactly what it means. > You use visibleRect here and in the implementation you use viewport.... > confusing Fixed. > > Source/WebCore/platform/graphics/TiledBackingStore.cpp:286 > > + // all tiles in place, can drop remainting overhanging tiles > > Please use proper sentences for comments. Start with capital and end with dot. Fixed. > > Source/WebCore/platform/graphics/TiledBackingStore.cpp:394 > > + RefPtr<Tile> ret = tileAt(oldCoordinate); > > Please use something more descriptive than ret Fixed. > > Source/WebCore/platform/graphics/TiledBackingStore.cpp:398 > > + setTile(newCoordinate, ret); > > + return ret; > > duplicated code Fixed. > > Source/WebCore/platform/graphics/TiledBackingStore.cpp:402 > > + setTile(newCoordinate, ret); > > + return ret; > > same code here Fixed. > > Source/WebCore/platform/graphics/TiledBackingStore.cpp:410 > > + return (int)((va->distance() - vb->distance()) * 1000); > > why not static_cast<int>() ? Fixed. > > Source/WebCore/platform/graphics/TiledBackingStore.cpp:413 > > +void TiledBackingStore::getOverhangingTiles(const IntRect& rect, const IntRect& viewport, CoordinateDistanceVector& overhangingTiles) > > Why not return CoordinateDistanceVector& ? would make the api a lot more clear Fixed, but return value will be never used. > > Source/WebCore/platform/graphics/TiledBackingStore.h:100 > > + CoordinateDistancePair(const Tile::Coordinate& coordinate, double distance) : > > + m_coordinate(coordinate), m_distance(distance) {} > > This is not valid style > > > Source/WebCore/platform/graphics/TiledBackingStore.h:106 > > + Tile::Coordinate m_coordinate; > > + double m_distance; > > + }; > > Remove the spacing between Coordinate and m_coordinate etc Sorry, it appeared that I was running check-webkit-style --diff-files incorrectly :( (In reply to comment #17) > (From update of attachment 96344 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96344&action=review > > Jocelyn did something similar for webkit2, you need to sync up. > > > Source/WebCore/platform/graphics/TiledBackingStore.cpp:197 > > + // viewport covering tile should be closer > > Not a proper sentence Fixed. > > Source/WebCore/platform/graphics/TiledBackingStore.cpp:210 > > + if (m_scrollDelta.x() && xOffset) { > > + double xBiasAlpha = (m_keepAreaMultiplier.width() - m_coverAreaMultiplier.width()) / > > + m_coverAreaMultiplier.width() / 2 + 1; > > + if ((xOffset > 0 && m_scrollDelta.x() > 0) || (xOffset < 0 && m_scrollDelta.x() < 0)) > > + xBias = 1.f / xBiasAlpha; > > + else > > + xBias = xBiasAlpha; > > + } > > I would almost make a method for calculating this... Ok. Fixed. > > Source/WebCore/platform/graphics/TiledBackingStore.h:68 > > - void setKeepAndCoverAreaMultipliers(const FloatSize& keepMultiplier, const FloatSize& coverMultiplier); > > + void setKeepAndCoverAreaMultipliers(const FloatSize& keepMultiplier, const FloatSize& coverMultiplier); > > what was changed here? qtcreator ate spaces that were at the end of the line in original code. Should I return them back? ;) > > Source/WebCore/platform/graphics/TiledBackingStore.h:69 > > + void setScrollDelta(const IntPoint&); > > We normally use IntSize for scrolling etc. Check the rest of webkit Fixed. > Why not scrollBy(const IntSize&) ? scrollBy assumes that some scrolling would happen, but in this case backing store just gets notified that scrolling happens and update strategy should be adjusted. I think better name would be setLastViewportMotion similar to naming Jocelyn use in wk2. > > Source/WebKit/qt/Api/qwebpage.cpp:1215 > > + } else if (event->propertyName() == "_q_TiledBackingStoreScrollDelta") { > > Why this private stuff? What kind of API it should be? 1st I tried just to hook it into QWebFrame::setScrollPosition . The problem, that nobody calls setScrollPosition twice at the end to reset motion delta. Resetting it by timer also didn't work well. On wk2 Jocelyn has special panning/scrolling mode. Would it work better? > > Source/WebKit/qt/ChangeLog:10 > > + [Qt] Very jerky scrolling because of blocking in tiled backing store. > > + https://bugs.webkit.org/show_bug.cgi?id=53894 > > + > > + Part1 of tiles painting redesing. > > + Add API to notify tiling about scrolling direction. Bias tile distance against > > + scrolling direction. > > The description doesnt have so much to do with the bug... why not create another bug report Modification of update strategy during scrolling is a way to fix scrolling jerkiness, isn't it? IMHO, all this patches (may be except the last one) should go together. They all build, but cause some visual regressions with tiled backing store on. Real improvement can be seen if all patches are applied. > Reallocating pixmap takes time. Even with sorting 50 element array (10 times bigger than usual size) reuse function is much faster. > Creating one tile with the associated pixmap cause at least 5 allocations. I want to avoid it here. Add this to the commit message > In Antti's implementation tile rect is clipped by content rect, so if content size changes (javascript or loading) tiles on a border need to be dropped in recreated. In addition to extra cpu usage that causes some flicker (not very bad). A description like the above should also be part of the commit message. "In the original implementation, the tile rect is clipped by the contents rect, which means that contents size changes (due to loading, or javascript) can result in the tiles near the border being dropped and recreated. This results in some flicker which this patch tries to avoid" > I want to avoid this. Stored in tile rects are not clipped by content and have full tile area. In any case tile painting is clipped by painter clipping (coming from QWebFrame::renderFromTiledBackingStore for example). "The way this is accomplished is by not clipping the tile rects to the contents .... " > > > Source/WebCore/platform/graphics/TiledBackingStore.cpp:245 > > > + getOverhangingTiles(keepRect, visibleRect, overhangingTiles); > > > > getTilesExtendingOutsideContentsRect ? or similar. > > I picked up "overhanging" from previous code. > ExtendingOutside might mean "partially outside" and in my case tile rects shouldn't be intersecting keepRect. > Also, they are outside keep rect. > What about getTilesOutsizeKeepRect ? This should describe exactly what it means. Tiles(Fully|Partially)OutsideKeepRect sounds a lot more clear to me. escription doesnt have so much to do with the bug... why not create another bug report
>
> Modification of update strategy during scrolling is a way to fix scrolling jerkiness, isn't it?
> IMHO, all this patches (may be except the last one) should go together. They all build, but cause some visual regressions with tiled backing store on. Real improvement can be seen if all patches are applied.
We support the creating of meta bugs.
Comment on attachment 96339 [details] Patch 7: Fixes repainting problems if scale != 1 . Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 96339 [details] so that this bug does not appear in http://webkit.org/pending-commit. Removing the block for Bug 55055 (QtWebKit-2.2 release-critical), as this change is too complex/risky at this point. Old bug, architecture completely changed since. (In reply to comment #26) > Old bug, architecture completely changed since. Have you verified that the issue originally reported is solved? (In reply to comment #27) > (In reply to comment #26) > > Old bug, architecture completely changed since. > > Have you verified that the issue originally reported is solved? Frankly, no, and I should have. If someone feels like this was closed in error, please reopen and explain. My reason for closing it is that there seems to be a lot of work on tiled-backing-store, but it's not really being updated to this bug, which makes it low value in relevance. (In reply to comment #27) > Have you verified that the issue originally reported is solved? This was for WebKit1, TiledBackingStore was originally supposed to only be a step stone to eventually get threaded rendering, and then we switched to WebKit2. The non-threaded tiling case just can't work smoothly and, if I remember correctly, this bug was about the best efforts Slava made about it. It wasn't without compromise so I think we should focus on WebKit2 for smooth panning instead. Yes, this was about WebKit1. If people interested I can dig and find some fixes that Slava and myself did for improving perfomance of the WebKit1 tiled rendering. There were several fixes that might help people with webkit1... |