From 7ebd389d0f7a0e47fe731e1d024753bce0f0614d Mon Sep 17 00:00:00 2001 From: Chris Craik Date: Thu, 23 Feb 2012 15:35:20 -0800 Subject: Revert "simplify texture generation filters" Seems to be a problem with the refcounting. bug:6050242 bug:6058365 This reverts commit 92a7e4bc6b67b150cbb30f78374173ecfeb43607 --- .../platform/graphics/android/BaseLayerAndroid.cpp | 2 +- .../platform/graphics/android/TextureOwner.h | 4 +- .../graphics/android/TexturesGenerator.cpp | 39 ++++++++++- .../platform/graphics/android/TexturesGenerator.h | 10 ++- .../platform/graphics/android/TiledPage.cpp | 76 ++++++++++------------ .../WebCore/platform/graphics/android/TiledPage.h | 5 +- .../platform/graphics/android/TiledTexture.cpp | 5 +- .../platform/graphics/android/TilesManager.h | 8 +-- .../platform/graphics/android/TilesProfiler.cpp | 10 +-- .../platform/graphics/android/TilesProfiler.h | 2 +- .../platform/graphics/android/TransferQueue.cpp | 44 +++++-------- .../platform/graphics/android/TransferQueue.h | 4 ++ 12 files changed, 115 insertions(+), 94 deletions(-) (limited to 'Source/WebCore/platform/graphics') diff --git a/Source/WebCore/platform/graphics/android/BaseLayerAndroid.cpp b/Source/WebCore/platform/graphics/android/BaseLayerAndroid.cpp index 2d2867c..a3c92cd 100644 --- a/Source/WebCore/platform/graphics/android/BaseLayerAndroid.cpp +++ b/Source/WebCore/platform/graphics/android/BaseLayerAndroid.cpp @@ -247,7 +247,7 @@ bool BaseLayerAndroid::prepareBasePictureInGL(SkRect& viewport, float scale, nextTiledPage->prepare(goingDown, goingLeft, viewportTileBounds, TiledPage::VisibleBounds); // Cancel pending paints for the foreground page - TilesManager::instance()->removePaintOperationsForPage(tiledPage); + TilesManager::instance()->removePaintOperationsForPage(tiledPage, false); } // If we fired a request, let's check if it's ready to use diff --git a/Source/WebCore/platform/graphics/android/TextureOwner.h b/Source/WebCore/platform/graphics/android/TextureOwner.h index 25d234b..5434dbf 100644 --- a/Source/WebCore/platform/graphics/android/TextureOwner.h +++ b/Source/WebCore/platform/graphics/android/TextureOwner.h @@ -26,8 +26,6 @@ #ifndef TextureOwner_h #define TextureOwner_h -#include "SkRefCnt.h" - class SkCanvas; class Layer; @@ -37,7 +35,7 @@ class TiledPage; class BaseTileTexture; class GLWebViewState; -class TextureOwner : public SkRefCnt { +class TextureOwner { public: virtual ~TextureOwner() { } virtual bool removeTexture(BaseTileTexture* texture) = 0; diff --git a/Source/WebCore/platform/graphics/android/TexturesGenerator.cpp b/Source/WebCore/platform/graphics/android/TexturesGenerator.cpp index d27631b..bccb99b 100644 --- a/Source/WebCore/platform/graphics/android/TexturesGenerator.cpp +++ b/Source/WebCore/platform/graphics/android/TexturesGenerator.cpp @@ -65,13 +65,18 @@ void TexturesGenerator::removeOperationsForPage(TiledPage* page) removeOperationsForFilter(new PageFilter(page)); } -void TexturesGenerator::removePaintOperationsForPage(TiledPage* page) +void TexturesGenerator::removePaintOperationsForPage(TiledPage* page, bool waitForRunning) { - removeOperationsForFilter(new PagePaintFilter(page)); + removeOperationsForFilter(new PagePaintFilter(page), waitForRunning); } void TexturesGenerator::removeOperationsForFilter(OperationFilter* filter) { + removeOperationsForFilter(filter, true); +} + +void TexturesGenerator::removeOperationsForFilter(OperationFilter* filter, bool waitForRunning) +{ if (!filter) return; @@ -85,7 +90,34 @@ void TexturesGenerator::removeOperationsForFilter(OperationFilter* filter) i++; } } - delete filter; + + if (waitForRunning && m_currentOperation) { + QueuedOperation* operation = m_currentOperation; + + if (operation && filter->check(operation)) { + m_waitForCompletion = true; + // The reason we are signaling the transferQueue is : + // TransferQueue may be waiting a slot to work on, but now UI + // thread is waiting for Tex Gen thread to finish first before the + // UI thread can free a slot for the transferQueue. + // Therefore, it could be a deadlock. + // The solution is use this as a flag to tell Tex Gen thread that + // UI thread is waiting now, Tex Gen thread should not wait for the + // queue any more. + TilesManager::instance()->transferQueue()->interruptTransferQueue(true); + } + + delete filter; + + // At this point, it means that we are currently executing an operation that + // we want to be removed -- we should wait until it is done, so that + // when we return our caller can be sure that there is no more operations + // in the queue matching the given filter. + while (m_waitForCompletion) + mRequestedOperationsCond.wait(mRequestedOperationsLock); + } else { + delete filter; + } } status_t TexturesGenerator::readyToRun() @@ -160,6 +192,7 @@ bool TexturesGenerator::threadLoop() stop = true; if (m_waitForCompletion) { m_waitForCompletion = false; + TilesManager::instance()->transferQueue()->interruptTransferQueue(false); mRequestedOperationsCond.signal(); } mRequestedOperationsLock.unlock(); diff --git a/Source/WebCore/platform/graphics/android/TexturesGenerator.h b/Source/WebCore/platform/graphics/android/TexturesGenerator.h index c7924cc..2e3b6b4 100644 --- a/Source/WebCore/platform/graphics/android/TexturesGenerator.h +++ b/Source/WebCore/platform/graphics/android/TexturesGenerator.h @@ -42,18 +42,16 @@ class LayerAndroid; class TexturesGenerator : public Thread { public: - TexturesGenerator() - : Thread(false) + TexturesGenerator() : Thread(false) , m_waitForCompletion(false) - , m_currentOperation(0) - {} - + , m_currentOperation(0) { } virtual ~TexturesGenerator() { } virtual status_t readyToRun(); void removeOperationsForPage(TiledPage* page); - void removePaintOperationsForPage(TiledPage* page); + void removePaintOperationsForPage(TiledPage* page, bool waitForRunning); void removeOperationsForFilter(OperationFilter* filter); + void removeOperationsForFilter(OperationFilter* filter, bool waitForRunning); void scheduleOperation(QueuedOperation* operation); diff --git a/Source/WebCore/platform/graphics/android/TiledPage.cpp b/Source/WebCore/platform/graphics/android/TiledPage.cpp index 80d2c3f..8c43fc4 100644 --- a/Source/WebCore/platform/graphics/android/TiledPage.cpp +++ b/Source/WebCore/platform/graphics/android/TiledPage.cpp @@ -59,7 +59,8 @@ namespace WebCore { using namespace android; TiledPage::TiledPage(int id, GLWebViewState* state) - : m_baseTileSize(0) + : m_baseTiles(0) + , m_baseTileSize(0) , m_id(id) , m_scale(1) , m_invScale(1) @@ -69,11 +70,7 @@ TiledPage::TiledPage(int id, GLWebViewState* state) , m_isPrefetchPage(false) , m_willDraw(false) { - int maxTiles = TilesManager::getMaxTextureAllocation() + 1; - m_baseTiles = new BaseTile*[maxTiles]; - for (int i = 0; i < maxTiles; i++) - m_baseTiles[i] = new BaseTile(); - + m_baseTiles = new BaseTile[TilesManager::getMaxTextureAllocation() + 1]; #ifdef DEBUG_COUNT ClassTracker::instance()->increment("TiledPage"); #endif @@ -97,14 +94,13 @@ void TiledPage::updateBaseTileSize() TiledPage::~TiledPage() { TilesManager* tilesManager = TilesManager::instance(); - // remove as many painting operations for this page as possible, to avoid - // unnecessary work. + // In order to delete the page we must ensure that none of its BaseTiles are + // currently painting or scheduled to be painted by the TextureGenerator tilesManager->removeOperationsForPage(this); - - int maxTiles = TilesManager::getMaxTextureAllocation() + 1; - for (int i = 0; i < maxTiles; i++) - SkSafeUnref(m_baseTiles[i]); - delete m_baseTiles; + // Discard the transfer queue after the removal operation to make sure + // no tiles for this page will be left in the transfer queue. + tilesManager->transferQueue()->setPendingDiscardWithLock(); + delete[] m_baseTiles; #ifdef DEBUG_COUNT ClassTracker::instance()->decrement("TiledPage"); #endif @@ -114,9 +110,9 @@ BaseTile* TiledPage::getBaseTile(int x, int y) const { // TODO: replace loop over array with HashMap indexing for (int j = 0; j < m_baseTileSize; j++) { - BaseTile* tile = m_baseTiles[j]; - if (tile->x() == x && tile->y() == y) - return tile; + BaseTile& tile = m_baseTiles[j]; + if (tile.x() == x && tile.y() == y) + return &tile; } return 0; } @@ -124,8 +120,8 @@ BaseTile* TiledPage::getBaseTile(int x, int y) const void TiledPage::discardTextures() { for (int j = 0; j < m_baseTileSize; j++) { - BaseTile* tile = m_baseTiles[j]; - tile->discardTextures(); + BaseTile& tile = m_baseTiles[j]; + tile.discardTextures(); } return; } @@ -165,14 +161,14 @@ void TiledPage::prepareRow(bool goingLeft, int tilesInRow, int firstTileX, int y BaseTile* currentTile = 0; BaseTile* availableTile = 0; for (int j = 0; j < m_baseTileSize; j++) { - BaseTile* tile = m_baseTiles[j]; - if (tile->x() == x && tile->y() == y) { - currentTile = tile; + BaseTile& tile = m_baseTiles[j]; + if (tile.x() == x && tile.y() == y) { + currentTile = &tile; break; } - if (!availableTile || (tile->drawCount() < availableTile->drawCount())) - availableTile = tile; + if (!availableTile || (tile.drawCount() < availableTile->drawCount())) + availableTile = &tile; } if (!currentTile && availableTile) { @@ -220,12 +216,12 @@ bool TiledPage::updateTileDirtiness(const SkIRect& tileBounds) bool visibleTileIsDirty = false; for (int x = 0; x < m_baseTileSize; x++) { - BaseTile* tile = m_baseTiles[x]; + BaseTile& tile = m_baseTiles[x]; // if the tile is in the dirty region then we must invalidate it - if (m_invalRegion.contains(tile->x(), tile->y())) { - tile->markAsDirty(m_latestPictureInval, m_invalTilesRegion); - if (tileBounds.contains(tile->x(), tile->y())) + if (m_invalRegion.contains(tile.x(), tile.y())) { + tile.markAsDirty(m_latestPictureInval, m_invalTilesRegion); + if (tileBounds.contains(tile.x(), tile.y())) visibleTileIsDirty = true; } } @@ -305,9 +301,9 @@ bool TiledPage::hasMissingContent(const SkIRect& tileBounds) { int neededTiles = tileBounds.width() * tileBounds.height(); for (int j = 0; j < m_baseTileSize; j++) { - BaseTile* tile = m_baseTiles[j]; - if (tileBounds.contains(tile->x(), tile->y())) { - if (tile->frontTexture()) + BaseTile& tile = m_baseTiles[j]; + if (tileBounds.contains(tile.x(), tile.y())) { + if (tile.frontTexture()) neededTiles--; } } @@ -319,9 +315,9 @@ bool TiledPage::isReady(const SkIRect& tileBounds) int neededTiles = tileBounds.width() * tileBounds.height(); XLOG("tiled page %p needs %d ready tiles", this, neededTiles); for (int j = 0; j < m_baseTileSize; j++) { - BaseTile* tile = m_baseTiles[j]; - if (tileBounds.contains(tile->x(), tile->y())) { - if (tile->isTileReady()) + BaseTile& tile = m_baseTiles[j]; + if (tileBounds.contains(tile.x(), tile.y())) { + if (tile.isTileReady()) neededTiles--; } } @@ -352,8 +348,8 @@ bool TiledPage::swapBuffersIfReady(const SkIRect& tileBounds, float scale) // swap every tile on page (even if off screen) for (int j = 0; j < m_baseTileSize; j++) { - BaseTile* tile = m_baseTiles[j]; - if (tile->swapTexturesIfNeeded()) + BaseTile& tile = m_baseTiles[j]; + if (tile.swapTexturesIfNeeded()) swaps++; } @@ -377,16 +373,16 @@ void TiledPage::drawGL() const float tileHeight = TilesManager::tileHeight() * m_invScale; for (int j = 0; j < m_baseTileSize; j++) { - BaseTile* tile = m_baseTiles[j]; - bool tileInView = m_tileBounds.contains(tile->x(), tile->y()); + BaseTile& tile = m_baseTiles[j]; + bool tileInView = m_tileBounds.contains(tile.x(), tile.y()); if (tileInView) { SkRect rect; - rect.fLeft = tile->x() * tileWidth; - rect.fTop = tile->y() * tileHeight; + rect.fLeft = tile.x() * tileWidth; + rect.fTop = tile.y() * tileHeight; rect.fRight = rect.fLeft + tileWidth; rect.fBottom = rect.fTop + tileHeight; - tile->draw(m_transparency, rect, m_scale); + tile.draw(m_transparency, rect, m_scale); } TilesManager::instance()->getProfiler()->nextTile(tile, m_invScale, tileInView); diff --git a/Source/WebCore/platform/graphics/android/TiledPage.h b/Source/WebCore/platform/graphics/android/TiledPage.h index d858cc2..791e1f6 100644 --- a/Source/WebCore/platform/graphics/android/TiledPage.h +++ b/Source/WebCore/platform/graphics/android/TiledPage.h @@ -105,10 +105,9 @@ private: BaseTile* getBaseTile(int x, int y) const; - // array of tile ptrs used to compose a page. The tiles are allocated in the + // array of tiles used to compose a page. The tiles are allocated in the // constructor to prevent them from potentially being allocated on the stack - BaseTile** m_baseTiles; - + BaseTile* m_baseTiles; // stores the number of tiles in the m_baseTiles array. This enables us to // quickly iterate over the array without have to check it's size int m_baseTileSize; diff --git a/Source/WebCore/platform/graphics/android/TiledTexture.cpp b/Source/WebCore/platform/graphics/android/TiledTexture.cpp index 4b872a3..1e8b946 100644 --- a/Source/WebCore/platform/graphics/android/TiledTexture.cpp +++ b/Source/WebCore/platform/graphics/android/TiledTexture.cpp @@ -317,8 +317,9 @@ const TransformationMatrix* TiledTexture::transform() void TiledTexture::removeTiles() { - for (unsigned int i = 0; i < m_tiles.size(); i++) - SkSafeUnref(m_tiles[i]); + for (unsigned int i = 0; i < m_tiles.size(); i++) { + delete m_tiles[i]; + } m_tiles.clear(); } diff --git a/Source/WebCore/platform/graphics/android/TilesManager.h b/Source/WebCore/platform/graphics/android/TilesManager.h index d3852d9..b670055 100644 --- a/Source/WebCore/platform/graphics/android/TilesManager.h +++ b/Source/WebCore/platform/graphics/android/TilesManager.h @@ -58,9 +58,9 @@ public: return gInstance != 0; } - void removeOperationsForFilter(OperationFilter* filter) + void removeOperationsForFilter(OperationFilter* filter, bool waitForRunning = false) { - m_pixmapsGenerationThread->removeOperationsForFilter(filter); + m_pixmapsGenerationThread->removeOperationsForFilter(filter, waitForRunning); } void removeOperationsForPage(TiledPage* page) @@ -68,9 +68,9 @@ public: m_pixmapsGenerationThread->removeOperationsForPage(page); } - void removePaintOperationsForPage(TiledPage* page) + void removePaintOperationsForPage(TiledPage* page, bool waitForCompletion) { - m_pixmapsGenerationThread->removePaintOperationsForPage(page); + m_pixmapsGenerationThread->removePaintOperationsForPage(page, waitForCompletion); } void scheduleOperation(QueuedOperation* operation) diff --git a/Source/WebCore/platform/graphics/android/TilesProfiler.cpp b/Source/WebCore/platform/graphics/android/TilesProfiler.cpp index d422904..0271ee3 100644 --- a/Source/WebCore/platform/graphics/android/TilesProfiler.cpp +++ b/Source/WebCore/platform/graphics/android/TilesProfiler.cpp @@ -103,14 +103,14 @@ void TilesProfiler::nextFrame(int left, int top, int right, int bottom, float sc scale, true, (int)(timeDelta * 1000))); } -void TilesProfiler::nextTile(BaseTile* tile, float scale, bool inView) +void TilesProfiler::nextTile(BaseTile& tile, float scale, bool inView) { if (!m_enabled || (m_records.size() > MAX_PROF_FRAMES) || (m_records.size() == 0)) return; - bool isReady = tile->isTileReady(); - int left = tile->x() * TilesManager::tileWidth(); - int top = tile->y() * TilesManager::tileWidth(); + bool isReady = tile.isTileReady(); + int left = tile.x() * TilesManager::tileWidth(); + int top = tile.y() * TilesManager::tileWidth(); int right = left + TilesManager::tileWidth(); int bottom = top + TilesManager::tileWidth(); @@ -122,7 +122,7 @@ void TilesProfiler::nextTile(BaseTile* tile, float scale, bool inView) } m_records.last().append(TileProfileRecord( left, top, right, bottom, - scale, isReady, (int)tile->drawCount())); + scale, isReady, (int)tile.drawCount())); XLOG("adding tile %d %d %d %d, scale %f", left, top, right, bottom, scale); } diff --git a/Source/WebCore/platform/graphics/android/TilesProfiler.h b/Source/WebCore/platform/graphics/android/TilesProfiler.h index 7a3fe59..286d350 100644 --- a/Source/WebCore/platform/graphics/android/TilesProfiler.h +++ b/Source/WebCore/platform/graphics/android/TilesProfiler.h @@ -58,7 +58,7 @@ public: float stop(); void clear(); void nextFrame(int left, int top, int right, int bottom, float scale); - void nextTile(BaseTile* tile, float scale, bool inView); + void nextTile(BaseTile& tile, float scale, bool inView); void nextInval(const IntRect& rect, float scale); int numFrames() { return m_records.size(); diff --git a/Source/WebCore/platform/graphics/android/TransferQueue.cpp b/Source/WebCore/platform/graphics/android/TransferQueue.cpp index 3c2ed1b..2d5be64 100644 --- a/Source/WebCore/platform/graphics/android/TransferQueue.cpp +++ b/Source/WebCore/platform/graphics/android/TransferQueue.cpp @@ -67,6 +67,7 @@ TransferQueue::TransferQueue(bool useMinimalMem) , m_fboID(0) , m_sharedSurfaceTextureId(0) , m_hasGLContext(true) + , m_interruptedByRemovingOp(false) , m_currentDisplay(EGL_NO_DISPLAY) , m_currentUploadType(DEFAULT_UPLOAD_TYPE) { @@ -245,8 +246,17 @@ void TransferQueue::blitTileFromQueue(GLuint fboID, BaseTileTexture* destTex, #endif } +void TransferQueue::interruptTransferQueue(bool interrupt) +{ + m_transferQueueItemLocks.lock(); + m_interruptedByRemovingOp = interrupt; + if (m_interruptedByRemovingOp) + m_transferQueueItemCond.signal(); + m_transferQueueItemLocks.unlock(); +} + // This function must be called inside the m_transferQueueItemLocks, for the -// wait and getHasGLContext(). +// wait, m_interruptedByRemovingOp and getHasGLContext(). // Only called by updateQueueWithBitmap() for now. bool TransferQueue::readyForUpdate() { @@ -254,8 +264,13 @@ bool TransferQueue::readyForUpdate() return false; // Don't use a while loop since when the WebView tear down, the emptyCount // will still be 0, and we bailed out b/c of GL context lost. - if (!m_emptyItemCount) + if (!m_emptyItemCount) { + if (m_interruptedByRemovingOp) + return false; m_transferQueueItemCond.wait(m_transferQueueItemLocks); + if (m_interruptedByRemovingOp) + return false; + } if (!getHasGLContext()) return false; @@ -315,8 +330,6 @@ void TransferQueue::setPendingDiscard() if (m_transferQueue[i].status == pendingBlit) m_transferQueue[i].status = pendingDiscard; - for (unsigned int i = 0 ; i < m_pureColorTileQueue.size(); i++) - SkSafeUnref(m_pureColorTileQueue[i].savedBaseTilePtr); m_pureColorTileQueue.clear(); bool GLContextExisted = getHasGLContext(); @@ -345,7 +358,6 @@ void TransferQueue::updatePureColorTiles() // The queue should be clear instead of setting to different status. XLOG("Warning: Don't expect an emptyItem here."); } - SkSafeUnref(data->savedBaseTilePtr); } m_pureColorTileQueue.clear(); } @@ -386,11 +398,6 @@ void TransferQueue::updateDirtyBaseTiles() if (result != OK) XLOGC("unexpected error: updateTexImage return %d", result); } - - XLOG("removing tile %p from %p, update", - m_transferQueue[index].savedBaseTilePtr, - &m_transferQueue[index]); - SkSafeUnref(m_transferQueue[index].savedBaseTilePtr); m_transferQueue[index].savedBaseTilePtr = 0; m_transferQueue[index].status = emptyItem; if (obsoleteBaseTile) { @@ -511,7 +518,7 @@ bool TransferQueue::tryUpdateQueueWithBitmap(const TileRenderInfo* renderInfo, ANativeWindow_unlockAndPost(m_ANW.get()); } - // b) After update the Surface Texture, now update the transfer queue info. + // b) After update the Surface Texture, now udpate the transfer queue info. addItemInTransferQueue(renderInfo, currentUploadType, &bitmap); XLOG("Bitmap updated x, y %d %d, baseTile %p", @@ -536,19 +543,8 @@ void TransferQueue::addItemCommon(const TileRenderInfo* renderInfo, TextureUploadType type, TileTransferData* data) { - BaseTile* old = data->savedBaseTilePtr; - SkSafeRef(renderInfo->baseTile); - SkSafeUnref(data->savedBaseTilePtr); - data->savedBaseTileTexturePtr = renderInfo->baseTile->backTexture(); data->savedBaseTilePtr = renderInfo->baseTile; - XLOG("adding tile %p, %d by %d, refs %d, removed %p, dataPtr %p", - data->savedBaseTilePtr, - data->savedBaseTilePtr->x(), - data->savedBaseTilePtr->y(), - data->savedBaseTilePtr->getRefCnt(), - old, - data); data->status = pendingBlit; data->uploadType = type; @@ -642,10 +638,6 @@ void TransferQueue::cleanupPendingDiscard() XLOG("transfer queue discarded tile %p, removed texture", tile); } - XLOG("removing tile %p from %p, cleanup", - m_transferQueue[index].savedBaseTilePtr, - &m_transferQueue[index]); - SkSafeUnref(m_transferQueue[index].savedBaseTilePtr); m_transferQueue[index].savedBaseTilePtr = 0; m_transferQueue[index].savedBaseTileTexturePtr = 0; m_transferQueue[index].status = emptyItem; diff --git a/Source/WebCore/platform/graphics/android/TransferQueue.h b/Source/WebCore/platform/graphics/android/TransferQueue.h index 30dd0c6..5dd2e0a 100644 --- a/Source/WebCore/platform/graphics/android/TransferQueue.h +++ b/Source/WebCore/platform/graphics/android/TransferQueue.h @@ -129,6 +129,8 @@ public: // The lock will be done when returning true. bool readyForUpdate(); + void interruptTransferQueue(bool); + void lockQueue() { m_transferQueueItemLocks.lock(); } void unlockQueue() { m_transferQueueItemLocks.unlock(); } @@ -194,6 +196,8 @@ private: int m_emptyItemCount; + bool m_interruptedByRemovingOp; + // We are using wait/signal to handle our own queue sync. // First of all, if we don't have our own lock, then while WebView is // destroyed, the UI thread will wait for the Tex Gen to get out from -- cgit v1.1