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

Revert of add asserts around results from requestLock (patchset #3 id:40001 of https://codereview.chromium.or… (patchset #1 id:1 of https://codereview.chromium.org/1165583005/)

Browse Source 
Reason for revert:
failure in cc_unittests (need to diagnose)

@@@STEP_LOG_LINE@ResourceProviderTests_ResourceProviderTest.TransferInvalidSoftware_1@ResourceProviderTests/ResourceProviderTest.TransferInvalidSoftware/1 (run #1):@@@
@@@STEP_LOG_LINE@ResourceProviderTests_ResourceProviderTest.TransferInvalidSoftware_1@[ RUN      ] ResourceProviderTests/ResourceProviderTest.TransferInvalidSoftware/1@@@
@@@STEP_LOG_LINE@ResourceProviderTests_ResourceProviderTest.TransferInvalidSoftware_1@[17114:17114:0529/180822:15336467671:INFO:SkPixelRef.cpp(168)] ../../third_party/skia/src/core/SkPixelRef.cpp:168: failed assertion "pixels"@@@

Original issue's description:
> add asserts around results from requestLock (patchset #3 id:40001 of https://codereview.chromium.org/1155403003/)"
>
> This reverts commit 3953d36041.
>
> BUG=skia:
> TBR=
>
> Committed: https://skia.googlesource.com/skia/+/6980f5a96455c8062403c995a64b654a0e9a1319

TBR=
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=skia:

Review URL: https://codereview.chromium.org/1151573009
This commit is contained in:
reed 2015-05-29 18:36:07 -07:00 committed by Commit bot
parent 6980f5a964
commit 19663e54c0
3 changed files with 28 additions and 64 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
include/core/SkPixelRef.h
View File

@ -75,8 +75,6 @@ public:
* Calling lockPixels returns a LockRec struct (on success).
*/
struct LockRec {
LockRec() : fPixels(NULL), fColorTable(NULL) {}
void* fPixels;
SkColorTable* fColorTable;
size_t fRowBytes;
@ -201,13 +199,11 @@ public:
};
struct LockResult {
LockResult() : fPixels(NULL), fCTable(NULL) {}
void (*fUnlockProc)(void* ctx);
void* fUnlockContext;
const void* fPixels;
SkColorTable* fCTable; // should be NULL unless colortype is kIndex8
const void* fPixels;
size_t fRowBytes;
SkISize fSize;
@ -349,7 +345,7 @@ private:
LockRec fRec;
int fLockCount;
bool lockPixelsInsideMutex();
bool lockPixelsInsideMutex(LockRec* rec);
// Bottom bit indicates the Gen ID is unique.
bool genIDIsUnique() const { return SkToBool(fTaggedGenID.load() & 1); }

68
src/core/SkPixelRef.cpp
View File

@ -160,22 +160,8 @@ void SkPixelRef::cloneGenID(const SkPixelRef& that) {
SkASSERT(!that. genIDIsUnique());
}
static void validate_pixels_ctable(const SkImageInfo& info, const void* pixels,
const SkColorTable* ctable) {
if (info.isEmpty()) {
return; // can't require pixels if the dimensions are empty
}
SkASSERT(pixels);
if (kIndex_8_SkColorType == info.colorType()) {
SkASSERT(ctable);
} else {
SkASSERT(NULL == ctable);
}
}
void SkPixelRef::setPreLocked(void* pixels, size_t rowBytes, SkColorTable* ctable) {
#ifndef SK_IGNORE_PIXELREF_SETPRELOCKED
validate_pixels_ctable(fInfo, pixels, ctable);
// only call me in your constructor, otherwise fLockCount tracking can get
// out of sync.
fRec.fPixels = pixels;
@ -186,33 +172,38 @@ void SkPixelRef::setPreLocked(void* pixels, size_t rowBytes, SkColorTable* ctabl
#endif
}
// Increments fLockCount only on success
bool SkPixelRef::lockPixelsInsideMutex() {
bool SkPixelRef::lockPixelsInsideMutex(LockRec* rec) {
fMutex->assertHeld();
// For historical reasons, we always inc fLockCount, even if we return false.
// It would be nice to change this (it seems), and only inc if we actually succeed...
if (1 == ++fLockCount) {
SkASSERT(fRec.isZero());
if (!this->onNewLockPixels(&fRec)) {
fRec.zero();
LockRec rec;
if (!this->onNewLockPixels(&rec)) {
fLockCount -= 1; // we return fLockCount unchanged if we fail.
return false;
}
SkASSERT(!rec.isZero()); // else why did onNewLock return true?
fRec = rec;
}
validate_pixels_ctable(fInfo, fRec.fPixels, fRec.fColorTable);
*rec = fRec;
return true;
}
// For historical reasons, we always inc fLockCount, even if we return false.
// It would be nice to change this (it seems), and only inc if we actually succeed...
bool SkPixelRef::lockPixels() {
bool SkPixelRef::lockPixels(LockRec* rec) {
SkASSERT(!fPreLocked || SKPIXELREF_PRELOCKED_LOCKCOUNT == fLockCount);
if (!fPreLocked) {
if (fPreLocked) {
*rec = fRec;
return true;
} else {
TRACE_EVENT_BEGIN0("skia", "SkPixelRef::lockPixelsMutex");
SkAutoMutexAcquire ac(*fMutex);
TRACE_EVENT_END0("skia", "SkPixelRef::lockPixelsMutex");
SkDEBUGCODE(int oldCount = fLockCount;)
bool success = this->lockPixelsInsideMutex();
bool success = this->lockPixelsInsideMutex(rec);
// lockPixelsInsideMutex only increments the count if it succeeds.
SkASSERT(oldCount + (int)success == fLockCount);
@ -220,19 +211,14 @@ bool SkPixelRef::lockPixels() {
// For compatibility with SkBitmap calling lockPixels, we still want to increment
// fLockCount even if we failed. If we updated SkBitmap we could remove this oddity.
fLockCount += 1;
return false;
}
return success;
}
validate_pixels_ctable(fInfo, fRec.fPixels, fRec.fColorTable);
return true;
}
bool SkPixelRef::lockPixels(LockRec* rec) {
if (this->lockPixels()) {
*rec = fRec;
return true;
}
return false;
bool SkPixelRef::lockPixels() {
LockRec rec;
return this->lockPixels(&rec);
}
void SkPixelRef::unlockPixels() {
@ -267,14 +253,11 @@ bool SkPixelRef::requestLock(const LockRequest& request, LockResult* result) {
result->fPixels = fRec.fPixels;
result->fRowBytes = fRec.fRowBytes;
result->fSize.set(fInfo.width(), fInfo.height());
return true;
} else {
SkAutoMutexAcquire ac(*fMutex);
if (!this->onRequestLock(request, result)) {
return false;
}
return this->onRequestLock(request, result);
}
validate_pixels_ctable(fInfo, result->fPixels, result->fCTable);
return true;
}
bool SkPixelRef::lockPixelsAreWritable() const {
@ -375,15 +358,16 @@ static void unlock_legacy_result(void* ctx) {
}
bool SkPixelRef::onRequestLock(const LockRequest& request, LockResult* result) {
if (!this->lockPixelsInsideMutex()) {
LockRec rec;
if (!this->lockPixelsInsideMutex(&rec)) {
return false;
}
result->fUnlockProc = unlock_legacy_result;
result->fUnlockContext = SkRef(this); // this is balanced in our fUnlockProc
result->fCTable = fRec.fColorTable;
result->fPixels = fRec.fPixels;
result->fRowBytes = fRec.fRowBytes;
result->fCTable = rec.fColorTable;
result->fPixels = rec.fPixels;
result->fRowBytes = rec.fRowBytes;
result->fSize.set(fInfo.width(), fInfo.height());
return true;
}

16
tests/PixelRefTest.cpp
View File

@ -1,22 +1,8 @@
/*
* Copyright 2015 Google Inc.
*
* Use of this source code is governed by a BSD-style license that can be
* found in the LICENSE file.
*/
#include "Test.h"
#include "SkMallocPixelRef.h"
#include "SkPixelRef.h"
static void test_install(skiatest::Reporter* reporter) {
SkImageInfo info = SkImageInfo::MakeN32Premul(0, 0);
SkBitmap bm;
// make sure we don't assert on an empty install
bm.installPixels(info, NULL, 0);
}
class TestListener : public SkPixelRef::GenIDChangeListener {
public:
explicit TestListener(int* ptr) : fPtr(ptr) {}
@ -57,6 +43,4 @@ DEF_TEST(PixelRef_GenIDChange, r) {
REPORTER_ASSERT(r, 0 != pixelRef->getGenerationID());
pixelRef->addGenIDChangeListener(NULL);
pixelRef->notifyPixelsChanged();
test_install(r);
}