Summary: | [BlackBerry] LayerTiler fails to tile really big layers | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Arvid Nilsson <anilsson> | ||||||||||||||
Component: | WebKit BlackBerry | Assignee: | Arvid Nilsson <anilsson> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | anilsson, anlo, cgarcia, commit-queue, jpetsovits, mlattanzio, rwlbuis | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 117067 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Arvid Nilsson
2013-06-04 15:05:45 PDT
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. |