Bug 73920 – [Qt] Tiled backing store failing to update view under certain conditions

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Bruno Abinader (history only)]

Created attachment 118039 [details]
QtWebKit testcase for QGraphicsWebView

When loading a new webpage on a QGraphicsWebView with Tiled Backing Store option enabled, some dirty tiles from the previous webpage are still seen when scrolling down the webpage. The tiles are only updated after a scale change. A testcase is provided as a patch. Desktop and Harmattan device results are the same:

FAIL!  : tst_QGraphicsWebView::widgetsRenderingAfterScale(3 NoResize, TBS, NoFrozen, BeforeScene) 'email2AfterScrollPixmap.toImage() == email2AfterScalePixmap.toImage()' returned FALSE. ()
   Loc: [tst_qgraphicswebview.cpp(352)]
FAIL!  : tst_QGraphicsWebView::widgetsRenderingAfterScale(4 NoResize, TBS, NoFrozen, AfterScene) 'email2AfterScrollPixmap.toImage() == email2AfterScalePixmap.toImage()' returned FALSE. ()
   Loc: [tst_qgraphicswebview.cpp(352)]
FAIL!  : tst_QGraphicsWebView::widgetsRenderingAfterScale(9 Resize, TBS, NoFrozen, AfterScene) 'email2AfterScrollPixmap.toImage() == email2AfterScalePixmap.toImage()' returned FALSE. ()

[Bruno Abinader (history only)]

Also worth noticing that this bug has been tested on QtWebKit 2.2 release and trunk version as well (using Qt 4.8).

[Lamarque V. Souza]

Created attachment 170541 [details]
Patch

Proposed patch

[Lamarque V. Souza]

Created attachment 170646 [details]
Patch

Merge last two patches into one

[Jocelyn Turcotte]

Comment on attachment 170646 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:2026
> +        frame()->tiledBackingStore()->removeAllTiles();

This feels quite wrong to drop all tiles just because of an issue edge tiles, the same tiles can be used between pages, and invalidate calls are responsible of triggering a refresh of the tile contents.
There is a call to tiledBackingStoreContentsRect in TiledBackingStore::createTiles that should return a different contents size for the new page. A change of that rect should be responsible of triggering a call to resizeEdgeTiles. Please check around there to fix your issue.

[Lamarque V. Souza]

