Deprecate SkDevice::accessBitmap method
Relies on https://codereview.chromium.org/2162423003/ (Add SK_SUPPORT_LEGACY_ACCESSBITMAP Skia guard) landing in Chromium first.
Calved off: https://codereview.chromium.org/2163323002/ (Add desired width & height to drawContext (as opposed to using the width & height of the RT))
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2168483003
Review-Url: https://codereview.chromium.org/2168483003
diff --git a/include/core/SkBitmapDevice.h b/include/core/SkBitmapDevice.h
index b62a88b..85beb16 100644
--- a/include/core/SkBitmapDevice.h
+++ b/include/core/SkBitmapDevice.h
@@ -135,7 +135,11 @@
altered. The config/width/height/rowbytes must remain unchanged.
@return the device contents as a bitmap
*/
+#ifdef SK_SUPPORT_LEGACY_ACCESSBITMAP
const SkBitmap& onAccessBitmap() override;
+#else
+ const SkBitmap& onAccessBitmap();
+#endif
SkPixelRef* getPixelRef() const { return fBitmap.pixelRef(); }
// just for subclasses, to assign a custom pixelref
diff --git a/include/core/SkDevice.h b/include/core/SkDevice.h
index f06abde..4a2d656 100644
--- a/include/core/SkDevice.h
+++ b/include/core/SkDevice.h
@@ -77,6 +77,7 @@
return this->imageInfo().isOpaque();
}
+#ifdef SK_SUPPORT_LEGACY_ACCESSBITMAP
/** Return the bitmap associated with this device. Call this each time you need
to access the bitmap, as it notifies the subclass to perform any flushing
etc. before you examine the pixels.
@@ -84,6 +85,7 @@
@return the device's bitmap
*/
const SkBitmap& accessBitmap(bool changePixels);
+#endif
bool writePixels(const SkImageInfo&, const void*, size_t rowBytes, int x, int y);
@@ -276,11 +278,16 @@
///////////////////////////////////////////////////////////////////////////
+#ifdef SK_SUPPORT_LEGACY_ACCESSBITMAP
/** Update as needed the pixel value in the bitmap, so that the caller can
access the pixels directly.
@return The device contents as a bitmap
*/
- virtual const SkBitmap& onAccessBitmap() = 0;
+ virtual const SkBitmap& onAccessBitmap() {
+ SkASSERT(0);
+ return fLegacyBitmap;
+ }
+#endif
virtual GrContext* context() const { return nullptr; }
@@ -387,6 +394,10 @@
SkMetaData* fMetaData;
SkSurfaceProps fSurfaceProps;
+#ifdef SK_SUPPORT_LEGACY_ACCESSBITMAP
+ SkBitmap fLegacyBitmap;
+#endif
+
#ifdef SK_DEBUG
bool fAttachedToCanvas;
#endif
diff --git a/src/core/SkDevice.cpp b/src/core/SkDevice.cpp
index 5a613ff..75d481e 100644
--- a/src/core/SkDevice.cpp
+++ b/src/core/SkDevice.cpp
@@ -48,6 +48,7 @@
return SkImageInfo::MakeUnknown();
}
+#ifdef SK_SUPPORT_LEGACY_ACCESSBITMAP
const SkBitmap& SkBaseDevice::accessBitmap(bool changePixels) {
const SkBitmap& bitmap = this->onAccessBitmap();
if (changePixels) {
@@ -55,6 +56,7 @@
}
return bitmap;
}
+#endif
SkPixelGeometry SkBaseDevice::CreateInfo::AdjustGeometry(const SkImageInfo& info,
TileUsage tileUsage,
diff --git a/src/gpu/SkGpuDevice.cpp b/src/gpu/SkGpuDevice.cpp
index f8033ec..314281c 100644
--- a/src/gpu/SkGpuDevice.cpp
+++ b/src/gpu/SkGpuDevice.cpp
@@ -182,14 +182,9 @@
, fContext(SkRef(drawContext->accessRenderTarget()->getContext()))
, fRenderTarget(drawContext->renderTarget())
, fDrawContext(std::move(drawContext)) {
+ fSize.set(width, height);
fOpaque = SkToBool(flags & kIsOpaque_Flag);
- SkAlphaType at = fOpaque ? kOpaque_SkAlphaType : kPremul_SkAlphaType;
- SkImageInfo info = fRenderTarget->surfacePriv().info(at).makeWH(width, height);
- SkPixelRef* pr = new SkGrPixelRef(info, fRenderTarget.get());
- fLegacyBitmap.setInfo(info);
- fLegacyBitmap.setPixelRef(pr)->unref();
-
if (flags & kNeedClear_Flag) {
this->clearAll();
}
@@ -284,23 +279,11 @@
}
fRenderTarget->writePixels(x, y, info.width(), info.height(), config, pixels, rowBytes, flags);
- // need to bump our genID for compatibility with clients that "know" we have a bitmap
- fLegacyBitmap.notifyPixelsChanged();
-
return true;
}
-const SkBitmap& SkGpuDevice::onAccessBitmap() {
- ASSERT_SINGLE_OWNER
- return fLegacyBitmap;
-}
-
bool SkGpuDevice::onAccessPixels(SkPixmap* pmap) {
ASSERT_SINGLE_OWNER
- // For compatibility with clients the know we're backed w/ a bitmap, and want to inspect its
- // genID. When we can hide/remove that fact, we can eliminate this call to notify.
- // ... ugh.
- fLegacyBitmap.notifyPixelsChanged();
return false;
}
@@ -370,14 +353,6 @@
fRenderTarget = newDC->renderTarget();
-#ifdef SK_DEBUG
- SkImageInfo info = fRenderTarget->surfacePriv().info(fOpaque ? kOpaque_SkAlphaType :
- kPremul_SkAlphaType);
- SkASSERT(info == fLegacyBitmap.info());
-#endif
- SkPixelRef* pr = new SkGrPixelRef(fLegacyBitmap.info(), fRenderTarget.get());
- fLegacyBitmap.setPixelRef(pr)->unref();
-
fDrawContext = newDC;
}
diff --git a/src/gpu/SkGpuDevice.h b/src/gpu/SkGpuDevice.h
index 0b1e6a3..d341b39 100644
--- a/src/gpu/SkGpuDevice.h
+++ b/src/gpu/SkGpuDevice.h
@@ -81,7 +81,10 @@
GrDrawContext* accessDrawContext() override;
SkImageInfo imageInfo() const override {
- return fLegacyBitmap.info();
+ SkAlphaType at = fOpaque ? kOpaque_SkAlphaType : kPremul_SkAlphaType;
+ SkImageInfo info = fRenderTarget->surfacePriv().info(at).makeWH(fSize.fWidth,
+ fSize.fHeight);
+ return info;
}
void drawPaint(const SkDraw&, const SkPaint& paint) override;
@@ -141,7 +144,6 @@
void onAttachToCanvas(SkCanvas* canvas) override;
void onDetachFromCanvas() override;
- const SkBitmap& onAccessBitmap() override;
bool onAccessPixels(SkPixmap*) override;
// for debugging purposes only
@@ -161,8 +163,7 @@
SkAutoTUnref<const SkClipStack> fClipStack;
SkIPoint fClipOrigin;
GrClipStackClip fClip;
- // remove when our clients don't rely on accessBitmap()
- SkBitmap fLegacyBitmap;
+ SkISize fSize;
bool fOpaque;
enum Flags {
diff --git a/src/image/SkSurface_Gpu.cpp b/src/image/SkSurface_Gpu.cpp
index fa37814..8d5601d 100644
--- a/src/image/SkSurface_Gpu.cpp
+++ b/src/image/SkSurface_Gpu.cpp
@@ -34,8 +34,6 @@
case SkSurface::kDiscardWrite_BackendHandleAccess:
// for now we don't special-case on Discard, but we may in the future.
surface->notifyContentWillChange(SkSurface::kRetain_ContentChangeMode);
- // legacy: need to dirty the bitmap's genID in our device (curse it)
- surface->getDevice()->accessBitmap(false).notifyPixelsChanged();
break;
}
diff --git a/src/pdf/SkPDFDevice.cpp b/src/pdf/SkPDFDevice.cpp
index bcbff32..8370eab 100644
--- a/src/pdf/SkPDFDevice.cpp
+++ b/src/pdf/SkPDFDevice.cpp
@@ -658,8 +658,7 @@
, fDocument(doc) {
SkASSERT(pageSize.width() > 0);
SkASSERT(pageSize.height() > 0);
- fLegacyBitmap.setInfo(
- SkImageInfo::MakeUnknown(pageSize.width(), pageSize.height()));
+
if (flip) {
// Skia generally uses the top left as the origin but PDF
// natively has the origin at the bottom left. This matrix
@@ -1402,7 +1401,8 @@
}
SkImageInfo SkPDFDevice::imageInfo() const {
- return fLegacyBitmap.info();
+ SkImageInfo info = SkImageInfo::MakeUnknown(fPageSize.width(), fPageSize.height());
+ return info;
}
void SkPDFDevice::onAttachToCanvas(SkCanvas* canvas) {
diff --git a/src/pdf/SkPDFDevice.h b/src/pdf/SkPDFDevice.h
index ca78663..f8497c6 100644
--- a/src/pdf/SkPDFDevice.h
+++ b/src/pdf/SkPDFDevice.h
@@ -193,10 +193,6 @@
};
protected:
- const SkBitmap& onAccessBitmap() override {
- return fLegacyBitmap;
- }
-
sk_sp<SkSurface> makeSurface(const SkImageInfo&, const SkSurfaceProps&) override;
void drawAnnotation(const SkDraw&, const SkRect&, const char key[], SkData* value) override;
@@ -264,8 +260,6 @@
SkScalar fRasterDpi;
- SkBitmap fLegacyBitmap;
-
SkPDFDocument* fDocument;
////////////////////////////////////////////////////////////////////////////
diff --git a/src/svg/SkSVGDevice.cpp b/src/svg/SkSVGDevice.cpp
index f0805b5..4330901 100644
--- a/src/svg/SkSVGDevice.cpp
+++ b/src/svg/SkSVGDevice.cpp
@@ -572,11 +572,10 @@
SkSVGDevice::SkSVGDevice(const SkISize& size, SkXMLWriter* writer)
: INHERITED(SkSurfaceProps(0, kUnknown_SkPixelGeometry))
, fWriter(writer)
- , fResourceBucket(new ResourceBucket) {
+ , fResourceBucket(new ResourceBucket)
+ , fSize(size) {
SkASSERT(writer);
- fLegacyBitmap.setInfo(SkImageInfo::MakeUnknown(size.width(), size.height()));
-
fWriter->writeHeader();
// The root <svg> tag gets closed by the destructor.
@@ -592,11 +591,8 @@
}
SkImageInfo SkSVGDevice::imageInfo() const {
- return fLegacyBitmap.info();
-}
-
-const SkBitmap& SkSVGDevice::onAccessBitmap() {
- return fLegacyBitmap;
+ SkImageInfo info = SkImageInfo::MakeUnknown(fSize.fWidth, fSize.fHeight);
+ return info;
}
void SkSVGDevice::drawPaint(const SkDraw& draw, const SkPaint& paint) {
diff --git a/src/svg/SkSVGDevice.h b/src/svg/SkSVGDevice.h
index 3323471..bf86e15 100644
--- a/src/svg/SkSVGDevice.h
+++ b/src/svg/SkSVGDevice.h
@@ -55,7 +55,6 @@
void drawDevice(const SkDraw&, SkBaseDevice*, int x, int y,
const SkPaint&) override;
- const SkBitmap& onAccessBitmap() override;
private:
SkSVGDevice(const SkISize& size, SkXMLWriter* writer);
@@ -69,7 +68,7 @@
SkXMLWriter* fWriter;
SkAutoTDelete<AutoElement> fRootElement;
SkAutoTDelete<ResourceBucket> fResourceBucket;
- SkBitmap fLegacyBitmap;
+ SkISize fSize;
typedef SkBaseDevice INHERITED;
};
diff --git a/tests/SurfaceTest.cpp b/tests/SurfaceTest.cpp
index 3996ab7..16db3e3 100644
--- a/tests/SurfaceTest.cpp
+++ b/tests/SurfaceTest.cpp
@@ -128,38 +128,6 @@
}
#endif
-// For compatibility with clients that still call accessBitmap(), we need to ensure that we bump
-// the bitmap's genID when we draw to it, else they won't know it has new values. When they are
-// exclusively using surface/image, and we can hide accessBitmap from device, we can remove this
-// test.
-void test_access_pixels(skiatest::Reporter* reporter, const sk_sp<SkSurface>& surface) {
- SkCanvas* canvas = surface->getCanvas();
- canvas->clear(0);
-
- SkBaseDevice* device = canvas->getDevice_just_for_deprecated_compatibility_testing();
- SkBitmap bm = device->accessBitmap(false);
- uint32_t genID0 = bm.getGenerationID();
- // Now we draw something, which needs to "dirty" the genID (sorta like copy-on-write)
- canvas->drawColor(SK_ColorBLUE);
- // Now check that we get a different genID
- uint32_t genID1 = bm.getGenerationID();
- REPORTER_ASSERT(reporter, genID0 != genID1);
-}
-DEF_TEST(SurfaceAccessPixels, reporter) {
- for (auto& surface_func : { &create_surface, &create_direct_surface }) {
- auto surface(surface_func(kPremul_SkAlphaType, nullptr));
- test_access_pixels(reporter, surface);
- }
-}
-#if SK_SUPPORT_GPU
-DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SurfaceAccessPixels_Gpu, reporter, ctxInfo) {
- for (auto& surface_func : { &create_gpu_surface, &create_gpu_scratch_surface }) {
- auto surface(surface_func(ctxInfo.grContext(), kPremul_SkAlphaType, nullptr));
- test_access_pixels(reporter, surface);
- }
-}
-#endif
-
static void test_snapshot_alphatype(skiatest::Reporter* reporter, const sk_sp<SkSurface>& surface,
bool expectOpaque) {
REPORTER_ASSERT(reporter, surface);
@@ -380,36 +348,7 @@
#endif
#if SK_SUPPORT_GPU
-// May we (soon) eliminate the need to keep testing this, by hiding the bloody device!
-static uint32_t get_legacy_gen_id(SkSurface* surface) {
- SkBaseDevice* device =
- surface->getCanvas()->getDevice_just_for_deprecated_compatibility_testing();
- return device->accessBitmap(false).getGenerationID();
-}
-/*
- * Test legacy behavor of bumping the surface's device's bitmap's genID when we access its
- * texture handle for writing.
- *
- * Note: this needs to be tested separately from checking makeImageSnapshot, as calling that
- * can also incidentally bump the genID (when a new backing surface is created).
- */
-static void test_backend_handle_gen_id(
- skiatest::Reporter* reporter, SkSurface* surface,
- GrBackendObject (*func)(SkSurface*, SkSurface::BackendHandleAccess)) {
- const uint32_t gen0 = get_legacy_gen_id(surface);
- func(surface, SkSurface::kFlushRead_BackendHandleAccess);
- const uint32_t gen1 = get_legacy_gen_id(surface);
- REPORTER_ASSERT(reporter, gen0 == gen1);
- func(surface, SkSurface::kFlushWrite_BackendHandleAccess);
- const uint32_t gen2 = get_legacy_gen_id(surface);
- REPORTER_ASSERT(reporter, gen0 != gen2);
-
- func(surface, SkSurface::kDiscardWrite_BackendHandleAccess);
- const uint32_t gen3 = get_legacy_gen_id(surface);
- REPORTER_ASSERT(reporter, gen0 != gen3);
- REPORTER_ASSERT(reporter, gen2 != gen3);
-}
static void test_backend_handle_unique_id(
skiatest::Reporter* reporter, SkSurface* surface,
GrBackendObject (*func)(SkSurface*, SkSurface::BackendHandleAccess)) {
@@ -436,7 +375,7 @@
// No CPU test.
DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SurfaceBackendHandleAccessIDs_Gpu, reporter, ctxInfo) {
for (auto& surface_func : { &create_gpu_surface, &create_gpu_scratch_surface }) {
- for (auto& test_func : { &test_backend_handle_unique_id, &test_backend_handle_gen_id }) {
+ for (auto& test_func : { &test_backend_handle_unique_id }) {
for (auto& handle_access_func :
{ &get_surface_backend_texture_handle, &get_surface_backend_render_target_handle}) {
auto surface(surface_func(ctxInfo.grContext(), kPremul_SkAlphaType, nullptr));