change API contract: disallow zero-sized images or surfaces

BUG=skia:

Review URL: https://codereview.chromium.org/830033003

diff --git a/include/core/SkImage.h b/include/core/SkImage.h
index 6070b6b..4cff5db 100644 (file)
--- a/include/core/SkImage.h
+++ b/include/core/SkImage.h
@@ -32,6 +32,10 @@ class GrTexture;
  *  The content of SkImage is always immutable, though the actual storage may
  *  change, if for example that image can be re-created via encoded data or
  *  other means.
+ *
+ *  SkImage always has a non-zero dimensions. If there is a request to create a new image, either
+ *  directly or via SkSurface, and either of the requested dimensions are zero, then NULL will be
+ *  returned.
  */
 class SK_API SkImage : public SkRefCnt {
 public:
@@ -130,8 +134,8 @@ protected:
         fHeight(height),
         fUniqueID(NextUniqueID()) {
 
-        SkASSERT(width >= 0);
-        SkASSERT(height >= 0);
+        SkASSERT(width > 0);
+        SkASSERT(height > 0);
     }
 
 private:

diff --git a/include/core/SkSurface.h b/include/core/SkSurface.h
index 019bf21..aa1e2cc 100644 (file)
--- a/include/core/SkSurface.h
+++ b/include/core/SkSurface.h
@@ -24,6 +24,9 @@ class GrRenderTarget;
  *
  *  To draw into a canvas, first create the appropriate type of Surface, and
  *  then request the canvas from the surface.
+ *
+ *  SkSurface always has non-zero dimensions. If there is a request for a new surface, and either
+ *  of the requested dimensions are zero, then NULL will be returned.
  */
 class SK_API SkSurface : public SkRefCnt {
 public:

diff --git a/src/image/SkImage_Raster.cpp b/src/image/SkImage_Raster.cpp
index a06cca6..0ee3914 100644 (file)
--- a/src/image/SkImage_Raster.cpp
+++ b/src/image/SkImage_Raster.cpp
@@ -19,7 +19,7 @@ public:
         const int maxDimension = SK_MaxS32 >> 2;
         const size_t kMaxPixelByteSize = SK_MaxS32;
 
-        if (info.width() < 0 || info.height() < 0) {
+        if (info.width() <= 0 || info.height() <= 0) {
             return false;
         }
         if (info.width() > maxDimension || info.height() > maxDimension) {
@@ -49,8 +49,6 @@ public:
         return true;
     }
 
-    static SkImage* NewEmpty();
-
     SkImage_Raster(const SkImageInfo&, SkData*, size_t rb, const SkSurfaceProps*);
     virtual ~SkImage_Raster();
 
@@ -86,16 +84,6 @@ private:
 
 ///////////////////////////////////////////////////////////////////////////////
 
-SkImage* SkImage_Raster::NewEmpty() {
-    // Returns lazily created singleton
-    static SkImage* gEmpty;
-    if (NULL == gEmpty) {
-        gEmpty = SkNEW(SkImage_Raster);
-    }
-    gEmpty->ref();
-    return gEmpty;
-}
-
 static void release_data(void* addr, void* context) {
     SkData* data = static_cast<SkData*>(context);
     data->unref();
@@ -169,14 +157,7 @@ bool SkImage_Raster::getROPixels(SkBitmap* dst) const {
 ///////////////////////////////////////////////////////////////////////////////
 
 SkImage* SkImage::NewRasterCopy(const SkImageInfo& info, const void* pixels, size_t rowBytes) {
-    if (!SkImage_Raster::ValidArgs(info, rowBytes)) {
-        return NULL;
-    }
-    if (0 == info.width() && 0 == info.height()) {
-        return SkImage_Raster::NewEmpty();
-    }
-    // check this after empty-check
-    if (NULL == pixels) {
+    if (!SkImage_Raster::ValidArgs(info, rowBytes) || !pixels) {
         return NULL;
     }
 
@@ -187,14 +168,7 @@ SkImage* SkImage::NewRasterCopy(const SkImageInfo& info, const void* pixels, siz
 
 
 SkImage* SkImage::NewRasterData(const SkImageInfo& info, SkData* data, size_t rowBytes) {
-    if (!SkImage_Raster::ValidArgs(info, rowBytes)) {
-        return NULL;
-    }
-    if (0 == info.width() && 0 == info.height()) {
-        return SkImage_Raster::NewEmpty();
-    }
-    // check this after empty-check
-    if (NULL == data) {
+    if (!SkImage_Raster::ValidArgs(info, rowBytes) || !data) {
         return NULL;
     }
 

diff --git a/src/image/SkSurface.cpp b/src/image/SkSurface.cpp
index dca4ce7..6fd40c1 100644 (file)
--- a/src/image/SkSurface.cpp
+++ b/src/image/SkSurface.cpp
@@ -123,16 +123,16 @@ static SkSurface_Base* asSB(SkSurface* surface) {
 SkSurface::SkSurface(int width, int height, const SkSurfaceProps* props)
     : fProps(SkSurfacePropsCopyOrDefault(props)), fWidth(width), fHeight(height)
 {
-    SkASSERT(fWidth >= 0);
-    SkASSERT(fHeight >= 0);
+    SkASSERT(fWidth > 0);
+    SkASSERT(fHeight > 0);
     fGenerationID = 0;
 }
 
 SkSurface::SkSurface(const SkImageInfo& info, const SkSurfaceProps* props)
     : fProps(SkSurfacePropsCopyOrDefault(props)), fWidth(info.width()), fHeight(info.height())
 {
-    SkASSERT(fWidth >= 0);
-    SkASSERT(fHeight >= 0);
+    SkASSERT(fWidth > 0);
+    SkASSERT(fHeight > 0);
     fGenerationID = 0;
 }
 

diff --git a/src/image/SkSurface_Raster.cpp b/src/image/SkSurface_Raster.cpp
index b221c13..2bac3f3 100644 (file)
--- a/src/image/SkSurface_Raster.cpp
+++ b/src/image/SkSurface_Raster.cpp
@@ -39,6 +39,10 @@ private:
 ///////////////////////////////////////////////////////////////////////////////
 
 bool SkSurface_Raster::Valid(const SkImageInfo& info, size_t rowBytes) {
+    if (info.isEmpty()) {
+        return false;
+    }
+
     static const size_t kMaxTotalSize = SK_MaxS32;
 
     int shift = 0;

diff --git a/src/lazy/SkDiscardablePixelRef.cpp b/src/lazy/SkDiscardablePixelRef.cpp
index c4e3654..84152ed 100644 (file)
--- a/src/lazy/SkDiscardablePixelRef.cpp
+++ b/src/lazy/SkDiscardablePixelRef.cpp
@@ -98,6 +98,7 @@ bool SkInstallDiscardablePixelRef(SkImageGenerator* generator, SkBitmap* dst,
     SkAutoTDelete<SkImageGenerator> autoGenerator(generator);
     if ((NULL == autoGenerator.get())
         || (!autoGenerator->getInfo(&info))
+        || info.isEmpty()
         || (!dst->setInfo(info))) {
         return false;
     }

diff --git a/tests/SurfaceTest.cpp b/tests/SurfaceTest.cpp
index 28abf98..d8c6d9d 100644 (file)
--- a/tests/SurfaceTest.cpp
+++ b/tests/SurfaceTest.cpp
@@ -71,6 +71,35 @@ enum ImageType {
     kCodec_ImageType,
 };
 
+#include "SkImageGenerator.h"
+
+class EmptyGenerator : public SkImageGenerator {
+protected:
+    bool onGetInfo(SkImageInfo* info) SK_OVERRIDE {
+        *info = SkImageInfo::Make(0, 0, kN32_SkColorType, kPremul_SkAlphaType);
+        return true;
+    }
+};
+
+static void test_empty_image(skiatest::Reporter* reporter) {
+    const SkImageInfo info = SkImageInfo::Make(0, 0, kN32_SkColorType, kPremul_SkAlphaType);
+    
+    REPORTER_ASSERT(reporter, NULL == SkImage::NewRasterCopy(info, NULL, 0));
+    REPORTER_ASSERT(reporter, NULL == SkImage::NewRasterData(info, NULL, 0));
+    REPORTER_ASSERT(reporter, NULL == SkImage::NewFromGenerator(SkNEW(EmptyGenerator)));
+}
+
+static void test_empty_surface(skiatest::Reporter* reporter, GrContext* ctx) {
+    const SkImageInfo info = SkImageInfo::Make(0, 0, kN32_SkColorType, kPremul_SkAlphaType);
+    
+    REPORTER_ASSERT(reporter, NULL == SkSurface::NewRaster(info));
+    REPORTER_ASSERT(reporter, NULL == SkSurface::NewRasterDirect(info, NULL, 0));
+    if (ctx) {
+        REPORTER_ASSERT(reporter, NULL == SkSurface::NewRenderTarget(ctx, info, 0, NULL));
+        REPORTER_ASSERT(reporter, NULL == SkSurface::NewScratchRenderTarget(ctx, info, 0, NULL));
+    }
+}
+
 static void test_image(skiatest::Reporter* reporter) {
     SkImageInfo info = SkImageInfo::MakeN32Premul(1, 1);
     size_t rowBytes = info.minRowBytes();
@@ -475,6 +504,9 @@ DEF_GPUTEST(Surface, reporter, factory) {
     TestSurfaceNoCanvas(reporter, kRaster_SurfaceType, NULL, SkSurface::kDiscard_ContentChangeMode);
     TestSurfaceNoCanvas(reporter, kRaster_SurfaceType, NULL, SkSurface::kRetain_ContentChangeMode);
 
+    test_empty_image(reporter);
+    test_empty_surface(reporter, NULL);
+
     test_imagepeek(reporter, factory);
     test_canvaspeek(reporter, factory);
 
@@ -500,6 +532,7 @@ DEF_GPUTEST(Surface, reporter, factory) {
                 TestSurfaceNoCanvas(reporter, kGpuScratch_SurfaceType, context, SkSurface::kRetain_ContentChangeMode);
                 TestGetTexture(reporter, kGpu_SurfaceType, context);
                 TestGetTexture(reporter, kGpuScratch_SurfaceType, context);
+                test_empty_surface(reporter, context);
             }
         }
     }