Bug 64728 - TiledDrawingArea: Use tile sets to paint old content while rendering for a new scale.
Summary: TiledDrawingArea: Use tile sets to paint old content while rendering for a ne...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Jocelyn Turcotte
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-18 08:46 PDT by Jocelyn Turcotte
Modified: 2011-07-29 08:30 PDT (History)
4 users (show)

See Also:


Attachments
Patch (27.56 KB, patch)
2011-07-18 08:59 PDT, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Move to own file patch (21.41 KB, patch)
2011-07-18 09:03 PDT, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (28.80 KB, patch)
2011-07-26 05:36 PDT, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (27.51 KB, patch)
2011-07-27 03:57 PDT, Jocelyn Turcotte
benjamin: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jocelyn Turcotte 2011-07-18 08:46:59 PDT
TiledDrawingArea: Use tile sets to paint old content while rendering for a new scale.
Comment 1 Jocelyn Turcotte 2011-07-18 08:59:52 PDT
Created attachment 101163 [details]
Patch
Comment 2 Jocelyn Turcotte 2011-07-18 09:03:05 PDT
Created attachment 101165 [details]
Move to own file patch

Tell me if you would prefer them squashed.
Comment 3 Benjamin Poulain 2011-07-18 09:11:13 PDT
Comment on attachment 101163 [details]
Patch

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

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.h:183
> +    TiledDrawingAreaTileSet* m_currentTiles;
> +    TiledDrawingAreaTileSet* m_previousScaleTiles;