(In reply to comment #4)
> (From update of attachment 170646 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170646&action=review
> 
> > Source/WebCore/page/FrameView.cpp:2026
> > +        frame()->tiledBackingStore()->removeAllTiles();
> 
> This feels quite wrong to drop all tiles just because of an issue edge tiles, the same tiles can be used between pages, and invalidate calls are responsible of triggering a refresh of the tile contents.
> There is a call to tiledBackingStoreContentsRect in TiledBackingStore::createTiles that should return a different contents size for the new page. A change of that rect should be responsible of triggering a call to resizeEdgeTiles. Please check around there to fix your issue.

I did some tests and WebKit calls resizeEdgeTiles() when loading the new page. resizeEdgeTiles() does not remove any tile, but TiledBackingStore::updateTileBuffers() detects some dirty tiles, but they are not being updated according to the new update when updateTileBuffers() calls dirtyTiles[n]->updateBackBuffer() for each of those dirty tiles.

[Lamarque V. Souza]

Just as a note, TiledBackingStore::commitScaleChange() always clears all tiles. Probably that is why this problem does not happen when scaling the page contents.

[Lamarque V. Souza]

Created attachment 171921 [details]
Patch

Fix some issues

[Lamarque V. Souza]

Comment on attachment 171921 [details]
Patch

This patch should have not gone to this bug entry.

[Lamarque V. Souza]

I tried to find a solution that does not involve dropping all tiles but none of my attempts worked for all cases. I tried dropping only the tiles marked as dirty when loading a new page, but if I scroll down to the end of the new page there is a wrong tile there. I fail to see how the same tile can be used for different pages. That would work only if the tile is part of the pagen's background or the pages are very similar, but the current code does not detect that situation and only takes the coordinates into account, not the page's url. tiledBackingStoreContentsRect can eventually return the same size for two different pages, can't it?

[Jocelyn Turcotte]

(In reply to comment #9)
When a page change, the part of the backing store that needs to be re-rendered is invalidated. When the new page is loaded, all the backing store needs to be invalidated.
If the page grows or shrink, then the backing store's size needs to be adjusted as well (an eventual call to createTiles), adding tiles allow growing, and resizeEdgeTiles takes care of resizing or dropping tiles until the size fits.
So to put it differently, each time that the value returned by tiledBackingStoreContentsRect changes, a call to createTiles should be triggered, and this is what seems to be missing.
Right now the only way to trigger it seems to be:
- setContentsFrozen (doesn't make much sense in this case)
- setTileSize (neither)
- coverWithTilesIfNeeded (won't call createTiles if the visible rect didn't change)

So maybe a way to fix it would be to add a TiledBackingStore::contentsRectChanged that would be called when the FrameView size changed and would call createTiles.
Another, less clean way, would be to keep the last ContentsRect and check if it changed in coverWithTilesIfNeeded, so that we also call createTiles in that case.

[Jocelyn Turcotte]

Comment on attachment 170646 [details]
Patch

Setting it to r- so that it doesn't appear in the review queue.

[Jocelyn Turcotte]

(In reply to comment #10)
> - coverWithTilesIfNeeded (won't call createTiles if the visible rect didn't change)

Humm if you say that you scroll to the bottom and still see the old tiles, then maybe the problem is different, maybe the value of tiledBackingStoreContentsRect isn't correct, or resizeEdgeTiles isn't doing its job properly.

[Lamarque V. Souza]

Created attachment 172206 [details]
Log with printf of some calls

Check the attached file, it contains some printfs of some calls without any modification in the current behaviour of WebKit (the method WebCore::TiledBackingStore::removeAllTiles() just prints the printf, it does not remove any tile). You can see after the printf in removeAllTiles() that WebKit calls createTiles(), then setKeepRect(), which does not remove any tile ("will remove 0/4" means will remove 0 tile from a total of 4 tiles), then call resizeEdgeTiles() (because the current rect changed). resizeEdgeTiles() resizes two tiles and does not remove any (line with "WebCore::TiledBackingStore::resizeEdgeTiles(): will remove 0/6 1"). In the end it calls updateTileBuffers(), which detects that there are four dirty tiles, but updateTileBuffers() seems to use the old page and not the new page to update the buffers.

The change I mentioned in comment #9 is changing updateTileBuffers() to delete the dirty tiles and create new tiles instead of calling dirtyTiles[n]->updateBackBuffer(). That change fixed the problem for what I think is the visible area (I have not checked this yet). So when I scroll down to the end of the new page and probably reach a tile that does not intersect the visible area the tile is incorrect.

dirtyTiles[n]->updateBackBuffer() is platform dependent. I am using Qt platform to test it, I have not tested if this problem also happens with other platform yet.

[Jocelyn Turcotte]

(In reply to comment #13)
Ok, from your output it seems like it's fine that no tile are removed. The two bottom tiles would be removed only if the height dropped lower than 1024 (1185 in your case).

Maybe the removal and resizing of tiles isn't the problem here, the problem could be that a resized tile won't trigger a repaint of the graphics view in the resized area.
The way a shrinking tile repaint works is that TileQt::resize will update TileQt::m_rect which will lead TileQt::paint not to paint over the background on this area.

There should be code that would pass the shrunk rect to the paintedArea with tiledBackingStorePaintEnd to force the QGraphicsView to repaint the background at this area and then call TileQt::paint, but I can't find any.
A way to do it would be to remove the "if (m_rect.maxX() > oldRect.maxX())" conditions in TileQt::resize and also invalidate the shrunk part of a tile.

If it was the case though you would probably not see the old part of the tile when the tile isn't visible in the QGraphicsView and then scroll down to see them (which would call TileQt::paint on that area), so it might be a somehow different issue.

[Lamarque V. Souza]

(In reply to comment #14)
> (In reply to comment #13)
> Ok, from your output it seems like it's fine that no tile are removed. The two bottom tiles would be removed only if the height dropped lower than 1024 (1185 in your case).
> 
> Maybe the removal and resizing of tiles isn't the problem here, the problem could be that a resized tile won't trigger a repaint of the graphics view in the resized area.
> The way a shrinking tile repaint works is that TileQt::resize will update TileQt::m_rect which will lead TileQt::paint not to paint over the background on this area.
> 
> There should be code that would pass the shrunk rect to the paintedArea with tiledBackingStorePaintEnd to force the QGraphicsView to repaint the background at this area and then call TileQt::paint, but I can't find any.
> A way to do it would be to remove the "if (m_rect.maxX() > oldRect.maxX())" conditions in TileQt::resize and also invalidate the shrunk part of a tile.

you mean doing this?

void TileQt::resize(const IntSize& newSize)
{
    IntRect oldRect = m_rect;
    m_rect = IntRect(m_rect.location(), newSize);

    //if (m_rect.maxX() > oldRect.maxX())
        invalidate(IntRect(oldRect.maxX(), oldRect.y(), m_rect.maxX() - oldRect.maxX(), m_rect.height()));
    //if (m_rect.maxY() > oldRect.maxY())
        invalidate(IntRect(oldRect.x(), oldRect.maxY(), m_rect.width(), m_rect.maxY() - oldRect.maxY()));
    invalidate(m_rect);
}

If so then it does not fix the entire issue :-( The tiles at the end of the second page are correct now, but not the ones in the middle.
 
> If it was the case though you would probably not see the old part of the tile when the tile isn't visible in the QGraphicsView and then scroll down to see them (which would call TileQt::paint on that area), so it might be a somehow different issue.

[Jocelyn Turcotte]

(In reply to comment #15)
The coordinates need to be fixed too, so maybe something like this:

void TileQt::resize(const IntSize& newSize)
{
    IntRect oldRect = m_rect;
    m_rect = IntRect(m_rect.location(), newSize);
    invalidate(unionRect(m_rect, oldRect));
}

[Lamarque V. Souza]

(In reply to comment #16)
> (In reply to comment #15)
> The coordinates need to be fixed too, so maybe something like this:
> 
> void TileQt::resize(const IntSize& newSize)
> {
>     IntRect oldRect = m_rect;
>     m_rect = IntRect(m_rect.location(), newSize);
>     invalidate(unionRect(m_rect, oldRect));
> }

Nope, the result is the same as in comment #15.

By what I can see in the logs there are 6 tiles created after loading the second page. 2 of them are resized and 4 are marked as dirty. I guess the 4 marked as dirty are the ones righ below the visible area when I start scrolling and the 2 being resized are the ones at the end of the scrolling. I think that because if I change TiledBackingStore::updateTileBuffers() to delete/recreate the dirty tiles instead of just calling dirtyTiles[n]->updateBackBuffer() then it almost work (the tiles are created but with a delay, at first the page becomes fully transparent and only after I start scrolling the new tiles show up). I think changing TileQt::resize() is not affecting the 4 dirty tiles in the test.

[Lamarque V. Souza]

Does anyone have any thought about what comment #17? Is my comment there correct?

[Jocelyn Turcotte]

(In reply to comment #18)
> Does anyone have any thought about what comment #17? Is my comment there correct?

Your comment seems right, additionally the only things I can assert are:
- All 6 tiles should be marked as dirty when the next page is loaded, at least under the area covered by the new page
- The two bottom tiles should be resized, no tile should be removed
- The 6 tiles updated should trigger a full repaint of the QGraphicsWebView

You could try to debug a bit more in those areas, printing out dirty and repainted rects. I can't say much more from the data you provided yet.

[Lamarque V. Souza]

(In reply to comment #19)
> (In reply to comment #18)
> > Does anyone have any thought about what comment #17? Is my comment there correct?
> 
> Your comment seems right, additionally the only things I can assert are:
> - All 6 tiles should be marked as dirty when the next page is loaded, at least under the area covered by the new page
> - The two bottom tiles should be resized, no tile should be removed
> - The 6 tiles updated should trigger a full repaint of the QGraphicsWebView
> 
> You could try to debug a bit more in those areas, printing out dirty and repainted rects. I can't say much more from the data you provided yet.


If I change the line below in TileQt::invalidate() from

IntRect tileDirtyRect = intersection(dirtyRect, m_rect);

to

IntRect tileDirtyRect = unionRect(dirtyRect, m_rect);

The the problem reduces itself. As long as the scroll is small it works as expected while with bigger scrolls the problem still remains although minimized (the area with wrong tiles is smaller). So there must be a problem in the code that calculates the size of the dirty area.

[Lamarque V. Souza]

(In reply to comment #19)
> - The 6 tiles updated should trigger a full repaint of the QGraphicsWebView

How can I trigger a full repaint of the QGraphicsWebView.
 
> You could try to debug a bit more in those areas, printing out dirty and repainted rects. I can't say much more from the data you provided yet.

I did several tests with this bug and just invalidating the tile does not fix the problem. In one of my tests I did this change (the "#if 0" part is the original code):

    Tile::Coordinate topLeft = tileCoordinateForPoint(coverRect.location());
    Tile::Coordinate bottomRight = tileCoordinateForPoint(innerBottomRight(coverRect));
    Vector<Tile::Coordinate> toRemove;
    for (int yCoordinate = topLeft.y(); yCoordinate <= bottomRight.y(); ++yCoordinate) {
        for (int xCoordinate = topLeft.x(); xCoordinate <= bottomRight.x(); ++xCoordinate) {
            Tile::Coordinate currentCoordinate(xCoordinate, yCoordinate);
            RefPtr<Tile> currentTile = tileAt(currentCoordinate);
            if (currentTile) {
#if 0
		continue;
#else
                if (m_removeOutdatedTiles)
                    toRemove.append(currentCoordinate);
                else
                    continue;
#endif
            }
            ++requiredTileCount;
            double distance = tileDistance(visibleRect, currentCoordinate);
            if (distance > shortestDistance)
                continue;
            if (distance < shortestDistance) {
                tilesToCreate.clear();
                shortestDistance = distance;
            }
            tilesToCreate.append(currentCoordinate);
        }
    }

m_removeOutdatedTiles is set to true right after a new page is loaded and set to false at the end of TiledBackingStore::createTiles(). With this change it works, but it is almost equal to the patch in comment #3.

If I change "toRemove.append(currentCoordinate);" with "currentTile->invalidate(currentTile->rect()); continue;" then the correct tile image is mixed with outdated tile image as if currentTile->invalidate() does not erase the outdated image before drawing the new tile image.

I also tried changing TileQt::updateBackBuffer() to always recreate m_backBuffer instead of reusing the m_buffer:

    if (!m_backBuffer) {

#if 1
        if (m_buffer) {
            delete m_buffer;
            m_buffer = 0;
        }
#endif

        if (!m_buffer) {
            m_backBuffer = new QPixmap(m_backingStore->tileSize().width(), m_backingStore->tileSize().height());
            m_backBuffer->fill(m_backingStore->client()->tiledBackingStoreBackgroundColor());
        } else {
            // Currently all buffers are updated synchronously at the same time so there is no real need
            // to have separate back and front buffers. Just use the existing buffer.
            m_backBuffer = m_buffer;
            m_buffer = 0;
        }
    }


With this change there is no mixed outdated and new tile, but there is also a blank area in the final page where it should not be. So this change erases more data than it should.

[Jocelyn Turcotte]

(In reply to comment #21)
> How can I trigger a full repaint of the QGraphicsWebView.

This should be a result of the paintedArea passed to tiledBackingStorePaintEnd.
The QGraphicsView should repaint the parts of its visible area that intersect with the area made dirty by tile modifications/removals.

[Lamarque V. Souza]

(In reply to comment #22)
> (In reply to comment #21)
> > How can I trigger a full repaint of the QGraphicsWebView.
> 
> This should be a result of the paintedArea passed to tiledBackingStorePaintEnd.
> The QGraphicsView should repaint the parts of its visible area that intersect with the area made dirty by tile modifications/removals.

Well, the line "currentTile->invalidate(currentTile->rect()); continue;" I mentioned in comment #21 is supposed to add the whole tile's area to the paintedArea. Unless currentTile->rect() does not return the whole tile's area. So this is not working as expected when loading a new page. What seems to happen is that webkit is painting the new tile on top of the old one. That (painting a tile on top of another) is happening even when removing the tile instead of just invalidating it. The unit tests still returns error in this case (the image are not pixel by pixel equal), some parts of the text are darker in one of the images.

[Jocelyn Turcotte]

I believe that those are the same bug, judging from the similar symptoms.

*** This bug has been marked as a duplicate of bug 113752 ***

[Lamarque V. Souza]

(In reply to comment #24)
> I believe that those are the same bug, judging from the similar symptoms.
> 
> *** This bug has been marked as a duplicate of bug 113752 ***

The patch in bug 113752 does not fix this bug. The unit tests in my last patch still catch the same errors. You can also see errors when browsing pages with QtTestBrowser with tiled backing store enabled.

I really do not think there is any other option but erasing all tiles when loading a new page, at least for now. When loading a new page the tile objects keep the image buffer of the old page and draws the tile for the new page over that buffer (that is, over the old page's tile). It should clear the buffer first. That is one of the problems I see. Even after erasing all tiles after loading a new page I still can see errors when browsing with QtTestBrowser, so erasing the tiles is a progress, not a final fix.

[Jocelyn Turcotte]

(In reply to comment #25)
> I really do not think there is any other option but erasing all tiles when loading a new page, at least for now. When loading a new page the tile objects keep the image buffer of the old page and draws the tile for the new page over that buffer (that is, over the old page's tile). It should clear the buffer first. That is one of the problems I see. Even after erasing all tiles after loading a new page I still can see errors when browsing with QtTestBrowser, so erasing the tiles is a progress, not a final fix.

Ok thanks for verifying, it could then be a bug somewhere between TiledBackingStore and QGraphicsWebView.
Erasing tiles must not be necessary, the backing store is completely invalidated when the page is navigated and this should be enough. Applying a workaround like this has a maintenance cost and should only be done if we understand the problem enough to know that this is the best solution we have. Right now it doesn't seem to be the case.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.