Bug 117211 – [BlackBerry] LayerTiler fails to tile really big layers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Arvid Nilsson]

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.

[Arvid Nilsson]

Created attachment 203731 [details]
Patch

[Arvid Nilsson]

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.

[Arvid Nilsson]

I forgot to mention, this is PR 273550

[Arvid Nilsson]

Created attachment 203732 [details]
Patch

Add PR number to ChangeLog entry

[Jakob Petsovits]

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.

[Arvid Nilsson]

(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

[Arvid Nilsson]

Created attachment 204532 [details]
Patch

[Arvid Nilsson]

(In reply to comment #7)
> Created an attachment (id=204532) [details]
> Patch

I went with bool& for "ok" after all...

[Arvid Nilsson]

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.

[Carlos Garcia Campos]

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.

[Arvid Nilsson]

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 =)

[Arvid Nilsson]

Created attachment 204690 [details]
Patch

[Arvid Nilsson]

Comment on attachment 204690 [details]
Patch

Uploaded to wrong bug

[Arvid Nilsson]

Created attachment 204694 [details]
Patch

[Arvid Nilsson]

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"

[Arvid Nilsson]

Created attachment 204696 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 204696 [details]
Patch

Clearing flags on attachment: 204696

Committed r151595: <http://trac.webkit.org/changeset/151595>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.