AuroraMiddleware/skia2
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Make it safe to enter the cache from a GrTexture idle proc.

Browse Source 
This fixes an issue in Chrome where Skia is calling a promise SkImage
texture release proc from ~SkImage that in turn flushes a SkSurface.
Prior to this change this caused an assert because we had already
decremented the GrTexture's ref count priot to calling the release
proc. This made the GrTexture purgeable, but the cache had not yet
been notified that the texture was purgeable and still had it in its
array of non-purgeable resources. This triggered an assert in the
cache's self-validation checks during the flush.

Now we call the release proc just prior to decrementing the ref count.
This also makes it legal to re-ref the resources from the release proc.

Bug: chromium:933526
Change-Id: I8cd921b77ca3dfe112089f9a553c1a625160d16d
Reviewed-on: https://skia-review.googlesource.com/c/194000
Reviewed-by: Greg Daniel <egdaniel@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
This commit is contained in:
Brian Salomon 2019-02-22 10:44:19 -05:00 committed by Skia Commit-Bot
parent aa76bc1388
commit 8cabb327e7
10 changed files with 174 additions and 92 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
include/gpu/GrGpuResource.h
View File

@ -33,12 +33,14 @@ class SkTraceMemoryDump;
*
* The latter two ref types are private and intended only for Gr core code.
*
* When all the ref/io counts reach zero DERIVED::notifyAllCntsAreZero() will be called (static poly
* morphism using CRTP). Similarly when the ref (but not necessarily pending read/write) count
* reaches 0 DERIVED::notifyRefCountIsZero() will be called. In the case when an unref() causes both
* PRIOR to the last ref/IO count being removed DERIVED::notifyAllCntsWillBeZero() will be called
* (static poly morphism using CRTP). It is legal for additional ref's or pending IOs to be added
* during this time. AFTER all the ref/io counts reach zero DERIVED::notifyAllCntsAreZero() will be
* called. Similarly when the ref (but not necessarily pending read/write) count reaches 0
* DERIVED::notifyRefCountIsZero() will be called. In the case when an unref() causes both
* the ref cnt to reach zero and the other counts are zero, notifyRefCountIsZero() will be called
* before notifyAllCntsAreZero(). Moreover, if notifyRefCountIsZero() returns false then
* notifyAllRefCntsAreZero() won't be called at all. notifyRefCountIsZero() must return false if the
* notifyAllCntsAreZero() won't be called at all. notifyRefCountIsZero() must return false if the
* object may be deleted after notifyRefCntIsZero() returns.
*
* GrIORef and GrGpuResource are separate classes for organizational reasons and to be
@ -58,7 +60,13 @@ public:
void unref() const {
this->validate();
if (!(--fRefCnt)) {
if (fRefCnt == 1) {
if (!this->internalHasPendingIO()) {
static_cast<const DERIVED*>(this)->notifyAllCntsWillBeZero();
}
SkASSERT(fRefCnt > 0);
}
if (--fRefCnt == 0) {
if (!static_cast<const DERIVED*>(this)->notifyRefCountIsZero()) {
return;
}
@ -104,6 +112,9 @@ private:
void completedRead() const {
this->validate();
if (fPendingReads == 1 && !fPendingWrites && !fRefCnt) {
static_cast<const DERIVED*>(this)->notifyAllCntsWillBeZero();
}
--fPendingReads;
this->didRemoveRefOrPendingIO(kPendingRead_CntType);
}
@ -115,6 +126,9 @@ private:
void completedWrite() const {
this->validate();
if (fPendingWrites == 1 && !fPendingReads && !fRefCnt) {
static_cast<const DERIVED*>(this)->notifyAllCntsWillBeZero();
}
--fPendingWrites;
this->didRemoveRefOrPendingIO(kPendingWrite_CntType);
}
@ -306,11 +320,12 @@ private:
/**
* Called by GrResourceCache when a resource loses its last ref or pending IO.
*/
virtual void removedLastRefOrPendingIO() {}
virtual void willRemoveLastRefOrPendingIO() {}
// See comments in CacheAccess and ResourcePriv.
void setUniqueKey(const GrUniqueKey&);
void removeUniqueKey();
void notifyAllCntsWillBeZero() const;
void notifyAllCntsAreZero(CntType) const;
bool notifyRefCountIsZero() const;
void removeScratchKey();

9
src/gpu/GrGpuResource.cpp
View File

@ -155,9 +155,12 @@ void GrGpuResource::setUniqueKey(const GrUniqueKey& key) {
get_resource_cache(fGpu)->resourceAccess().changeUniqueKey(this, key);
}
void GrGpuResource::notifyAllCntsAreZero(CntType lastCntTypeToReachZero) const {
void GrGpuResource::notifyAllCntsWillBeZero() const {
GrGpuResource* mutableThis = const_cast<GrGpuResource*>(this);
mutableThis->removedLastRefOrPendingIO();
mutableThis->willRemoveLastRefOrPendingIO();
}
void GrGpuResource::notifyAllCntsAreZero(CntType lastCntTypeToReachZero) const {
if (this->wasDestroyed()) {
// We've already been removed from the cache. Goodbye cruel world!
delete this;
@ -169,6 +172,7 @@ void GrGpuResource::notifyAllCntsAreZero(CntType lastCntTypeToReachZero) const {
static const uint32_t kFlag =
GrResourceCache::ResourceAccess::kAllCntsReachedZero_RefNotificationFlag;
GrGpuResource* mutableThis = const_cast<GrGpuResource*>(this);
get_resource_cache(fGpu)->resourceAccess().notifyCntReachedZero(mutableThis, kFlag);
}
@ -182,7 +186,6 @@ bool GrGpuResource::notifyRefCountIsZero() const {
uint32_t flags = GrResourceCache::ResourceAccess::kRefCntReachedZero_RefNotificationFlag;
if (!this->internalHasPendingIO()) {
flags |= GrResourceCache::ResourceAccess::kAllCntsReachedZero_RefNotificationFlag;
mutableThis->removedLastRefOrPendingIO();
}
get_resource_cache(fGpu)->resourceAccess().notifyCntReachedZero(mutableThis, flags);

2
src/gpu/gl/GrGLTexture.h
View File

@ -126,7 +126,7 @@ protected:
private:
void onSetRelease(sk_sp<GrReleaseProcHelper> releaseHelper) override {}
void removedLastRefOrPendingIO() override {
void willRemoveLastRefOrPendingIO() override {
if (fIdleProc) {
fIdleProc(fIdleProcContext);
fIdleProc = nullptr;

4
src/gpu/mock/GrMockTexture.h
View File

@ -72,7 +72,7 @@ protected:
// protected so that GrMockTextureRenderTarget can call this to avoid "inheritance via
// dominance" warning.
void removedLastRefOrPendingIO() override {
void willRemoveLastRefOrPendingIO() override {
if (fIdleProc) {
fIdleProc(fIdleProcContext);
fIdleProc = nullptr;
@ -191,7 +191,7 @@ private:
}
// We implement this to avoid the inheritance via dominance warning.
void removedLastRefOrPendingIO() override { GrMockTexture::removedLastRefOrPendingIO(); }
void willRemoveLastRefOrPendingIO() override { GrMockTexture::willRemoveLastRefOrPendingIO(); }
size_t onGpuMemorySize() const override {
int numColorSamples = this->numColorSamples();

2
src/gpu/mtl/GrMtlTexture.h
View File

@ -68,7 +68,7 @@ private:
// the GPU. Thus we do nothing special here with the releaseHelper.
void onSetRelease(sk_sp<GrReleaseProcHelper> releaseHelper) override {}
void removedLastRefOrPendingIO() override {
void willRemoveLastRefOrPendingIO() override {
if (fIdleProc) {
fIdleProc(fIdleProcContext);
fIdleProc = nullptr;

2
src/gpu/vk/GrVkImage.cpp
View File

@ -246,7 +246,7 @@ void GrVkImage::setResourceRelease(sk_sp<GrReleaseProcHelper> releaseHelper) {
}
void GrVkImage::Resource::freeGPUData(GrVkGpu* gpu) const {
SkASSERT(!fReleaseHelper);
this->invokeReleaseProc();
VK_CALL(gpu, DestroyImage(gpu->device(), fImage, nullptr));
bool isLinear = (VK_IMAGE_TILING_LINEAR == fImageTiling);
GrVkMemory::FreeImageMemory(gpu, isLinear, fAlloc);

17
src/gpu/vk/GrVkImage.h
View File

@ -198,9 +198,18 @@ private:
protected:
mutable sk_sp<GrReleaseProcHelper> fReleaseHelper;
void invokeReleaseProc() const {
if (fReleaseHelper) {
// Depending on the ref count of fReleaseHelper this may or may not actually trigger
// the ReleaseProc to be called.
fReleaseHelper.reset();
}
}
private:
void freeGPUData(GrVkGpu* gpu) const override;
void abandonGPUData() const override {
this->invokeReleaseProc();
SkASSERT(!fReleaseHelper);
}
@ -222,14 +231,6 @@ private:
: Resource(image, alloc, tiling) {
}
private:
void invokeReleaseProc() const {
if (fReleaseHelper) {
// Depending on the ref count of fReleaseHelper this may or may not actually trigger
// the ReleaseProc to be called.
fReleaseHelper.reset();
}
}
void freeGPUData(GrVkGpu* gpu) const override;
void abandonGPUData() const override;
};

2
src/gpu/vk/GrVkTexture.cpp
View File

@ -179,7 +179,7 @@ void GrVkTexture::setIdleProc(IdleProc proc, void* context) {
}
}
void GrVkTexture::removedLastRefOrPendingIO() {
void GrVkTexture::willRemoveLastRefOrPendingIO() {
if (!fIdleProc) {
return;
}

2
src/gpu/vk/GrVkTexture.h
View File

@ -69,7 +69,7 @@ private:
this->setResourceRelease(std::move(releaseHelper));
}
void removedLastRefOrPendingIO() override;
void willRemoveLastRefOrPendingIO() override;
const GrVkImageView* fTextureView;
GrTexture::IdleProc* fIdleProc = nullptr;

175
tests/GrSurfaceTest.cpp
View File

@ -336,13 +336,18 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(ReadOnlyTexture, reporter, context_info) {
}
}
DEF_GPUTEST(TextureIdleProcTest, reporter, options) {
static const int kS = 10;
// Helper to delete a backend texture in a GrTexture's release proc.
static const auto installBackendTextureReleaseProc = [](GrTexture* texture) {
auto backendTexture = texture->getBackendTexture();
auto context = texture->getContext();
static sk_sp<GrTexture> make_wrapped_texture(GrContext* context, bool renderable) {
auto backendTexture = context->priv().getGpu()->createTestingOnlyBackendTexture(
nullptr, 10, 10, GrColorType::kRGBA_8888, renderable, GrMipMapped::kNo);
sk_sp<GrTexture> texture;
if (renderable) {
texture = context->priv().resourceProvider()->wrapRenderableBackendTexture(
backendTexture, 1, kBorrow_GrWrapOwnership, GrWrapCacheable::kNo);
} else {
texture = context->priv().resourceProvider()->wrapBackendTexture(
backendTexture, kBorrow_GrWrapOwnership, GrWrapCacheable::kNo, kRW_GrIOType);
}
// Add a release proc that deletes the GrBackendTexture.
struct ReleaseContext {
GrContext* fContext;
GrBackendTexture fBackendTexture;
@ -356,43 +361,27 @@ DEF_GPUTEST(TextureIdleProcTest, reporter, options) {
}
delete releaseContext;
};
texture->setRelease(sk_make_sp<GrReleaseProcHelper>(
release, new ReleaseContext{context, backendTexture}));
};
texture->setRelease(
sk_make_sp<GrReleaseProcHelper>(release, new ReleaseContext{context, backendTexture}));
return texture;
}
static sk_sp<GrTexture> make_normal_texture(GrContext* context, bool renderable) {
GrSurfaceDesc desc;
desc.fConfig = kRGBA_8888_GrPixelConfig;
desc.fWidth = desc.fHeight = 10;
desc.fFlags = renderable ? kRenderTarget_GrSurfaceFlag : kNone_GrSurfaceFlags;
return context->priv().resourceProvider()->createTexture(desc, SkBudgeted::kNo);
}
DEF_GPUTEST(TextureIdleProcTest, reporter, options) {
// Various ways of making textures.
auto makeWrapped = [](GrContext* context) {
auto backendTexture = context->priv().getGpu()->createTestingOnlyBackendTexture(
nullptr, kS, kS, GrColorType::kRGBA_8888, false, GrMipMapped::kNo);
auto texture = context->priv().resourceProvider()->wrapBackendTexture(
backendTexture, kBorrow_GrWrapOwnership, GrWrapCacheable::kNo, kRW_GrIOType);
installBackendTextureReleaseProc(texture.get());
return texture;
};
auto makeWrapped = [](GrContext* context) { return make_wrapped_texture(context, false); };
auto makeWrappedRenderable = [](GrContext* context) {
auto backendTexture = context->priv().getGpu()->createTestingOnlyBackendTexture(
nullptr, kS, kS, GrColorType::kRGBA_8888, true, GrMipMapped::kNo);
auto texture = context->priv().resourceProvider()->wrapRenderableBackendTexture(
backendTexture, 1, kBorrow_GrWrapOwnership, GrWrapCacheable::kNo);
installBackendTextureReleaseProc(texture.get());
return texture;
};
auto makeNormal = [](GrContext* context) {
GrSurfaceDesc desc;
desc.fConfig = kRGBA_8888_GrPixelConfig;
desc.fWidth = desc.fHeight = kS;
return context->priv().resourceProvider()->createTexture(desc, SkBudgeted::kNo);
};
auto makeRenderable = [](GrContext* context) {
GrSurfaceDesc desc;
desc.fFlags = kRenderTarget_GrSurfaceFlag;
desc.fConfig = kRGBA_8888_GrPixelConfig;
desc.fWidth = desc.fHeight = kS;
return context->priv().resourceProvider()->createTexture(desc, SkBudgeted::kNo);
return make_wrapped_texture(context, true);
};
auto makeNormal = [](GrContext* context) { return make_normal_texture(context, false); };
auto makeRenderable = [](GrContext* context) { return make_normal_texture(context, true); };
std::function<sk_sp<GrTexture>(GrContext*)> makers[] = {makeWrapped, makeWrappedRenderable,
makeNormal, makeRenderable};
@ -448,15 +437,18 @@ DEF_GPUTEST(TextureIdleProcTest, reporter, options) {
REPORTER_ASSERT(reporter, idleIDs.find(1) != idleIDs.end());
texture = make(context, 2);
int w = texture->width();
int h = texture->height();
SkImageInfo info =
SkImageInfo::Make(kS, kS, kRGBA_8888_SkColorType, kPremul_SkAlphaType);
SkImageInfo::Make(w, h, kRGBA_8888_SkColorType, kPremul_SkAlphaType);
auto rt = SkSurface::MakeRenderTarget(context, SkBudgeted::kNo, info, 0, nullptr);
auto rtc = rt->getCanvas()->internal_private_accessTopLayerRenderTargetContext();
auto singleUseLazyCB = [&texture](GrResourceProvider* rp) {
return std::move(texture);
};
GrSurfaceDesc desc;
desc.fWidth = desc.fHeight = kS;
desc.fWidth = w;
desc.fHeight = h;
desc.fConfig = kRGBA_8888_GrPixelConfig;
if (isRT) {
desc.fFlags = kRenderTarget_GrSurfaceFlag;
@ -473,8 +465,8 @@ DEF_GPUTEST(TextureIdleProcTest, reporter, options) {
GrInternalSurfaceFlags ::kNone, SkBackingFit::kExact, budgeted,
GrSurfaceProxy::LazyInstantiationType::kSingleUse);
rtc->drawTexture(GrNoClip(), proxy, GrSamplerState::Filter::kNearest,
SkBlendMode::kSrcOver, SkPMColor4f(), SkRect::MakeWH(kS, kS),
SkRect::MakeWH(kS, kS), GrAA::kNo, GrQuadAAFlags::kNone,
SkBlendMode::kSrcOver, SkPMColor4f(), SkRect::MakeWH(w, h),
SkRect::MakeWH(w, h), GrAA::kNo, GrQuadAAFlags::kNone,
SkCanvas::kFast_SrcRectConstraint, SkMatrix::I(), nullptr);
// We still have the proxy, which should remain instantiated, thereby keeping the
// texture not purgeable.
@ -486,8 +478,8 @@ DEF_GPUTEST(TextureIdleProcTest, reporter, options) {
// This time we move the proxy into the draw.
rtc->drawTexture(GrNoClip(), std::move(proxy), GrSamplerState::Filter::kNearest,
SkBlendMode::kSrcOver, SkPMColor4f(), SkRect::MakeWH(kS, kS),
SkRect::MakeWH(kS, kS), GrAA::kNo, GrQuadAAFlags::kNone,
SkBlendMode::kSrcOver, SkPMColor4f(), SkRect::MakeWH(w, h),
SkRect::MakeWH(w, h), GrAA::kNo, GrQuadAAFlags::kNone,
SkCanvas::kFast_SrcRectConstraint, SkMatrix::I(), nullptr);
REPORTER_ASSERT(reporter, idleIDs.find(2) == idleIDs.end());
context->flush();
@ -505,8 +497,8 @@ DEF_GPUTEST(TextureIdleProcTest, reporter, options) {
GrInternalSurfaceFlags ::kNone, SkBackingFit::kExact, budgeted,
GrSurfaceProxy::LazyInstantiationType::kDeinstantiate);
rtc->drawTexture(GrNoClip(), std::move(proxy), GrSamplerState::Filter::kNearest,
SkBlendMode::kSrcOver, SkPMColor4f(), SkRect::MakeWH(kS, kS),
SkRect::MakeWH(kS, kS), GrAA::kNo, GrQuadAAFlags::kNone,
SkBlendMode::kSrcOver, SkPMColor4f(), SkRect::MakeWH(w, h),
SkRect::MakeWH(w, h), GrAA::kNo, GrQuadAAFlags::kNone,
SkCanvas::kFast_SrcRectConstraint, SkMatrix::I(), nullptr);
// At this point the proxy shouldn't even be instantiated, there is no texture with
// id 3.
@ -539,11 +531,12 @@ DEF_GPUTEST(TextureIdleProcTest, reporter, options) {
for (auto drawType :
{DrawType::kNoDraw, DrawType::kDraw, DrawType::kDrawAndFlush}) {
for (bool unrefFirst : {false, true}) {
auto possiblyDrawAndFlush = [&context, &texture, drawType, unrefFirst] {
auto possiblyDrawAndFlush = [&context, &texture, drawType, unrefFirst, w,
h] {
if (drawType == DrawType::kNoDraw) {
return;
}
SkImageInfo info = SkImageInfo::Make(kS, kS, kRGBA_8888_SkColorType,
SkImageInfo info = SkImageInfo::Make(w, h, kRGBA_8888_SkColorType,
kPremul_SkAlphaType);
auto rt = SkSurface::MakeRenderTarget(context, SkBudgeted::kNo, info, 0,
nullptr);
@ -551,12 +544,11 @@ DEF_GPUTEST(TextureIdleProcTest, reporter, options) {
->internal_private_accessTopLayerRenderTargetContext();
auto proxy = context->priv().proxyProvider()->testingOnly_createWrapped(
texture, kTopLeft_GrSurfaceOrigin);
rtc->drawTexture(GrNoClip(), proxy, GrSamplerState::Filter::kNearest,
SkBlendMode::kSrcOver, SkPMColor4f(),
SkRect::MakeWH(kS, kS), SkRect::MakeWH(kS, kS),
GrAA::kNo, GrQuadAAFlags::kNone,
SkCanvas::kFast_SrcRectConstraint, SkMatrix::I(),
nullptr);
rtc->drawTexture(
GrNoClip(), proxy, GrSamplerState::Filter::kNearest,
SkBlendMode::kSrcOver, SkPMColor4f(), SkRect::MakeWH(w, h),
SkRect::MakeWH(w, h), GrAA::kNo, GrQuadAAFlags::kNone,
SkCanvas::kFast_SrcRectConstraint, SkMatrix::I(), nullptr);
if (drawType == DrawType::kDrawAndFlush) {
context->flush();
}
@ -605,3 +597,74 @@ DEF_GPUTEST(TextureIdleProcTest, reporter, options) {
}
}
}
// Tests an idle proc that unrefs another resource down to zero.
DEF_GPUTEST_FOR_ALL_CONTEXTS(TextureIdleProcCacheManipulationTest, reporter, contextInfo) {
GrContext* context = contextInfo.grContext();
// idle proc that releases another texture.
auto idleProc = [](void* texture) { reinterpret_cast<GrTexture*>(texture)->unref(); };
for (const auto& idleMaker : {make_wrapped_texture, make_normal_texture}) {
for (const auto& otherMaker : {make_wrapped_texture, make_normal_texture}) {
auto idleTexture = idleMaker(context, false);
auto otherTexture = otherMaker(context, false);
otherTexture->ref();
idleTexture->setIdleProc(idleProc, otherTexture.get());
otherTexture.reset();
idleTexture.reset();
}
}
}
// Similar to above but more complicated. This flushes the context from the idle proc.
// crbug.com/933526.
DEF_GPUTEST_FOR_ALL_CONTEXTS(TextureIdleProcFlushTest, reporter, contextInfo) {
GrContext* context = contextInfo.grContext();
// idle proc that flushes the context.
auto idleProc = [](void* context) { reinterpret_cast<GrContext*>(context)->flush(); };
for (const auto& idleMaker : {make_wrapped_texture, make_normal_texture}) {
auto idleTexture = idleMaker(context, false);
idleTexture->setIdleProc(idleProc, context);
auto info = SkImageInfo::Make(10, 10, kRGBA_8888_SkColorType, kPremul_SkAlphaType);
auto surf = SkSurface::MakeRenderTarget(context, SkBudgeted::kNo, info, 1, nullptr);
// We'll draw two images to the canvas. One is a normal texture-backed image. The other is
// a wrapped-texture backed image.
surf->getCanvas()->clear(SK_ColorWHITE);
auto img1 = surf->makeImageSnapshot();
auto gpu = context->priv().getGpu();
std::unique_ptr<uint32_t[]> pixels(new uint32_t[info.width() * info.height()]);
auto backendTexture = gpu->createTestingOnlyBackendTexture(
pixels.get(), info.width(), info.height(), kRGBA_8888_SkColorType, false,
GrMipMapped::kNo);
auto img2 = SkImage::MakeFromTexture(context, backendTexture, kTopLeft_GrSurfaceOrigin,
info.colorType(), info.alphaType(), nullptr);
surf->getCanvas()->drawImage(std::move(img1), 0, 0);
surf->getCanvas()->drawImage(std::move(img2), 1, 1);
idleTexture.reset();
gpu->deleteTestingOnlyBackendTexture(backendTexture);
}
}
DEF_GPUTEST_FOR_ALL_CONTEXTS(TextureIdleProcRerefTest, reporter, contextInfo) {
GrContext* context = contextInfo.grContext();
// idle proc that refs the texture
auto idleProc = [](void* texture) { reinterpret_cast<GrTexture*>(texture)->ref(); };
// release proc to check whether the texture was released or not.
auto releaseProc = [](void* isReleased) { *reinterpret_cast<bool*>(isReleased) = true; };
bool isReleased = false;
auto idleTexture = make_normal_texture(context, false);
// This test assumes the texture won't be cached (or else the release proc doesn't get
// called).
idleTexture->resourcePriv().removeScratchKey();
context->flush();
idleTexture->setIdleProc(idleProc, idleTexture.get());
idleTexture->setRelease(releaseProc, &isReleased);
auto* raw = idleTexture.get();
idleTexture.reset();
REPORTER_ASSERT(reporter, !isReleased);
raw->unref();
REPORTER_ASSERT(reporter, isReleased);
}