The reason is that LayerTiler computes which tiles are visible by exhaustively mapping every tile to normalized device coordinates and checking if it intersects the viewport. If there's a lot of tiles, it gets stuck in this loop, iterating over all tiles, for a very long time. Also, the visibility information is transferred to the WebKit thread using mutexes, which adds unnecessary mutex contention to the mix. We can fix this by doing the reverse calculation, unprojecting the visible part of the layer to layer coordinate space to find which tiles are visible. We can use the w-coordinates of triangle vertices to perform "perspective correct texturing" of the intersection points, which is equivalent to unprojection since perspective correct texturing computes a 2D-coordinate in (normalized) layer coordinate space which is easily scaled up to the layer bounds and get the visible region expressed in layer coordinate space. We approximate this visible area using a rectangle, and suddenly we have a much simpler representation of tile visibility. A set of tiles needing render are added to the layer visibility data structure, and suddenly we have a swappable data structure so visbility can be transferred to WebKit thread without mutexes, but instead using atomic swap. This patch depends on the fix for bug #117067 to be landed before it can apply.
Created attachment 203731 [details] Patch
This also fixes a bug where you'd get a flash of white (or background color) for unpopulated tiles after zooming out, due to WebKit thread using stale visibility information. Now, visibility is expressed as a rectangle in document coordinates, and can't get stale since it's decoupled from the "transformed" coordinate system change caused by retiling.
I forgot to mention, this is PR 273550
Created attachment 203732 [details] Patch Add PR number to ChangeLog entry
Comment on attachment 203732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203732&action=review Slightly intimidating and borderline great. Informal r+ with a few minor remarks. > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:62 > + bool needsRender() const { return !m_tilesNeedingRender.isEmpty(); } > + bool tileNeedsRender(const TileIndex& index) const { return m_tilesNeedingRender.contains(index); } According to WebKit style guides, these function names should get a bool-indicating prefix, such as doesNeedRender()/doesTileNeedRender() or willNeedRender() or something of that sort. Alternatively it could also go a different route and use vocabulary in the vein of isTileDirty(). Especially when the next line defines willRenderTile() which actually modifies the object, I believe this distinction makes sense here. > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:63 > + void willRenderTile(const TileIndex& index) { m_tilesNeedingRender.remove(index); m_tilesRendered.add(index); } (Just throwing into the room: markTileAsRendered() might also work for what this function is doing. If you feel the current name is good though, feel free to ignore, I'm not set on this.) > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:172 > +#if DEBUG_LAYER_VISIBILITY > + if (m_backVisibility && !m_backVisibility->visibleRect().isEmpty()) > + printf("Layer 0x%p local visible rect %s\n", m_layer, BlackBerry::Platform::FloatRect(m_backVisibility->visibleRect()).toString().c_str()); > +#endif Should this be using BlackBerry::Platform::logAlways()? > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:196 > - if (scale != 1.0) { > + if (scale != 1) { Why are you changing this? scale is already a double, so what benefit is there from not comparing it with a double literal? (Same question for the one in the previous hunk.) > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:263 > + printf("Tile at (%d, %d) needs render\n", index.i(), index.j()); Like above, printf() or logAlways()? > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:508 > + FloatPoint origin = boundsRect.location(); > + FloatSize bounds = boundsRect.size(); Given that you're touching every line of this function anyway and in the interest of minimizing the amount of terminology used, I would suggest not having these two constants and using boundsRect directly instead. > Source/WebCore/platform/graphics/blackberry/LayerTiler.h:31 > +#include <BlackBerryPlatformThreading.h> If you're removing the mutex and manually including the header for _smp_xchg() in the .cpp file, what's prompting the addition of this #include line? Can it be removed or, if not that, pushed down into other files that still use BlackBerry::Platform threading classes? > Source/WebCore/platform/graphics/blackberry/LayerUtilities.h:187 > + *ok = (denominator != 0.0); With "ok" being a pointer, should this have a check for "if (ok)"? Otherwise, I'm not clear on where you draw the line between this and your previous patch where you added a return value for "float& w" that is not a pointer. > Source/WebCore/platform/graphics/blackberry/LayerUtilities.h:255 > + size_t i = (i0 + di) % vertices.size(); > + const FloatPoint& p1 = vertices[i].xy; > + float w1 = vertices[i].w; > + FloatSize uv1 = vertices[i].uv; > + const FloatPoint& p2 = vertices[i + 1].xy; > + float w2 = vertices[i + 1].w; > + FloatSize uv2 = vertices[i + 1].uv; While I agree with the idea behind ordering it this way, it looks really cluttered. I feel grouping the two FloatPoints, the two floats and the two FloatSizes together respectively would make a cleaner picture.
(In reply to comment #5) > (From update of attachment 203732 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=203732&action=review > > Slightly intimidating and borderline great. Informal r+ with a few minor remarks. > > > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:62 > > + bool needsRender() const { return !m_tilesNeedingRender.isEmpty(); } > > + bool tileNeedsRender(const TileIndex& index) const { return m_tilesNeedingRender.contains(index); } > > According to WebKit style guides, these function names should get a bool-indicating prefix, such as doesNeedRender()/doesTileNeedRender() or willNeedRender() or something of that sort. > Alternatively it could also go a different route and use vocabulary in the vein of isTileDirty(). > Especially when the next line defines willRenderTile() which actually modifies the object, I believe this distinction makes sense here. I searched for "needs" across the code base and found enough methods starting with "needs" that I believe "needs" could actually be considered to be one of these bool-indicating prefixes. Placing the bool-indicating prefix after a subject is also common across Cocoa and WebKit. So I prefer not to change these method names. > > > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:63 > > + void willRenderTile(const TileIndex& index) { m_tilesNeedingRender.remove(index); m_tilesRendered.add(index); } > > (Just throwing into the room: markTileAsRendered() might also work for what this function is doing. If you feel the current name is good though, feel free to ignore, I'm not set on this.) > willRenderTile is naming typical of a Cocoa delegate method, and the delegate is typically free to do mutating actions in its implementation. Since there's no actual delegate interface being implemented here, I think changing the name makes more sense. > > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:172 > > +#if DEBUG_LAYER_VISIBILITY > > + if (m_backVisibility && !m_backVisibility->visibleRect().isEmpty()) > > + printf("Layer 0x%p local visible rect %s\n", m_layer, BlackBerry::Platform::FloatRect(m_backVisibility->visibleRect()).toString().c_str()); > > +#endif > > Should this be using BlackBerry::Platform::logAlways()? > > > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:196 > > - if (scale != 1.0) { > > + if (scale != 1) { > > Why are you changing this? scale is already a double, so what benefit is there from not comparing it with a double literal? (Same question for the one in the previous hunk.) > Carlos pointed out that the style guidelines said to prefer integer constants over float or double constants unless that is explicitly needed to promote to floating point arithmetic. > > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:263 > > + printf("Tile at (%d, %d) needs render\n", index.i(), index.j()); > > Like above, printf() or logAlways()? > > > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:508 > > + FloatPoint origin = boundsRect.location(); > > + FloatSize bounds = boundsRect.size(); > > Given that you're touching every line of this function anyway and in the interest of minimizing the amount of terminology used, I would suggest not having these two constants and using boundsRect directly instead. > Makes sense > > Source/WebCore/platform/graphics/blackberry/LayerTiler.h:31 > > +#include <BlackBerryPlatformThreading.h> > > If you're removing the mutex and manually including the header for _smp_xchg() in the .cpp file, what's prompting the addition of this #include line? > Can it be removed or, if not that, pushed down into other files that still use BlackBerry::Platform threading classes? > I think this is not needed any more. I had an intermediate version of the patch where I had a pthread rwlock wrapper down in platform, but I ended up doing swapping instead of rwlock'ing. > > Source/WebCore/platform/graphics/blackberry/LayerUtilities.h:187 > > + *ok = (denominator != 0.0); > > With "ok" being a pointer, should this have a check for "if (ok)"? Otherwise, I'm not clear on where you draw the line between this and your previous patch where you added a return value for "float& w" that is not a pointer. > I should add an if (ok) I guess - the reason I use pointer for the "ok" is because it's a classic Qt'ism... > > Source/WebCore/platform/graphics/blackberry/LayerUtilities.h:255 > > + size_t i = (i0 + di) % vertices.size(); > > + const FloatPoint& p1 = vertices[i].xy; > > + float w1 = vertices[i].w; > > + FloatSize uv1 = vertices[i].uv; > > + const FloatPoint& p2 = vertices[i + 1].xy; > > + float w2 = vertices[i + 1].w; > > + FloatSize uv2 = vertices[i + 1].uv; > > While I agree with the idea behind ordering it this way, it looks really cluttered. I feel grouping the two FloatPoints, the two floats and the two FloatSizes together respectively would make a cleaner picture. I agree
Created attachment 204532 [details] Patch
(In reply to comment #7) > Created an attachment (id=204532) [details] > Patch I went with bool& for "ok" after all...
Also, it would be inconvenient to use BB::P::logAlways, printf is easier to follow when running torch-launcher or weblauncher from the command line.
Comment on attachment 204532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=204532&action=review > Source/WebCore/platform/graphics/blackberry/LayerTile.cpp:30 > + : m_scale(0.0) m_scale is double, you can just use 0 instead of 0.0 > Source/WebCore/platform/graphics/blackberry/LayerTile.cpp:44 > + m_scale = 0.0; // Resolution independent Ditto. > Source/WebCore/platform/graphics/blackberry/LayerTile.cpp:56 > + m_scale = 0.0; // Unknown scale Ditto. > Source/WebCore/platform/graphics/blackberry/LayerTile.h:52 > + // Returns 0 if contents are resolution independent or scale is simply unknown Nit: finish comments with a period. > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:124 > + delete m_frontVisibility; > + delete m_backVisibility; You could use OwnPtr for these. > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:157 > + // If we're dirty, start fresh. Otherwise, keep track of tiles rendered so far, to avoid re-rendering the same content Nit: finish comments with a period. > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:184 > + FloatRect visibleRect = m_backVisibility ? m_backVisibility->visibleRect() : FloatRect(BlackBerry::Platform::FloatRect(BlackBerry::Platform::Settings::instance()->layerTilerPrefillRect())); > + if (m_layer->isMask() || m_layer->filters().size()) > + visibleRect = FloatRect(IntPoint::zero(), m_layer->bounds()); In this case you didn't need the previous visibleRect, maybe this could be something like: FloatRect visibleRect; if (m_layer->isMask() || m_layer->filters().size()) visibleRect = FloatRect(IntPoint::zero(), m_layer->bounds()); else if (m_backVisibility) visibleRect = m_backVisibility->visibleRect(); else visibleRect = FloatRect(BlackBerry::Platform::FloatRect(BlackBerry::Platform::Settings::instance()->layerTilerPrefillRect()); > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:203 > + // The visible rect is using unscaled coordinates in most cases Nit: you know :-) > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:248 > + // Resolution independent layers have all the needed data in the first tile Nit: :-) > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:461 > + // Even if the tile has wrong scale, it can be used as a "preview" if there is no risk of overlap with other tiles Nit: period missing . . . > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:490 > + // Everything is expressed in normalized device coordinates Nit: period > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:646 > + LayerTile* tile = m_tiles.take(*it); > + tile->discardContents(); > + delete tile; You can use OwnPtr here. > Source/WebCore/platform/graphics/blackberry/LayerUtilities.h:37 > +inline float det(const FloatSize& a, const FloatSize& b) Use the complete word instead of the abbreviation "determinant" > Source/WebCore/platform/graphics/blackberry/LayerUtilities.h:187 > + ok = (denominator != 0.0); Maybe you can return early in this case to avoid the zero division.
Comment on attachment 204532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=204532&action=review >> Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:124 >> + delete m_backVisibility; > > You could use OwnPtr for these. The reason I went with raw delete here is because of all the swapping that goes on. I don't want be swapping in and out new pointer values into the OwnPtr internal storage, and I don't want to give the impression that a "common" or "usual" pattern of memory management is going on here by using an OwnPtr. In fact, there's a bunch of swapping that goes on which makes this rather unusual case of memory management. >> Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:646 >> + delete tile; > > You can use OwnPtr here. Yes, this looks like a good place to use OwnPtr =) >> Source/WebCore/platform/graphics/blackberry/LayerUtilities.h:187 >> + ok = (denominator != 0.0); > > Maybe you can return early in this case to avoid the zero division. Good point =)
Created attachment 204690 [details] Patch
Comment on attachment 204690 [details] Patch Uploaded to wrong bug
Created attachment 204694 [details] Patch
Comment on attachment 204694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=204694&action=review > Source/WebCore/platform/graphics/blackberry/LayerTiler.cpp:648 > + OwnPtr<LayerTile> tile = m_tiles.take(*it); Oops, I forgot "adoptPtr"
Created attachment 204696 [details] Patch
Comment on attachment 204696 [details] Patch Clearing flags on attachment: 204696 Committed r151595: <http://trac.webkit.org/changeset/151595>
All reviewed patches have been landed. Closing bug.