OwnPtr!!! :)
Comment 4 Benjamin Poulain 2011-07-18 09:26:20 PDT
(In reply to comment #2)
> Created an attachment (id=101165) [details]
> Move to own file patch
> 
> Tell me if you would prefer them squashed.

Yep, that would be nice actually.
Comment 5 Benjamin Poulain 2011-07-18 09:43:07 PDT
Comment on attachment 101163 [details]
Patch

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

Quick comments:

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:60
> +{
> +}

It'd be nice to have ASSERT(m_proxy) here so that fails early.

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.h:78
> +    TileMap& tiles() { return m_tiles; }

This is breaking the abstraction of TiledDrawingAreaTileSet.

It looks to me like TiledDrawingAreaProxy::updateTileBuffers() does not use any attribute so it could be a method of TiledDrawingAreaTileSet.
The case of TiledDrawingAreaProxy::tileBufferUpdateComplete() is a bit more tricky. If you manage to refactor it to avoid knowing about the tiles, the TiledDrawingAreaTileSet would be the only owner of m_tiles making a nicer abstraction.

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.h:82
> +    void setTile(const TiledDrawingAreaTile::Coordinate&, PassRefPtr<TiledDrawingAreaTile>);

Should this be private? It looks for me like the TiledDrawingAreaTileSet has the responsibility to manage its TiledDrawingAreaTile.
Comment 6 Jocelyn Turcotte 2011-07-20 13:21:18 PDT
(In reply to comment #5)
> > Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:60
> > +{
> > +}
> 
> It'd be nice to have ASSERT(m_proxy) here so that fails early.
> 
Good idea.

> > Source/WebKit2/UIProcess/TiledDrawingAreaProxy.h:78
> > +    TileMap& tiles() { return m_tiles; }
> 
> This is breaking the abstraction of TiledDrawingAreaTileSet.
> 
> It looks to me like TiledDrawingAreaProxy::updateTileBuffers() does not use any attribute so it could be a method of TiledDrawingAreaTileSet.
> The case of TiledDrawingAreaProxy::tileBufferUpdateComplete() is a bit more tricky. If you manage to refactor it to avoid knowing about the tiles, the TiledDrawingAreaTileSet would be the only owner of m_tiles making a nicer abstraction.
> 
> > Source/WebKit2/UIProcess/TiledDrawingAreaProxy.h:82
> > +    void setTile(const TiledDrawingAreaTile::Coordinate&, PassRefPtr<TiledDrawingAreaTile>);
> 
> Should this be private? It looks for me like the TiledDrawingAreaTileSet has the responsibility to manage its TiledDrawingAreaTile.

Humm, at first I wanted to split responsibilities but I soon realized that there is no apparent reason to do so, handling tiles, their updates and painting them are not things easy to split apart and will introduce complexity with few benefits if done so.
The tile set was then just supposed to be a tile container with an attached scale ratio and a bare minimum of methods in it.

Though since this seems to annoy you as well, having this result in the end might give the impression of an incomplete lazy half-opened class. It might be better to just have the tile set as a struct { tiles; scale; }; and move those functions back to the TDAProxy with a tile set struct reference parameter. What do you think? Or tell me if you have other ideas.
Because anyway the only need I have is to group those tiles together.
Comment 7 Jocelyn Turcotte 2011-07-20 13:29:20 PDT
(In reply to comment #6)
Humm wait, better than that, move the contentsScale attribute in the Tile along with knowledge of it's contentsRect in the page and just have the tile set replace by a normal container.

I think I'll give that one a try.
Comment 8 Jocelyn Turcotte 2011-07-20 13:43:09 PDT
(In reply to comment #7)
> (In reply to comment #6)
> Humm wait, better than that, move the contentsScale attribute in the Tile along with knowledge of it's contentsRect in the page and just have the tile set replace by a normal container.
> 
> I think I'll give that one a try.

Ahhh... this won't work since I would need to remove the gc.setScale(invert) trick at the beginning of paint(). I tried that last weeks and had an annoying slight scaling with the opengl graphics system that I couldn't get rid of as I told you.

Anyway, please tell me what you think about the struct { tiles; scale; } idea.
I could also move the scale into the tile class as said and do a if (tile->scale() != lastScale) { gc.setScale(lastScale / tile->scale(); }, that might be clean actually since I would paint tiles with the same scale all at once.
Comment 9 Jocelyn Turcotte 2011-07-26 05:36:48 PDT
Created attachment 101988 [details]
Patch

Removed unnecessary stuff from the TileSet and changed add/remove tile for attach/detach in the proxy.
Tell me what you think.
Comment 10 Benjamin Poulain 2011-07-26 07:17:52 PDT
Comment on attachment 101988 [details]
Patch

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

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:49
> +    typedef HashMap<TiledDrawingAreaTile::Coordinate, RefPtr<TiledDrawingAreaTile> > TileMap;

Totally unrelated to your patch but I wonder why TiledDrawingAreaTile is refcounted. It looks like total ownership by TiledDrawingAreaTileSet to me. I guess it is simply because HashMap does not handle OwnPtr due to the lack of copy constructor?

That is something to keep in mind because RefPtr are a bit more complicated to handle right. We could probably have a destructor for TiledDrawingAreaTileSet that call deleteAllPairSeconds() and get rid of RefPtr.

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:350
> +

Uh?

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:366
> +    float rectArea = dirtyRect.width() * dirtyRect.height();

Can dirty rect be empty here?
In which case the return would likely be +inf (which might fine with the callers? :))

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.h:158
> +    OwnPtr<TiledDrawingAreaTileSet> m_currentTiles;
> +    OwnPtr<TiledDrawingAreaTileSet> m_previousScaleTiles;

I think it'd be nice to rename them to m_currentTileSet and m_previousTileSet.

> Source/WebKit2/UIProcess/TiledDrawingAreaTile.h:53
> +    void setProxy(TiledDrawingAreaProxy*);

I am not convinced by this new API of TiledDrawingAreaTile.
Comment 11 Benjamin Poulain 2011-07-26 10:15:20 PDT
Comment on attachment 101988 [details]
Patch

Clearing the review flag. We discussed with Jocelyn and he wants to do some improvement.
Comment 12 Jocelyn Turcotte 2011-07-27 03:57:58 PDT
Created attachment 102118 [details]
Patch
Comment 13 Jocelyn Turcotte 2011-07-27 04:02:15 PDT
(In reply to comment #10)
> Totally unrelated to your patch but I wonder why TiledDrawingAreaTile is refcounted. It looks like total ownership by TiledDrawingAreaTileSet to me. I guess it is simply because HashMap does not handle OwnPtr due to the lack of copy constructor?
> 
> That is something to keep in mind because RefPtr are a bit more complicated to handle right. We could probably have a destructor for TiledDrawingAreaTileSet that call deleteAllPairSeconds() and get rid of RefPtr.
> 
Right now I don't think there is any reason why we need them refcounted. What do you mean by "to handle right", what is the cost/risk? 

> > Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:350
> > +
> 
> Uh?
> 
Oops

> > Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:366
> > +    float rectArea = dirtyRect.width() * dirtyRect.height();
> 
> Can dirty rect be empty here?
> In which case the return would likely be +inf (which might fine with the callers? :))
> 
It shouldn't be empty, but if it does, +inf would be the correct return value.

> > Source/WebKit2/UIProcess/TiledDrawingAreaProxy.h:158
> > +    OwnPtr<TiledDrawingAreaTileSet> m_currentTiles;
> > +    OwnPtr<TiledDrawingAreaTileSet> m_previousScaleTiles;
> 
> I think it'd be nice to rename them to m_currentTileSet and m_previousTileSet.
> 
Changed

> > Source/WebKit2/UIProcess/TiledDrawingAreaTile.h:53
> > +    void setProxy(TiledDrawingAreaProxy*);
> 
> I am not convinced by this new API of TiledDrawingAreaTile.
Changed with Tile::disableUpdates() that calls Proxy::unregisterTile.
Also called disableUpdates() in the tile's destructor so that deleting the tile object will also cleanup the proxy's tileByID map.
Comment 14 WebKit Review Bot 2011-07-27 04:20:14 PDT
Comment on attachment 102118 [details]
Patch

Attachment 102118 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9247736

New failing tests:
fast/canvas/webgl/framebuffer-test.html
Comment 15 Benjamin Poulain 2011-07-27 05:03:26 PDT
> > That is something to keep in mind because RefPtr are a bit more complicated to handle right. We could probably have a destructor for TiledDrawingAreaTileSet that call deleteAllPairSeconds() and get rid of RefPtr.
> > 
> Right now I don't think there is any reason why we need them refcounted. What do you mean by "to handle right", what is the cost/risk? 

See this discussion: https://lists.webkit.org/pipermail/webkit-dev/2011-June/017178.html

It is really easy to write code that look correct and is wrong with PassRefPtr.
Comment 16 Benjamin Poulain 2011-07-27 12:46:23 PDT
Comment on attachment 102118 [details]
Patch

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

Looks good :)

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:396
> +    if (m_currentTileSet->contentsScale() == scale)

I doubt this is gonna be hit often :)

> Source/WebKit2/UIProcess/TiledDrawingAreaProxy.cpp:558
> +void TiledDrawingAreaProxy::removeAllTiles()

I don't think you need this method at all now. The tiles are cleared when the old tileset is not needed anymore.
Comment 17 Jocelyn Turcotte 2011-07-29 01:41:42 PDT
Thx! As told in the corridor, removeAllTiles was already used by setTileSize so I kept it there.

Committed r91979: <http://trac.webkit.org/changeset/91979>
Comment 18 Jocelyn Turcotte 2011-07-29 06:38:51 PDT
The patch caused a RefPtr assert in debug (hummm see comment #15).
Fixed it in http://trac.webkit.org/changeset/91990 .

There is also a segmentation fault on shutdown that was introduced by the tileset patch, working on it.
Comment 19 Jocelyn Turcotte 2011-07-29 08:30:06 PDT
Fixed the segfault in http://trac.webkit.org/changeset/91994