Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
533323002:
Use SkBitmapCache to optimize readPixels on a texture-backed bitmap (Closed)
|
Created:
6 years, 3 months ago by Rémi Piotaix Modified:
6 years, 3 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Project:
skia Visibility:
Public. |
DescriptionUse SkBitmapCache to optimize readPixels on a texture-backed bitmap
BUG=skia:2786
Committed: https://skia.googlesource.com/skia/+/0e9770515cab45decb56a5926d1741b71854fb4c
Patch Set 1 #Patch Set 2 : Fail if unable to allocate pixels #
Total comments: 10
Patch Set 3 : Corrections #
Total comments: 2
Patch Set 4 : Correcting nit #Patch Set 5 : Nit #Patch Set 6 : Rebase master #Patch Set 7 : Fix bug: use extractSubset causes cache miss in SkBitmapDevice #Patch Set 8 : Rebase master #Messages
Total messages: 20 (5 generated)
piotaixr@chromium.org changed reviewers: + bsalomon@google.com, junov@chromium.org, reed@google.com
Re-implementation of https://codereview.chromium.org/428773002 in a better way. This is breaking chromium on the test fast/canvas/webgl/canvas-test.html ASan say that is it a buffer overflow in fSurface->readPixels but i'm unable to find the problem...
https://codereview.chromium.org/533323002/diff/20001/src/gpu/SkGrPixelRef.cpp File src/gpu/SkGrPixelRef.cpp (right): https://codereview.chromium.org/533323002/diff/20001/src/gpu/SkGrPixelRef.cpp... src/gpu/SkGrPixelRef.cpp:158: static bool tryAllocBitmapPixels(SkBitmap& bitmap) { style: skia passes mutable args by pointer, not ref (except SkReadBuffer for some reason). https://codereview.chromium.org/533323002/diff/20001/src/gpu/SkGrPixelRef.cpp... src/gpu/SkGrPixelRef.cpp:184: SkBitmap cachedBitmap; nit: could use this->info().makeWH(bounds.width(), bounds.height()) to make the desired info. https://codereview.chromium.org/533323002/diff/20001/src/gpu/SkGrPixelRef.cpp... src/gpu/SkGrPixelRef.cpp:189: if (!tryAllocBitmapPixels(cachedBitmap)) style: Skia *always* use { } with "if" blocks. https://codereview.chromium.org/533323002/diff/20001/src/gpu/SkGrPixelRef.cpp... src/gpu/SkGrPixelRef.cpp:193: SkAutoLockPixels alp(cachedBitmap); allocPixels always returns a locked bitmap, so this autolock is unnecessary. https://codereview.chromium.org/533323002/diff/20001/src/gpu/SkGrPixelRef.cpp... src/gpu/SkGrPixelRef.cpp:211: // Unlock the pixels (lock comes from Find or allocPixelRef) I see that the old code did this, but I think we don't think we need to do this. readPixels can (and probably should) return a locked bitmap, since by definition its pixels are direct allocated, and can never be "lazy".
https://codereview.chromium.org/533323002/diff/20001/src/gpu/SkGrPixelRef.cpp File src/gpu/SkGrPixelRef.cpp (right): https://codereview.chromium.org/533323002/diff/20001/src/gpu/SkGrPixelRef.cpp... src/gpu/SkGrPixelRef.cpp:158: static bool tryAllocBitmapPixels(SkBitmap& bitmap) { On 2014/09/04 18:49:59, reed1 wrote: > style: skia passes mutable args by pointer, not ref (except SkReadBuffer for > some reason). Done. https://codereview.chromium.org/533323002/diff/20001/src/gpu/SkGrPixelRef.cpp... src/gpu/SkGrPixelRef.cpp:184: SkBitmap cachedBitmap; On 2014/09/04 18:49:59, reed1 wrote: > nit: could use > > this->info().makeWH(bounds.width(), bounds.height()) > > to make the desired info. Done. https://codereview.chromium.org/533323002/diff/20001/src/gpu/SkGrPixelRef.cpp... src/gpu/SkGrPixelRef.cpp:189: if (!tryAllocBitmapPixels(cachedBitmap)) On 2014/09/04 18:49:59, reed1 wrote: > style: Skia *always* use { } with "if" blocks. Done. https://codereview.chromium.org/533323002/diff/20001/src/gpu/SkGrPixelRef.cpp... src/gpu/SkGrPixelRef.cpp:193: SkAutoLockPixels alp(cachedBitmap); On 2014/09/04 18:49:59, reed1 wrote: > allocPixels always returns a locked bitmap, so this autolock is unnecessary. Done. https://codereview.chromium.org/533323002/diff/20001/src/gpu/SkGrPixelRef.cpp... src/gpu/SkGrPixelRef.cpp:211: // Unlock the pixels (lock comes from Find or allocPixelRef) On 2014/09/04 18:49:59, reed1 wrote: > I see that the old code did this, but I think we don't think we need to do this. > readPixels can (and probably should) return a locked bitmap, since by definition > its pixels are direct allocated, and can never be "lazy". Done.
defers to brian https://codereview.chromium.org/533323002/diff/40001/src/gpu/SkGrPixelRef.cpp File src/gpu/SkGrPixelRef.cpp (right): https://codereview.chromium.org/533323002/diff/40001/src/gpu/SkGrPixelRef.cpp... src/gpu/SkGrPixelRef.cpp:194: bool readPixelsOk = fSurface->readPixels(bounds.fLeft, bounds.fTop, bounds.width(), bounds.height(), nit: 100cols
On 2014/09/08 12:29:58, reed1 wrote: > defers to brian > > https://codereview.chromium.org/533323002/diff/40001/src/gpu/SkGrPixelRef.cpp > File src/gpu/SkGrPixelRef.cpp (right): > > https://codereview.chromium.org/533323002/diff/40001/src/gpu/SkGrPixelRef.cpp... > src/gpu/SkGrPixelRef.cpp:194: bool readPixelsOk = > fSurface->readPixels(bounds.fLeft, bounds.fTop, bounds.width(), bounds.height(), > nit: 100cols lgtm
https://codereview.chromium.org/533323002/diff/40001/src/gpu/SkGrPixelRef.cpp File src/gpu/SkGrPixelRef.cpp (right): https://codereview.chromium.org/533323002/diff/40001/src/gpu/SkGrPixelRef.cpp... src/gpu/SkGrPixelRef.cpp:194: bool readPixelsOk = fSurface->readPixels(bounds.fLeft, bounds.fTop, bounds.width(), bounds.height(), On 2014/09/08 12:29:58, reed1 wrote: > nit: 100cols Done.
The CQ bit was checked by piotaixr@chromium.org
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/piotaixr@chromium.org/533323002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for src/gpu/SkGrPixelRef.cpp:
While running git apply --index -p1;
error: patch failed: src/gpu/SkGrPixelRef.cpp:153
error: src/gpu/SkGrPixelRef.cpp: patch does not apply
Patch: src/gpu/SkGrPixelRef.cpp
Index: src/gpu/SkGrPixelRef.cpp
diff --git a/src/gpu/SkGrPixelRef.cpp b/src/gpu/SkGrPixelRef.cpp
index
f56c3b63907d202f4dd3f3b500f340c643439e92..52e3b5d666a7fbc52f68e235a007eb622c08e4e6
100644
--- a/src/gpu/SkGrPixelRef.cpp
+++ b/src/gpu/SkGrPixelRef.cpp
@@ -9,8 +9,10 @@
#include "SkGrPixelRef.h"
+
#include "GrContext.h"
#include "GrTexture.h"
+#include "SkBitmapCache.h"
#include "SkGr.h"
#include "SkRect.h"
@@ -143,7 +145,7 @@ SkPixelRef* SkGrPixelRef::deepCopy(SkColorType dstCT, const
SkIRect* subset) {
if (NULL == fSurface) {
return NULL;
}
-
+
// Note that when copying a render-target-backed pixel ref, we
// return a texture-backed pixel ref instead. This is because
// render-target pixel refs are usually created in conjunction with
@@ -153,30 +155,59 @@ SkPixelRef* SkGrPixelRef::deepCopy(SkColorType dstCT,
const SkIRect* subset) {
return copyToTexturePixelRef(fSurface->asTexture(), dstCT, subset);
}
+static bool tryAllocBitmapPixels(SkBitmap* bitmap) {
+ SkBitmap::Allocator* allocator = SkBitmapCache::GetAllocator();
+ if (NULL != allocator) {
+ return allocator->allocPixelRef(bitmap, 0);
+ } else {
+ // DiscardableMemory is not available, fallback to default allocator
+ return bitmap->tryAllocPixels();
+ }
+}
+
bool SkGrPixelRef::onReadPixels(SkBitmap* dst, const SkIRect* subset) {
if (NULL == fSurface || fSurface->wasDestroyed()) {
return false;
}
- int left, top, width, height;
+ SkIRect bounds;
if (NULL != subset) {
- left = subset->fLeft;
- width = subset->width();
- top = subset->fTop;
- height = subset->height();
+ bounds = *subset;
} else {
- left = 0;
- width = this->info().width();
- top = 0;
- height = this->info().height();
+ bounds = SkIRect::MakeWH(this->info().width(), this->info().height());
}
- if (!dst->tryAllocN32Pixels(width, height)) {
- SkDebugf("SkGrPixelRef::onReadPixels failed to alloc bitmap for
result!\n");
- return false;
- }
- SkAutoLockPixels al(*dst);
- void* buffer = dst->getPixels();
- return fSurface->readPixels(left, top, width, height,
+
+ //Check the cache
+ if(!SkBitmapCache::Find(this->getGenerationID(), bounds, dst)) {
+ //Cache miss
+
+ SkBitmap cachedBitmap;
+ cachedBitmap.setInfo(this->info().makeWH(bounds.width(),
bounds.height()));
+
+ // If we can't alloc the pixels, then fail
+ if (!tryAllocBitmapPixels(&cachedBitmap)) {
+ return false;
+ }
+
+ // Try to read the pixels from the surface
+ void* buffer = cachedBitmap.getPixels();
+ bool readPixelsOk = fSurface->readPixels(bounds.fLeft, bounds.fTop,
+ bounds.width(), bounds.height(),
kSkia8888_GrPixelConfig,
- buffer, dst->rowBytes());
+ buffer, cachedBitmap.rowBytes());
+
+ if (!readPixelsOk) {
+ return false;
+ }
+
+ // If we are here, pixels were read correctly from the surface.
+ cachedBitmap.setImmutable();
+ //Add to the cache
+ SkBitmapCache::Add(this->getGenerationID(), bounds, cachedBitmap);
+
+ dst->swap(cachedBitmap);
+ }
+
+ return true;
+
}
The CQ bit was checked by piotaixr@chromium.org
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/piotaixr@chromium.org/533323002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as 95fd68e5ccd242a91e6dd827dd695f18661efbe6
Message was sent while issue was closed.
On 2014/09/08 15:07:51, I haz the power (commit-bot) wrote: > Committed patchset #6 (id:100001) as 95fd68e5ccd242a91e6dd827dd695f18661efbe6 This is triggering Blink layout test failures: https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... Specifically, printing/webgl-repeated-printing.html shows a R/G (RGBA/ARGB?) inversion: PASS pixel[1] is within 1 of 255 PASS pixel[2] is within 1 of 0 Test 1: canvas should be red -PASS pixel[0] is within 1 of 255 -PASS pixel[1] is within 1 of 0 +FAIL pixel[0] should be within 1 of 255. Was 0. +FAIL pixel[1] should be within 1 of 0. Was 255. PASS pixel[2] is within 1 of 0 Test 2: canvas should be blue PASS pixel[0] is within 1 of 0 -PASS pixel[1] is within 1 of 0 -PASS pixel[2] is within 1 of 255 +FAIL pixel[1] should be within 1 of 0. Was 255. +FAIL pixel[2] should be within 1 of 255. Was 0.
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/551523003/ by fmalita@chromium.org. The reason for reverting is: Component inversions in Blink's printing/webgl-repeated-printing.html: https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... Reverting to unblock rolls..
On 2014/09/09 15:11:47, Florin Malita wrote: > A revert of this CL (patchset #6 id:100001) has been created in > https://codereview.chromium.org/551523003/ by mailto:fmalita@chromium.org. > > The reason for reverting is: Component inversions in Blink's > printing/webgl-repeated-printing.html: > https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... > > Reverting to unblock rolls.. A patch correcting this has landed in Blink (and is in the last Blink roll), resubmitting the patch...
The CQ bit was checked by piotaixr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/533323002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as 0e9770515cab45decb56a5926d1741b71854fb4c |
